Message-ID: <20240411184740.782809eb@kernel.org>
Date: Thu, 11 Apr 2024 18:47:40 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: "Nambiar, Amritha" <amritha.nambiar@...el.com>
Cc: <netdev@...r.kernel.org>, <davem@...emloft.net>, <edumazet@...gle.com>,
 <pabeni@...hat.com>, <ast@...nel.org>, <sdf@...gle.com>,
 <lorenzo@...nel.org>, <tariqt@...dia.com>, <daniel@...earbox.net>,
 <anthony.l.nguyen@...el.com>, <lucien.xin@...il.com>, <hawk@...nel.org>,
 <sridhar.samudrala@...el.com>
Subject: Re: [net-next,RFC PATCH 0/5] Configuring NAPI instance for a queue

On Thu, 11 Apr 2024 15:46:45 -0700 Nambiar, Amritha wrote:
> On 4/9/2024 4:21 PM, Jakub Kicinski wrote:
> > On Fri, 05 Apr 2024 13:09:28 -0700 Amritha Nambiar wrote:  
> >> $ ./cli.py --spec netdev.yaml --do queue-set  --json='{"ifindex": 12, "id": 0, "type": 0, "napi-id": 595}'
> >> {'id': 0, 'ifindex': 12, 'napi-id': 595, 'type': 'rx'}  
> > 
> > NAPI ID is not stable. What happens if you set the ID, bring the
> > device down and up again? I think we need to make NAPI IDs stable.
> 
> I tried this (device down/up and check NAPIs) on both bnxt and intel/ice.
> On bnxt: New NAPI IDs are created sequentially once the device is up 
> after turning down.
> On ice: The NAPI IDs are stable and remains the same once the device is 
> up after turning down.
> 
> In case of ice, device down/up executes napi_disable/napi_enable. The 
> NAPI IDs are not lost as netif_napi_del is not called at IFF_DOWN. On 
> IFF_DOWN, the IRQs associations with the OS are freed, but the resources 
> allocated for the vectors and hence the NAPIs for the vectors persists 
> (unless unload/reconfig).

SG! So let's just make sure we cover that in tests.

> > What happens if you change the channel count? Do we lose the config?
> > We try never to lose explicit user config. I think for simplicity
> > we should store the config in the core / common code.
> 
> Yes, we lose the config in case of re-configuring channels. The reconfig 
> path involves freeing the vectors and reallocating based on the new 
> channel config, so, for the NAPIs associated with the vectors, 
> netif_napi_del and netif_napi_add executes creating new NAPI IDs 
> sequentially.
> 
> Wouldn't losing the explicit user config make sense in this case? By 
> changing the channel count, the user has updated the queue layout, the 
> queue<>vector mappings etc., so I think, the previous configs from set 
> queue<>NAPI should be overwritten with the new config from set-channel.

We do prevent indirection table from being reset on channel count
change. I think same logic applies here..

> > How does the user know whether queue <> NAPI association is based
> > on driver defaults or explicit configuration?  
> 
> I am not sure of this. ethtool shows pre-set defaults and current 
> settings, but in this case, it is tricky :(

Can you say more about the use case for moving the queues around?
If you just want to have fewer NAPI vectors and more queues, but
don't care about exact mapping - we could probably come up with
a simpler API, no? Are the queues stack queues or also AF_XDP?

> > I think I mentioned
> > this in earlier discussions but the configuration may need to be
> > detached from the existing objects (for one thing they may not exist
> > at all when the device is down).
> 
> Yes, we did have that discussion about detaching queues from NAPI. But, 
> I am not sure how to accomplish that. Any thoughts on what other 
> possible object can be used for the configuration?

We could stick to the queue as the object perhaps. The "target NAPI"
would just be part of the config passed to the alloc/start callbacks.

> WRT ice, when the device is down, the queues are listed and exists as 
> inactive queues, NAPI IDs exists, IRQs associations with the OS are freed.
> 
> > Last but not least your driver patch implements the start/stop steps
> > of the "queue API" I think we should pull that out into the core.
> 
> Agree, it would be good to have these steps in the core, but I think the 
> challenge is that we would still end up with a lot of code in the driver 
> as well, due to all the hardware-centric bits in it.

For one feature I think adding code in the core is not beneficial.
But we have multiple adjacent needs, so when we add up your work,
zero copy, page pool config, maybe queue alloc.. hopefully the code
in the core will be net positive.

> > Also the tests now exist - take a look at the sample one in
> > tools/testing/selftests/drivers/net/stats.py
> > Would be great to have all future netdev family extensions accompanied
> > by tests which can run both on real HW and netdevsim.  
> 
> Okay, I will write tests for the new extensions here.

Powered by blists - more mailing lists