Re: [RFC net-next v3 2/4] virtio_net: Prepare for NAPI to queue mapping

[Joe Damato <jdamato@fastly.com>]

On Fri, Jan 24, 2025 at 09:14:54AM +0800, Jason Wang wrote:
> On Thu, Jan 23, 2025 at 10:47 AM Joe Damato <jdamato@fastly.com> wrote:
> >
> > On Thu, Jan 23, 2025 at 10:40:43AM +0800, Jason Wang wrote:
> > > On Thu, Jan 23, 2025 at 1:41 AM Joe Damato <jdamato@fastly.com> wrote:
> > > >
> > > > On Wed, Jan 22, 2025 at 02:12:46PM +0800, Jason Wang wrote:
> > > > > On Wed, Jan 22, 2025 at 3:11 AM Joe Damato <jdamato@fastly.com> wrote:
> > > > > >
> > > > > > Slight refactor to prepare the code for NAPI to queue mapping. No
> > > > > > functional changes.
> > > > > >
> > > > > > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > > > > > Reviewed-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> > > > > > Tested-by: Lei Yang <leiyang@redhat.com>
> > > > > > ---
> > > > > >  v2:
> > > > > >    - Previously patch 1 in the v1.
> > > > > >    - Added Reviewed-by and Tested-by tags to commit message. No
> > > > > >      functional changes.
> > > > > >
> > > > > >  drivers/net/virtio_net.c | 10 ++++++++--
> > > > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > index 7646ddd9bef7..cff18c66b54a 100644
> > > > > > --- a/drivers/net/virtio_net.c
> > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > @@ -2789,7 +2789,8 @@ static void skb_recv_done(struct virtqueue *rvq)
> > > > > >         virtqueue_napi_schedule(&rq->napi, rvq);
> > > > > >  }
> > > > > >
> > > > > > -static void virtnet_napi_enable(struct virtqueue *vq, struct napi_struct *napi)
> > > > > > +static void virtnet_napi_do_enable(struct virtqueue *vq,
> > > > > > +                                  struct napi_struct *napi)
> > > > > >  {
> > > > > >         napi_enable(napi);
> > > > >
> > > > > Nit: it might be better to not have this helper to avoid a misuse of
> > > > > this function directly.
> > > >
> > > > Sorry, I'm probably missing something here.
> > > >
> > > > Both virtnet_napi_enable and virtnet_napi_tx_enable need the logic
> > > > in virtnet_napi_do_enable.
> > > >
> > > > Are you suggesting that I remove virtnet_napi_do_enable and repeat
> > > > the block of code in there twice (in virtnet_napi_enable and
> > > > virtnet_napi_tx_enable)?
> > >
> > > I think I miss something here, it looks like virtnet_napi_tx_enable()
> > > calls virtnet_napi_do_enable() directly.
> > >
> > > I would like to know why we don't call netif_queue_set_napi() for TX NAPI here?
> >
> > Please see both the cover letter and the commit message of the next
> > commit which addresses this question.
> >
> > TX-only NAPIs do not have NAPI IDs so there is nothing to map.
> 
> Interesting, but I have more questions
> 
> 1) why need a driver to know the NAPI implementation like this?

I'm not sure I understand the question, but I'll try to give an
answer and please let me know if you have another question.

Mapping the NAPI IDs to queue IDs is useful for applications that
use epoll based busy polling (which relies on the NAPI ID, see also
SO_INCOMING_NAPI_ID and [1]), IRQ suspension [2], and generally
per-NAPI configuration [3].

Without this code added to the driver, the user application can get
the NAPI ID of an incoming connection, but has no way to know which
queue (or NIC) that NAPI ID is associated with or to set per-NAPI
configuration settings.

[1]: https://lore.kernel.org/all/20240213061652.6342-1-jdamato@fastly.com/
[2]: https://lore.kernel.org/netdev/20241109050245.191288-5-jdamato@fastly.com/T/
[3]: https://lore.kernel.org/lkml/20241011184527.16393-1-jdamato@fastly.com/

> 2) does NAPI know (or why it needs to know) whether or not it's a TX
> or not? I only see the following code in napi_hash_add():

Note that I did not write the original implementation of NAPI IDs or
epoll-based busy poll, so I can only comment on what I know :)

I don't know why TX-only NAPIs do not have NAPI IDs. My guess is
that in the original implementation, the code was designed only for
RX busy polling, so TX-only NAPIs were not assigned NAPI IDs.

Perhaps in the future, TX-only NAPIs will be assigned NAPI IDs, but
currently they do not have NAPI IDs.

> static void napi_hash_add(struct napi_struct *napi)
> {
>         unsigned long flags;
> 
>         if (test_bit(NAPI_STATE_NO_BUSY_POLL, &napi->state))
>                 return;
> 
> ...
> 
>         __napi_hash_add_with_id(napi, napi_gen_id);
> 
>         spin_unlock_irqrestore(&napi_hash_lock, flags);
> }
> 
> It seems it only matters with NAPI_STATE_NO_BUSY_POLL.
> 
> And if NAPI knows everything, should it be better to just do the
> linking in napi_enable/disable() instead of letting each driver do it
> by itself?

It would be nice if this were possible, I agree. Perhaps in the
future some work could be done to make this possible.

I believe that this is not currently possible because the NAPI does
not know which queue ID it is associated with. That mapping of which
queue is associated with which NAPI is established in patch 3
(please see the commit message of patch 3 to see an example of the
output).

The driver knows both the queue ID and the NAPI for that queue, so
the mapping can be established only by the driver.

Let me know if that helps.

- Joe