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

["Íñigo Huguet" <ihuguet@redhat.com>]

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)

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

--
Íñigo Huguet