[Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

Keller, Jacob E jacob.e.keller at intel.com
Thu Oct 5 22:24:19 UTC 2017



> -----Original Message-----
> From: Duyck, Alexander H
> Sent: Thursday, October 05, 2017 11:32 AM
> To: Keller, Jacob E <jacob.e.keller at intel.com>
> Cc: Bowers, AndrewX <andrewx.bowers at intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set
> the CLEARPBA flag when re-enabling interrupts
> 
> On Tue, 2017-09-12 at 20:46 +0000, Bowers, AndrewX wrote:
> > > -----Original Message-----
> > > From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> > > Behalf Of Alice Michael
> > > Sent: Thursday, September 7, 2017 5:06 AM
> > > To: Michael, Alice <alice.michael at intel.com>; intel-wired-
> > > lan at lists.osuosl.org
> > > Subject: [Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set
> > > the CLEARPBA flag when re-enabling interrupts
> > >
> > > From: Jacob Keller <jacob.e.keller at intel.com>
> > >
> > > In the past we changed driver behavior to not clear the PBA when re-
> > > enabling interrupts. This change was motivated by the flawed belief that
> > > clearing the PBA would cause a lost interrupt if a receive interrupt occurred
> > > while interrupts were disabled.
> > >
> > > According to empirical testing this isn't the case. Additionally, the data sheet
> > > specifically says that we should set the CLEARPBA bit when re-enabling
> > > interrupts in a polling setup.
> > >
> > > This reverts commit 40d72a509862 ("i40e/i40evf: don't lose interrupts")
> > >
> > > Signed-off-by: Jacob Keller <jacob.e.keller at intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/i40e/i40e.h             |  5 +----
> > >  drivers/net/ethernet/intel/i40e/i40e_main.c        | 11 +++++------
> > >  drivers/net/ethernet/intel/i40e/i40e_txrx.c        |  6 ++----
> > >  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |  2 +-
> > >  drivers/net/ethernet/intel/i40evf/i40e_txrx.c      |  4 +---
> > >  5 files changed, 10 insertions(+), 18 deletions(-)
> >
> > Tested-by: Andrew Bowers <andrewx.bowers at intel.com>
> >
> 
> This patch just got pointed out to me in a hallway conversation.
> 
> I am pretty sure this _will_ cause you to lose interrupts. Specifically
> you shouldn't be clearing the PBA bit at the end of the polling routine
> unless you know you are going to poll again.
> 
> The PBA bit should only be cleared if:
> 1. You are at the start of your clean-up routine and want to clear it
> in case any additional work has come in since the original vector
> fired. (currently handled via an auto-clear function for MSI-X)
> 2. You are at the end of your polling routine and you know you are
> going to be polling again.
> 
> The original patch was more aggressive than it needed to be. For
> example you could probably go ahead and clear the PBA in the
> i40e_intr()q interrupt routine itself since all it is doing is
> scheduling the polling routine which will run after the interrupt
> routine has completed. You shouldn't be clearing the PBA at the end of
> NAPI poll unless you know you are not going to be exiting polling.
> 
> I hope this helps to clarify things.
> 
> - Alex

Ok so only clear it if we're continuing to poll. I'll cook up a patch for that.

Thanks,
Jake


More information about the Intel-wired-lan mailing list