| Message ID | 20181012111339.1361-1-mlichvar@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Jeff Kirsher |
| Headers | show |
| Series | igb: shorten maximum PHC timecounter update interval | expand |
On Fri, Oct 12, 2018 at 01:13:39PM +0200, Miroslav Lichvar wrote: > Since commit 500462a9d ("timers: Switch to a non-cascading wheel"), > scheduling of delayed work seems to be less accurate and a requested > delay of 540 seconds may actually be longer than 550 seconds. Shorten > the delay to 480 seconds to be sure the timecounter is updated in time. Good catch. This timer wheel change will affect other, similar drivers. Guess I'll go through and adjust their timeouts, too. Thanks, Richard
On Fri, Oct 12, 2018 at 01:13:39PM +0200, Miroslav Lichvar wrote: > This fixes an issue with HW timestamps on 82580/I350/I354 being off by > ~1100 seconds for few seconds every ~9 minutes. This patch should go to the stable trees starting with v4.8. Thanks, Richard
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On > Behalf Of Miroslav Lichvar > Sent: Friday, October 12, 2018 4:14 AM > To: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org > Cc: Thomas Gleixner <tglx@linutronix.de>; Richard Cochran > <richardcochran@gmail.com> > Subject: [Intel-wired-lan] [PATCH] igb: shorten maximum PHC timecounter > update interval > > The timecounter needs to be updated at least once per ~550 seconds in > order to avoid a 40-bit SYSTIM timestamp to be misinterpreted as an old > timestamp. > > Since commit 500462a9d ("timers: Switch to a non-cascading wheel"), > scheduling of delayed work seems to be less accurate and a requested > delay of 540 seconds may actually be longer than 550 seconds. Shorten > the delay to 480 seconds to be sure the timecounter is updated in time. > > This fixes an issue with HW timestamps on 82580/I350/I354 being off by > ~1100 seconds for few seconds every ~9 minutes. > > Cc: Jacob Keller <jacob.e.keller@intel.com> > Cc: Richard Cochran <richardcochran@gmail.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com> > --- > drivers/net/ethernet/intel/igb/igb_ptp.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > Tested-by: Aaron Brown <aaron.f.brown@intel.com>
On Fri, Oct 12, 2018 at 07:05:30AM -0700, Richard Cochran wrote: > On Fri, Oct 12, 2018 at 01:13:39PM +0200, Miroslav Lichvar wrote: > > Since commit 500462a9d ("timers: Switch to a non-cascading wheel"), > > scheduling of delayed work seems to be less accurate and a requested > > delay of 540 seconds may actually be longer than 550 seconds. Shorten > > the delay to 480 seconds to be sure the timecounter is updated in time. > > Good catch. This timer wheel change will affect other, similar > drivers. Guess I'll go through and adjust their timeouts, too. I just realized that we need to fit there also any frequency adjustments of the PHC and system clock. The PHC can be set to run up to 6% faster and the system clock can be slowed down by up to 10%. Those 480 seconds in the igb driver is not short enough for that. Should I fix and resend this patch, or send a new one? Other drivers may have a similar problem.
> -----Original Message----- > From: Miroslav Lichvar [mailto:mlichvar@redhat.com] > Sent: Friday, October 26, 2018 5:04 AM > To: Richard Cochran <richardcochran@gmail.com> > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Keller, Jacob E > <jacob.e.keller@intel.com>; Thomas Gleixner <tglx@linutronix.de> > Subject: Re: [PATCH] igb: shorten maximum PHC timecounter update interval > > On Fri, Oct 12, 2018 at 07:05:30AM -0700, Richard Cochran wrote: > > On Fri, Oct 12, 2018 at 01:13:39PM +0200, Miroslav Lichvar wrote: > > > Since commit 500462a9d ("timers: Switch to a non-cascading wheel"), > > > scheduling of delayed work seems to be less accurate and a requested > > > delay of 540 seconds may actually be longer than 550 seconds. Shorten > > > the delay to 480 seconds to be sure the timecounter is updated in time. > > > > Good catch. This timer wheel change will affect other, similar > > drivers. Guess I'll go through and adjust their timeouts, too. > > I just realized that we need to fit there also any frequency > adjustments of the PHC and system clock. The PHC can be set to run up > to 6% faster and the system clock can be slowed down by up to 10%. > > Those 480 seconds in the igb driver is not short enough for that. > Should I fix and resend this patch, or send a new one? > > Other drivers may have a similar problem. > Hmm, good point. I'd send a v2 of this patch, unless it's already been applied to net or net-next. Thanks, Jake > -- > Miroslav Lichvar
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c index 9f4d700e09df..29ced6b74d36 100644 --- a/drivers/net/ethernet/intel/igb/igb_ptp.c +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c @@ -51,9 +51,15 @@ * * The 40 bit 82580 SYSTIM overflows every * 2^40 * 10^-9 / 60 = 18.3 minutes. + * + * SYSTIM is converted to real time using a timecounter. As + * timecounter_cyc2time() allows old timestamps, the timecounter + * needs to be updated at least once per half of the SYSTIM interval. + * Scheduling of delayed work is not very accurate, so we aim for 8 + * minutes to be sure the actual interval is shorter than 9.16 minutes. */ -#define IGB_SYSTIM_OVERFLOW_PERIOD (HZ * 60 * 9) +#define IGB_SYSTIM_OVERFLOW_PERIOD (HZ * 60 * 8) #define IGB_PTP_TX_TIMEOUT (HZ * 15) #define INCPERIOD_82576 BIT(E1000_TIMINCA_16NS_SHIFT) #define INCVALUE_82576_MASK GENMASK(E1000_TIMINCA_16NS_SHIFT - 1, 0)
The timecounter needs to be updated at least once per ~550 seconds in order to avoid a 40-bit SYSTIM timestamp to be misinterpreted as an old timestamp. Since commit 500462a9d ("timers: Switch to a non-cascading wheel"), scheduling of delayed work seems to be less accurate and a requested delay of 540 seconds may actually be longer than 550 seconds. Shorten the delay to 480 seconds to be sure the timecounter is updated in time. This fixes an issue with HW timestamps on 82580/I350/I354 being off by ~1100 seconds for few seconds every ~9 minutes. Cc: Jacob Keller <jacob.e.keller@intel.com> Cc: Richard Cochran <richardcochran@gmail.com> Cc: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com> --- drivers/net/ethernet/intel/igb/igb_ptp.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)