| Message ID | 1233752517-30010-2-git-send-email-patrick.ohly@intel.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2009-02-04 at 14:01 +0100, Patrick Ohly wrote: > /** > + * struct cyclecounter - hardware abstraction for a free running counter > + * Provides completely state-free accessors to the underlying hardware. > + * Depending on which hardware it reads, the cycle counter may wrap > + * around quickly. Locking rules (if necessary) have to be defined > + * by the implementor and user of specific instances of this API. > + * > + * @read: returns the current cycle value > + * @mask: bitmask for two's complement > + * subtraction of non 64 bit counters, > + * see CLOCKSOURCE_MASK() helper macro > + * @mult: cycle to nanosecond multiplier > + * @shift: cycle to nanosecond divisor (power of two) > + */ > +struct cyclecounter { > + cycle_t (*read)(const struct cyclecounter *cc); > + cycle_t mask; > + u32 mult; > + u32 shift; > +}; Where are these defined? I don't see any in created in your code. > +/** > + * struct timecounter - layer above a %struct cyclecounter which counts nanoseconds > + * Contains the state needed by timecounter_read() to detect > + * cycle counter wrap around. Initialize with > + * timecounter_init(). Also used to convert cycle counts into the > + * corresponding nanosecond counts with timecounter_cyc2time(). Users > + * of this code are responsible for initializing the underlying > + * cycle counter hardware, locking issues and reading the time > + * more often than the cycle counter wraps around. The nanosecond > + * counter will only wrap around after ~585 years. > + * > + * @cc: the cycle counter used by this instance > + * @cycle_last: most recent cycle counter value seen by > + * timecounter_read() > + * @nsec: continuously increasing count > + */ > +struct timecounter { > + const struct cyclecounter *cc; > + cycle_t cycle_last; > + u64 nsec; > +}; If your mixing generic and non-generic code, it seems a little presumptuous to assume the code would get called more often than the counter wraps. If this cyclecounter is what I think it is (a clocksource) they wrap at varied times. > +/** > + * cyclecounter_cyc2ns - converts cycle counter cycles to nanoseconds > + * @tc: Pointer to cycle counter. > + * @cycles: Cycles > + * > + * XXX - This could use some mult_lxl_ll() asm optimization. Same code > + * as in cyc2ns, but with unsigned result. > + */ > +static inline u64 cyclecounter_cyc2ns(const struct cyclecounter *cc, > + cycle_t cycles) > +{ > + u64 ret = (u64)cycles; > + ret = (ret * cc->mult) >> cc->shift; > + return ret; > +} This is just outright duplication.. Why wouldn't you use the function that already exists for this? > +/** > + * clocksource_read_ns - get nanoseconds since last call of this function > + * @tc: Pointer to time counter > + * > + * When the underlying cycle counter runs over, this will be handled > + * correctly as long as it does not run over more than once between > + * calls. > + * > + * The first call to this function for a new time counter initializes > + * the time tracking and returns bogus results. > + */ "bogus results" doesn't sound very pretty.. Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Daniel! Thanks for looking at this. I think the previous discussion of this patch under the same subject on LKML already addressed your concerns, but I'll also reply in detail below. On Wed, 2009-02-04 at 14:03 +0000, Daniel Walker wrote: > On Wed, 2009-02-04 at 14:01 +0100, Patrick Ohly wrote: > > > /** > > + * struct cyclecounter - hardware abstraction for a free running counter > > + * Provides completely state-free accessors to the underlying hardware. > > + * Depending on which hardware it reads, the cycle counter may wrap > > + * around quickly. Locking rules (if necessary) have to be defined > > + * by the implementor and user of specific instances of this API. > > + * > > + * @read: returns the current cycle value > > + * @mask: bitmask for two's complement > > + * subtraction of non 64 bit counters, > > + * see CLOCKSOURCE_MASK() helper macro > > + * @mult: cycle to nanosecond multiplier > > + * @shift: cycle to nanosecond divisor (power of two) > > + */ > > +struct cyclecounter { > > + cycle_t (*read)(const struct cyclecounter *cc); > > + cycle_t mask; > > + u32 mult; > > + u32 shift; > > +}; > > Where are these defined? I don't see any in created in your code. What do you mean with "these"? cycle_t? That type is defined in clocksource.h. This patch intentionally adds these definitions to the existing file because of the large conceptual overlap. In an earlier revision of the patch I had adapted clocksource itself so that it could be used outside of the time keeping code; John wanted me to use these smaller structs instead that you now find in the current patch. Eventually John wants to refactor clocksource so that it uses them and just adds additional elements in clocksource. Right now clocksource is a mixture of different concepts. Breaking out cyclecounter and timecounter is a first step towards that cleanup. > > +/** > > + * struct timecounter - layer above a %struct cyclecounter which counts nanoseconds > > + * Contains the state needed by timecounter_read() to detect > > + * cycle counter wrap around. Initialize with > > + * timecounter_init(). Also used to convert cycle counts into the > > + * corresponding nanosecond counts with timecounter_cyc2time(). Users > > + * of this code are responsible for initializing the underlying > > + * cycle counter hardware, locking issues and reading the time > > + * more often than the cycle counter wraps around. The nanosecond > > + * counter will only wrap around after ~585 years. > > + * > > + * @cc: the cycle counter used by this instance > > + * @cycle_last: most recent cycle counter value seen by > > + * timecounter_read() > > + * @nsec: continuously increasing count > > + */ > > +struct timecounter { > > + const struct cyclecounter *cc; > > + cycle_t cycle_last; > > + u64 nsec; > > +}; > > If your mixing generic and non-generic code, it seems a little > presumptuous to assume the code would get called more often than the > counter wraps. If this cyclecounter is what I think it is (a > clocksource) they wrap at varied times. timecounter and cyclecounter will have the same owner, so that owner knows how often the cycle counter will run over and can use it accordingly. The cyclecounter is a pointer and not just a an instance of cyclecounter so that the owner can pick an instance of a timecounter which has additional private data attached to it. The initial user of the new code will have a 64 bit hardware register as cycle counter. We decided to leave the "counter runs over too often" problem unsolved until we actually have hardware which exhibits the problem. > > +/** > > + * cyclecounter_cyc2ns - converts cycle counter cycles to nanoseconds > > + * @tc: Pointer to cycle counter. > > + * @cycles: Cycles > > + * > > + * XXX - This could use some mult_lxl_ll() asm optimization. Same code > > + * as in cyc2ns, but with unsigned result. > > + */ > > +static inline u64 cyclecounter_cyc2ns(const struct cyclecounter *cc, > > + cycle_t cycles) > > +{ > > + u64 ret = (u64)cycles; > > + ret = (ret * cc->mult) >> cc->shift; > > + return ret; > > +} > > This is just outright duplication.. Why wouldn't you use the function > that already exists for this? Because it only works with a clocksource. Adding a second function also allows using a saner return type (unsigned instead of signed). Removing the code duplication can be done as part of the code refactoring. But that really is beyond the scope of the patch series and shouldn't be done in the network tree. > > +/** > > + * clocksource_read_ns - get nanoseconds since last call of this function > > + * @tc: Pointer to time counter > > + * > > + * When the underlying cycle counter runs over, this will be handled > > + * correctly as long as it does not run over more than once between > > + * calls. > > + * > > + * The first call to this function for a new time counter initializes > > + * the time tracking and returns bogus results. > > + */ > > "bogus results" doesn't sound very pretty.. I can call it "undefined" if that sounds better ;-) When you quoted the comment I noticed that the function name was still the old one - fixed.
On Wed, 2009-02-04 at 15:46 +0100, Patrick Ohly wrote: > Hi Daniel! > > Thanks for looking at this. I think the previous discussion of this > patch under the same subject on LKML already addressed your concerns, > but I'll also reply in detail below. > > On Wed, 2009-02-04 at 14:03 +0000, Daniel Walker wrote: > > On Wed, 2009-02-04 at 14:01 +0100, Patrick Ohly wrote: > > > > > /** > > > + * struct cyclecounter - hardware abstraction for a free running counter > > > + * Provides completely state-free accessors to the underlying hardware. > > > + * Depending on which hardware it reads, the cycle counter may wrap > > > + * around quickly. Locking rules (if necessary) have to be defined > > > + * by the implementor and user of specific instances of this API. > > > + * > > > + * @read: returns the current cycle value > > > + * @mask: bitmask for two's complement > > > + * subtraction of non 64 bit counters, > > > + * see CLOCKSOURCE_MASK() helper macro > > > + * @mult: cycle to nanosecond multiplier > > > + * @shift: cycle to nanosecond divisor (power of two) > > > + */ > > > +struct cyclecounter { > > > + cycle_t (*read)(const struct cyclecounter *cc); > > > + cycle_t mask; > > > + u32 mult; > > > + u32 shift; > > > +}; > > > > Where are these defined? I don't see any in created in your code. > > What do you mean with "these"? cycle_t? That type is defined in > clocksource.h. This patch intentionally adds these definitions to the > existing file because of the large conceptual overlap. No, your creating a new structure here that wasn't declared. I was referring to "struct cyclecounter". I did look up one of your prior submission (Dec. 15) and reviewed that. > In an earlier revision of the patch I had adapted clocksource itself so > that it could be used outside of the time keeping code; John wanted me > to use these smaller structs instead that you now find in the current > patch. Well, I think your original idea was better.. I don't think we need the duplication of underlying clocksource mechanics. > Eventually John wants to refactor clocksource so that it uses them and > just adds additional elements in clocksource. Right now clocksource is a > mixture of different concepts. Breaking out cyclecounter and timecounter > is a first step towards that cleanup. The problem I see is that your putting off the cleanup of struct clocksource with duplication.. It should go in reverse , you should use clocksources for your patch set. Which will motivate John to clean up the clocksource structure. There's a whole other issue which is that we have many architectures already declaring struct clocksource for it's most basic usage. So I think we want clocksource to be the base "cycle counter" structure (in name and usage). Almost everything else in struct clocksource is specific to generic timekeeping and could be factored out easily. Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2009-02-04 at 15:09 +0000, Daniel Walker wrote: > On Wed, 2009-02-04 at 15:46 +0100, Patrick Ohly wrote: > > On Wed, 2009-02-04 at 14:03 +0000, Daniel Walker wrote: > > > On Wed, 2009-02-04 at 14:01 +0100, Patrick Ohly wrote: > > > > > > > /** > > > > + * struct cyclecounter - hardware abstraction for a free running counter > > > > + * Provides completely state-free accessors to the underlying hardware. > > > > + * Depending on which hardware it reads, the cycle counter may wrap > > > > + * around quickly. Locking rules (if necessary) have to be defined > > > > + * by the implementor and user of specific instances of this API. > > > > + * > > > > + * @read: returns the current cycle value > > > > + * @mask: bitmask for two's complement > > > > + * subtraction of non 64 bit counters, > > > > + * see CLOCKSOURCE_MASK() helper macro > > > > + * @mult: cycle to nanosecond multiplier > > > > + * @shift: cycle to nanosecond divisor (power of two) > > > > + */ > > > > +struct cyclecounter { > > > > + cycle_t (*read)(const struct cyclecounter *cc); > > > > + cycle_t mask; > > > > + u32 mult; > > > > + u32 shift; > > > > +}; > > > > > > Where are these defined? I don't see any in created in your code. > > > > What do you mean with "these"? cycle_t? That type is defined in > > clocksource.h. This patch intentionally adds these definitions to the > > existing file because of the large conceptual overlap. > > No, your creating a new structure here that wasn't declared. I was > referring to "struct cyclecounter". Sorry, I still don't see the problem. I'm declaring and defining struct cyclecounter here. Why should it also be declared (= "struct cyclecounter;") elsewhere? > I did look up one of your prior > submission (Dec. 15) and reviewed that. Right, I saw that a bit later. > > In an earlier revision of the patch I had adapted clocksource itself so > > that it could be used outside of the time keeping code; John wanted me > > to use these smaller structs instead that you now find in the current > > patch. > > Well, I think your original idea was better.. I don't think we need the > duplication of underlying clocksource mechanics. Please discuss this with John. I'd prefer to not get caught in the cross-fire of a discussion that I don't know enough about :-/
On Wed, 2009-02-04 at 07:09 -0800, Daniel Walker wrote: > On Wed, 2009-02-04 at 15:46 +0100, Patrick Ohly wrote: > > In an earlier revision of the patch I had adapted clocksource itself so > > that it could be used outside of the time keeping code; John wanted me > > to use these smaller structs instead that you now find in the current > > patch. > > Well, I think your original idea was better.. I don't think we need the > duplication of underlying clocksource mechanics. > > > Eventually John wants to refactor clocksource so that it uses them and > > just adds additional elements in clocksource. Right now clocksource is a > > mixture of different concepts. Breaking out cyclecounter and timecounter > > is a first step towards that cleanup. > > The problem I see is that your putting off the cleanup of struct > clocksource with duplication.. It should go in reverse , you should use > clocksources for your patch set. Which will motivate John to clean up > the clocksource structure. I strongly disagree. Misusing a established structure for unintended use is just bad. What Patrick wants to use the counters for has very different semantics then how clocksources are used. I think having a bit of redundancy in two structures is good motivation for me to clean up the clocksources to use cyclecounters. thanks -john -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2009-02-04 at 11:25 -0800, john stultz wrote: > On Wed, 2009-02-04 at 07:09 -0800, Daniel Walker wrote: > > On Wed, 2009-02-04 at 15:46 +0100, Patrick Ohly wrote: > > > In an earlier revision of the patch I had adapted clocksource itself so > > > that it could be used outside of the time keeping code; John wanted me > > > to use these smaller structs instead that you now find in the current > > > patch. > > > > Well, I think your original idea was better.. I don't think we need the > > duplication of underlying clocksource mechanics. > > > > > Eventually John wants to refactor clocksource so that it uses them and > > > just adds additional elements in clocksource. Right now clocksource is a > > > mixture of different concepts. Breaking out cyclecounter and timecounter > > > is a first step towards that cleanup. > > > > The problem I see is that your putting off the cleanup of struct > > clocksource with duplication.. It should go in reverse , you should use > > clocksources for your patch set. Which will motivate John to clean up > > the clocksource structure. > > I strongly disagree. Misusing a established structure for unintended use > is just bad. What Patrick wants to use the counters for has very > different semantics then how clocksources are used. I don't think it's at all bad to reuse an established system like that, especially when the purposed one is largely duplication of the other.. The issue is more that struct clocksource has been loaded up with too many timekeeping specific ornaments .. I would think a good clean up would be just to remove those from the structure. > I think having a bit of redundancy in two structures is good motivation > for me to clean up the clocksources to use cyclecounters. I'm not sure what kind of clean up you have in mind. Given that clocksources are used across many architectures, do you really want to do a mass name change for all of them? Not to mention most people see clocksources as cycle counters , so you would just add confusion in addition to code flux. If your going to end up merging this new cycle counter structure with clocksources anyway, why wouldn't we do it now instead of doing temporary duplication .. Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2009-02-04 at 11:40 -0800, Daniel Walker wrote: > On Wed, 2009-02-04 at 11:25 -0800, john stultz wrote: > > On Wed, 2009-02-04 at 07:09 -0800, Daniel Walker wrote: > > > On Wed, 2009-02-04 at 15:46 +0100, Patrick Ohly wrote: > > > > In an earlier revision of the patch I had adapted clocksource itself so > > > > that it could be used outside of the time keeping code; John wanted me > > > > to use these smaller structs instead that you now find in the current > > > > patch. > > > > > > Well, I think your original idea was better.. I don't think we need the > > > duplication of underlying clocksource mechanics. > > > > > > > Eventually John wants to refactor clocksource so that it uses them and > > > > just adds additional elements in clocksource. Right now clocksource is a > > > > mixture of different concepts. Breaking out cyclecounter and timecounter > > > > is a first step towards that cleanup. > > > > > > The problem I see is that your putting off the cleanup of struct > > > clocksource with duplication.. It should go in reverse , you should use > > > clocksources for your patch set. Which will motivate John to clean up > > > the clocksource structure. > > > > I strongly disagree. Misusing a established structure for unintended use > > is just bad. What Patrick wants to use the counters for has very > > different semantics then how clocksources are used. > > I don't think it's at all bad to reuse an established system like that, > especially when the purposed one is largely duplication of the other.. The duplication is only at a very low level. He could not reuse the established clocksource system without really breaking its semantics. > The issue is more that struct clocksource has been loaded up with too > many timekeeping specific ornaments .. I would think a good clean up > would be just to remove those from the structure. I do agree. And I gave it a quick attempt and its not quite trivial. There is some timekeeping ornaments that are required in the clocksource (the two notions of mult, the flags values, etc). Trying to best split this out I think will take some care. > > I think having a bit of redundancy in two structures is good motivation > > for me to clean up the clocksources to use cyclecounters. > > I'm not sure what kind of clean up you have in mind. Given that > clocksources are used across many architectures, do you really want to > do a mass name change for all of them? Not to mention most people see > clocksources as cycle counters , so you would just add confusion in > addition to code flux. No matter what we do, we will have to touch the arch code in some way. Right now I suspect the arch code will still define clocksources, but we can refactor clocksources to use cyclecounters internally. > If your going to end up merging this new cycle counter structure with > clocksources anyway, why wouldn't we do it now instead of doing > temporary duplication .. Patrick could have created his own NICcounter infrastructure somewhere far away from the generic time code and no one would gripe (much like sched_clock in many cases). I think he's doing the right thing by establishing a base structure that we can reuse in the clocksource/timekeeping code. Doing that conversion well so the code is more readable as well as removing structure duplication will take some time and will touch a lot of historically delicate time code, so it will need additional time to sit in testing trees before we push it upward. I see no reason to block his code in the meantime. thanks -john -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2009-02-04 at 12:06 -0800, john stultz wrote: > The duplication is only at a very low level. He could not reuse the > established clocksource system without really breaking its semantics. He gave a link to the first version, http://kerneltrap.org/mailarchive/linux-netdev/2008/11/19/4164204 What specific semantics is he breaking there? Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2009-02-04 at 13:04 -0800, Daniel Walker wrote: > On Wed, 2009-02-04 at 12:06 -0800, john stultz wrote: > > > The duplication is only at a very low level. He could not reuse the > > established clocksource system without really breaking its semantics. > > He gave a link to the first version, > > http://kerneltrap.org/mailarchive/linux-netdev/2008/11/19/4164204 > > What specific semantics is he breaking there? His re-usage of cycle_last and xtime_nsec for other means then how they're defined. In that case his use of xtime_nsec doesn't even store the same unit. Plus he adds other accessors to the clocksource structure that are not compatible with the clocksources registered for timekeeping. He's really doing something different here, and while it does access a counter, and it does translate that into nanoseconds, its not the same as whats done in the timekeeping core which the clocksource was designed around. So by creating his own infrastructure in a shared manner, splitting out a chunk of it to be reused in the clocksource/timekeeping core I think is really a good thing and the right approach. thanks -john -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2009-02-04 at 13:15 -0800, john stultz wrote: > On Wed, 2009-02-04 at 13:04 -0800, Daniel Walker wrote: > > On Wed, 2009-02-04 at 12:06 -0800, john stultz wrote: > > > > > The duplication is only at a very low level. He could not reuse the > > > established clocksource system without really breaking its semantics. > > > > He gave a link to the first version, > > > > http://kerneltrap.org/mailarchive/linux-netdev/2008/11/19/4164204 > > > > What specific semantics is he breaking there? > > His re-usage of cycle_last and xtime_nsec for other means then how > they're defined. > > In that case his use of xtime_nsec doesn't even store the same unit. Those values to me are timekeeping ornaments .. It's not part of the clocksource it's stuff you added from timekeeping. He could easily add his own ornaments to the clocksource instead of re-use , then they could be merged later .. It's not perfect, but it's at least a start. > Plus he adds other accessors to the clocksource structure that are not > compatible with the clocksources registered for timekeeping. It's akin to vread, I think. I'd prefer to see the read() used instead, but I can see why there could be a need for a structure getting passed. > He's really doing something different here, and while it does access a > counter, and it does translate that into nanoseconds, its not the same > as whats done in the timekeeping core which the clocksource was designed > around. I think it's different from _timekeeping_. However the clocksource isn't getting used differently. Like you said the lowlevel parts are the same, ultimately that's all the clocksource should be. It sounds like that's what you want anyway .. You can merge the lowlevel parts with different sets of ornaments, that seems fairly acceptable to me. Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2009-02-05 at 02:18 +0200, Daniel Walker wrote: > > Plus he adds other accessors to the clocksource structure that are not > > compatible with the clocksources registered for timekeeping. > > It's akin to vread, I think. I'd prefer to see the read() used > instead, > but I can see why there could be a need for a structure getting > passed. There is indeed: the igb driver supports multiple different interfaces. Each interface has its own NIC time, so the callback needs the additional pointer to find out which NIC time it is expected to read. Regarding the rest of the discussion: I understand that it is useful to have it and perhaps get serious about refactoring clocksource, but I also hope that this will not delay including the patches. They are needed for HW time stamping now and cannot wait for the clocksource refactoring.
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h index f88d32f..0729ce2 100644 --- a/include/linux/clocksource.h +++ b/include/linux/clocksource.h @@ -22,8 +22,109 @@ typedef u64 cycle_t; struct clocksource; /** + * struct cyclecounter - hardware abstraction for a free running counter + * Provides completely state-free accessors to the underlying hardware. + * Depending on which hardware it reads, the cycle counter may wrap + * around quickly. Locking rules (if necessary) have to be defined + * by the implementor and user of specific instances of this API. + * + * @read: returns the current cycle value + * @mask: bitmask for two's complement + * subtraction of non 64 bit counters, + * see CLOCKSOURCE_MASK() helper macro + * @mult: cycle to nanosecond multiplier + * @shift: cycle to nanosecond divisor (power of two) + */ +struct cyclecounter { + cycle_t (*read)(const struct cyclecounter *cc); + cycle_t mask; + u32 mult; + u32 shift; +}; + +/** + * struct timecounter - layer above a %struct cyclecounter which counts nanoseconds + * Contains the state needed by timecounter_read() to detect + * cycle counter wrap around. Initialize with + * timecounter_init(). Also used to convert cycle counts into the + * corresponding nanosecond counts with timecounter_cyc2time(). Users + * of this code are responsible for initializing the underlying + * cycle counter hardware, locking issues and reading the time + * more often than the cycle counter wraps around. The nanosecond + * counter will only wrap around after ~585 years. + * + * @cc: the cycle counter used by this instance + * @cycle_last: most recent cycle counter value seen by + * timecounter_read() + * @nsec: continuously increasing count + */ +struct timecounter { + const struct cyclecounter *cc; + cycle_t cycle_last; + u64 nsec; +}; + +/** + * cyclecounter_cyc2ns - converts cycle counter cycles to nanoseconds + * @tc: Pointer to cycle counter. + * @cycles: Cycles + * + * XXX - This could use some mult_lxl_ll() asm optimization. Same code + * as in cyc2ns, but with unsigned result. + */ +static inline u64 cyclecounter_cyc2ns(const struct cyclecounter *cc, + cycle_t cycles) +{ + u64 ret = (u64)cycles; + ret = (ret * cc->mult) >> cc->shift; + return ret; +} + +/** + * timecounter_init - initialize a time counter + * @tc: Pointer to time counter which is to be initialized/reset + * @cc: A cycle counter, ready to be used. + * @start_tstamp: Arbitrary initial time stamp. + * + * After this call the current cycle register (roughly) corresponds to + * the initial time stamp. Every call to timecounter_read() increments + * the time stamp counter by the number of elapsed nanoseconds. + */ +extern void timecounter_init(struct timecounter *tc, + const struct cyclecounter *cc, + u64 start_tstamp); + +/** + * timecounter_read - return nanoseconds elapsed since timecounter_init() + * plus the initial time stamp + * @tc: Pointer to time counter. + * + * In other words, keeps track of time since the same epoch as + * the function which generated the initial time stamp. + */ +extern u64 timecounter_read(struct timecounter *tc); + +/** + * timecounter_cyc2time - convert a cycle counter to same + * time base as values returned by + * timecounter_read() + * @tc: Pointer to time counter. + * @cycle: a value returned by tc->cc->read() + * + * Cycle counts that are converted correctly as long as they + * fall into the interval [-1/2 max cycle count, +1/2 max cycle count], + * with "max cycle count" == cs->mask+1. + * + * This allows conversion of cycle counter values which were generated + * in the past. + */ +extern u64 timecounter_cyc2time(struct timecounter *tc, + cycle_t cycle_tstamp); + +/** * struct clocksource - hardware abstraction for a free running counter * Provides mostly state-free accessors to the underlying hardware. + * This is the structure used for system time. * * @name: ptr to clocksource name * @list: list head for registration diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c index ca89e15..6d4a267 100644 --- a/kernel/time/clocksource.c +++ b/kernel/time/clocksource.c @@ -31,6 +31,82 @@ #include <linux/sched.h> /* for spin_unlock_irq() using preempt_count() m68k */ #include <linux/tick.h> +void timecounter_init(struct timecounter *tc, + const struct cyclecounter *cc, + u64 start_tstamp) +{ + tc->cc = cc; + tc->cycle_last = cc->read(cc); + tc->nsec = start_tstamp; +} +EXPORT_SYMBOL(timecounter_init); + +/** + * clocksource_read_ns - get nanoseconds since last call of this function + * @tc: Pointer to time counter + * + * When the underlying cycle counter runs over, this will be handled + * correctly as long as it does not run over more than once between + * calls. + * + * The first call to this function for a new time counter initializes + * the time tracking and returns bogus results. + */ +static u64 timecounter_read_delta(struct timecounter *tc) +{ + cycle_t cycle_now, cycle_delta; + u64 ns_offset; + + /* read cycle counter: */ + cycle_now = tc->cc->read(tc->cc); + + /* calculate the delta since the last timecounter_read_delta(): */ + cycle_delta = (cycle_now - tc->cycle_last) & tc->cc->mask; + + /* convert to nanoseconds: */ + ns_offset = cyclecounter_cyc2ns(tc->cc, cycle_delta); + + /* update time stamp of timecounter_read_delta() call: */ + tc->cycle_last = cycle_now; + + return ns_offset; +} + +u64 timecounter_read(struct timecounter *tc) +{ + u64 nsec; + + /* increment time by nanoseconds since last call */ + nsec = timecounter_read_delta(tc); + nsec += tc->nsec; + tc->nsec = nsec; + + return nsec; +} +EXPORT_SYMBOL(timecounter_read); + +u64 timecounter_cyc2time(struct timecounter *tc, + cycle_t cycle_tstamp) +{ + u64 cycle_delta = (cycle_tstamp - tc->cycle_last) & tc->cc->mask; + u64 nsec; + + /* + * Instead of always treating cycle_tstamp as more recent + * than tc->cycle_last, detect when it is too far in the + * future and treat it as old time stamp instead. + */ + if (cycle_delta > tc->cc->mask / 2) { + cycle_delta = (tc->cycle_last - cycle_tstamp) & tc->cc->mask; + nsec = tc->nsec - cyclecounter_cyc2ns(tc->cc, cycle_delta); + } else { + nsec = cyclecounter_cyc2ns(tc->cc, cycle_delta) + tc->nsec; + } + + return nsec; +} +EXPORT_SYMBOL(timecounter_cyc2time); + /* XXX - Would like a better way for initializing curr_clocksource */ extern struct clocksource clocksource_jiffies;
So far struct clocksource acted as the interface between time/timekeeping.c and hardware. This patch generalizes the concept so that a similar interface can also be used in other contexts. For that it introduces new structures and related functions *without* touching the existing struct clocksource. The reasons for adding these new structures to clocksource.[ch] are * the APIs are clearly related * struct clocksource could be cleaned up to use the new structs * avoids proliferation of files with similar names (timesource.h? timecounter.h?) As outlined in the discussion with John Stultz, this patch adds * struct cyclecounter: stateless API to hardware which counts clock cycles * struct timecounter: stateful utility code built on a cyclecounter which provides a nanosecond counter * only the function to read the nanosecond counter; deltas are used internally and not exposed to users of timecounter The code does no locking of the shared state. It must be called at least as often as the cycle counter wraps around to detect these wrap arounds. Both is the responsibility of the timecounter user. Acked-by: John Stultz <johnstul@us.ibm.com> --- include/linux/clocksource.h | 101 +++++++++++++++++++++++++++++++++++++++++++ kernel/time/clocksource.c | 76 ++++++++++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 0 deletions(-)