On Thu, Jan 23, 2025 at 10:47 AM Joe Damato [off-list ref] wrote:

On Thu, Jan 23, 2025 at 10:40:43AM +0800, Jason Wang wrote:

quoted

On Thu, Jan 23, 2025 at 1:41 AM Joe Damato [off-list ref] wrote:

quoted

On Wed, Jan 22, 2025 at 02:12:46PM +0800, Jason Wang wrote:

quoted

On Wed, Jan 22, 2025 at 3:11 AM Joe Damato [off-list ref] wrote:

quoted

Slight refactor to prepare the code for NAPI to queue mapping. No
functional changes.

Signed-off-by: Joe Damato <redacted>
Reviewed-by: Gerhard Engleder <redacted>
Tested-by: Lei Yang <redacted>
---
 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?

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():

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?

Thanks