Re: [PATCH net] sfc: use budget for TX completions

[Jakub Kicinski <kuba@kernel.org>]

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@gmail.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.

> > > +#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.