Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Thu, 21 Feb 2008, john stultz wrote: > > Again, what kind of crappy hardware do you expect? Aren't clocks supposed > > to get better and not worse? > > Well, while I've seen much worse, I consider crappy hardware to be 100 > +ppm error. So if the hardware is perfect and the system results in > 153ppm error, I'd consider that pretty crappy, especially if its not the > hardware's fault. Nevertheless this error is real, why are you trying to hide it? This is isn't an error we can't handle, it's still perfectly within the limit and except that NTP reports a somewhat larger drift than you'd like to see, everything works fine. > > Where do you get this idea that the 500ppm are exclusively for hardware > > errors? If you have such bad hardware, there is another simple solution: > > change HZ to 100 and the error is reduced to 15ppm. > > True its not exclusively for hardware errors, and if we were talking > about only 15ppm I wouldn't really worry about it. But when we're saying > the system is adding 30% of the maximum error, that's just not good. Another 30% is required for normal to crappy hardware clocks and then there is still enough room left. > > I would see the point if this problem had actually any practically > > relevance, but this error is not a problem for pretty much all existing > > standard hardware. Why are you insisting on redesigning timekeeping for > > broken hardware? > > Remember my earlier data? Where I was talking about the acpi_pm being a > multiple of the PIT frequency? By removing CLOCK_TICK_ADJUST we got a > 127ppm error when HZ=1000. NO_HZ drops that down to where we don't care, > but this _does_ effect current hardware, so I'd call it relevant. How exactly does it effect current hardware in a way that it breaks them? Despite this error everything still works fine, the hardware doesn't care. > > There's nothing 'injected', that resolution error is very real and the > > 500ppm limit is more than enough to deal with this. _Nobody_ is hurt by > > this. > > Sure, 500ppm is enough for most people with good hardware. But remember > the alpha example you brought up earlier? The HZ=1200 case, with the > CLOCK_TICK_RATE=32768? If we don't take CLOCK_TICK_ADJUST into account, > we end up with a **11230ppm** error from the granularity issue. NTP just > won't work on those systems. > > Now granted, the three types of alpha systems that actually use that HZ > value is probably as close to "nobody" as you're going to get, but I > don't think we can just throw the granularity issue aside. That's actually a good example, why it's irrelevant. First it's using a cycle based clock, thus the rounding error is irrelevant. Second in the common case they already use 1024 as HZ to reduce this error, so something similiar could be done for the HZ=1200 case and I suspect that it was already done and only CLOCK_TICK_RATE is just wrong. This mail http://consortiumlibrary.org/axp-list/archive/2002-11/0101.html suggest that this is the right thing to do. There is _no_ reason to artificially optimize this error value, there are still enough other ways to improve timekeeping. The granularity error is there no matter what you do and as long as it's within a reasonable limit there is nothing that needs fixing. bye, Roman -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Thu, 21 Feb 2008, john stultz wrote: Again, what kind of crappy hardware do you expect? Aren't clocks supposed to get better and not worse? Well, while I've seen much worse, I consider crappy hardware to be 100 +ppm error. So if the hardware is perfect and the system results in 153ppm error, I'd consider that pretty crappy, especially if its not the hardware's fault. Nevertheless this error is real, why are you trying to hide it? This is isn't an error we can't handle, it's still perfectly within the limit and except that NTP reports a somewhat larger drift than you'd like to see, everything works fine. Where do you get this idea that the 500ppm are exclusively for hardware errors? If you have such bad hardware, there is another simple solution: change HZ to 100 and the error is reduced to 15ppm. True its not exclusively for hardware errors, and if we were talking about only 15ppm I wouldn't really worry about it. But when we're saying the system is adding 30% of the maximum error, that's just not good. Another 30% is required for normal to crappy hardware clocks and then there is still enough room left. I would see the point if this problem had actually any practically relevance, but this error is not a problem for pretty much all existing standard hardware. Why are you insisting on redesigning timekeeping for broken hardware? Remember my earlier data? Where I was talking about the acpi_pm being a multiple of the PIT frequency? By removing CLOCK_TICK_ADJUST we got a 127ppm error when HZ=1000. NO_HZ drops that down to where we don't care, but this _does_ effect current hardware, so I'd call it relevant. How exactly does it effect current hardware in a way that it breaks them? Despite this error everything still works fine, the hardware doesn't care. There's nothing 'injected', that resolution error is very real and the 500ppm limit is more than enough to deal with this. _Nobody_ is hurt by this. Sure, 500ppm is enough for most people with good hardware. But remember the alpha example you brought up earlier? The HZ=1200 case, with the CLOCK_TICK_RATE=32768? If we don't take CLOCK_TICK_ADJUST into account, we end up with a **11230ppm** error from the granularity issue. NTP just won't work on those systems. Now granted, the three types of alpha systems that actually use that HZ value is probably as close to nobody as you're going to get, but I don't think we can just throw the granularity issue aside. That's actually a good example, why it's irrelevant. First it's using a cycle based clock, thus the rounding error is irrelevant. Second in the common case they already use 1024 as HZ to reduce this error, so something similiar could be done for the HZ=1200 case and I suspect that it was already done and only CLOCK_TICK_RATE is just wrong. This mail http://consortiumlibrary.org/axp-list/archive/2002-11/0101.html suggest that this is the right thing to do. There is _no_ reason to artificially optimize this error value, there are still enough other ways to improve timekeeping. The granularity error is there no matter what you do and as long as it's within a reasonable limit there is nothing that needs fixing. bye, Roman -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Wed, 2008-02-20 at 18:08 +0100, Roman Zippel wrote: > > Well, it is a problem if its large. The 500ppm limit is supposed to be > > for hardware frequency error correction, not hardware frequency + > > software error correction. Now, if it were 1-10ppm, it wouldn't be that > > big of an issue, but with the jiffies example above, 153ppm does cut > > into the correctable space a good bit. > > Again, what kind of crappy hardware do you expect? Aren't clocks supposed > to get better and not worse? Well, while I've seen much worse, I consider crappy hardware to be 100 +ppm error. So if the hardware is perfect and the system results in 153ppm error, I'd consider that pretty crappy, especially if its not the hardware's fault. > Where do you get this idea that the 500ppm are exclusively for hardware > errors? If you have such bad hardware, there is another simple solution: > change HZ to 100 and the error is reduced to 15ppm. True its not exclusively for hardware errors, and if we were talking about only 15ppm I wouldn't really worry about it. But when we're saying the system is adding 30% of the maximum error, that's just not good. > I would see the point if this problem had actually any practically > relevance, but this error is not a problem for pretty much all existing > standard hardware. Why are you insisting on redesigning timekeeping for > broken hardware? Remember my earlier data? Where I was talking about the acpi_pm being a multiple of the PIT frequency? By removing CLOCK_TICK_ADJUST we got a 127ppm error when HZ=1000. NO_HZ drops that down to where we don't care, but this _does_ effect current hardware, so I'd call it relevant. > > > > My understanding of your approach (removing CLOCK_TICK_ADJUST), > > > > addresses issues #1 and #3, but hurts issue #2. > > > > > > What exactly is hurt? > > > > By injecting 153ppm of error, the ability for NTP to correct hardware > > error within 500ppm is hurt. > > There's nothing 'injected', that resolution error is very real and the > 500ppm limit is more than enough to deal with this. _Nobody_ is hurt by > this. Sure, 500ppm is enough for most people with good hardware. But remember the alpha example you brought up earlier? The HZ=1200 case, with the CLOCK_TICK_RATE=32768? If we don't take CLOCK_TICK_ADJUST into account, we end up with a **11230ppm** error from the granularity issue. NTP just won't work on those systems. Now granted, the three types of alpha systems that actually use that HZ value is probably as close to "nobody" as you're going to get, but I don't think we can just throw the granularity issue aside. > Revert bbe4d18ac2e058c56adb0cd71f49d9ed3216a405 and > e13a2e61dd5152f5499d2003470acf9c838eab84 and remove CLOCK_TICK_ADJUST > completely. Add a optional kernel parameter ntp_tick_adj instead to allow > adjusting of a large base drift and thus keeping ntpd happy. > The CLOCK_TICK_ADJUST mechanism was introduced at a time PIT was the > primary clock, but we have a varity of clock sources now, so a global PIT > specific adjustment makes little sense anymore. > > Signed-off-by: Roman Zippel <[EMAIL PROTECTED]> So thanks so much for sending the patch. It makes clear your solution. My initial comments: Its simple and that really does have its merits. It resolves the inconsistent comparison issue, and does not have the smallish scaling error we talked about as well. As I've said before, I do like the idea, I'm just worried about the corner cases (mainly jiffies based systems). The granularity issue is still present. Depending on the HZ settings and the clocksource hardware, systems may see large errors added on to the actual hardware error, and its possible the kernel error may dominate the actual hardware error. The ntp_tick_adjust option does give a way out if you have, for example, one of those alpha systems where it would be necessary, but I do wish there was a better way then forcing users to calculate for themselves what the granularity adjustment should be (esp given that it is more a function of the kernel compile options, so different kernels would need different values for the same system). So then yes, your patch is simple and corrects the issue that started the discussion. I think we're closing the gaps. :) I still think my claims hold that your patch as it stands may worsen the drift error depending on HZ settings, especially on jiffies based systems (which means every non-GENERIC_TIME arch). However, if folks don't really care, then that may be acceptable. As promised, here is my own patch, which takes the scaling error you pointed out into account, as well as resolving the granularity issue in a way similar to your ntp_tick_adjust option does, only the kernel will calculate such a granularity correction on a per-clocksource base, so users don't have to do the math. Sadly I've not had the chance to really test and debug this (there's a lot of shifting logic, so I may have flubed something there), but I
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Wed, 2008-02-20 at 18:08 +0100, Roman Zippel wrote: Well, it is a problem if its large. The 500ppm limit is supposed to be for hardware frequency error correction, not hardware frequency + software error correction. Now, if it were 1-10ppm, it wouldn't be that big of an issue, but with the jiffies example above, 153ppm does cut into the correctable space a good bit. Again, what kind of crappy hardware do you expect? Aren't clocks supposed to get better and not worse? Well, while I've seen much worse, I consider crappy hardware to be 100 +ppm error. So if the hardware is perfect and the system results in 153ppm error, I'd consider that pretty crappy, especially if its not the hardware's fault. Where do you get this idea that the 500ppm are exclusively for hardware errors? If you have such bad hardware, there is another simple solution: change HZ to 100 and the error is reduced to 15ppm. True its not exclusively for hardware errors, and if we were talking about only 15ppm I wouldn't really worry about it. But when we're saying the system is adding 30% of the maximum error, that's just not good. I would see the point if this problem had actually any practically relevance, but this error is not a problem for pretty much all existing standard hardware. Why are you insisting on redesigning timekeeping for broken hardware? Remember my earlier data? Where I was talking about the acpi_pm being a multiple of the PIT frequency? By removing CLOCK_TICK_ADJUST we got a 127ppm error when HZ=1000. NO_HZ drops that down to where we don't care, but this _does_ effect current hardware, so I'd call it relevant. My understanding of your approach (removing CLOCK_TICK_ADJUST), addresses issues #1 and #3, but hurts issue #2. What exactly is hurt? By injecting 153ppm of error, the ability for NTP to correct hardware error within 500ppm is hurt. There's nothing 'injected', that resolution error is very real and the 500ppm limit is more than enough to deal with this. _Nobody_ is hurt by this. Sure, 500ppm is enough for most people with good hardware. But remember the alpha example you brought up earlier? The HZ=1200 case, with the CLOCK_TICK_RATE=32768? If we don't take CLOCK_TICK_ADJUST into account, we end up with a **11230ppm** error from the granularity issue. NTP just won't work on those systems. Now granted, the three types of alpha systems that actually use that HZ value is probably as close to nobody as you're going to get, but I don't think we can just throw the granularity issue aside. Revert bbe4d18ac2e058c56adb0cd71f49d9ed3216a405 and e13a2e61dd5152f5499d2003470acf9c838eab84 and remove CLOCK_TICK_ADJUST completely. Add a optional kernel parameter ntp_tick_adj instead to allow adjusting of a large base drift and thus keeping ntpd happy. The CLOCK_TICK_ADJUST mechanism was introduced at a time PIT was the primary clock, but we have a varity of clock sources now, so a global PIT specific adjustment makes little sense anymore. Signed-off-by: Roman Zippel [EMAIL PROTECTED] So thanks so much for sending the patch. It makes clear your solution. My initial comments: Its simple and that really does have its merits. It resolves the inconsistent comparison issue, and does not have the smallish scaling error we talked about as well. As I've said before, I do like the idea, I'm just worried about the corner cases (mainly jiffies based systems). The granularity issue is still present. Depending on the HZ settings and the clocksource hardware, systems may see large errors added on to the actual hardware error, and its possible the kernel error may dominate the actual hardware error. The ntp_tick_adjust option does give a way out if you have, for example, one of those alpha systems where it would be necessary, but I do wish there was a better way then forcing users to calculate for themselves what the granularity adjustment should be (esp given that it is more a function of the kernel compile options, so different kernels would need different values for the same system). So then yes, your patch is simple and corrects the issue that started the discussion. I think we're closing the gaps. :) I still think my claims hold that your patch as it stands may worsen the drift error depending on HZ settings, especially on jiffies based systems (which means every non-GENERIC_TIME arch). However, if folks don't really care, then that may be acceptable. As promised, here is my own patch, which takes the scaling error you pointed out into account, as well as resolving the granularity issue in a way similar to your ntp_tick_adjust option does, only the kernel will calculate such a granularity correction on a per-clocksource base, so users don't have to do the math. Sadly I've not had the chance to really test and debug this (there's a lot of shifting logic, so I may have flubed something there), but I wanted to send it out for comments, as I think it
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Tue, 19 Feb 2008, john stultz wrote: > To better keep with your analogy, you'd have to imagine a scale that > only reads in X pound increments. As long as X is fairly small, it > should measure everyone's weight fairly well. However, if X is large, > like say 50kg, then it won't weigh a 70kg person very accurately (even > if he is a liar and he really weighs 77kgs). > > > This is the granularity error I'm talking about. There is a big difference between accuracy and granularity. Even a coarse grained scale can be accurate (just within its resolution) and a fine grained scale can be very inaccurate if you shift around the scale. Please keep this separate, otherwise this can go on forever... > Now, when NTP starts up, if we had perfect hardware and there was no > hardware drift, NTP would have to inject a -153ppm correction to offset > the systematic error we've introduced. > > If we do not have perfect hardware, then NTP would have to correct for > both the 153ppm error and the hardware error. Sp if the hardware error > was in the same direction, we can now only compensate for up to a 347ppm > hardware drift, before we hit the 500ppm bound in NTP. Out of curiosity, what kind of hardware error do you expect? If you have that crappy hardware you're better off initializing the clock with the correct frequency. > > To keep in mind what time adjusting is supposed to do: > > > > freq = 1sec + time_freq > > But it is this fixation on 1sec that is the cause of the granularity > error. You need some kind of fix point and everyone is using 1sec for as base length, for some mysterious reason you're now trying to redefine it. The PIT (and any clock based on it) has a certain resolution, so with an update frequency of HZ=1000 it produces a certain error, but why on earth are you trying to introduce this error as universal constant? This is a property of the PIT clock, applying this error to every other clock makes no sense. > I believe the following is also correct (assuming time_freq is in ppm units): > > adjusted_freq = interval_length + (interval_length * time_freq)/MILLION > > This properly scales the adjustment to any interval length. Actually it's not that simple, ntp_update_frequency() only calculates the base length, time_offset had to be scaled as well (and back when requested from user space). You would add a lot of complexity for a silly little error, which the current mechanisms can handle just fine. > > What we do instead is: > > > > freq + tick_adj = 1sec + time_freq > > > > Where exactly is now the problem to integrate tick_adj into time_freq? The > > result is _exactly_ the same. The only visible difference is a slightly > > higher time_freq value and as long as it is within the 500 ppm limit there > > is simply no problem. > > Well, it is a problem if its large. The 500ppm limit is supposed to be > for hardware frequency error correction, not hardware frequency + > software error correction. Now, if it were 1-10ppm, it wouldn't be that > big of an issue, but with the jiffies example above, 153ppm does cut > into the correctable space a good bit. Again, what kind of crappy hardware do you expect? Aren't clocks supposed to get better and not worse? Where do you get this idea that the 500ppm are exclusively for hardware errors? If you have such bad hardware, there is another simple solution: change HZ to 100 and the error is reduced to 15ppm. I would see the point if this problem had actually any practically relevance, but this error is not a problem for pretty much all existing standard hardware. Why are you insisting on redesigning timekeeping for broken hardware? > > > My understanding of your approach (removing CLOCK_TICK_ADJUST), > > > addresses issues #1 and #3, but hurts issue #2. > > > > What exactly is hurt? > > By injecting 153ppm of error, the ability for NTP to correct hardware > error within 500ppm is hurt. There's nothing 'injected', that resolution error is very real and the 500ppm limit is more than enough to deal with this. _Nobody_ is hurt by this. > Sigh. So at this point, if we're not closing the gap in our > understanding, I'm not sure how much its worth to continue on the > discussion in this manner. I'd welcome anyone to help clarify what I'm > missing, or maybe assistance in better communicating my point. The point is you are redefining a mechanism which has _never_ been intendend for purpose you're trying to abuse it for now. Reread the original patch, it was intended for kernel with HZ of 2000, where the error would be 687ppm. Now we have other ways to increase the resolution, timekeeping isn't solely based on the PIT anymore. The whole reason for the original patch is pretty much gone by now. If you really need some kind of adjustment for your extremely broken hardware, below is the absolute maximum you need, which doesn't inflict more insanity on all the sane hardware. bye, Roman Revert
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Tue, 19 Feb 2008, john stultz wrote: To better keep with your analogy, you'd have to imagine a scale that only reads in X pound increments. As long as X is fairly small, it should measure everyone's weight fairly well. However, if X is large, like say 50kg, then it won't weigh a 70kg person very accurately (even if he is a liar and he really weighs 77kgs). This is the granularity error I'm talking about. There is a big difference between accuracy and granularity. Even a coarse grained scale can be accurate (just within its resolution) and a fine grained scale can be very inaccurate if you shift around the scale. Please keep this separate, otherwise this can go on forever... Now, when NTP starts up, if we had perfect hardware and there was no hardware drift, NTP would have to inject a -153ppm correction to offset the systematic error we've introduced. If we do not have perfect hardware, then NTP would have to correct for both the 153ppm error and the hardware error. Sp if the hardware error was in the same direction, we can now only compensate for up to a 347ppm hardware drift, before we hit the 500ppm bound in NTP. Out of curiosity, what kind of hardware error do you expect? If you have that crappy hardware you're better off initializing the clock with the correct frequency. To keep in mind what time adjusting is supposed to do: freq = 1sec + time_freq But it is this fixation on 1sec that is the cause of the granularity error. You need some kind of fix point and everyone is using 1sec for as base length, for some mysterious reason you're now trying to redefine it. The PIT (and any clock based on it) has a certain resolution, so with an update frequency of HZ=1000 it produces a certain error, but why on earth are you trying to introduce this error as universal constant? This is a property of the PIT clock, applying this error to every other clock makes no sense. I believe the following is also correct (assuming time_freq is in ppm units): adjusted_freq = interval_length + (interval_length * time_freq)/MILLION This properly scales the adjustment to any interval length. Actually it's not that simple, ntp_update_frequency() only calculates the base length, time_offset had to be scaled as well (and back when requested from user space). You would add a lot of complexity for a silly little error, which the current mechanisms can handle just fine. What we do instead is: freq + tick_adj = 1sec + time_freq Where exactly is now the problem to integrate tick_adj into time_freq? The result is _exactly_ the same. The only visible difference is a slightly higher time_freq value and as long as it is within the 500 ppm limit there is simply no problem. Well, it is a problem if its large. The 500ppm limit is supposed to be for hardware frequency error correction, not hardware frequency + software error correction. Now, if it were 1-10ppm, it wouldn't be that big of an issue, but with the jiffies example above, 153ppm does cut into the correctable space a good bit. Again, what kind of crappy hardware do you expect? Aren't clocks supposed to get better and not worse? Where do you get this idea that the 500ppm are exclusively for hardware errors? If you have such bad hardware, there is another simple solution: change HZ to 100 and the error is reduced to 15ppm. I would see the point if this problem had actually any practically relevance, but this error is not a problem for pretty much all existing standard hardware. Why are you insisting on redesigning timekeeping for broken hardware? My understanding of your approach (removing CLOCK_TICK_ADJUST), addresses issues #1 and #3, but hurts issue #2. What exactly is hurt? By injecting 153ppm of error, the ability for NTP to correct hardware error within 500ppm is hurt. There's nothing 'injected', that resolution error is very real and the 500ppm limit is more than enough to deal with this. _Nobody_ is hurt by this. Sigh. So at this point, if we're not closing the gap in our understanding, I'm not sure how much its worth to continue on the discussion in this manner. I'd welcome anyone to help clarify what I'm missing, or maybe assistance in better communicating my point. The point is you are redefining a mechanism which has _never_ been intendend for purpose you're trying to abuse it for now. Reread the original patch, it was intended for kernel with HZ of 2000, where the error would be 687ppm. Now we have other ways to increase the resolution, timekeeping isn't solely based on the PIT anymore. The whole reason for the original patch is pretty much gone by now. If you really need some kind of adjustment for your extremely broken hardware, below is the absolute maximum you need, which doesn't inflict more insanity on all the sane hardware. bye, Roman Revert bbe4d18ac2e058c56adb0cd71f49d9ed3216a405 and
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Tue, 2008-02-19 at 05:04 +0100, Roman Zippel wrote: > Hi, > > On Mon, 18 Feb 2008, john stultz wrote: > > > If we are building a train station, and each train car is 60ft, it > > doesn't make much sense to build 1000ft stations, right? Instead you'll > > do better if you build a 1020ft station. > > That would assume trains are always 60ft long, which is the basic error in > your assumption. > > Since we're using analogies: What you're doing is to put you winter > clothes on your weight scale and reset the scale to zero to offset for the > weigth of the clothes. If you stand now with your bathing clothes on the > scale, does that mean you lost weight? Sheesh, now you're calling me deceiving *and* fat! ;) > That's all you do - you change the scale and slightly screw the scale for > everyone else trying to use it. I don't think so. This all has to do with intervals, not scale offsets. I'm having a hard time seeing how we're not connecting on this. To better keep with your analogy, you'd have to imagine a scale that only reads in X pound increments. As long as X is fairly small, it should measure everyone's weight fairly well. However, if X is large, like say 50kg, then it won't weigh a 70kg person very accurately (even if he is a liar and he really weighs 77kgs). This is the granularity error I'm talking about. Again, this all has to do with our accumulation intervals. Lets look at the code. I'm going to focus on jiffies as its the best example, being both very common and very course. Let's say we make our base interval 1,000,000ns. Using the jiffies clocksource, the closest we can get to this interval is 1 jiffy, which is 999,847ns. Now every interval we will see 153ns error. The clocksource_adjust() function will watch this error accumulate and adjust the jiffies's frequency by 153ppm to try to compensate. Note, this compensation is not adjusting for any hardware drift, it is only compensating for the error we've injected by choosing an interval the clocksource cannot match. Now, when NTP starts up, if we had perfect hardware and there was no hardware drift, NTP would have to inject a -153ppm correction to offset the systematic error we've introduced. If we do not have perfect hardware, then NTP would have to correct for both the 153ppm error and the hardware error. Sp if the hardware error was in the same direction, we can now only compensate for up to a 347ppm hardware drift, before we hit the 500ppm bound in NTP. Using the test program I sent out before: HZ=1000 CLOCK_TICK_ADJUST=0 jiffies == 1 cycles per 100 ns 999847 ns per cycle 999847 ns per interval 153000 ppb error jiffies NOHZ == 500 cycles per 5 ns 999847 ns per cycle 499923500 ns per interval 153000 ppb error Now, if we make our base interval match the clocksource granularity, this can avoid the error. If the base interval we accumulate with is 999,847ns, we avoid needlessly accumulating error. This means without NTP correction, the system clock drift will match the hardware clock drift. So when NTP does kick in, it only has to correct for that hardware drift. HZ=1000 CLOCK_TICK_ADJUST=-152533 jiffies == 1 cycles per 999847 ns 999847 ns per cycle 999847 ns per interval 0 ppb error jiffies NOHZ == 500 cycles per 499923733 ns 999847 ns per cycle 499923500 ns per interval 466 ppb error > To keep in mind what time adjusting is supposed to do: > > freq = 1sec + time_freq But it is this fixation on 1sec that is the cause of the granularity error. I believe the following is also correct (assuming time_freq is in ppm units): adjusted_freq = interval_length + (interval_length * time_freq)/MILLION This properly scales the adjustment to any interval length. > What we do instead is: > > freq + tick_adj = 1sec + time_freq > > Where exactly is now the problem to integrate tick_adj into time_freq? The > result is _exactly_ the same. The only visible difference is a slightly > higher time_freq value and as long as it is within the 500 ppm limit there > is simply no problem. Well, it is a problem if its large. The 500ppm limit is supposed to be for hardware frequency error correction, not hardware frequency + software error correction. Now, if it were 1-10ppm, it wouldn't be that big of an issue, but with the jiffies example above, 153ppm does cut into the correctable space a good bit. Show me how we cut that error down, and I'll be happy to kill CLOCK_TICK_ADJUST. > > And yes, if we remove CLOCK_TICK_ADJUST, that would also resolve the > > (A!=B) issue, but it doesn't address the error from #2 below. > > [..] > > 2) We need a solution that handles granularity error well, as this is a > > moderate source of error for course clocksources such as jiffies. > > CLOCK_TICK_ADJUST does cover this fairly well in most cases. I suspect > > we could do even better, but that will take some deeper changes. > > How exactly does CLOCK_TICK_ADJUST address
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Tue, 2008-02-19 at 05:04 +0100, Roman Zippel wrote: Hi, On Mon, 18 Feb 2008, john stultz wrote: If we are building a train station, and each train car is 60ft, it doesn't make much sense to build 1000ft stations, right? Instead you'll do better if you build a 1020ft station. That would assume trains are always 60ft long, which is the basic error in your assumption. Since we're using analogies: What you're doing is to put you winter clothes on your weight scale and reset the scale to zero to offset for the weigth of the clothes. If you stand now with your bathing clothes on the scale, does that mean you lost weight? Sheesh, now you're calling me deceiving *and* fat! ;) That's all you do - you change the scale and slightly screw the scale for everyone else trying to use it. I don't think so. This all has to do with intervals, not scale offsets. I'm having a hard time seeing how we're not connecting on this. To better keep with your analogy, you'd have to imagine a scale that only reads in X pound increments. As long as X is fairly small, it should measure everyone's weight fairly well. However, if X is large, like say 50kg, then it won't weigh a 70kg person very accurately (even if he is a liar and he really weighs 77kgs). This is the granularity error I'm talking about. Again, this all has to do with our accumulation intervals. Lets look at the code. I'm going to focus on jiffies as its the best example, being both very common and very course. Let's say we make our base interval 1,000,000ns. Using the jiffies clocksource, the closest we can get to this interval is 1 jiffy, which is 999,847ns. Now every interval we will see 153ns error. The clocksource_adjust() function will watch this error accumulate and adjust the jiffies's frequency by 153ppm to try to compensate. Note, this compensation is not adjusting for any hardware drift, it is only compensating for the error we've injected by choosing an interval the clocksource cannot match. Now, when NTP starts up, if we had perfect hardware and there was no hardware drift, NTP would have to inject a -153ppm correction to offset the systematic error we've introduced. If we do not have perfect hardware, then NTP would have to correct for both the 153ppm error and the hardware error. Sp if the hardware error was in the same direction, we can now only compensate for up to a 347ppm hardware drift, before we hit the 500ppm bound in NTP. Using the test program I sent out before: HZ=1000 CLOCK_TICK_ADJUST=0 jiffies == 1 cycles per 100 ns 999847 ns per cycle 999847 ns per interval 153000 ppb error jiffies NOHZ == 500 cycles per 5 ns 999847 ns per cycle 499923500 ns per interval 153000 ppb error Now, if we make our base interval match the clocksource granularity, this can avoid the error. If the base interval we accumulate with is 999,847ns, we avoid needlessly accumulating error. This means without NTP correction, the system clock drift will match the hardware clock drift. So when NTP does kick in, it only has to correct for that hardware drift. HZ=1000 CLOCK_TICK_ADJUST=-152533 jiffies == 1 cycles per 999847 ns 999847 ns per cycle 999847 ns per interval 0 ppb error jiffies NOHZ == 500 cycles per 499923733 ns 999847 ns per cycle 499923500 ns per interval 466 ppb error To keep in mind what time adjusting is supposed to do: freq = 1sec + time_freq But it is this fixation on 1sec that is the cause of the granularity error. I believe the following is also correct (assuming time_freq is in ppm units): adjusted_freq = interval_length + (interval_length * time_freq)/MILLION This properly scales the adjustment to any interval length. What we do instead is: freq + tick_adj = 1sec + time_freq Where exactly is now the problem to integrate tick_adj into time_freq? The result is _exactly_ the same. The only visible difference is a slightly higher time_freq value and as long as it is within the 500 ppm limit there is simply no problem. Well, it is a problem if its large. The 500ppm limit is supposed to be for hardware frequency error correction, not hardware frequency + software error correction. Now, if it were 1-10ppm, it wouldn't be that big of an issue, but with the jiffies example above, 153ppm does cut into the correctable space a good bit. Show me how we cut that error down, and I'll be happy to kill CLOCK_TICK_ADJUST. And yes, if we remove CLOCK_TICK_ADJUST, that would also resolve the (A!=B) issue, but it doesn't address the error from #2 below. [..] 2) We need a solution that handles granularity error well, as this is a moderate source of error for course clocksources such as jiffies. CLOCK_TICK_ADJUST does cover this fairly well in most cases. I suspect we could do even better, but that will take some deeper changes. How exactly does CLOCK_TICK_ADJUST address this problem? The error due to insufficient
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Mon, 18 Feb 2008, john stultz wrote: > If we are building a train station, and each train car is 60ft, it > doesn't make much sense to build 1000ft stations, right? Instead you'll > do better if you build a 1020ft station. That would assume trains are always 60ft long, which is the basic error in your assumption. Since we're using analogies: What you're doing is to put you winter clothes on your weight scale and reset the scale to zero to offset for the weigth of the clothes. If you stand now with your bathing clothes on the scale, does that mean you lost weight? That's all you do - you change the scale and slightly screw the scale for everyone else trying to use it. To keep in mind what time adjusting is supposed to do: freq = 1sec + time_freq What we do instead is: freq + tick_adj = 1sec + time_freq Where exactly is now the problem to integrate tick_adj into time_freq? The result is _exactly_ the same. The only visible difference is a slightly higher time_freq value and as long as it is within the 500 ppm limit there is simply no problem. > And yes, if we remove CLOCK_TICK_ADJUST, that would also resolve the > (A!=B) issue, but it doesn't address the error from #2 below. > [..] > 2) We need a solution that handles granularity error well, as this is a > moderate source of error for course clocksources such as jiffies. > CLOCK_TICK_ADJUST does cover this fairly well in most cases. I suspect > we could do even better, but that will take some deeper changes. How exactly does CLOCK_TICK_ADJUST address this problem? The error due to insufficient resolution is still there, all it does is shifting the scale, so it's not immediately visible. > My understanding of your approach (removing CLOCK_TICK_ADJUST), > addresses issues #1 and #3, but hurts issue #2. What exactly is hurt? bye, Roman -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Sat, 2008-02-16 at 05:24 +0100, Roman Zippel wrote: > Hi, > > On Wed, 13 Feb 2008, john stultz wrote: > > > Oh! So your issue is that since time_freq is stored in ppm, or in effect > > a usecs per sec offset, when we add it to something other then 1 second > > we mis-apply what NTP tells us to. Is that right? > > Pretty much everything is centered around that 1sec, so the closer the > update frequency is to it the better. > > > Right, so if NTP has us apply a 10ppm adjustment, instead of doing: > > NSEC_PER_SEC + 10,000 > > > > We're doing: > > NSEC_PER_SEC + CLOCK_TICK_ADJUST + 10,000 > > > > Which, if I'm doing my math right, results in a 10.002ppm adjustment > > (using the 999847467ns number above), instead of just a 10ppm > > adjustment. > > > > Now, true, this is an error, but it is a pretty small one. Even at the > > maximum 500ppm value, it only results in an error of 76 parts per > > billion. As you'll see below, that tends to be less error then what we > > get from the clock granularity. Is there something else I'm missing here > > or is this really the core issue you're concerned with? > > The error accumulates and there is no good reason to do this for the > common case. I agree, but we can also easily resolve this by scaling the ppm adjustment to the interval length, rather then just applying it as usec/sec. No? So instead of: adjusted_length = NSEC_PER_SEC + time_adj We could do: adjusted_length = ntp_interval_length + (ntp_interval_length * time_adj)/MILLION So this fixes the time_adj scaling error, while letting us be more flexible w/ our interval length, so we can better map the requested length to the clocksource granularity, lessening the granularity error. > > HZ=1000 CLOCK_TICK_ADJUST=-152533 > > jiffies 467 ppb error > > jiffies NOHZ467 ppb error > > pit 0 ppb error > > pit NOHZ0 ppb error > > acpi_pm -280 ppb error > > acpi_pm NOHZ279 ppb error > > > > HZ=1000 CLOCK_TICK_ADJUST=0 > > jiffies 153000 ppb error > > jiffies NOHZ153000 ppb error > > pit 152533 ppb error > > pit NOHZ0 ppb error > > acpi_pm -127112 ppb error > > acpi_pm NOHZ279 ppb error > > > > So you are right, w/ pit & NO_HZ, the granularity error is always very > > small both with or without CLOCK_TICK_ADJUST. > > If you change the frequency of acpi_pm to 3579000 you'll get this: > > HZ=1000 CLOCK_TICK_ADJUST=0 > jiffies 153000 ppb error > jiffies NOHZ153000 ppb error > pit 152533 ppb error > pit NOHZ0 ppb error > acpi_pm 0 ppb error > acpi_pm NOHZ0 ppb error > > HZ=1000 CLOCK_TICK_ADJUST=-152533 > jiffies 0 ppb error > jiffies NOHZ466 ppb error > pit -467 ppb error > pit NOHZ-1 ppb error > acpi_pm 126407 ppb error > acpi_pm NOHZ22 ppb error Right, but the acpi_pm's frequency isn't 3579000. It's supposed to be 3579545. That is injecting error, and I believe it to be different then what I'm claiming is achieved by CLOCK_TICK_ADJUST. Further, the specific example above was agreeing with you that that CLOCK_TICK_ADJUST=0 looks ok for NOHZ, with the exception in the case of the jiffies clocksource. That one still has the 153ppm drift. What do we do about that? > CLOCK_TICK_ADJUST has only any meaning for PIT (and indirectly for > jiffies). For every other clock you just add some random value, where > it doesn't do _any_ good. While not the main point, the aside I included about the acpi_pm being interesting, is that because the ACPI PM's frequency is a multiple of the PIT's (and thus jiffies), the same granularity issues arise (although to a lesser degree). That is why CLOCK_TICK_ADJUST helps it in the !NOHZ case. > The worst case error there will always be (ntp_hz/freq/2*10^9nsec), all > you do with CLOCK_TICK_ADJUST is to do shift it around, but it doesn't > actually fix the error - it's still there. > > > However, without CLOCK_TICK_ADJUST, the jiffies error increases for all > > values of HZ except 100 (which at first seems odd, but seems to be due > > to loss from rounding in the ACTHZ calculation). > > jiffies depends on the timer resolution, so it will practically produce > the same results as PIT (assuming it's used to generate the timer tick). > > > One interesting surprise in the data: With CLOCK_TICK_ADJUST=0, the > > acpi_pm's error frequency shot up in the !NO_HZ cases. This ends up > > being due to the acpi_pm being a very close to a multiple (3x) of the > > pit frequency, so CLOCK_TICK_ADJUST helps it as well. > > What exactly does it help with? > All you are doing is number cosmetics, it has _no_ practically value and > only decreases the quality of timekeeping. Unless you want to clarify what aspect of "quality" you're talking about, I'd very much disagree with your claim. This is not cosmetics, this is about
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Sat, 2008-02-16 at 05:24 +0100, Roman Zippel wrote: Hi, On Wed, 13 Feb 2008, john stultz wrote: Oh! So your issue is that since time_freq is stored in ppm, or in effect a usecs per sec offset, when we add it to something other then 1 second we mis-apply what NTP tells us to. Is that right? Pretty much everything is centered around that 1sec, so the closer the update frequency is to it the better. Right, so if NTP has us apply a 10ppm adjustment, instead of doing: NSEC_PER_SEC + 10,000 We're doing: NSEC_PER_SEC + CLOCK_TICK_ADJUST + 10,000 Which, if I'm doing my math right, results in a 10.002ppm adjustment (using the 999847467ns number above), instead of just a 10ppm adjustment. Now, true, this is an error, but it is a pretty small one. Even at the maximum 500ppm value, it only results in an error of 76 parts per billion. As you'll see below, that tends to be less error then what we get from the clock granularity. Is there something else I'm missing here or is this really the core issue you're concerned with? The error accumulates and there is no good reason to do this for the common case. I agree, but we can also easily resolve this by scaling the ppm adjustment to the interval length, rather then just applying it as usec/sec. No? So instead of: adjusted_length = NSEC_PER_SEC + time_adj We could do: adjusted_length = ntp_interval_length + (ntp_interval_length * time_adj)/MILLION So this fixes the time_adj scaling error, while letting us be more flexible w/ our interval length, so we can better map the requested length to the clocksource granularity, lessening the granularity error. HZ=1000 CLOCK_TICK_ADJUST=-152533 jiffies 467 ppb error jiffies NOHZ467 ppb error pit 0 ppb error pit NOHZ0 ppb error acpi_pm -280 ppb error acpi_pm NOHZ279 ppb error HZ=1000 CLOCK_TICK_ADJUST=0 jiffies 153000 ppb error jiffies NOHZ153000 ppb error pit 152533 ppb error pit NOHZ0 ppb error acpi_pm -127112 ppb error acpi_pm NOHZ279 ppb error So you are right, w/ pit NO_HZ, the granularity error is always very small both with or without CLOCK_TICK_ADJUST. If you change the frequency of acpi_pm to 3579000 you'll get this: HZ=1000 CLOCK_TICK_ADJUST=0 jiffies 153000 ppb error jiffies NOHZ153000 ppb error pit 152533 ppb error pit NOHZ0 ppb error acpi_pm 0 ppb error acpi_pm NOHZ0 ppb error HZ=1000 CLOCK_TICK_ADJUST=-152533 jiffies 0 ppb error jiffies NOHZ466 ppb error pit -467 ppb error pit NOHZ-1 ppb error acpi_pm 126407 ppb error acpi_pm NOHZ22 ppb error Right, but the acpi_pm's frequency isn't 3579000. It's supposed to be 3579545. That is injecting error, and I believe it to be different then what I'm claiming is achieved by CLOCK_TICK_ADJUST. Further, the specific example above was agreeing with you that that CLOCK_TICK_ADJUST=0 looks ok for NOHZ, with the exception in the case of the jiffies clocksource. That one still has the 153ppm drift. What do we do about that? CLOCK_TICK_ADJUST has only any meaning for PIT (and indirectly for jiffies). For every other clock you just add some random value, where it doesn't do _any_ good. While not the main point, the aside I included about the acpi_pm being interesting, is that because the ACPI PM's frequency is a multiple of the PIT's (and thus jiffies), the same granularity issues arise (although to a lesser degree). That is why CLOCK_TICK_ADJUST helps it in the !NOHZ case. The worst case error there will always be (ntp_hz/freq/2*10^9nsec), all you do with CLOCK_TICK_ADJUST is to do shift it around, but it doesn't actually fix the error - it's still there. However, without CLOCK_TICK_ADJUST, the jiffies error increases for all values of HZ except 100 (which at first seems odd, but seems to be due to loss from rounding in the ACTHZ calculation). jiffies depends on the timer resolution, so it will practically produce the same results as PIT (assuming it's used to generate the timer tick). One interesting surprise in the data: With CLOCK_TICK_ADJUST=0, the acpi_pm's error frequency shot up in the !NO_HZ cases. This ends up being due to the acpi_pm being a very close to a multiple (3x) of the pit frequency, so CLOCK_TICK_ADJUST helps it as well. What exactly does it help with? All you are doing is number cosmetics, it has _no_ practically value and only decreases the quality of timekeeping. Unless you want to clarify what aspect of quality you're talking about, I'd very much disagree with your claim. This is not cosmetics, this is about addressing granularity error. By using a base interval that does not match the clocksource's natural granularity, we effectively inject a
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Mon, 18 Feb 2008, john stultz wrote: If we are building a train station, and each train car is 60ft, it doesn't make much sense to build 1000ft stations, right? Instead you'll do better if you build a 1020ft station. That would assume trains are always 60ft long, which is the basic error in your assumption. Since we're using analogies: What you're doing is to put you winter clothes on your weight scale and reset the scale to zero to offset for the weigth of the clothes. If you stand now with your bathing clothes on the scale, does that mean you lost weight? That's all you do - you change the scale and slightly screw the scale for everyone else trying to use it. To keep in mind what time adjusting is supposed to do: freq = 1sec + time_freq What we do instead is: freq + tick_adj = 1sec + time_freq Where exactly is now the problem to integrate tick_adj into time_freq? The result is _exactly_ the same. The only visible difference is a slightly higher time_freq value and as long as it is within the 500 ppm limit there is simply no problem. And yes, if we remove CLOCK_TICK_ADJUST, that would also resolve the (A!=B) issue, but it doesn't address the error from #2 below. [..] 2) We need a solution that handles granularity error well, as this is a moderate source of error for course clocksources such as jiffies. CLOCK_TICK_ADJUST does cover this fairly well in most cases. I suspect we could do even better, but that will take some deeper changes. How exactly does CLOCK_TICK_ADJUST address this problem? The error due to insufficient resolution is still there, all it does is shifting the scale, so it's not immediately visible. My understanding of your approach (removing CLOCK_TICK_ADJUST), addresses issues #1 and #3, but hurts issue #2. What exactly is hurt? bye, Roman -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Wed, 13 Feb 2008, john stultz wrote: > Oh! So your issue is that since time_freq is stored in ppm, or in effect > a usecs per sec offset, when we add it to something other then 1 second > we mis-apply what NTP tells us to. Is that right? Pretty much everything is centered around that 1sec, so the closer the update frequency is to it the better. > Right, so if NTP has us apply a 10ppm adjustment, instead of doing: > NSEC_PER_SEC + 10,000 > > We're doing: > NSEC_PER_SEC + CLOCK_TICK_ADJUST + 10,000 > > Which, if I'm doing my math right, results in a 10.002ppm adjustment > (using the 999847467ns number above), instead of just a 10ppm > adjustment. > > Now, true, this is an error, but it is a pretty small one. Even at the > maximum 500ppm value, it only results in an error of 76 parts per > billion. As you'll see below, that tends to be less error then what we > get from the clock granularity. Is there something else I'm missing here > or is this really the core issue you're concerned with? The error accumulates and there is no good reason to do this for the common case. > > In consequence this means, if we want to improve timekeeping, we first set > > the (update_cycles * NTP_INTERVAL_FREQ) interval as close as possible to > > the real frequency. It doesn't has to be perfect as we usually don't know > > the real frequency with 100% certainty anyway. > > This might need some more explanation, as I'm not certain I know what > update_cycles refers to. Do you mean cycle_interval? I guess I'm not > completely sure how you're suggesting we change things here. clock->cycle_interval > > Second, we drop the tick > > adjustment if possible and leave the adjustments to the NTP daemon and as > > long as the drift is within the 500ppm limit it has no problem to manage > > this. > > Dropping the tick adjustment? By that do you mean the tick_usec value > set by adjtimex()? I don't quite see why we want that. Could you expand > here? CLOCK_TICK_ADJUST > HZ=1000 CLOCK_TICK_ADJUST=-152533 > jiffies 467 ppb error > jiffies NOHZ 467 ppb error > pit 0 ppb error > pit NOHZ 0 ppb error > acpi_pm -280 ppb error > acpi_pm NOHZ 279 ppb error > > HZ=1000 CLOCK_TICK_ADJUST=0 > jiffies 153000 ppb error > jiffies NOHZ 153000 ppb error > pit 152533 ppb error > pit NOHZ 0 ppb error > acpi_pm -127112 ppb error > acpi_pm NOHZ 279 ppb error > > So you are right, w/ pit & NO_HZ, the granularity error is always very > small both with or without CLOCK_TICK_ADJUST. If you change the frequency of acpi_pm to 3579000 you'll get this: HZ=1000 CLOCK_TICK_ADJUST=0 jiffies 153000 ppb error jiffies NOHZ153000 ppb error pit 152533 ppb error pit NOHZ0 ppb error acpi_pm 0 ppb error acpi_pm NOHZ0 ppb error HZ=1000 CLOCK_TICK_ADJUST=-152533 jiffies 0 ppb error jiffies NOHZ466 ppb error pit -467 ppb error pit NOHZ-1 ppb error acpi_pm 126407 ppb error acpi_pm NOHZ22 ppb error CLOCK_TICK_ADJUST has only any meaning for PIT (and indirectly for jiffies). For every other clock you just add some random value, where it doesn't do _any_ good. The worst case error there will always be (ntp_hz/freq/2*10^9nsec), all you do with CLOCK_TICK_ADJUST is to do shift it around, but it doesn't actually fix the error - it's still there. > However, without CLOCK_TICK_ADJUST, the jiffies error increases for all > values of HZ except 100 (which at first seems odd, but seems to be due > to loss from rounding in the ACTHZ calculation). jiffies depends on the timer resolution, so it will practically produce the same results as PIT (assuming it's used to generate the timer tick). > One interesting surprise in the data: With CLOCK_TICK_ADJUST=0, the > acpi_pm's error frequency shot up in the !NO_HZ cases. This ends up > being due to the acpi_pm being a very close to a multiple (3x) of the > pit frequency, so CLOCK_TICK_ADJUST helps it as well. What exactly does it help with? All you are doing is number cosmetics, it has _no_ practically value and only decreases the quality of timekeeping. > Further it seems to point that if we are going to be chasing down small > sub-100ppb errors (which I think would be great to do, but lets not make > users to endure 200+ppm errors while we debate the fine-tuning :) we > might want to consider a method where we let ntp_update_freq take into > account the current clocksource's interval length, so it becomes the > base value against which we apply adjustments (scaled appropriately). The error at least is real, the use value of CLOCK_TICK_ADJUST for the common case is not existent. > There are 3 sources of error that we've discussed here: > 1) The large (280ppm) error seen with no-NTP adjustment, caused by the > inconsistent (A!=B) interval comparisons which started this discussion, > which my patch does address. Part of the error is caused by
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Wed, 13 Feb 2008, john stultz wrote: Oh! So your issue is that since time_freq is stored in ppm, or in effect a usecs per sec offset, when we add it to something other then 1 second we mis-apply what NTP tells us to. Is that right? Pretty much everything is centered around that 1sec, so the closer the update frequency is to it the better. Right, so if NTP has us apply a 10ppm adjustment, instead of doing: NSEC_PER_SEC + 10,000 We're doing: NSEC_PER_SEC + CLOCK_TICK_ADJUST + 10,000 Which, if I'm doing my math right, results in a 10.002ppm adjustment (using the 999847467ns number above), instead of just a 10ppm adjustment. Now, true, this is an error, but it is a pretty small one. Even at the maximum 500ppm value, it only results in an error of 76 parts per billion. As you'll see below, that tends to be less error then what we get from the clock granularity. Is there something else I'm missing here or is this really the core issue you're concerned with? The error accumulates and there is no good reason to do this for the common case. In consequence this means, if we want to improve timekeeping, we first set the (update_cycles * NTP_INTERVAL_FREQ) interval as close as possible to the real frequency. It doesn't has to be perfect as we usually don't know the real frequency with 100% certainty anyway. This might need some more explanation, as I'm not certain I know what update_cycles refers to. Do you mean cycle_interval? I guess I'm not completely sure how you're suggesting we change things here. clock-cycle_interval Second, we drop the tick adjustment if possible and leave the adjustments to the NTP daemon and as long as the drift is within the 500ppm limit it has no problem to manage this. Dropping the tick adjustment? By that do you mean the tick_usec value set by adjtimex()? I don't quite see why we want that. Could you expand here? CLOCK_TICK_ADJUST HZ=1000 CLOCK_TICK_ADJUST=-152533 jiffies 467 ppb error jiffies NOHZ 467 ppb error pit 0 ppb error pit NOHZ 0 ppb error acpi_pm -280 ppb error acpi_pm NOHZ 279 ppb error HZ=1000 CLOCK_TICK_ADJUST=0 jiffies 153000 ppb error jiffies NOHZ 153000 ppb error pit 152533 ppb error pit NOHZ 0 ppb error acpi_pm -127112 ppb error acpi_pm NOHZ 279 ppb error So you are right, w/ pit NO_HZ, the granularity error is always very small both with or without CLOCK_TICK_ADJUST. If you change the frequency of acpi_pm to 3579000 you'll get this: HZ=1000 CLOCK_TICK_ADJUST=0 jiffies 153000 ppb error jiffies NOHZ153000 ppb error pit 152533 ppb error pit NOHZ0 ppb error acpi_pm 0 ppb error acpi_pm NOHZ0 ppb error HZ=1000 CLOCK_TICK_ADJUST=-152533 jiffies 0 ppb error jiffies NOHZ466 ppb error pit -467 ppb error pit NOHZ-1 ppb error acpi_pm 126407 ppb error acpi_pm NOHZ22 ppb error CLOCK_TICK_ADJUST has only any meaning for PIT (and indirectly for jiffies). For every other clock you just add some random value, where it doesn't do _any_ good. The worst case error there will always be (ntp_hz/freq/2*10^9nsec), all you do with CLOCK_TICK_ADJUST is to do shift it around, but it doesn't actually fix the error - it's still there. However, without CLOCK_TICK_ADJUST, the jiffies error increases for all values of HZ except 100 (which at first seems odd, but seems to be due to loss from rounding in the ACTHZ calculation). jiffies depends on the timer resolution, so it will practically produce the same results as PIT (assuming it's used to generate the timer tick). One interesting surprise in the data: With CLOCK_TICK_ADJUST=0, the acpi_pm's error frequency shot up in the !NO_HZ cases. This ends up being due to the acpi_pm being a very close to a multiple (3x) of the pit frequency, so CLOCK_TICK_ADJUST helps it as well. What exactly does it help with? All you are doing is number cosmetics, it has _no_ practically value and only decreases the quality of timekeeping. Further it seems to point that if we are going to be chasing down small sub-100ppb errors (which I think would be great to do, but lets not make users to endure 200+ppm errors while we debate the fine-tuning :) we might want to consider a method where we let ntp_update_freq take into account the current clocksource's interval length, so it becomes the base value against which we apply adjustments (scaled appropriately). The error at least is real, the use value of CLOCK_TICK_ADJUST for the common case is not existent. There are 3 sources of error that we've discussed here: 1) The large (280ppm) error seen with no-NTP adjustment, caused by the inconsistent (A!=B) interval comparisons which started this discussion, which my patch does address. Part of the error is caused by CLOCK_TICK_ADJUST, but the other part of the error is real, all you do is hiding
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Tue, 2008-02-12 at 15:36 +0100, Roman Zippel wrote: > On Mon, 11 Feb 2008, john stultz wrote: > > This fine grained error accounting is where the bug I'm trying to > > address is cropping up from. In order to have the comparison we need to > > have two values: > > A: The clocksource's notion of how long the fixed interval is. > > B: NTP's notion of how long the fixed interval is. > > > > When no NTP adjustment is being made, these two values should be equal, > > but currently they are not. This is what causes the 280ppm error seen on > > my test system. > > > > Part A is calculated in the following fashion: > > #define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ) > > > > Which is then functionally shifted up by TICK_LENGTH_SHIFT, but for the > > course of this discussion, lets ignore that. > > > > Part B is calculated in ntp_update_frequency() as: > > > > u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) > > << TICK_LENGTH_SHIFT; > > second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT; > > second_length += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC); > > > > tick_length_base = second_length; > > do_div(tick_length_base, NTP_INTERVAL_FREQ); > > > > > > If we're assuming there is no NTP adjustment, and avoiding the > > TICK_LENGTH_SHIFT, this can be shorted to: > > B = ((TICK_USEC * NSEC_PER_USEC * USER_HZ) > > + CLOCK_TICK_ADJUST)/NTP_ITNERVAL_FREQ > > > > > > The A vs B comparison can be shortened further to: > > NSEC_PER_SEC != (TICK_USEC * NSEC_PER_USEC * USER_HZ) > > + CLOCK_TICK_ADJUST > > > > So now on to what seems to be the point of contention: > > If A != B, which side do we fix? > > > > > > My patches fix the A side so that it matches B, which on its face isn't > > terribly complicated, but you seem to be suggesting we fix the B side > > instead (Now I'm assuming here, because there's no patch. So I can only > > speak to your emails, which were not clear to me). > > If we go from your base assumption above "there is no NTP adjustment", I > would actually agree with you and it wouldn't matter much on which side > to correct the equation. > > The question is now what happens, if there are NTP adjustments, i.e. when > the time_freq value isn't zero. Based on this initialization we tell the > NTP daemon the base frequency, although not directly but it knows the > length freq1 == 1 sec. If the clock now needs adjustment, the NTP daemon > tells the kernel via time_freq how to change the speed so that freq1 == > 1 sec + time_freq. > > The problem is now that by using CLOCK_TICK_ADJUST we are cheating and we > don't tell the NTP daemon the real frequency. We define 1 sec as freq2 + > tick_adjust and with the NTP adjustment we have freq2 + tick_adj == 1 sec > + time_freq. Above initialization now calcalutes the base time length for > an update interval of freq2 / NTP_INTERVAL_FREQ, this means the requested > time_freq adjustment isn't applied to (freq2 + tick_adj) cycles but to > freq2 cycles, so this finally means any adjustment made by the NTP daemon > is slightly off. Oh! So your issue is that since time_freq is stored in ppm, or in effect a usecs per sec offset, when we add it to something other then 1 second we mis-apply what NTP tells us to. Is that right? > To demonstrate this let's look at some real values and let's use the PIT > for it (as this is where this originated and on which CLOCK_TICK_ADJUST is > based on). With freq1=1193182 and HZ=1000 we program the timer with 1193 > cycles and the actual update frequency is freq2=1193000. To adjust for > this difference we change the length of a timer tick: > > (NSEC_PER_SEC + CLOCK_TICK_ADJUST) / NTP_INTERVAL_FREQ > > or > > (10^9 nsec - 152533 nsec) / 1000 > > This way every 1000 interrupts or 1193000 cycles the clock is advanced by > 999847467 nsec, so that we advance time according to freq1, but any > further adjustments aren't applied to an interval of what the NTP daemon > thinks is 1 sec but 999847467 nsec instead. Right, so if NTP has us apply a 10ppm adjustment, instead of doing: NSEC_PER_SEC + 10,000 We're doing: NSEC_PER_SEC + CLOCK_TICK_ADJUST + 10,000 Which, if I'm doing my math right, results in a 10.002ppm adjustment (using the 999847467ns number above), instead of just a 10ppm adjustment. Now, true, this is an error, but it is a pretty small one. Even at the maximum 500ppm value, it only results in an error of 76 parts per billion. As you'll see below, that tends to be less error then what we get from the clock granularity. Is there something else I'm missing here or is this really the core issue you're concerned with? If not, I'll agree that we may need to tune the "B" side of the equation, but can we also agree that the very large error caused, even without NTP adjustments, being involved by the A != B can be
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Tue, 2008-02-12 at 15:36 +0100, Roman Zippel wrote: On Mon, 11 Feb 2008, john stultz wrote: This fine grained error accounting is where the bug I'm trying to address is cropping up from. In order to have the comparison we need to have two values: A: The clocksource's notion of how long the fixed interval is. B: NTP's notion of how long the fixed interval is. When no NTP adjustment is being made, these two values should be equal, but currently they are not. This is what causes the 280ppm error seen on my test system. Part A is calculated in the following fashion: #define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ) Which is then functionally shifted up by TICK_LENGTH_SHIFT, but for the course of this discussion, lets ignore that. Part B is calculated in ntp_update_frequency() as: u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) TICK_LENGTH_SHIFT; second_length += (s64)CLOCK_TICK_ADJUST TICK_LENGTH_SHIFT; second_length += (s64)time_freq (TICK_LENGTH_SHIFT - SHIFT_NSEC); tick_length_base = second_length; do_div(tick_length_base, NTP_INTERVAL_FREQ); If we're assuming there is no NTP adjustment, and avoiding the TICK_LENGTH_SHIFT, this can be shorted to: B = ((TICK_USEC * NSEC_PER_USEC * USER_HZ) + CLOCK_TICK_ADJUST)/NTP_ITNERVAL_FREQ The A vs B comparison can be shortened further to: NSEC_PER_SEC != (TICK_USEC * NSEC_PER_USEC * USER_HZ) + CLOCK_TICK_ADJUST So now on to what seems to be the point of contention: If A != B, which side do we fix? My patches fix the A side so that it matches B, which on its face isn't terribly complicated, but you seem to be suggesting we fix the B side instead (Now I'm assuming here, because there's no patch. So I can only speak to your emails, which were not clear to me). If we go from your base assumption above there is no NTP adjustment, I would actually agree with you and it wouldn't matter much on which side to correct the equation. The question is now what happens, if there are NTP adjustments, i.e. when the time_freq value isn't zero. Based on this initialization we tell the NTP daemon the base frequency, although not directly but it knows the length freq1 == 1 sec. If the clock now needs adjustment, the NTP daemon tells the kernel via time_freq how to change the speed so that freq1 == 1 sec + time_freq. The problem is now that by using CLOCK_TICK_ADJUST we are cheating and we don't tell the NTP daemon the real frequency. We define 1 sec as freq2 + tick_adjust and with the NTP adjustment we have freq2 + tick_adj == 1 sec + time_freq. Above initialization now calcalutes the base time length for an update interval of freq2 / NTP_INTERVAL_FREQ, this means the requested time_freq adjustment isn't applied to (freq2 + tick_adj) cycles but to freq2 cycles, so this finally means any adjustment made by the NTP daemon is slightly off. Oh! So your issue is that since time_freq is stored in ppm, or in effect a usecs per sec offset, when we add it to something other then 1 second we mis-apply what NTP tells us to. Is that right? To demonstrate this let's look at some real values and let's use the PIT for it (as this is where this originated and on which CLOCK_TICK_ADJUST is based on). With freq1=1193182 and HZ=1000 we program the timer with 1193 cycles and the actual update frequency is freq2=1193000. To adjust for this difference we change the length of a timer tick: (NSEC_PER_SEC + CLOCK_TICK_ADJUST) / NTP_INTERVAL_FREQ or (10^9 nsec - 152533 nsec) / 1000 This way every 1000 interrupts or 1193000 cycles the clock is advanced by 999847467 nsec, so that we advance time according to freq1, but any further adjustments aren't applied to an interval of what the NTP daemon thinks is 1 sec but 999847467 nsec instead. Right, so if NTP has us apply a 10ppm adjustment, instead of doing: NSEC_PER_SEC + 10,000 We're doing: NSEC_PER_SEC + CLOCK_TICK_ADJUST + 10,000 Which, if I'm doing my math right, results in a 10.002ppm adjustment (using the 999847467ns number above), instead of just a 10ppm adjustment. Now, true, this is an error, but it is a pretty small one. Even at the maximum 500ppm value, it only results in an error of 76 parts per billion. As you'll see below, that tends to be less error then what we get from the clock granularity. Is there something else I'm missing here or is this really the core issue you're concerned with? If not, I'll agree that we may need to tune the B side of the equation, but can we also agree that the very large error caused, even without NTP adjustments, being involved by the A != B can be addressed separately. That way if A is calculated in the same way as B, when we fix B, we will at the same time fix A. Does that sound
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Mon, 11 Feb 2008, john stultz wrote: > > I don't want to just send a patch, I want you to understand why your > > approach is wrong. > > With all due respect, it also keeps the critique in one direction and > makes your review less collaborative and more confrontational then I > suspect (or maybe just hope) you intend. I don't think that's necessarily a contradiction, if we keep it to confronting the problem. A simple patch wouldn't have provided any further understanding of the problem compared to what I already said. You would have seen what the patch does (which I described already differently), but not really why it does that. In this sense I prefer to force the confrontation of the problem. I'm afraid a working patch would encourage to simply ignore the problem, as your problem at hand would be solved without completely understanding it. The point is that I'd really like you to understand the problem, so I'm not the only one who understands this code :) and in the end it might allow better collaboration to further improve this code. To make it very clear this is just about understanding the problem, I don't want to force a specific solution (which a patch would practically do). If we both understand the problem, we can also discuss the solution and maybe we find something better, but maybe I'm also totally wrong, which would be a little embarrassing :), but that would be fine too. There may be better ways to go about this problem, but IMO it would still be better than just ignoring the problem and force it with a patch. > This fine grained error accounting is where the bug I'm trying to > address is cropping up from. In order to have the comparison we need to > have two values: > A: The clocksource's notion of how long the fixed interval is. > B: NTP's notion of how long the fixed interval is. > > When no NTP adjustment is being made, these two values should be equal, > but currently they are not. This is what causes the 280ppm error seen on > my test system. > > Part A is calculated in the following fashion: > #define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ) > > Which is then functionally shifted up by TICK_LENGTH_SHIFT, but for the > course of this discussion, lets ignore that. > > Part B is calculated in ntp_update_frequency() as: > > u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) > << TICK_LENGTH_SHIFT; > second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT; > second_length += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC); > > tick_length_base = second_length; > do_div(tick_length_base, NTP_INTERVAL_FREQ); > > > If we're assuming there is no NTP adjustment, and avoiding the > TICK_LENGTH_SHIFT, this can be shorted to: > B = ((TICK_USEC * NSEC_PER_USEC * USER_HZ) > + CLOCK_TICK_ADJUST)/NTP_ITNERVAL_FREQ > > > The A vs B comparison can be shortened further to: > NSEC_PER_SEC != (TICK_USEC * NSEC_PER_USEC * USER_HZ) > + CLOCK_TICK_ADJUST > > So now on to what seems to be the point of contention: > If A != B, which side do we fix? > > > My patches fix the A side so that it matches B, which on its face isn't > terribly complicated, but you seem to be suggesting we fix the B side > instead (Now I'm assuming here, because there's no patch. So I can only > speak to your emails, which were not clear to me). If we go from your base assumption above "there is no NTP adjustment", I would actually agree with you and it wouldn't matter much on which side to correct the equation. The question is now what happens, if there are NTP adjustments, i.e. when the time_freq value isn't zero. Based on this initialization we tell the NTP daemon the base frequency, although not directly but it knows the length freq1 == 1 sec. If the clock now needs adjustment, the NTP daemon tells the kernel via time_freq how to change the speed so that freq1 == 1 sec + time_freq. The problem is now that by using CLOCK_TICK_ADJUST we are cheating and we don't tell the NTP daemon the real frequency. We define 1 sec as freq2 + tick_adjust and with the NTP adjustment we have freq2 + tick_adj == 1 sec + time_freq. Above initialization now calcalutes the base time length for an update interval of freq2 / NTP_INTERVAL_FREQ, this means the requested time_freq adjustment isn't applied to (freq2 + tick_adj) cycles but to freq2 cycles, so this finally means any adjustment made by the NTP daemon is slightly off. To demonstrate this let's look at some real values and let's use the PIT for it (as this is where this originated and on which CLOCK_TICK_ADJUST is based on). With freq1=1193182 and HZ=1000 we program the timer with 1193 cycles and the actual update frequency is freq2=1193000. To adjust for this difference we change the length of a timer tick: (NSEC_PER_SEC + CLOCK_TICK_ADJUST) /
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Mon, 11 Feb 2008, john stultz wrote: I don't want to just send a patch, I want you to understand why your approach is wrong. With all due respect, it also keeps the critique in one direction and makes your review less collaborative and more confrontational then I suspect (or maybe just hope) you intend. I don't think that's necessarily a contradiction, if we keep it to confronting the problem. A simple patch wouldn't have provided any further understanding of the problem compared to what I already said. You would have seen what the patch does (which I described already differently), but not really why it does that. In this sense I prefer to force the confrontation of the problem. I'm afraid a working patch would encourage to simply ignore the problem, as your problem at hand would be solved without completely understanding it. The point is that I'd really like you to understand the problem, so I'm not the only one who understands this code :) and in the end it might allow better collaboration to further improve this code. To make it very clear this is just about understanding the problem, I don't want to force a specific solution (which a patch would practically do). If we both understand the problem, we can also discuss the solution and maybe we find something better, but maybe I'm also totally wrong, which would be a little embarrassing :), but that would be fine too. There may be better ways to go about this problem, but IMO it would still be better than just ignoring the problem and force it with a patch. This fine grained error accounting is where the bug I'm trying to address is cropping up from. In order to have the comparison we need to have two values: A: The clocksource's notion of how long the fixed interval is. B: NTP's notion of how long the fixed interval is. When no NTP adjustment is being made, these two values should be equal, but currently they are not. This is what causes the 280ppm error seen on my test system. Part A is calculated in the following fashion: #define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ) Which is then functionally shifted up by TICK_LENGTH_SHIFT, but for the course of this discussion, lets ignore that. Part B is calculated in ntp_update_frequency() as: u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) TICK_LENGTH_SHIFT; second_length += (s64)CLOCK_TICK_ADJUST TICK_LENGTH_SHIFT; second_length += (s64)time_freq (TICK_LENGTH_SHIFT - SHIFT_NSEC); tick_length_base = second_length; do_div(tick_length_base, NTP_INTERVAL_FREQ); If we're assuming there is no NTP adjustment, and avoiding the TICK_LENGTH_SHIFT, this can be shorted to: B = ((TICK_USEC * NSEC_PER_USEC * USER_HZ) + CLOCK_TICK_ADJUST)/NTP_ITNERVAL_FREQ The A vs B comparison can be shortened further to: NSEC_PER_SEC != (TICK_USEC * NSEC_PER_USEC * USER_HZ) + CLOCK_TICK_ADJUST So now on to what seems to be the point of contention: If A != B, which side do we fix? My patches fix the A side so that it matches B, which on its face isn't terribly complicated, but you seem to be suggesting we fix the B side instead (Now I'm assuming here, because there's no patch. So I can only speak to your emails, which were not clear to me). If we go from your base assumption above there is no NTP adjustment, I would actually agree with you and it wouldn't matter much on which side to correct the equation. The question is now what happens, if there are NTP adjustments, i.e. when the time_freq value isn't zero. Based on this initialization we tell the NTP daemon the base frequency, although not directly but it knows the length freq1 == 1 sec. If the clock now needs adjustment, the NTP daemon tells the kernel via time_freq how to change the speed so that freq1 == 1 sec + time_freq. The problem is now that by using CLOCK_TICK_ADJUST we are cheating and we don't tell the NTP daemon the real frequency. We define 1 sec as freq2 + tick_adjust and with the NTP adjustment we have freq2 + tick_adj == 1 sec + time_freq. Above initialization now calcalutes the base time length for an update interval of freq2 / NTP_INTERVAL_FREQ, this means the requested time_freq adjustment isn't applied to (freq2 + tick_adj) cycles but to freq2 cycles, so this finally means any adjustment made by the NTP daemon is slightly off. To demonstrate this let's look at some real values and let's use the PIT for it (as this is where this originated and on which CLOCK_TICK_ADJUST is based on). With freq1=1193182 and HZ=1000 we program the timer with 1193 cycles and the actual update frequency is freq2=1193000. To adjust for this difference we change the length of a timer tick: (NSEC_PER_SEC + CLOCK_TICK_ADJUST) / NTP_INTERVAL_FREQ or (10^9 nsec - 152533 nsec) / 1000 This way
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Sun, 2008-02-10 at 19:45 +0100, Roman Zippel wrote: > Hi, > > On Fri, 8 Feb 2008, john stultz wrote: > > > -ENOPATCH > > > > We're taking weeks to critique fairly small bug fix. I'm sure we both > > have better things to do then continue to misunderstand each other. I'll > > do the testing and will happily ack it if it resolves the issue. > > I don't want to just send a patch, I want you to understand why your > approach is wrong. With all due respect, it also keeps the critique in one direction and makes your review less collaborative and more confrontational then I suspect (or maybe just hope) you intend. > > Now, If you're disputing that I'm correcting the wrong side of the > > equation, then we're getting somewhere. But its still not clear to me > > how you're suggesting the other side (which is calculated in > > ntp_update_frequency) be changed. > > [..] > > You keep on bringing up NO_HZ, and again, the bug I'm trying to fix > > occurs with or without NO_HZ. The fix I proposed resolves the issue with > > or without NO_HZ. > > The correction is incorrect for NO_HZ. > Let's try it the other way around, as my explanation seem to lack > something. > Please try to explain what this correction actually means and why it > should be correct for NO_HZ as well. For the course of this discussion, lets assume we're talking about 2.6.24. One of the goals of the timekeeping design is to let it be flexible enough that accumulation interval can be irregular and it will still behave correctly. This stems from the one of original issues with the old tick based timekeeping: lost or irregular timer interrupts. Now, we do used fixed interval accumulation in update_wall_time, but that was for two reasons: 1) In the common case, its faster to do the compare and add/subtract then the mult orignally used (*a) 2) It allows for the extra fine-grained NTP error accounting. This fine grained error accounting is where the bug I'm trying to address is cropping up from. In order to have the comparison we need to have two values: A: The clocksource's notion of how long the fixed interval is. B: NTP's notion of how long the fixed interval is. When no NTP adjustment is being made, these two values should be equal, but currently they are not. This is what causes the 280ppm error seen on my test system. Part A is calculated in the following fashion: #define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ) Which is then functionally shifted up by TICK_LENGTH_SHIFT, but for the course of this discussion, lets ignore that. Part B is calculated in ntp_update_frequency() as: u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) << TICK_LENGTH_SHIFT; second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT; second_length += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC); tick_length_base = second_length; do_div(tick_length_base, NTP_INTERVAL_FREQ); If we're assuming there is no NTP adjustment, and avoiding the TICK_LENGTH_SHIFT, this can be shorted to: B = ((TICK_USEC * NSEC_PER_USEC * USER_HZ) + CLOCK_TICK_ADJUST)/NTP_ITNERVAL_FREQ The A vs B comparison can be shortened further to: NSEC_PER_SEC != (TICK_USEC * NSEC_PER_USEC * USER_HZ) + CLOCK_TICK_ADJUST So now on to what seems to be the point of contention: If A != B, which side do we fix? My patches fix the A side so that it matches B, which on its face isn't terribly complicated, but you seem to be suggesting we fix the B side instead (Now I'm assuming here, because there's no patch. So I can only speak to your emails, which were not clear to me). Really it doesn't matter to me, as long as it works in all the cases needed. You're argument of simplifying B so its closer to A does sound initially compelling. Simpler is better, so lets follow the discussion below to see if it will work, or if I'm just missing something. *a: Note: this does backfire on us if the fixed interval is too small, causing the loop to run too many times, which was seen when NO_HZ was being merged. This was why we changed the NTP_INTERVAL_FREQ to be about half a second w/ NO_HZ, which reduced the time spent in the accumulation loop. > This is the original patch which introduced the correction: > > http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=69c1cd9218e4cf3016b0f435d6ef3dffb5a53860 > > Keep in mind that at that time PIT was used as the base clock (even if the > tsc was used, it was relative to PIT). So how much of those assumptions > are still valid today (especially with NO_HZ enabled)? Indeed! When we use the TSC, acpi_pm, HPET or other fine grained clocksource (regardless of NO_HZ), it does seem silly to take these odd PIT based values into account. However, what happens if a system uses the PIT or jiffies clocksource, which do suffer from the HZ / ACTHZ
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Sun, 2008-02-10 at 19:45 +0100, Roman Zippel wrote: Hi, On Fri, 8 Feb 2008, john stultz wrote: -ENOPATCH We're taking weeks to critique fairly small bug fix. I'm sure we both have better things to do then continue to misunderstand each other. I'll do the testing and will happily ack it if it resolves the issue. I don't want to just send a patch, I want you to understand why your approach is wrong. With all due respect, it also keeps the critique in one direction and makes your review less collaborative and more confrontational then I suspect (or maybe just hope) you intend. Now, If you're disputing that I'm correcting the wrong side of the equation, then we're getting somewhere. But its still not clear to me how you're suggesting the other side (which is calculated in ntp_update_frequency) be changed. [..] You keep on bringing up NO_HZ, and again, the bug I'm trying to fix occurs with or without NO_HZ. The fix I proposed resolves the issue with or without NO_HZ. The correction is incorrect for NO_HZ. Let's try it the other way around, as my explanation seem to lack something. Please try to explain what this correction actually means and why it should be correct for NO_HZ as well. For the course of this discussion, lets assume we're talking about 2.6.24. One of the goals of the timekeeping design is to let it be flexible enough that accumulation interval can be irregular and it will still behave correctly. This stems from the one of original issues with the old tick based timekeeping: lost or irregular timer interrupts. Now, we do used fixed interval accumulation in update_wall_time, but that was for two reasons: 1) In the common case, its faster to do the compare and add/subtract then the mult orignally used (*a) 2) It allows for the extra fine-grained NTP error accounting. This fine grained error accounting is where the bug I'm trying to address is cropping up from. In order to have the comparison we need to have two values: A: The clocksource's notion of how long the fixed interval is. B: NTP's notion of how long the fixed interval is. When no NTP adjustment is being made, these two values should be equal, but currently they are not. This is what causes the 280ppm error seen on my test system. Part A is calculated in the following fashion: #define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ) Which is then functionally shifted up by TICK_LENGTH_SHIFT, but for the course of this discussion, lets ignore that. Part B is calculated in ntp_update_frequency() as: u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) TICK_LENGTH_SHIFT; second_length += (s64)CLOCK_TICK_ADJUST TICK_LENGTH_SHIFT; second_length += (s64)time_freq (TICK_LENGTH_SHIFT - SHIFT_NSEC); tick_length_base = second_length; do_div(tick_length_base, NTP_INTERVAL_FREQ); If we're assuming there is no NTP adjustment, and avoiding the TICK_LENGTH_SHIFT, this can be shorted to: B = ((TICK_USEC * NSEC_PER_USEC * USER_HZ) + CLOCK_TICK_ADJUST)/NTP_ITNERVAL_FREQ The A vs B comparison can be shortened further to: NSEC_PER_SEC != (TICK_USEC * NSEC_PER_USEC * USER_HZ) + CLOCK_TICK_ADJUST So now on to what seems to be the point of contention: If A != B, which side do we fix? My patches fix the A side so that it matches B, which on its face isn't terribly complicated, but you seem to be suggesting we fix the B side instead (Now I'm assuming here, because there's no patch. So I can only speak to your emails, which were not clear to me). Really it doesn't matter to me, as long as it works in all the cases needed. You're argument of simplifying B so its closer to A does sound initially compelling. Simpler is better, so lets follow the discussion below to see if it will work, or if I'm just missing something. *a: Note: this does backfire on us if the fixed interval is too small, causing the loop to run too many times, which was seen when NO_HZ was being merged. This was why we changed the NTP_INTERVAL_FREQ to be about half a second w/ NO_HZ, which reduced the time spent in the accumulation loop. This is the original patch which introduced the correction: http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=69c1cd9218e4cf3016b0f435d6ef3dffb5a53860 Keep in mind that at that time PIT was used as the base clock (even if the tsc was used, it was relative to PIT). So how much of those assumptions are still valid today (especially with NO_HZ enabled)? Indeed! When we use the TSC, acpi_pm, HPET or other fine grained clocksource (regardless of NO_HZ), it does seem silly to take these odd PIT based values into account. However, what happens if a system uses the PIT or jiffies clocksource, which do suffer from the HZ / ACTHZ error resolved by the patch linked to above? Since
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Fri, 8 Feb 2008, john stultz wrote: > -ENOPATCH > > We're taking weeks to critique fairly small bug fix. I'm sure we both > have better things to do then continue to misunderstand each other. I'll > do the testing and will happily ack it if it resolves the issue. I don't want to just send a patch, I want you to understand why your approach is wrong. > Now, If you're disputing that I'm correcting the wrong side of the > equation, then we're getting somewhere. But its still not clear to me > how you're suggesting the other side (which is calculated in > ntp_update_frequency) be changed. > [..] > You keep on bringing up NO_HZ, and again, the bug I'm trying to fix > occurs with or without NO_HZ. The fix I proposed resolves the issue with > or without NO_HZ. The correction is incorrect for NO_HZ. Let's try it the other way around, as my explanation seem to lack something. Please try to explain what this correction actually means and why it should be correct for NO_HZ as well. > > The only other alternative would be to calculate this correction > > dynamically. For this you leave NTP_INTERVAL_LENGTH as is and when > > changing clocks you check whether "abs(((cs->xtime_interval * > > NTP_INTERVAL_FREQ) >> cs->shift) - NSEC_PER_SEC)" exceeds a certain limit > > (e.g. 200usec) and in this case you print a warning message, that the > > clock has large base drift value and is a bad ntp source and apply a > > correction value. This way the correction only hits the very few system > > which might need it and it would be the prefered solution, but it also > > requires a few more changes. > > Uh, that seems to be just checking if the xtime_interval is off base, or > if the ntp correction has gone too far. I just don't see how this > connects to the issue at hand. Above is the key to understanding the problem, if this difference is small enough there is no need to correct anything. This is the original patch which introduced the correction: http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=69c1cd9218e4cf3016b0f435d6ef3dffb5a53860 Keep in mind that at that time PIT was used as the base clock (even if the tsc was used, it was relative to PIT). So how much of those assumptions are still valid today (especially with NO_HZ enabled)? bye, Roman -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Fri, 8 Feb 2008, john stultz wrote: -ENOPATCH We're taking weeks to critique fairly small bug fix. I'm sure we both have better things to do then continue to misunderstand each other. I'll do the testing and will happily ack it if it resolves the issue. I don't want to just send a patch, I want you to understand why your approach is wrong. Now, If you're disputing that I'm correcting the wrong side of the equation, then we're getting somewhere. But its still not clear to me how you're suggesting the other side (which is calculated in ntp_update_frequency) be changed. [..] You keep on bringing up NO_HZ, and again, the bug I'm trying to fix occurs with or without NO_HZ. The fix I proposed resolves the issue with or without NO_HZ. The correction is incorrect for NO_HZ. Let's try it the other way around, as my explanation seem to lack something. Please try to explain what this correction actually means and why it should be correct for NO_HZ as well. The only other alternative would be to calculate this correction dynamically. For this you leave NTP_INTERVAL_LENGTH as is and when changing clocks you check whether abs(((cs-xtime_interval * NTP_INTERVAL_FREQ) cs-shift) - NSEC_PER_SEC) exceeds a certain limit (e.g. 200usec) and in this case you print a warning message, that the clock has large base drift value and is a bad ntp source and apply a correction value. This way the correction only hits the very few system which might need it and it would be the prefered solution, but it also requires a few more changes. Uh, that seems to be just checking if the xtime_interval is off base, or if the ntp correction has gone too far. I just don't see how this connects to the issue at hand. Above is the key to understanding the problem, if this difference is small enough there is no need to correct anything. This is the original patch which introduced the correction: http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=69c1cd9218e4cf3016b0f435d6ef3dffb5a53860 Keep in mind that at that time PIT was used as the base clock (even if the tsc was used, it was relative to PIT). So how much of those assumptions are still valid today (especially with NO_HZ enabled)? bye, Roman -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Fri, 8 Feb 2008, Andrew Morton wrote: > > Only now I noticed that the first patch had been merged without any > > further question. :-( > > What point is there in reviewing patches, if everything is merged anyway. > > :-( > > > > oops, mistake, sorry. There's plenty of time to fix it though. It has been signed off by both Ingo and Thomas and neither noticed anything? This makes me very afraid of the merging process... bye, Roman -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Fri, 8 Feb 2008, Andrew Morton wrote: Only now I noticed that the first patch had been merged without any further question. :-( What point is there in reviewing patches, if everything is merged anyway. :-( oops, mistake, sorry. There's plenty of time to fix it though. It has been signed off by both Ingo and Thomas and neither noticed anything? This makes me very afraid of the merging process... bye, Roman -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Sat, 9 Feb 2008 05:47:18 +0100 (CET) Roman Zippel <[EMAIL PROTECTED]> wrote: > Hi, > > On Fri, 8 Feb 2008, john stultz wrote: > > > > > clock = clocksource_get_next(); > > - clocksource_calculate_interval(clock, > > - (unsigned long)(current_tick_length()>>TICK_LENGTH_SHIFT)); > > + clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH); > > clock->cycle_last = clocksource_read(clock); > > > > Only now I noticed that the first patch had been merged without any > further question. :-( > What point is there in reviewing patches, if everything is merged anyway. :-( > oops, mistake, sorry. There's plenty of time to fix it though. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Fri, 8 Feb 2008, john stultz wrote: > > clock = clocksource_get_next(); > - clocksource_calculate_interval(clock, > - (unsigned long)(current_tick_length()>>TICK_LENGTH_SHIFT)); > + clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH); > clock->cycle_last = clocksource_read(clock); > Only now I noticed that the first patch had been merged without any further question. :-( What point is there in reviewing patches, if everything is merged anyway. :-( bye, Roman -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Fri, 2008-02-08 at 18:33 +0100, Roman Zippel wrote: > Hi, > > On Fri, 1 Feb 2008, John Stultz wrote: > > > > CLOCK_TICK_ADJUST is based on LATCH and HZ, if the update frequency isn't > > > based on HZ, there is no point in using it! > > > > Hey Roman, > > > > Again, I'm sorry I don't seem to be following your objections. If you > > want to suggest a different patch to fix the issue, it might help. > > I already gave you the necessary details for how to set > NTP_INTERVAL_LENGTH and in the previous mail I explained the basis for it. -ENOPATCH We're taking weeks to critique fairly small bug fix. I'm sure we both have better things to do then continue to misunderstand each other. I'll do the testing and will happily ack it if it resolves the issue. > I really don't understand what's your problem with it. Why do you try to > make it more complex than necessary? Again, I profusely apologize for not understanding your suggestions. I do appreciate your feedback and I really respect your insistence on getting this right, but I do not see exactly how your comments connect to resolving the bug I'm trying to fix. > > The big issue for me, is that we have to initialize the clocksource > > cycle interval so it matches the base tick_length that NTP uses. > > > > To be clear, the issue I'm trying to address is only this: > > Assuming there is no NTP adjustment yet to be made, if we initialize the > > clocksource interval to X, then compare it with Y when we accumulate, we > > introduce error if X and Y are not the same. > > > > It really doesn't matter how long the length is, if we're including > > CLOCK_TICK_ADJUST, or if it really matches the actual HZ tick length or > > not. The issue is that we have to be consistent. If we're not, then we > > introduce error that ntpd has to additionally correct for. > > You don't create consistency by adding corrections all over the place > until it adds up to the right sum. Uh, I'd argue that you create consistency by using the same method on both sides of the equation. That's what I'm trying to do. Now, If you're disputing that I'm correcting the wrong side of the equation, then we're getting somewhere. But its still not clear to me how you're suggesting the other side (which is calculated in ntp_update_frequency) be changed. > The current correction is already somewhat of a hack and I'd rather get > rid of it than to let it spread all over the place (it's really only > needed so that people with weird HZ settings don't hit the 500ppm limit > and we're basically cheating to the ntpd by not telling it the real > frequency). Please keep the knowledge about this crutch at a single place > and don't spread it. > Anyway, for NO_HZ this correction is completely irrelevant, so again > there's no point in adding random values all over the place until you get > the correct result. You keep on bringing up NO_HZ, and again, the bug I'm trying to fix occurs with or without NO_HZ. The fix I proposed resolves the issue with or without NO_HZ. > The only other alternative would be to calculate this correction > dynamically. For this you leave NTP_INTERVAL_LENGTH as is and when > changing clocks you check whether "abs(((cs->xtime_interval * > NTP_INTERVAL_FREQ) >> cs->shift) - NSEC_PER_SEC)" exceeds a certain limit > (e.g. 200usec) and in this case you print a warning message, that the > clock has large base drift value and is a bad ntp source and apply a > correction value. This way the correction only hits the very few system > which might need it and it would be the prefered solution, but it also > requires a few more changes. Uh, that seems to be just checking if the xtime_interval is off base, or if the ntp correction has gone too far. I just don't see how this connects to the issue at hand. Just so we're current, I've refined the patch further and included it below. It now addresses error caused by the intervals of adjusted clocksource being recalculated on clocksource changes (and thus including the adjustment in the base interval). thanks -john Fix time skew caused by inconsistent use of NTP_INTERVAL_LENGTH and current_tick_length(). Patch has three components: 1) Reverts the first version of this patch that made it upstream (while not perfect, its still avoids the issue for most users). 2) Changes NTP_INTERVAL_LENGTH to be consistent with the calculations made in ntp_update_frequency(). 3) Splits the clocksource mult value into two parts: the original mult component and the ntp adjusted mult_adj component. This allows us to correctly recalculate the base xtime_intervals if users switch back and forth between adjusted clocksources. Signed-off-by: John Stultz <[EMAIL PROTECTED]> diff --git a/arch/ia64/kernel/time.c b/arch/ia64/kernel/time.c index 3ab0427..07cacc9 100644 --- a/arch/ia64/kernel/time.c +++ b/arch/ia64/kernel/time.c @@ -357,7 +357,7 @@ void update_vsyscall(struct timespec *wall, struct clocksource *c) /* copy
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Fri, 1 Feb 2008, John Stultz wrote: > > CLOCK_TICK_ADJUST is based on LATCH and HZ, if the update frequency isn't > > based on HZ, there is no point in using it! > > Hey Roman, > > Again, I'm sorry I don't seem to be following your objections. If you > want to suggest a different patch to fix the issue, it might help. I already gave you the necessary details for how to set NTP_INTERVAL_LENGTH and in the previous mail I explained the basis for it. I really don't understand what's your problem with it. Why do you try to make it more complex than necessary? > The big issue for me, is that we have to initialize the clocksource > cycle interval so it matches the base tick_length that NTP uses. > > To be clear, the issue I'm trying to address is only this: > Assuming there is no NTP adjustment yet to be made, if we initialize the > clocksource interval to X, then compare it with Y when we accumulate, we > introduce error if X and Y are not the same. > > It really doesn't matter how long the length is, if we're including > CLOCK_TICK_ADJUST, or if it really matches the actual HZ tick length or > not. The issue is that we have to be consistent. If we're not, then we > introduce error that ntpd has to additionally correct for. You don't create consistency by adding corrections all over the place until it adds up to the right sum. The current correction is already somewhat of a hack and I'd rather get rid of it than to let it spread all over the place (it's really only needed so that people with weird HZ settings don't hit the 500ppm limit and we're basically cheating to the ntpd by not telling it the real frequency). Please keep the knowledge about this crutch at a single place and don't spread it. Anyway, for NO_HZ this correction is completely irrelevant, so again there's no point in adding random values all over the place until you get the correct result. The only other alternative would be to calculate this correction dynamically. For this you leave NTP_INTERVAL_LENGTH as is and when changing clocks you check whether "abs(((cs->xtime_interval * NTP_INTERVAL_FREQ) >> cs->shift) - NSEC_PER_SEC)" exceeds a certain limit (e.g. 200usec) and in this case you print a warning message, that the clock has large base drift value and is a bad ntp source and apply a correction value. This way the correction only hits the very few system which might need it and it would be the prefered solution, but it also requires a few more changes. bye, Roman -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Fri, 2008-02-08 at 18:33 +0100, Roman Zippel wrote: Hi, On Fri, 1 Feb 2008, John Stultz wrote: CLOCK_TICK_ADJUST is based on LATCH and HZ, if the update frequency isn't based on HZ, there is no point in using it! Hey Roman, Again, I'm sorry I don't seem to be following your objections. If you want to suggest a different patch to fix the issue, it might help. I already gave you the necessary details for how to set NTP_INTERVAL_LENGTH and in the previous mail I explained the basis for it. -ENOPATCH We're taking weeks to critique fairly small bug fix. I'm sure we both have better things to do then continue to misunderstand each other. I'll do the testing and will happily ack it if it resolves the issue. I really don't understand what's your problem with it. Why do you try to make it more complex than necessary? Again, I profusely apologize for not understanding your suggestions. I do appreciate your feedback and I really respect your insistence on getting this right, but I do not see exactly how your comments connect to resolving the bug I'm trying to fix. The big issue for me, is that we have to initialize the clocksource cycle interval so it matches the base tick_length that NTP uses. To be clear, the issue I'm trying to address is only this: Assuming there is no NTP adjustment yet to be made, if we initialize the clocksource interval to X, then compare it with Y when we accumulate, we introduce error if X and Y are not the same. It really doesn't matter how long the length is, if we're including CLOCK_TICK_ADJUST, or if it really matches the actual HZ tick length or not. The issue is that we have to be consistent. If we're not, then we introduce error that ntpd has to additionally correct for. You don't create consistency by adding corrections all over the place until it adds up to the right sum. Uh, I'd argue that you create consistency by using the same method on both sides of the equation. That's what I'm trying to do. Now, If you're disputing that I'm correcting the wrong side of the equation, then we're getting somewhere. But its still not clear to me how you're suggesting the other side (which is calculated in ntp_update_frequency) be changed. The current correction is already somewhat of a hack and I'd rather get rid of it than to let it spread all over the place (it's really only needed so that people with weird HZ settings don't hit the 500ppm limit and we're basically cheating to the ntpd by not telling it the real frequency). Please keep the knowledge about this crutch at a single place and don't spread it. Anyway, for NO_HZ this correction is completely irrelevant, so again there's no point in adding random values all over the place until you get the correct result. You keep on bringing up NO_HZ, and again, the bug I'm trying to fix occurs with or without NO_HZ. The fix I proposed resolves the issue with or without NO_HZ. The only other alternative would be to calculate this correction dynamically. For this you leave NTP_INTERVAL_LENGTH as is and when changing clocks you check whether abs(((cs-xtime_interval * NTP_INTERVAL_FREQ) cs-shift) - NSEC_PER_SEC) exceeds a certain limit (e.g. 200usec) and in this case you print a warning message, that the clock has large base drift value and is a bad ntp source and apply a correction value. This way the correction only hits the very few system which might need it and it would be the prefered solution, but it also requires a few more changes. Uh, that seems to be just checking if the xtime_interval is off base, or if the ntp correction has gone too far. I just don't see how this connects to the issue at hand. Just so we're current, I've refined the patch further and included it below. It now addresses error caused by the intervals of adjusted clocksource being recalculated on clocksource changes (and thus including the adjustment in the base interval). thanks -john Fix time skew caused by inconsistent use of NTP_INTERVAL_LENGTH and current_tick_length(). Patch has three components: 1) Reverts the first version of this patch that made it upstream (while not perfect, its still avoids the issue for most users). 2) Changes NTP_INTERVAL_LENGTH to be consistent with the calculations made in ntp_update_frequency(). 3) Splits the clocksource mult value into two parts: the original mult component and the ntp adjusted mult_adj component. This allows us to correctly recalculate the base xtime_intervals if users switch back and forth between adjusted clocksources. Signed-off-by: John Stultz [EMAIL PROTECTED] diff --git a/arch/ia64/kernel/time.c b/arch/ia64/kernel/time.c index 3ab0427..07cacc9 100644 --- a/arch/ia64/kernel/time.c +++ b/arch/ia64/kernel/time.c @@ -357,7 +357,7 @@ void update_vsyscall(struct timespec *wall, struct clocksource *c) /* copy fsyscall clock data */ fsyscall_gtod_data.clk_mask = c-mask; -
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Fri, 8 Feb 2008, john stultz wrote: clock = clocksource_get_next(); - clocksource_calculate_interval(clock, - (unsigned long)(current_tick_length()TICK_LENGTH_SHIFT)); + clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH); clock-cycle_last = clocksource_read(clock); Only now I noticed that the first patch had been merged without any further question. :-( What point is there in reviewing patches, if everything is merged anyway. :-( bye, Roman -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Sat, 9 Feb 2008 05:47:18 +0100 (CET) Roman Zippel [EMAIL PROTECTED] wrote: Hi, On Fri, 8 Feb 2008, john stultz wrote: clock = clocksource_get_next(); - clocksource_calculate_interval(clock, - (unsigned long)(current_tick_length()TICK_LENGTH_SHIFT)); + clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH); clock-cycle_last = clocksource_read(clock); Only now I noticed that the first patch had been merged without any further question. :-( What point is there in reviewing patches, if everything is merged anyway. :-( oops, mistake, sorry. There's plenty of time to fix it though. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Fri, 1 Feb 2008, John Stultz wrote: CLOCK_TICK_ADJUST is based on LATCH and HZ, if the update frequency isn't based on HZ, there is no point in using it! Hey Roman, Again, I'm sorry I don't seem to be following your objections. If you want to suggest a different patch to fix the issue, it might help. I already gave you the necessary details for how to set NTP_INTERVAL_LENGTH and in the previous mail I explained the basis for it. I really don't understand what's your problem with it. Why do you try to make it more complex than necessary? The big issue for me, is that we have to initialize the clocksource cycle interval so it matches the base tick_length that NTP uses. To be clear, the issue I'm trying to address is only this: Assuming there is no NTP adjustment yet to be made, if we initialize the clocksource interval to X, then compare it with Y when we accumulate, we introduce error if X and Y are not the same. It really doesn't matter how long the length is, if we're including CLOCK_TICK_ADJUST, or if it really matches the actual HZ tick length or not. The issue is that we have to be consistent. If we're not, then we introduce error that ntpd has to additionally correct for. You don't create consistency by adding corrections all over the place until it adds up to the right sum. The current correction is already somewhat of a hack and I'd rather get rid of it than to let it spread all over the place (it's really only needed so that people with weird HZ settings don't hit the 500ppm limit and we're basically cheating to the ntpd by not telling it the real frequency). Please keep the knowledge about this crutch at a single place and don't spread it. Anyway, for NO_HZ this correction is completely irrelevant, so again there's no point in adding random values all over the place until you get the correct result. The only other alternative would be to calculate this correction dynamically. For this you leave NTP_INTERVAL_LENGTH as is and when changing clocks you check whether abs(((cs-xtime_interval * NTP_INTERVAL_FREQ) cs-shift) - NSEC_PER_SEC) exceeds a certain limit (e.g. 200usec) and in this case you print a warning message, that the clock has large base drift value and is a bad ntp source and apply a correction value. This way the correction only hits the very few system which might need it and it would be the prefered solution, but it also requires a few more changes. bye, Roman -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Thu, 2008-01-31 at 06:02 +0100, Roman Zippel wrote: > Hi, > > On Wed, 30 Jan 2008, john stultz wrote: > > > My concern is we state the accumulation interval is X ns long. Then > > current_tick_length() is to return (X + ntp_adjustment), so each > > accumulation interval we can keep track of the error and adjust our > > interval length. > > > > So if ntp_update_frequency() sets tick_length_base to be: > > > > u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) > > << TICK_LENGTH_SHIFT; > > second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT; > > second_length += (s64)time_freq > > << (TICK_LENGTH_SHIFT - SHIFT_NSEC); > > > > tick_length_base = second_length; > > do_div(tick_length_base, NTP_INTERVAL_FREQ); > > > > > > The above is basically (X + part of ntp_adjustment) > > CLOCK_TICK_ADJUST is based on LATCH and HZ, if the update frequency isn't > based on HZ, there is no point in using it! Hey Roman, Again, I'm sorry I don't seem to be following your objections. If you want to suggest a different patch to fix the issue, it might help. The big issue for me, is that we have to initialize the clocksource cycle interval so it matches the base tick_length that NTP uses. To be clear, the issue I'm trying to address is only this: Assuming there is no NTP adjustment yet to be made, if we initialize the clocksource interval to X, then compare it with Y when we accumulate, we introduce error if X and Y are not the same. It really doesn't matter how long the length is, if we're including CLOCK_TICK_ADJUST, or if it really matches the actual HZ tick length or not. The issue is that we have to be consistent. If we're not, then we introduce error that ntpd has to additionally correct for. Now, once we're consistent, then CLOCK_TICK_ADJUST can be zero or not and it won't really matter, other then making sure we accumulate at exact tick length units. You can set it either way with my patch and things will still work out to be correct. My apologies for being so thick headed if I'm just missing something. I do appreciate the feedback. -john -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Thu, 2008-01-31 at 06:02 +0100, Roman Zippel wrote: Hi, On Wed, 30 Jan 2008, john stultz wrote: My concern is we state the accumulation interval is X ns long. Then current_tick_length() is to return (X + ntp_adjustment), so each accumulation interval we can keep track of the error and adjust our interval length. So if ntp_update_frequency() sets tick_length_base to be: u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) TICK_LENGTH_SHIFT; second_length += (s64)CLOCK_TICK_ADJUST TICK_LENGTH_SHIFT; second_length += (s64)time_freq (TICK_LENGTH_SHIFT - SHIFT_NSEC); tick_length_base = second_length; do_div(tick_length_base, NTP_INTERVAL_FREQ); The above is basically (X + part of ntp_adjustment) CLOCK_TICK_ADJUST is based on LATCH and HZ, if the update frequency isn't based on HZ, there is no point in using it! Hey Roman, Again, I'm sorry I don't seem to be following your objections. If you want to suggest a different patch to fix the issue, it might help. The big issue for me, is that we have to initialize the clocksource cycle interval so it matches the base tick_length that NTP uses. To be clear, the issue I'm trying to address is only this: Assuming there is no NTP adjustment yet to be made, if we initialize the clocksource interval to X, then compare it with Y when we accumulate, we introduce error if X and Y are not the same. It really doesn't matter how long the length is, if we're including CLOCK_TICK_ADJUST, or if it really matches the actual HZ tick length or not. The issue is that we have to be consistent. If we're not, then we introduce error that ntpd has to additionally correct for. Now, once we're consistent, then CLOCK_TICK_ADJUST can be zero or not and it won't really matter, other then making sure we accumulate at exact tick length units. You can set it either way with my patch and things will still work out to be correct. My apologies for being so thick headed if I'm just missing something. I do appreciate the feedback. -john -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Wed, 30 Jan 2008, john stultz wrote: > My concern is we state the accumulation interval is X ns long. Then > current_tick_length() is to return (X + ntp_adjustment), so each > accumulation interval we can keep track of the error and adjust our > interval length. > > So if ntp_update_frequency() sets tick_length_base to be: > > u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) > << TICK_LENGTH_SHIFT; > second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT; > second_length += (s64)time_freq > << (TICK_LENGTH_SHIFT - SHIFT_NSEC); > > tick_length_base = second_length; > do_div(tick_length_base, NTP_INTERVAL_FREQ); > > > The above is basically (X + part of ntp_adjustment) CLOCK_TICK_ADJUST is based on LATCH and HZ, if the update frequency isn't based on HZ, there is no point in using it! Let's look at what actually needs to be done: 1. initializing clock interval: clock_cycle_interval = timer_cycle_interval * clock_frequency / timer_frequency It's simply about converting timer cycles into clock cycles, so they're about the same time interval. We already make it a bit more complicated than necessary as we go via nsec: ntp_interval = timer_cycle_interval * 10^9nsec / timer_frequency and in clocksource_calculate_interval() basically: clock_cycle_interval = ntp_interval * clock_frequency / 10^9nsec Without a fixed timer tick it's actually even easier, then we use the same frequency for clock and timer and the cycle interval is simply: clock_cycle_interval = timer_cycle_interval = clock_frequency / NTP_INTERVAL_FREQ There is no need to use the adjustment here, you'll only cause a mismatch between the clock and timer cycle interval, which had to be corrected by NTP. 2. initializing clock adjustment: clock_adjust = timer_cycle_interval * NTP_INTERVAL_FREQ / timer_frequency - 1sec This adjustment is used make up for the difference that the timer frequency isn't evenly divisible by HZ, so that the clock is advanced by 1sec after timer_frequency cycles. Like above the clock frequency is used for the timer frequency for this calculation for CONFIG_NO_HZ, so it would be incorrect to use CLOCK_TICK_RATE/LATCH/HZ here and since NTP_INTERVAL_FREQ is quite small the resulting adjustment would be rather small, it's easier not to bother in this case. What you're basically trying is to add an error to the clock initialization, so that we can later compensate for it. The correct solution is really to not add the error in first place, so that there is no need to compensate for it. bye. Roman -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Thu, 2008-01-31 at 02:55 +0100, Roman Zippel wrote: > Hi, > > On Tue, 29 Jan 2008, john stultz wrote: > > > +/* Because using NSEC_PER_SEC would be too easy */ > > +#define NTP_INTERVAL_LENGTH > > s64)TICK_USEC*NSEC_PER_USEC*USER_HZ)+CLOCK_TICK_ADJUST)/NTP_INTERVAL_FREQ) > > Why are you using USER_HZ? Did you test this with HZ!=100? I'm using USER_HZ, because ntp_update_frequency() uses USER_HZ. And my tests were run at HZ=1000. > Anyway, please don't make more complicated than it already is. > What I said previously about the update interval is still valid, so the > correct solution is to use the simpler NTP_INTERVAL_LENGTH calculation > from my last mail and to omit the correction for NO_HZ. I'm not sure I follow how having two separate and totally different definitions of NTP_INTERVAL_LENGTH is less complicated then my last patch. Maybe I'm missing something from your suggestion? Or maybe could you contribute your suggestion as a patch? My concern is we state the accumulation interval is X ns long. Then current_tick_length() is to return (X + ntp_adjustment), so each accumulation interval we can keep track of the error and adjust our interval length. So if ntp_update_frequency() sets tick_length_base to be: u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) << TICK_LENGTH_SHIFT; second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT; second_length += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC); tick_length_base = second_length; do_div(tick_length_base, NTP_INTERVAL_FREQ); The above is basically (X + part of ntp_adjustment) So I'm trying to calculate the X as: #define NTP_INTERVAL_LENGTH s64)TICK_USEC*NSEC_PER_USEC*USER_HZ)+CLOCK_TICK_ADJUST)/NTP_INTERVAL_FREQ) thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Tue, 29 Jan 2008, john stultz wrote: > +/* Because using NSEC_PER_SEC would be too easy */ > +#define NTP_INTERVAL_LENGTH > s64)TICK_USEC*NSEC_PER_USEC*USER_HZ)+CLOCK_TICK_ADJUST)/NTP_INTERVAL_FREQ) Why are you using USER_HZ? Did you test this with HZ!=100? Anyway, please don't make more complicated than it already is. What I said previously about the update interval is still valid, so the correct solution is to use the simpler NTP_INTERVAL_LENGTH calculation from my last mail and to omit the correction for NO_HZ. bye, Roman -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Tue, 29 Jan 2008, john stultz wrote: +/* Because using NSEC_PER_SEC would be too easy */ +#define NTP_INTERVAL_LENGTH s64)TICK_USEC*NSEC_PER_USEC*USER_HZ)+CLOCK_TICK_ADJUST)/NTP_INTERVAL_FREQ) Why are you using USER_HZ? Did you test this with HZ!=100? Anyway, please don't make more complicated than it already is. What I said previously about the update interval is still valid, so the correct solution is to use the simpler NTP_INTERVAL_LENGTH calculation from my last mail and to omit the correction for NO_HZ. bye, Roman -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Thu, 2008-01-31 at 02:55 +0100, Roman Zippel wrote: Hi, On Tue, 29 Jan 2008, john stultz wrote: +/* Because using NSEC_PER_SEC would be too easy */ +#define NTP_INTERVAL_LENGTH s64)TICK_USEC*NSEC_PER_USEC*USER_HZ)+CLOCK_TICK_ADJUST)/NTP_INTERVAL_FREQ) Why are you using USER_HZ? Did you test this with HZ!=100? I'm using USER_HZ, because ntp_update_frequency() uses USER_HZ. And my tests were run at HZ=1000. Anyway, please don't make more complicated than it already is. What I said previously about the update interval is still valid, so the correct solution is to use the simpler NTP_INTERVAL_LENGTH calculation from my last mail and to omit the correction for NO_HZ. I'm not sure I follow how having two separate and totally different definitions of NTP_INTERVAL_LENGTH is less complicated then my last patch. Maybe I'm missing something from your suggestion? Or maybe could you contribute your suggestion as a patch? My concern is we state the accumulation interval is X ns long. Then current_tick_length() is to return (X + ntp_adjustment), so each accumulation interval we can keep track of the error and adjust our interval length. So if ntp_update_frequency() sets tick_length_base to be: u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) TICK_LENGTH_SHIFT; second_length += (s64)CLOCK_TICK_ADJUST TICK_LENGTH_SHIFT; second_length += (s64)time_freq (TICK_LENGTH_SHIFT - SHIFT_NSEC); tick_length_base = second_length; do_div(tick_length_base, NTP_INTERVAL_FREQ); The above is basically (X + part of ntp_adjustment) So I'm trying to calculate the X as: #define NTP_INTERVAL_LENGTH s64)TICK_USEC*NSEC_PER_USEC*USER_HZ)+CLOCK_TICK_ADJUST)/NTP_INTERVAL_FREQ) thanks -john -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Wed, 30 Jan 2008, john stultz wrote: My concern is we state the accumulation interval is X ns long. Then current_tick_length() is to return (X + ntp_adjustment), so each accumulation interval we can keep track of the error and adjust our interval length. So if ntp_update_frequency() sets tick_length_base to be: u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) TICK_LENGTH_SHIFT; second_length += (s64)CLOCK_TICK_ADJUST TICK_LENGTH_SHIFT; second_length += (s64)time_freq (TICK_LENGTH_SHIFT - SHIFT_NSEC); tick_length_base = second_length; do_div(tick_length_base, NTP_INTERVAL_FREQ); The above is basically (X + part of ntp_adjustment) CLOCK_TICK_ADJUST is based on LATCH and HZ, if the update frequency isn't based on HZ, there is no point in using it! Let's look at what actually needs to be done: 1. initializing clock interval: clock_cycle_interval = timer_cycle_interval * clock_frequency / timer_frequency It's simply about converting timer cycles into clock cycles, so they're about the same time interval. We already make it a bit more complicated than necessary as we go via nsec: ntp_interval = timer_cycle_interval * 10^9nsec / timer_frequency and in clocksource_calculate_interval() basically: clock_cycle_interval = ntp_interval * clock_frequency / 10^9nsec Without a fixed timer tick it's actually even easier, then we use the same frequency for clock and timer and the cycle interval is simply: clock_cycle_interval = timer_cycle_interval = clock_frequency / NTP_INTERVAL_FREQ There is no need to use the adjustment here, you'll only cause a mismatch between the clock and timer cycle interval, which had to be corrected by NTP. 2. initializing clock adjustment: clock_adjust = timer_cycle_interval * NTP_INTERVAL_FREQ / timer_frequency - 1sec This adjustment is used make up for the difference that the timer frequency isn't evenly divisible by HZ, so that the clock is advanced by 1sec after timer_frequency cycles. Like above the clock frequency is used for the timer frequency for this calculation for CONFIG_NO_HZ, so it would be incorrect to use CLOCK_TICK_RATE/LATCH/HZ here and since NTP_INTERVAL_FREQ is quite small the resulting adjustment would be rather small, it's easier not to bother in this case. What you're basically trying is to add an error to the clock initialization, so that we can later compensate for it. The correct solution is really to not add the error in first place, so that there is no need to compensate for it. bye. Roman -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Tue, 2008-01-29 at 05:02 +0100, Roman Zippel wrote:
> Hi,
>
> On Mon, 28 Jan 2008, john stultz wrote:
>
> > Regardless, current_tick_length() really is the base interval we're
> > using in the error accumulation loop, so it seems the cleanest interface
> > to use (just to avoid redundancy at least) when establishing the
> > clocksource's interval length. Or do you not agree?
>
> I see, what you need to use in timex.h for !CONFIG_NO_HZ is:
>
> #define NTP_INTERVAL_LENGTH ((s64)LATCH * NSEC_PER_SEC) /
> (s64)CLOCK_TICK_RATE)
>
> this calculates the base length of a clock tick in nsec.
>
> current_tick_length() would only work during boot. If you switch clocks
> later, it would include random adjustments specific to the old clock.
Ah. Good point. How about the following, tested on x86_64 both with and
without CONFIG_NO_HZ?
Thanks for the review and feedback!
-john
Correct NTP drift caused by using inconsistent NTP_INTERVAL_LENGTHs in
clocksource initialization and error accumulation. This corrects a
280ppm drift seen on some systems using acpi_pm, and affects other
clocksources as well (likely to a lesser degree).
Signed-off-by: John Stultz <[EMAIL PROTECTED]>
Index: linux/include/linux/timex.h
===
--- linux.orig/include/linux/timex.h
+++ linux/include/linux/timex.h
@@ -231,7 +231,13 @@ static inline int ntp_synced(void)
#else
#define NTP_INTERVAL_FREQ (HZ)
#endif
-#define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ)
+
+#define CLOCK_TICK_OVERFLOW(LATCH * HZ - CLOCK_TICK_RATE)
+#define CLOCK_TICK_ADJUST (((s64)CLOCK_TICK_OVERFLOW * NSEC_PER_SEC) / \
+ (s64)CLOCK_TICK_RATE)
+
+/* Because using NSEC_PER_SEC would be too easy */
+#define NTP_INTERVAL_LENGTH
s64)TICK_USEC*NSEC_PER_USEC*USER_HZ)+CLOCK_TICK_ADJUST)/NTP_INTERVAL_FREQ)
/* Returns how long ticks are at present, in ns / 2^(SHIFT_SCALE-10). */
extern u64 current_tick_length(void);
Index: linux/kernel/time/ntp.c
===
--- linux.orig/kernel/time/ntp.c
+++ linux/kernel/time/ntp.c
@@ -43,10 +43,6 @@ long time_freq; /* frequency
offset (
static long time_reftime; /* time at last adjustment (s) */
long time_adjust;
-#define CLOCK_TICK_OVERFLOW(LATCH * HZ - CLOCK_TICK_RATE)
-#define CLOCK_TICK_ADJUST (((s64)CLOCK_TICK_OVERFLOW * NSEC_PER_SEC) / \
- (s64)CLOCK_TICK_RATE)
-
static void ntp_update_frequency(void)
{
u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Tue, 2008-01-29 at 05:02 +0100, Roman Zippel wrote:
Hi,
On Mon, 28 Jan 2008, john stultz wrote:
Regardless, current_tick_length() really is the base interval we're
using in the error accumulation loop, so it seems the cleanest interface
to use (just to avoid redundancy at least) when establishing the
clocksource's interval length. Or do you not agree?
I see, what you need to use in timex.h for !CONFIG_NO_HZ is:
#define NTP_INTERVAL_LENGTH ((s64)LATCH * NSEC_PER_SEC) /
(s64)CLOCK_TICK_RATE)
this calculates the base length of a clock tick in nsec.
current_tick_length() would only work during boot. If you switch clocks
later, it would include random adjustments specific to the old clock.
Ah. Good point. How about the following, tested on x86_64 both with and
without CONFIG_NO_HZ?
Thanks for the review and feedback!
-john
Correct NTP drift caused by using inconsistent NTP_INTERVAL_LENGTHs in
clocksource initialization and error accumulation. This corrects a
280ppm drift seen on some systems using acpi_pm, and affects other
clocksources as well (likely to a lesser degree).
Signed-off-by: John Stultz [EMAIL PROTECTED]
Index: linux/include/linux/timex.h
===
--- linux.orig/include/linux/timex.h
+++ linux/include/linux/timex.h
@@ -231,7 +231,13 @@ static inline int ntp_synced(void)
#else
#define NTP_INTERVAL_FREQ (HZ)
#endif
-#define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ)
+
+#define CLOCK_TICK_OVERFLOW(LATCH * HZ - CLOCK_TICK_RATE)
+#define CLOCK_TICK_ADJUST (((s64)CLOCK_TICK_OVERFLOW * NSEC_PER_SEC) / \
+ (s64)CLOCK_TICK_RATE)
+
+/* Because using NSEC_PER_SEC would be too easy */
+#define NTP_INTERVAL_LENGTH
s64)TICK_USEC*NSEC_PER_USEC*USER_HZ)+CLOCK_TICK_ADJUST)/NTP_INTERVAL_FREQ)
/* Returns how long ticks are at present, in ns / 2^(SHIFT_SCALE-10). */
extern u64 current_tick_length(void);
Index: linux/kernel/time/ntp.c
===
--- linux.orig/kernel/time/ntp.c
+++ linux/kernel/time/ntp.c
@@ -43,10 +43,6 @@ long time_freq; /* frequency
offset (
static long time_reftime; /* time at last adjustment (s) */
long time_adjust;
-#define CLOCK_TICK_OVERFLOW(LATCH * HZ - CLOCK_TICK_RATE)
-#define CLOCK_TICK_ADJUST (((s64)CLOCK_TICK_OVERFLOW * NSEC_PER_SEC) / \
- (s64)CLOCK_TICK_RATE)
-
static void ntp_update_frequency(void)
{
u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Mon, 28 Jan 2008, john stultz wrote: > Regardless, current_tick_length() really is the base interval we're > using in the error accumulation loop, so it seems the cleanest interface > to use (just to avoid redundancy at least) when establishing the > clocksource's interval length. Or do you not agree? I see, what you need to use in timex.h for !CONFIG_NO_HZ is: #define NTP_INTERVAL_LENGTH ((s64)LATCH * NSEC_PER_SEC) / (s64)CLOCK_TICK_RATE) this calculates the base length of a clock tick in nsec. current_tick_length() would only work during boot. If you switch clocks later, it would include random adjustments specific to the old clock. bye, Roman -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Fri, 2008-01-25 at 15:07 +0100, Roman Zippel wrote: > Hi, > > On Wed, 23 Jan 2008, john stultz wrote: > > > This difference in calculation was causing the clocksource correction > > code to apply a correction factor to the clocksource so the two > > intervals were the same, however this results in the actual frequency of > > the clocksource to be made incorrect. I believe this difference would > > affect all clocksources, although to differing degrees depending on the > > clocksource resolution. > > Let's look at why the correction is done in first place. The update steps > don't add up precisely to 1sec (LATCH*HZ != CLOCK_TICK_RATE), so a small > addjustment is used to make up for it. The problem here is that if the > update frequency changes, the addjustment isn't correct anymore. > The simple fix is to just omit the addjustment in these cases in ntp.c: > > #if NTP_INTERVAL_FREQ == HZ > ... > #else > #define CLOCK_TICK_ADJUST 0 > #endif Hmmm, although this doesn't explain why the issue is seen when NTP_INTERVAL_FREQ == HZ (as it is in my system's case). Or am I missing something? Regardless, current_tick_length() really is the base interval we're using in the error accumulation loop, so it seems the cleanest interface to use (just to avoid redundancy at least) when establishing the clocksource's interval length. Or do you not agree? thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Fri, 2008-01-25 at 15:07 +0100, Roman Zippel wrote: Hi, On Wed, 23 Jan 2008, john stultz wrote: This difference in calculation was causing the clocksource correction code to apply a correction factor to the clocksource so the two intervals were the same, however this results in the actual frequency of the clocksource to be made incorrect. I believe this difference would affect all clocksources, although to differing degrees depending on the clocksource resolution. Let's look at why the correction is done in first place. The update steps don't add up precisely to 1sec (LATCH*HZ != CLOCK_TICK_RATE), so a small addjustment is used to make up for it. The problem here is that if the update frequency changes, the addjustment isn't correct anymore. The simple fix is to just omit the addjustment in these cases in ntp.c: #if NTP_INTERVAL_FREQ == HZ ... #else #define CLOCK_TICK_ADJUST 0 #endif Hmmm, although this doesn't explain why the issue is seen when NTP_INTERVAL_FREQ == HZ (as it is in my system's case). Or am I missing something? Regardless, current_tick_length() really is the base interval we're using in the error accumulation loop, so it seems the cleanest interface to use (just to avoid redundancy at least) when establishing the clocksource's interval length. Or do you not agree? thanks -john -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Mon, 28 Jan 2008, john stultz wrote: Regardless, current_tick_length() really is the base interval we're using in the error accumulation loop, so it seems the cleanest interface to use (just to avoid redundancy at least) when establishing the clocksource's interval length. Or do you not agree? I see, what you need to use in timex.h for !CONFIG_NO_HZ is: #define NTP_INTERVAL_LENGTH ((s64)LATCH * NSEC_PER_SEC) / (s64)CLOCK_TICK_RATE) this calculates the base length of a clock tick in nsec. current_tick_length() would only work during boot. If you switch clocks later, it would include random adjustments specific to the old clock. bye, Roman -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Wed, 23 Jan 2008, john stultz wrote: > This difference in calculation was causing the clocksource correction > code to apply a correction factor to the clocksource so the two > intervals were the same, however this results in the actual frequency of > the clocksource to be made incorrect. I believe this difference would > affect all clocksources, although to differing degrees depending on the > clocksource resolution. Let's look at why the correction is done in first place. The update steps don't add up precisely to 1sec (LATCH*HZ != CLOCK_TICK_RATE), so a small addjustment is used to make up for it. The problem here is that if the update frequency changes, the addjustment isn't correct anymore. The simple fix is to just omit the addjustment in these cases in ntp.c: #if NTP_INTERVAL_FREQ == HZ ... #else #define CLOCK_TICK_ADJUST 0 #endif bye, Roman -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
Hi, On Wed, 23 Jan 2008, john stultz wrote: This difference in calculation was causing the clocksource correction code to apply a correction factor to the clocksource so the two intervals were the same, however this results in the actual frequency of the clocksource to be made incorrect. I believe this difference would affect all clocksources, although to differing degrees depending on the clocksource resolution. Let's look at why the correction is done in first place. The update steps don't add up precisely to 1sec (LATCH*HZ != CLOCK_TICK_RATE), so a small addjustment is used to make up for it. The problem here is that if the update frequency changes, the addjustment isn't correct anymore. The simple fix is to just omit the addjustment in these cases in ntp.c: #if NTP_INTERVAL_FREQ == HZ ... #else #define CLOCK_TICK_ADJUST 0 #endif bye, Roman -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Wed, 23 Jan 2008 18:38:53 PST, john stultz said: > I recently noticed on one of my boxes that when synched with an NTP > server, the drift value reported for the system was ~283ppm. While in > some cases, clock hardware can be that bad, it struck me as unusual as > the system was using the acpi_pm clocksource, which is one of the more > trustworthy and accurate clocksources on x86 hardware. > > I brought up another system and let it sync to the same NTP server, and > I noticed a similar 280some ppm drift. So I got curious, and dropped the patch onto my workstation - 24-rc8-mm1 x86_64 and it applied just fine with an offset reported. Before the patch, the drift was reported as 140.67 or so. After, it's sitting at -0.681. Am running with hpet clocksource rather than acpi_pm, which probably explains the 140 I see rather than the 280 John saw pgpLDb8meCQbR.pgp Description: PGP signature
Re: [PATCH] correct inconsistent ntp interval/tick_length usage
On Wed, 23 Jan 2008 18:38:53 PST, john stultz said: I recently noticed on one of my boxes that when synched with an NTP server, the drift value reported for the system was ~283ppm. While in some cases, clock hardware can be that bad, it struck me as unusual as the system was using the acpi_pm clocksource, which is one of the more trustworthy and accurate clocksources on x86 hardware. I brought up another system and let it sync to the same NTP server, and I noticed a similar 280some ppm drift. So I got curious, and dropped the patch onto my workstation - 24-rc8-mm1 x86_64 and it applied just fine with an offset reported. Before the patch, the drift was reported as 140.67 or so. After, it's sitting at -0.681. Am running with hpet clocksource rather than acpi_pm, which probably explains the 140 I see rather than the 280 John saw pgpLDb8meCQbR.pgp Description: PGP signature
