Message-ID: <ZIrHBZ3amADde4zE@gmail.com>
Date: Thu, 15 Jun 2023 09:08:37 +0100
From: Martin Habets <habetsm.xilinx@...il.com>
To: Jakub Kicinski <kuba@...nel.org>,
	Íñigo Huguet <ihuguet@...hat.com>
Cc: Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
	ecree.xilinx@...il.com, davem@...emloft.net, edumazet@...gle.com,
	pabeni@...hat.com, netdev@...r.kernel.org,
	linux-net-drivers@....com, Fei Liu <feliu@...hat.com>
Subject: Re: [PATCH net] sfc: use budget for TX completions

On Wed, Jun 14, 2023 at 10:31:31AM -0700, Jakub Kicinski wrote:
> On Wed, 14 Jun 2023 12:13:11 +0200 Íñigo Huguet wrote:
> > On Wed, Jun 14, 2023 at 10:03 AM Martin Habets <habetsm.xilinx@...il.com> wrote:
> > > On Mon, Jun 12, 2023 at 04:42:54PM +0200, Íñigo Huguet wrote:  
> > > > Documentations says "drivers can process completions for any number of Tx
> > > > packets but should only process up to budget number of Rx packets".
> > > > However, many drivers do limit the amount of TX completions that they
> > > > process in a single NAPI poll.  
> > >
> > > I think your work and what other drivers do shows that the documentation is
> > > no longer correct. I haven't checked when that was written, but maybe it
> > > was years ago when link speeds were lower.
> > > Clearly for drivers that support higher link speeds this is an issue, so we
> > > should update the documentation. Not sure what constitutes a high link speed,
> > > with current CPUs for me it's anything >= 50G.  
> > 
> > I reproduced with a 10G link (with debug kernel, though)
> 
> Ah.

Hmm, we usually don't optimise the driver for debug kernels. The delays
introduced can make it impossible for CPUs to keep up with high bandwidths.
Having said that I did not expect this to fail at only 10G.

> 
> > > > +#define EFX_NAPI_MAX_TX 512  
> > >
> > > How did you determine this value? Is it what other driver use?  
> > 
> > A bit of trial and error. I wanted to find a value high enough to not
> > decrease performance but low enough to solve the issue.
> > 
> > Other drivers use lower values too, from 128. However, I decided to go
> > to the high values in sfc because otherwise it can affect too much to
> > RX. The most common case I saw in other drivers was: First process TX
> > completions up to the established limit, then process RX completions
> > up to the NAPI budget. But sfc processes TX and RX events serially,
> > intermixed. We need to put a limit to TX events, but if it was too
> > low, very few RX events would be processed with high TX traffic.
> > 
> > > > I would better like to hear the opinion from the sfc maintainers, but
> > > > I don't mind changing it because I'm neither happy with the chosen
> > > > location.  
> > >
> > > I think we should add it in include/linux/netdevice.h, close to
> > > NAPI_POLL_WEIGHT. That way all drivers can use it.
> > > Do we need to add this TX poll weight to struct napi_struct and
> > > extend netif_napi_add_weight()?
> > > That way all drivers could use the value from napi_struct instead of using
> > > a hard-coded define. And at some point we can adjust it.  
> > 
> > That's what I thought too, but then we'd need to determine what's the
> > exact meaning for that TX budget (as you see, it doesn't mean exactly
> > the same for sfc than for other drivers, and between the other drivers
> > there were small differences too).
> > 
> > We would also need to decide what the default value for the TX budget
> > is, so it is used in netif_napi_add. Right now, each driver is using
> > different values.
> > 
> > If something is done in that direction, it can take some time. May I
> > suggest including this fix until then?
> 
> Agreed. Still needs a fixes tag, tho.

Fine for me. From your other response it seems trying to generalise this
is not the longer term solution.

Martin

Powered by blists - more mailing lists