| Message ID | 1461167156-6737-3-git-send-email-dvlasenk@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Jeff Kirsher |
| Headers | show |
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On > Behalf Of Denys Vlasenko > Sent: Wednesday, April 20, 2016 8:46 AM > To: intel-wired-lan@lists.osuosl.org > Cc: Denys Vlasenko <dvlasenk@redhat.com> > Subject: [Intel-wired-lan] [PATCH 3/3] e1000e: e1000e_cyclecounter_read(): > do overflow check only if needed > > SYSTIMH:SYSTIML registers are incremented by 24-bit value TIMINCA[23..0] > > er32(SYSTIML) are probably moderately expensive (they are pci bus reads). > Can we avoid one of them? Yes, we can. > > If the SYSTIML value we see is smaller than 0xff000000, the overflow > into SYSTIMH would require at least two increments. > > We do two reads, er32(SYSTIML) and er32(SYSTIMH), in this order. > > Even if one increment happens between them, the overflow into SYSTIMH > is impossible, and we can avoid doing another er32(SYSTIML) read > and overflow check. > > Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com> > CC: intel-wired-lan@lists.osuosl.org > --- > drivers/net/ethernet/intel/e1000e/netdev.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) This one throws a different checkpatch warning: WARNING: Missing a blank line after declarations #61: FILE: drivers/net/ethernet/intel/e1000e/netdev.c:4327: + u32 systimel_2 = er32(SYSTIML); + if (systimel > systimel_2) { Still just a warning and I'm not seeing functional problems with it. Tested-by: Aaron Brown <aaron.f.brown@intel.com>
On 04/26/2016 01:12 AM, Brown, Aaron F wrote: >> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On >> Behalf Of Denys Vlasenko >> Sent: Wednesday, April 20, 2016 8:46 AM >> To: intel-wired-lan@lists.osuosl.org >> Cc: Denys Vlasenko <dvlasenk@redhat.com> >> Subject: [Intel-wired-lan] [PATCH 3/3] e1000e: e1000e_cyclecounter_read(): >> do overflow check only if needed >> >> SYSTIMH:SYSTIML registers are incremented by 24-bit value TIMINCA[23..0] >> >> er32(SYSTIML) are probably moderately expensive (they are pci bus reads). >> Can we avoid one of them? Yes, we can. >> >> If the SYSTIML value we see is smaller than 0xff000000, the overflow >> into SYSTIMH would require at least two increments. >> >> We do two reads, er32(SYSTIML) and er32(SYSTIMH), in this order. >> >> Even if one increment happens between them, the overflow into SYSTIMH >> is impossible, and we can avoid doing another er32(SYSTIML) read >> and overflow check. >> ... > Still just a warning and I'm not seeing functional problems with it. > > Tested-by: Aaron Brown <aaron.f.brown@intel.com> Aaron, there were some comments from Dima Ruinskiy <dima.ruinskiy@intel.com> that I might be underestimating the rate how quickly this counter increments: that it may be in fact incrementing something like every 40ns. In this case, more than one increment may happen between these two PCIe reads: systimel = er32(SYSTIML); systimeh = er32(SYSTIMH); and my check /* Is systimel is so large that overflow is possible? */ if (systimel >= (u32)0xffffffff - E1000_TIMINCA_INCVALUE_MASK) { can fail to catch an overflow. If you (Intel guys) indeed see that as possible, then either drop the 3/3 patch, or tweak the constant so that more than one increment is needed for overflow. Say: /* Is systimel is so large that overflow is possible? */ if (systimel >= (u32)0xf0000000) { This particular constant in comparison allows to skip overflow check if we are more than 16 updates away from overflow - should be ample.
> From: Denys Vlasenko [mailto:dvlasenk@redhat.com] > Sent: Tuesday, April 26, 2016 9:03 AM > To: Brown, Aaron F <aaron.f.brown@intel.com>; intel-wired- > lan@lists.osuosl.org > Subject: Re: [Intel-wired-lan] [PATCH 3/3] e1000e: > e1000e_cyclecounter_read(): do overflow check only if needed > > On 04/26/2016 01:12 AM, Brown, Aaron F wrote: > >> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] > On > >> Behalf Of Denys Vlasenko > >> Sent: Wednesday, April 20, 2016 8:46 AM > >> To: intel-wired-lan@lists.osuosl.org > >> Cc: Denys Vlasenko <dvlasenk@redhat.com> > >> Subject: [Intel-wired-lan] [PATCH 3/3] e1000e: > e1000e_cyclecounter_read(): > >> do overflow check only if needed > >> > >> SYSTIMH:SYSTIML registers are incremented by 24-bit value > TIMINCA[23..0] > >> > >> er32(SYSTIML) are probably moderately expensive (they are pci bus > reads). > >> Can we avoid one of them? Yes, we can. > >> > >> If the SYSTIML value we see is smaller than 0xff000000, the overflow > >> into SYSTIMH would require at least two increments. > >> > >> We do two reads, er32(SYSTIML) and er32(SYSTIMH), in this order. > >> > >> Even if one increment happens between them, the overflow into SYSTIMH > >> is impossible, and we can avoid doing another er32(SYSTIML) read > >> and overflow check. > >> > ... > > Still just a warning and I'm not seeing functional problems with it. > > > > Tested-by: Aaron Brown <aaron.f.brown@intel.com> > > Aaron, there were some comments from Dima Ruinskiy > <dima.ruinskiy@intel.com> > that I might be underestimating the rate how quickly this counter increments: > that it may be in fact incrementing something like every 40ns. I did see that come through, for some reason the comment did not get captured in https://patchwork.ozlabs.org/patch/612833/ which I was reading from when I ran it through my regression rig. Thanks for catching it. Yes, I guess we should hold off on this until Raanan can weigh in. Jeff, please ignore thisTested by for now. > > In this case, more than one increment may happen between these two PCIe > reads: > > systimel = er32(SYSTIML); > systimeh = er32(SYSTIMH); > > and my check > > /* Is systimel is so large that overflow is possible? */ > if (systimel >= (u32)0xffffffff - E1000_TIMINCA_INCVALUE_MASK) { > > can fail to catch an overflow. > > If you (Intel guys) indeed see that as possible, then either drop the 3/3 > patch, or tweak the constant so that more than one increment is needed > for overflow. Say: > > /* Is systimel is so large that overflow is possible? */ > if (systimel >= (u32)0xf0000000) { > > This particular constant in comparison allows to skip overflow check > if we are more than 16 updates away from overflow - should be ample.
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c index 99d0e6e..6f17f89 100644 --- a/drivers/net/ethernet/intel/e1000e/netdev.c +++ b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -4275,7 +4275,7 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc) struct e1000_adapter *adapter = container_of(cc, struct e1000_adapter, cc); struct e1000_hw *hw = &adapter->hw; - u32 systimel_1, systimel_2, systimeh; + u32 systimel, systimeh; cycle_t systim, systim_next; /* SYSTIMH latching upon SYSTIML read does not work well. * This means that if SYSTIML overflows after we read it but before @@ -4283,21 +4283,21 @@ static cycle_t e1000e_cyclecounter_read(const struct cyclecounter *cc) * will experience a huge non linear increment in the systime value * to fix that we test for overflow and if true, we re-read systime. */ - systimel_1 = er32(SYSTIML); + systimel = er32(SYSTIML); systimeh = er32(SYSTIMH); - systimel_2 = er32(SYSTIML); - /* Check for overflow. If there was no overflow, use the values */ - if (systimel_1 <= systimel_2) { - systim = (cycle_t)systimel_1; - systim |= (cycle_t)systimeh << 32; - } else { - /* There was an overflow, read again SYSTIMH, and use - * systimel_2 - */ - systimeh = er32(SYSTIMH); - systim = (cycle_t)systimel_2; - systim |= (cycle_t)systimeh << 32; + /* Is systimel is so large that overflow is possible? */ + if (systimel >= (u32)0xffffffff - E1000_TIMINCA_INCVALUE_MASK) { + u32 systimel_2 = er32(SYSTIML); + if (systimel > systimel_2) { + /* There was an overflow, read again SYSTIMH, and use + * systimel_2 + */ + systimeh = er32(SYSTIMH); + systimel = systimel_2; + } } + systim = (cycle_t)systimel; + systim |= (cycle_t)systimeh << 32; if ((hw->mac.type == e1000_82574) || (hw->mac.type == e1000_82583)) { u64 time_delta, rem, temp;
SYSTIMH:SYSTIML registers are incremented by 24-bit value TIMINCA[23..0] er32(SYSTIML) are probably moderately expensive (they are pci bus reads). Can we avoid one of them? Yes, we can. If the SYSTIML value we see is smaller than 0xff000000, the overflow into SYSTIMH would require at least two increments. We do two reads, er32(SYSTIML) and er32(SYSTIMH), in this order. Even if one increment happens between them, the overflow into SYSTIMH is impossible, and we can avoid doing another er32(SYSTIML) read and overflow check. Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com> CC: intel-wired-lan@lists.osuosl.org --- drivers/net/ethernet/intel/e1000e/netdev.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)