[PATCH] igb: shorten maximum PHC timecounter update interval

[PATCH] igb: shorten maximum PHC timecounter update interval

[Miroslav Lichvar]

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(-)

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)
-- 
2.17.1

[Intel-wired-lan] [PATCH] igb: shorten maximum PHC timecounter update interval

[Miroslav Lichvar]

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(-)

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)
-- 
2.17.1

Re: [PATCH] igb: shorten maximum PHC timecounter update interval

[Richard Cochran]

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

[Intel-wired-lan] [PATCH] igb: shorten maximum PHC timecounter update interval

[Richard Cochran]

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

Re: [PATCH] igb: shorten maximum PHC timecounter update interval

[Miroslav Lichvar]

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.

-- 
Miroslav Lichvar

[Intel-wired-lan] [PATCH] igb: shorten maximum PHC timecounter update interval

[Miroslav Lichvar]

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.

-- 
Miroslav Lichvar

RE: [PATCH] igb: shorten maximum PHC timecounter update interval

[Keller, Jacob E]

> -----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

[Intel-wired-lan] [PATCH] igb: shorten maximum PHC timecounter update interval

[Keller, Jacob E]

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mlichvar at redhat.com]
> Sent: Friday, October 26, 2018 5:04 AM
> To: Richard Cochran <richardcochran@gmail.com>
> Cc: intel-wired-lan at lists.osuosl.org; netdev at 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

Re: [PATCH] igb: shorten maximum PHC timecounter update interval

[Richard Cochran]

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

[Intel-wired-lan] [PATCH] igb: shorten maximum PHC timecounter update interval

[Richard Cochran]

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

RE: [Intel-wired-lan] [PATCH] igb: shorten maximum PHC timecounter update interval

[Brown, Aaron F]

> 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>

[Intel-wired-lan] [PATCH] igb: shorten maximum PHC timecounter update interval

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Miroslav Lichvar
> Sent: Friday, October 12, 2018 4:14 AM
> To: intel-wired-lan at lists.osuosl.org; netdev at 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>