| Message ID | 880d82bb8120f73973db27e0c48e949014b1a106.1292512461.git.richard.cochran@omicron.at |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 16 Dec 2010, Richard Cochran wrote: > + if (txc->modes & ADJ_SETOFFSET) { > + /* Validate the delta value. */ > + if (txc->time.tv_sec && txc->time.tv_usec < 0) > + return -EINVAL; > + > + if (txc->modes & ADJ_NANO) { > + struct timespec tmp; > + tmp.tv_sec = txc->time.tv_sec; > + tmp.tv_nsec = txc->time.tv_usec; > + delta = timespec_to_ktime(tmp); > + } else > + delta = timeval_to_ktime(txc->time); > + > + /* Adding the delta should be an "atomic" operation. */ > + local_irq_disable(); I really do not like that conditional irq_disable(), especially as we disable them further down again and we only safe the getnstimeofday() call in the non ADJSETOFFSET code path. So we really better do that unconditionally before getnstimeofday() with an appropriate comment and change the write_seqlock_irq() to write_seqlock(). > + } > + > getnstimeofday(&ts); > > + if (txc->modes & ADJ_SETOFFSET) { > + kt = timespec_to_ktime(ts); > + kt = ktime_add(kt, delta); > + ts = ktime_to_timespec(kt); > + do_settimeofday(&ts); > + local_irq_enable(); > + } > + > write_seqlock_irq(&xtime_lock); Thanks, tglx > -- 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 12/17/10, Richard Cochran <richardcochran@gmail.com> wrote: > This patch adds a new mode bit into the timex structure. When set, the bit > instructs the kernel to add the given time value to the current time. > The proposed new control mode, ADJ_SETOFFSET, is logically the same as ADJ_OFFSET with timex.constant == -INFINITY. So it is possible to do the same thing without risking forward compatibility. (I mean by "risking forward compatibility" that the mode bit 0x0040 may be defined differently by the upstream maintainer anytime in the future.) -- 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 Sat, Dec 18, 2010 at 05:16:52AM +0900, Kuwahara,T. wrote: > On 12/17/10, Richard Cochran <richardcochran@gmail.com> wrote: > > This patch adds a new mode bit into the timex structure. When set, the bit > > instructs the kernel to add the given time value to the current time. > > > > The proposed new control mode, ADJ_SETOFFSET, is logically the same as > ADJ_OFFSET with timex.constant == -INFINITY. So it is possible to do > the same thing without risking forward compatibility. (I mean by "risking > forward compatibility" that the mode bit 0x0040 may be defined differently > by the upstream maintainer anytime in the future.) Can you please elaborate? I don't see any way to use timex.constant with ADJ_OFFSET in order to correct a time offset. The 'time_constant' in kernel/time/ntp.c is restricted to the interval [0..MAXTC], and MAXTC is 10 in timex.h. Richard -- 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 Sat, 2010-12-18 at 05:16 +0900, Kuwahara,T. wrote: > On 12/17/10, Richard Cochran <richardcochran@gmail.com> wrote: > > This patch adds a new mode bit into the timex structure. When set, the bit > > instructs the kernel to add the given time value to the current time. > > > > The proposed new control mode, ADJ_SETOFFSET, is logically the same as > ADJ_OFFSET with timex.constant == -INFINITY. I'm not sure if this is correct. Its more like settimeofday, only giving a relative offset to jump the clock, rather then an absolute time. It does not slew the clock over time like ADJ_OFFSET does. > So it is possible to do > the same thing without risking forward compatibility. (I mean by "risking > forward compatibility" that the mode bit 0x0040 may be defined differently > by the upstream maintainer anytime in the future.) adjtimex is a linux specific interface, which is compatible but not identical to the ntp specified interfaces. The ntp client code already has Linux specific modifications, so I don't think we have to worry about 0x40 specifically being reserved by the NTP client. 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 Tue, Dec 21, 2010 at 4:56 PM, Richard Cochran <richardcochran@gmail.com> wrote: > Can you please elaborate? The timex.constant is defined as equal to the binary logarithm of the reciprocal of the natural frequency minus SHIFT_PLL. In other words, the following equation holds: log2(natural frequency) + time_constant + SHIFT_PLL = 0, which means that decreasing time_constant increases natural frequency exponentially. And since a larger natural frequency gives a smaller settling time, a sufficiently large negative time_constant results in immediate time step, at least in theory. > I don't see any way to use timex.constant with ADJ_OFFSET in order to > correct a time offset. How about this? if (txc->modes & ADJ_OFFSET) { if (txc->constant == INT32_MIN) { /* step time */ } else { /* slew time */ } } > The 'time_constant' in kernel/time/ntp.c is > restricted to the interval [0..MAXTC], and MAXTC is 10 in timex.h. Then let's just ignore the restriction. (It's possible by setting the timex.constant without setting the ADJ_TIMECONST flag.) That said, I'm somehow against the idea of using the adjtimex syscall for that purpose. -- 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, Dec 22, 2010 at 4:37 AM, john stultz <johnstul@us.ibm.com> wrote: > adjtimex is a linux specific interface, which is compatible but not > identical to the ntp specified interfaces. The ntp client code already > has Linux specific modifications, so I don't think we have to worry > about 0x40 specifically being reserved by the NTP client. But struct timex is not linux-specific... -- 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, 2010-12-22 at 06:13 +0900, Kuwahara,T. wrote: > On Wed, Dec 22, 2010 at 4:37 AM, john stultz <johnstul@us.ibm.com> wrote: > > adjtimex is a linux specific interface, which is compatible but not > > identical to the ntp specified interfaces. The ntp client code already > > has Linux specific modifications, so I don't think we have to worry > > about 0x40 specifically being reserved by the NTP client. > > But struct timex is not linux-specific... It is if you're compiling against linux's timex.h file. We already have a number of differences compared with BSD's timex mode definitions: We have ADJ_TICK: 0x4000, which is MOD_CLKB in FreeBSD. We also have ADJ_OFFSET_SINGLESHOT and ADJ_OFFSET_SS_READ which allow adjtimex act like the original ntp_adjtime. The key bit is that we map the shared MOD_* definitions that the NTP client uses to the linux specific ADJ_* values in the linux timex.h However, your concern does bring up a good point: 0x40 is MOD_PPSMAX in BSD, and we should at-least check to make sure that the PPS code that is currently floating around on the lists and is in akpm's tree hasn't already reserved that bit. Rodolfo, Alexander: Any comments here? 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, 2010-12-22 at 05:57 +0900, Kuwahara,T. wrote: > How about this? > > if (txc->modes & ADJ_OFFSET) { > if (txc->constant == INT32_MIN) { > /* step time */ > } else { > /* slew time */ > } > } This looks like magic behavior. Sort of a "knock twice and then say the password" interface. I don't see why that would be better then adding a clear new mode flag? > That said, I'm somehow against the idea of using the adjtimex syscall > for that purpose. Could you expand on why you don't like this? 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 Tue, Dec 21, 2010 at 01:59:44PM -0800, john stultz wrote: > However, your concern does bring up a good point: 0x40 is MOD_PPSMAX in > BSD, and we should at-least check to make sure that the PPS code that is > currently floating around on the lists and is in akpm's tree hasn't > already reserved that bit. If the concern is only about that bit, then we can choose another one from the range 0x0f00 instead. It seems to me that the ability to jump the clock is a very useful feature, at least when implementing a clock servo for PTP in user space, if not in other cases, too. I wonder why that option is missing from the interface in the first place. I am not aware of any serious attempt to get any kind of standardized PTP clock support into any other OS. As far as compatibility with NTP is concerned, why can't we let Linux set an example and let NTP/BSD/*nix follow us? Richard -- 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 Tue, Dec 21, 2010 at 02:25:55PM -0800, john stultz wrote: > On Wed, 2010-12-22 at 05:57 +0900, Kuwahara,T. wrote: > > > How about this? > > > > if (txc->modes & ADJ_OFFSET) { > > if (txc->constant == INT32_MIN) { > > /* step time */ > > } else { > > /* slew time */ > > } > > } > > This looks like magic behavior. Sort of a "knock twice and then say the > password" interface. I don't see why that would be better then adding a > clear new mode flag? I have to agree with John on this one. Looks very hacky to me. Richard -- 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
В Tue, 21 Dec 2010 13:59:44 -0800 john stultz <johnstul@us.ibm.com> пишет: > On Wed, 2010-12-22 at 06:13 +0900, Kuwahara,T. wrote: > > On Wed, Dec 22, 2010 at 4:37 AM, john stultz <johnstul@us.ibm.com> wrote: > > > adjtimex is a linux specific interface, which is compatible but not > > > identical to the ntp specified interfaces. The ntp client code already > > > has Linux specific modifications, so I don't think we have to worry > > > about 0x40 specifically being reserved by the NTP client. > > > > But struct timex is not linux-specific... > > It is if you're compiling against linux's timex.h file. > > We already have a number of differences compared with BSD's timex mode > definitions: > We have ADJ_TICK: 0x4000, which is MOD_CLKB in FreeBSD. > We also have ADJ_OFFSET_SINGLESHOT and ADJ_OFFSET_SS_READ which allow > adjtimex act like the original ntp_adjtime. > > The key bit is that we map the shared MOD_* definitions that the NTP > client uses to the linux specific ADJ_* values in the linux timex.h > > However, your concern does bring up a good point: 0x40 is MOD_PPSMAX in > BSD, and we should at-least check to make sure that the PPS code that is > currently floating around on the lists and is in akpm's tree hasn't > already reserved that bit. > > Rodolfo, Alexander: Any comments here? No, it is used neither in my patches nor in the original code. The only change my patches do to timex.h is adding hardpps().
On Wed, Dec 22, 2010 at 7:25 AM, john stultz <johnstul@us.ibm.com> wrote: > I don't see why that would be better then adding a > clear new mode flag? In short, time step is a special case of time slew. Those are the same, only different in one parameter, as is shown in my previous post. That's why I said there's no need for adding a new mode. -- 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, 2010-12-23 at 05:27 +0900, Kuwahara,T. wrote: > On Wed, Dec 22, 2010 at 7:25 AM, john stultz <johnstul@us.ibm.com> wrote: > > I don't see why that would be better then adding a > > clear new mode flag? > > In short, time step is a special case of time slew. Those are the same, > only different in one parameter, as is shown in my previous post. > That's why I said there's no need for adding a new mode. Sure, I get that a instantaneous offset adjustment could be represented as a instantaneous slew of the same amount. But what is the benefit of doing this? The ADJ_OFFSET isn't really a continuous function like you describe. For instance, you can't slew time backwards. The amount you can slow or speed up the clock is limited, so time will always increase. So to have a magic value way outside the bound of normal operation that does something special seems a bit obfuscated. ADJ_SETOFFSET isn't slewing the clock. So I think its much clearer to add a new mode, rather then to try to wedge the functionality into a corner case of ADJ_OFFSET slewing just because one could mathematically represent it the same way. I see the cost of adding it as a special case as adding extra complexity to an already complex subsystem. Doing things that make the code easier to understand and read makes it more maintainable. And I don't see the gain (other then saving a bit in the bit flag) to offset the complexity of trying to squeeze this functionality into the ADJ_OFFSET mode. 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 Thu, Dec 23, 2010 at 05:27:58AM +0900, Kuwahara,T. wrote: > On Wed, Dec 22, 2010 at 7:25 AM, john stultz <johnstul@us.ibm.com> wrote: > > I don't see why that would be better then adding a > > clear new mode flag? > > In short, time step is a special case of time slew. Those are the same, > only different in one parameter, as is shown in my previous post. > That's why I said there's no need for adding a new mode. Well, in addition to the objections raised by John, your suggested implementation is also shortsighted. The field timex.constant is copied into time_constant in some code paths. Obviously, this would be a bad thing when timex.constant==-huge. So, you need to clarify the interaction between ADJ_OFFSET, ADJ_TIMECONST, ADJ_TAI, timex.constant, time_constant, and MAXTC. If you would fully implement your idea, I expect it would become obvious that it a bit of a hack, both in the kernel code and in the user space interface. But, if you disagree, please just post a patch with the complete implementation... Thanks, Richard -- 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
After all, I'd prefer your earlier patchset. Leaving aside the compatibility issue, there's no particular reason we have to re-use the struct timex, which requires otherwise unnecessary conditional branches as well as unit conversions. Don't you agree? -- 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 Sun, Dec 26, 2010 at 05:38:57AM +0900, Kuwahara,T. wrote: > After all, I'd prefer your earlier patchset. Leaving aside the > compatibility issue, there's no particular reason we have to re-use > the struct timex, which requires otherwise unnecessary conditional > branches as well as unit conversions. Don't you agree? Well, from my point of view of wanting to allow a user space clock servo to be able to adjust a hardware clock, it would be sufficient to offer a way to jump the clock and to adjust the frequency via one or perhaps two new system calls. That is indeed what I first suggested, and I still think it would be a clean and simple interface. The NTP call is quite gross, since it multiplexes a whole bunch of different functions through the timex structure. However, several reviewers on the lkml prefered to keep with the NTP interface. It offers the needed functionality and is already well established, despite its ugliness. To me, the proposed ADJ_SETOFFSET is a simple and logical extenstion of the NTP interface. Also, the implementation is straightforward. In contrast, using a special -INF value seems a bit obtuse to me. Richard -- 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
diff --git a/include/linux/timex.h b/include/linux/timex.h index 32d852f..82d4b24 100644 --- a/include/linux/timex.h +++ b/include/linux/timex.h @@ -73,7 +73,7 @@ struct timex { long tolerance; /* clock frequency tolerance (ppm) * (read only) */ - struct timeval time; /* (read only) */ + struct timeval time; /* (read only, except for ADJ_SETOFFSET) */ long tick; /* (modified) usecs between clock ticks */ long ppsfreq; /* pps frequency (scaled ppm) (ro) */ @@ -101,6 +101,7 @@ struct timex { #define ADJ_ESTERROR 0x0008 /* estimated time error */ #define ADJ_STATUS 0x0010 /* clock status */ #define ADJ_TIMECONST 0x0020 /* pll time constant */ +#define ADJ_SETOFFSET 0x0040 /* add 'time' to current time */ #define ADJ_TAI 0x0080 /* set TAI offset */ #define ADJ_MICRO 0x1000 /* select microsecond resolution */ #define ADJ_NANO 0x2000 /* select nanosecond resolution */ diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c index d232189..e9e3915 100644 --- a/kernel/time/ntp.c +++ b/kernel/time/ntp.c @@ -454,6 +454,7 @@ static inline void process_adjtimex_modes(struct timex *txc, struct timespec *ts int do_adjtimex(struct timex *txc) { struct timespec ts; + ktime_t delta, kt; int result; /* Validate the data before disabling interrupts */ @@ -482,8 +483,33 @@ int do_adjtimex(struct timex *txc) hrtimer_cancel(&leap_timer); } + if (txc->modes & ADJ_SETOFFSET) { + /* Validate the delta value. */ + if (txc->time.tv_sec && txc->time.tv_usec < 0) + return -EINVAL; + + if (txc->modes & ADJ_NANO) { + struct timespec tmp; + tmp.tv_sec = txc->time.tv_sec; + tmp.tv_nsec = txc->time.tv_usec; + delta = timespec_to_ktime(tmp); + } else + delta = timeval_to_ktime(txc->time); + + /* Adding the delta should be an "atomic" operation. */ + local_irq_disable(); + } + getnstimeofday(&ts); + if (txc->modes & ADJ_SETOFFSET) { + kt = timespec_to_ktime(ts); + kt = ktime_add(kt, delta); + ts = ktime_to_timespec(kt); + do_settimeofday(&ts); + local_irq_enable(); + } + write_seqlock_irq(&xtime_lock); if (txc->modes & ADJ_ADJTIME) {
This patch adds a new mode bit into the timex structure. When set, the bit instructs the kernel to add the given time value to the current time. Signed-off-by: Richard Cochran <richard.cochran@omicron.at> --- include/linux/timex.h | 3 ++- kernel/time/ntp.c | 26 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletions(-)