| Message ID | 20240924234851.42348-2-jdamato@fastly.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | hyperv: Link queues to NAPIs | expand |
> -----Original Message----- > From: Joe Damato <jdamato@fastly.com> > Sent: Tuesday, September 24, 2024 7:49 PM > To: netdev@vger.kernel.org > Cc: Joe Damato <jdamato@fastly.com>; KY Srinivasan <kys@microsoft.com>; > Haiyang Zhang <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>; > Dexuan Cui <decui@microsoft.com>; David S. Miller <davem@davemloft.net>; > Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; > Paolo Abeni <pabeni@redhat.com>; open list:Hyper-V/Azure CORE AND DRIVERS > <linux-hyperv@vger.kernel.org>; open list <linux-kernel@vger.kernel.org> > Subject: [RFC net-next 1/1] hv_netvsc: Link queues to NAPIs > > [You don't often get email from jdamato@fastly.com. Learn why this is > important at https://aka.ms/LearnAboutSenderIdentification ] > > Use netif_queue_set_napi to link queues to NAPI instances so that they > can be queried with netlink. > > Signed-off-by: Joe Damato <jdamato@fastly.com> > --- > drivers/net/hyperv/netvsc.c | 11 ++++++++++- > drivers/net/hyperv/rndis_filter.c | 9 +++++++-- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > index 2b6ec979a62f..ccaa4690dba0 100644 > --- a/drivers/net/hyperv/netvsc.c > +++ b/drivers/net/hyperv/netvsc.c > @@ -712,8 +712,11 @@ void netvsc_device_remove(struct hv_device *device) > for (i = 0; i < net_device->num_chn; i++) { > /* See also vmbus_reset_channel_cb(). */ > /* only disable enabled NAPI channel */ > - if (i < ndev->real_num_rx_queues) > + if (i < ndev->real_num_rx_queues) { > + netif_queue_set_napi(ndev, i, > NETDEV_QUEUE_TYPE_TX, NULL); > + netif_queue_set_napi(ndev, i, > NETDEV_QUEUE_TYPE_RX, NULL); > napi_disable(&net_device->chan_table[i].napi); > + } > > netif_napi_del(&net_device->chan_table[i].napi); > } > @@ -1787,6 +1790,10 @@ struct netvsc_device *netvsc_device_add(struct > hv_device *device, > netdev_dbg(ndev, "hv_netvsc channel opened successfully\n"); > > napi_enable(&net_device->chan_table[0].napi); > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_RX, > + &net_device->chan_table[0].napi); > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_TX, > + &net_device->chan_table[0].napi); > > /* Connect with the NetVsp */ > ret = netvsc_connect_vsp(device, net_device, device_info); > @@ -1805,6 +1812,8 @@ struct netvsc_device *netvsc_device_add(struct > hv_device *device, > > close: > RCU_INIT_POINTER(net_device_ctx->nvdev, NULL); > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_TX, NULL); > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_RX, NULL); > napi_disable(&net_device->chan_table[0].napi); > > /* Now, we can close the channel safely */ > diff --git a/drivers/net/hyperv/rndis_filter.c > b/drivers/net/hyperv/rndis_filter.c > index ecc2128ca9b7..c0ceeef4fcd8 100644 > --- a/drivers/net/hyperv/rndis_filter.c > +++ b/drivers/net/hyperv/rndis_filter.c > @@ -1269,10 +1269,15 @@ static void netvsc_sc_open(struct vmbus_channel > *new_sc) > ret = vmbus_open(new_sc, netvsc_ring_bytes, > netvsc_ring_bytes, NULL, 0, > netvsc_channel_cb, nvchan); > - if (ret == 0) > + if (ret == 0) { > napi_enable(&nvchan->napi); > - else > + netif_queue_set_napi(ndev, chn_index, > NETDEV_QUEUE_TYPE_RX, > + &nvchan->napi); > + netif_queue_set_napi(ndev, chn_index, > NETDEV_QUEUE_TYPE_TX, > + &nvchan->napi); > + } else { > netdev_notice(ndev, "sub channel open failed: %d\n", > ret); > + } > > if (atomic_inc_return(&nvscdev->open_chn) == nvscdev->num_chn) > wake_up(&nvscdev->subchan_open); > -- The code change looks fine to me. @Shradha Gupta or @Erni Sri Satya Vennela, Do you have time to test this? Thanks, - Haiyang
On Wed, Sep 25, 2024 at 07:39:03PM +0000, Haiyang Zhang wrote: > > > > -----Original Message----- > > From: Joe Damato <jdamato@fastly.com> > > Sent: Tuesday, September 24, 2024 7:49 PM > > To: netdev@vger.kernel.org > > Cc: Joe Damato <jdamato@fastly.com>; KY Srinivasan <kys@microsoft.com>; > > Haiyang Zhang <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>; > > Dexuan Cui <decui@microsoft.com>; David S. Miller <davem@davemloft.net>; > > Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; > > Paolo Abeni <pabeni@redhat.com>; open list:Hyper-V/Azure CORE AND DRIVERS > > <linux-hyperv@vger.kernel.org>; open list <linux-kernel@vger.kernel.org> > > Subject: [RFC net-next 1/1] hv_netvsc: Link queues to NAPIs > > > > [You don't often get email from jdamato@fastly.com. Learn why this is > > important at https://aka.ms/LearnAboutSenderIdentification ] > > > > Use netif_queue_set_napi to link queues to NAPI instances so that they > > can be queried with netlink. > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > --- > > drivers/net/hyperv/netvsc.c | 11 ++++++++++- > > drivers/net/hyperv/rndis_filter.c | 9 +++++++-- > > 2 files changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > > index 2b6ec979a62f..ccaa4690dba0 100644 > > --- a/drivers/net/hyperv/netvsc.c > > +++ b/drivers/net/hyperv/netvsc.c > > @@ -712,8 +712,11 @@ void netvsc_device_remove(struct hv_device *device) > > for (i = 0; i < net_device->num_chn; i++) { > > /* See also vmbus_reset_channel_cb(). */ > > /* only disable enabled NAPI channel */ > > - if (i < ndev->real_num_rx_queues) > > + if (i < ndev->real_num_rx_queues) { > > + netif_queue_set_napi(ndev, i, > > NETDEV_QUEUE_TYPE_TX, NULL); > > + netif_queue_set_napi(ndev, i, > > NETDEV_QUEUE_TYPE_RX, NULL); > > napi_disable(&net_device->chan_table[i].napi); > > + } > > > > netif_napi_del(&net_device->chan_table[i].napi); > > } > > @@ -1787,6 +1790,10 @@ struct netvsc_device *netvsc_device_add(struct > > hv_device *device, > > netdev_dbg(ndev, "hv_netvsc channel opened successfully\n"); > > > > napi_enable(&net_device->chan_table[0].napi); > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_RX, > > + &net_device->chan_table[0].napi); > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_TX, > > + &net_device->chan_table[0].napi); > > > > /* Connect with the NetVsp */ > > ret = netvsc_connect_vsp(device, net_device, device_info); > > @@ -1805,6 +1812,8 @@ struct netvsc_device *netvsc_device_add(struct > > hv_device *device, > > > > close: > > RCU_INIT_POINTER(net_device_ctx->nvdev, NULL); > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_TX, NULL); > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_RX, NULL); > > napi_disable(&net_device->chan_table[0].napi); > > > > /* Now, we can close the channel safely */ > > diff --git a/drivers/net/hyperv/rndis_filter.c > > b/drivers/net/hyperv/rndis_filter.c > > index ecc2128ca9b7..c0ceeef4fcd8 100644 > > --- a/drivers/net/hyperv/rndis_filter.c > > +++ b/drivers/net/hyperv/rndis_filter.c > > @@ -1269,10 +1269,15 @@ static void netvsc_sc_open(struct vmbus_channel > > *new_sc) > > ret = vmbus_open(new_sc, netvsc_ring_bytes, > > netvsc_ring_bytes, NULL, 0, > > netvsc_channel_cb, nvchan); > > - if (ret == 0) > > + if (ret == 0) { > > napi_enable(&nvchan->napi); > > - else > > + netif_queue_set_napi(ndev, chn_index, > > NETDEV_QUEUE_TYPE_RX, > > + &nvchan->napi); > > + netif_queue_set_napi(ndev, chn_index, > > NETDEV_QUEUE_TYPE_TX, > > + &nvchan->napi); > > + } else { > > netdev_notice(ndev, "sub channel open failed: %d\n", > > ret); > > + } > > > > if (atomic_inc_return(&nvscdev->open_chn) == nvscdev->num_chn) > > wake_up(&nvscdev->subchan_open); > > -- > > The code change looks fine to me. > @Shradha Gupta or @Erni Sri Satya Vennela, Do you have time to test this? > > Thanks, > - Haiyang > > Sure, we will review and test and get back. Thanks
On Wed, Sep 25, 2024 at 07:39:03PM +0000, Haiyang Zhang wrote: > > > > -----Original Message----- > > From: Joe Damato <jdamato@fastly.com> > > Sent: Tuesday, September 24, 2024 7:49 PM > > To: netdev@vger.kernel.org > > Cc: Joe Damato <jdamato@fastly.com>; KY Srinivasan <kys@microsoft.com>; > > Haiyang Zhang <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>; > > Dexuan Cui <decui@microsoft.com>; David S. Miller <davem@davemloft.net>; > > Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; > > Paolo Abeni <pabeni@redhat.com>; open list:Hyper-V/Azure CORE AND DRIVERS > > <linux-hyperv@vger.kernel.org>; open list <linux-kernel@vger.kernel.org> > > Subject: [RFC net-next 1/1] hv_netvsc: Link queues to NAPIs > > > > [You don't often get email from jdamato@fastly.com. Learn why this is > > important at https://aka.ms/LearnAboutSenderIdentification ] > > > > Use netif_queue_set_napi to link queues to NAPI instances so that they > > can be queried with netlink. > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > --- > > drivers/net/hyperv/netvsc.c | 11 ++++++++++- > > drivers/net/hyperv/rndis_filter.c | 9 +++++++-- > > 2 files changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > > index 2b6ec979a62f..ccaa4690dba0 100644 > > --- a/drivers/net/hyperv/netvsc.c > > +++ b/drivers/net/hyperv/netvsc.c > > @@ -712,8 +712,11 @@ void netvsc_device_remove(struct hv_device *device) > > for (i = 0; i < net_device->num_chn; i++) { > > /* See also vmbus_reset_channel_cb(). */ > > /* only disable enabled NAPI channel */ > > - if (i < ndev->real_num_rx_queues) > > + if (i < ndev->real_num_rx_queues) { > > + netif_queue_set_napi(ndev, i, > > NETDEV_QUEUE_TYPE_TX, NULL); > > + netif_queue_set_napi(ndev, i, > > NETDEV_QUEUE_TYPE_RX, NULL); > > napi_disable(&net_device->chan_table[i].napi); > > + } > > > > netif_napi_del(&net_device->chan_table[i].napi); > > } > > @@ -1787,6 +1790,10 @@ struct netvsc_device *netvsc_device_add(struct > > hv_device *device, > > netdev_dbg(ndev, "hv_netvsc channel opened successfully\n"); > > > > napi_enable(&net_device->chan_table[0].napi); > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_RX, > > + &net_device->chan_table[0].napi); > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_TX, > > + &net_device->chan_table[0].napi); > > > > /* Connect with the NetVsp */ > > ret = netvsc_connect_vsp(device, net_device, device_info); > > @@ -1805,6 +1812,8 @@ struct netvsc_device *netvsc_device_add(struct > > hv_device *device, > > > > close: > > RCU_INIT_POINTER(net_device_ctx->nvdev, NULL); > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_TX, NULL); > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_RX, NULL); > > napi_disable(&net_device->chan_table[0].napi); > > > > /* Now, we can close the channel safely */ > > diff --git a/drivers/net/hyperv/rndis_filter.c > > b/drivers/net/hyperv/rndis_filter.c > > index ecc2128ca9b7..c0ceeef4fcd8 100644 > > --- a/drivers/net/hyperv/rndis_filter.c > > +++ b/drivers/net/hyperv/rndis_filter.c > > @@ -1269,10 +1269,15 @@ static void netvsc_sc_open(struct vmbus_channel > > *new_sc) > > ret = vmbus_open(new_sc, netvsc_ring_bytes, > > netvsc_ring_bytes, NULL, 0, > > netvsc_channel_cb, nvchan); > > - if (ret == 0) > > + if (ret == 0) { > > napi_enable(&nvchan->napi); > > - else > > + netif_queue_set_napi(ndev, chn_index, > > NETDEV_QUEUE_TYPE_RX, > > + &nvchan->napi); > > + netif_queue_set_napi(ndev, chn_index, > > NETDEV_QUEUE_TYPE_TX, > > + &nvchan->napi); > > + } else { > > netdev_notice(ndev, "sub channel open failed: %d\n", > > ret); > > + } > > > > if (atomic_inc_return(&nvscdev->open_chn) == nvscdev->num_chn) > > wake_up(&nvscdev->subchan_open); > > -- > > The code change looks fine to me. > @Shradha Gupta or @Erni Sri Satya Vennela, Do you have time to test this? > > Thanks, > - Haiyang > > Hi Joe, Haiyang, I have verified the patch on a VM with netvsc interfaces and the seems to be working as expected CLI output after applying the patch: [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'rx'}, {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'rx'}, {'id': 4, 'ifindex': 2, 'napi-id': 8197, 'type': 'rx'}, {'id': 5, 'ifindex': 2, 'napi-id': 8198, 'type': 'rx'}, {'id': 6, 'ifindex': 2, 'napi-id': 8199, 'type': 'rx'}, {'id': 7, 'ifindex': 2, 'napi-id': 8200, 'type': 'rx'}, {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}, {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'tx'}, {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'}, {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}, {'id': 4, 'ifindex': 2, 'napi-id': 8197, 'type': 'tx'}, {'id': 5, 'ifindex': 2, 'napi-id': 8198, 'type': 'tx'}, {'id': 6, 'ifindex': 2, 'napi-id': 8199, 'type': 'tx'}, {'id': 7, 'ifindex': 2, 'napi-id': 8200, 'type': 'tx'}] The code changes also look good. Tested-by: Shradha Gupta <shradhagupta@linux.microsoft.com> Thanks, Shradha.
> -----Original Message----- > From: Shradha Gupta <shradhagupta@linux.microsoft.com> > Sent: Thursday, September 26, 2024 6:35 AM > To: Haiyang Zhang <haiyangz@microsoft.com> > Cc: Joe Damato <jdamato@fastly.com>; netdev@vger.kernel.org; Shradha > Gupta <shradhagupta@microsoft.com>; Erni Sri Satya Vennela > <ernis@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Wei Liu > <wei.liu@kernel.org>; Dexuan Cui <decui@microsoft.com>; David S. Miller > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski > <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; open list:Hyper- > V/Azure CORE AND DRIVERS <linux-hyperv@vger.kernel.org>; open list > <linux-kernel@vger.kernel.org> > Subject: Re: [RFC net-next 1/1] hv_netvsc: Link queues to NAPIs > > On Wed, Sep 25, 2024 at 07:39:03PM +0000, Haiyang Zhang wrote: > > > > > > > -----Original Message----- > > > From: Joe Damato <jdamato@fastly.com> > > > Sent: Tuesday, September 24, 2024 7:49 PM > > > To: netdev@vger.kernel.org > > > Cc: Joe Damato <jdamato@fastly.com>; KY Srinivasan > <kys@microsoft.com>; > > > Haiyang Zhang <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>; > > > Dexuan Cui <decui@microsoft.com>; David S. Miller > <davem@davemloft.net>; > > > Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; > > > Paolo Abeni <pabeni@redhat.com>; open list:Hyper-V/Azure CORE AND > DRIVERS > > > <linux-hyperv@vger.kernel.org>; open list <linux- > kernel@vger.kernel.org> > > > Subject: [RFC net-next 1/1] hv_netvsc: Link queues to NAPIs > > > > > > [You don't often get email from jdamato@fastly.com. Learn why this is > > > important at https://aka.ms/LearnAboutSenderIdentification ] > > > > > > Use netif_queue_set_napi to link queues to NAPI instances so that > they > > > can be queried with netlink. > > > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > > --- > > > drivers/net/hyperv/netvsc.c | 11 ++++++++++- > > > drivers/net/hyperv/rndis_filter.c | 9 +++++++-- > > > 2 files changed, 17 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/hyperv/netvsc.c > b/drivers/net/hyperv/netvsc.c > > > index 2b6ec979a62f..ccaa4690dba0 100644 > > > --- a/drivers/net/hyperv/netvsc.c > > > +++ b/drivers/net/hyperv/netvsc.c > > > @@ -712,8 +712,11 @@ void netvsc_device_remove(struct hv_device > *device) > > > for (i = 0; i < net_device->num_chn; i++) { > > > /* See also vmbus_reset_channel_cb(). */ > > > /* only disable enabled NAPI channel */ > > > - if (i < ndev->real_num_rx_queues) > > > + if (i < ndev->real_num_rx_queues) { > > > + netif_queue_set_napi(ndev, i, > > > NETDEV_QUEUE_TYPE_TX, NULL); > > > + netif_queue_set_napi(ndev, i, > > > NETDEV_QUEUE_TYPE_RX, NULL); > > > napi_disable(&net_device- > >chan_table[i].napi); > > > + } > > > > > > netif_napi_del(&net_device->chan_table[i].napi); > > > } > > > @@ -1787,6 +1790,10 @@ struct netvsc_device *netvsc_device_add(struct > > > hv_device *device, > > > netdev_dbg(ndev, "hv_netvsc channel opened successfully\n"); > > > > > > napi_enable(&net_device->chan_table[0].napi); > > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_RX, > > > + &net_device->chan_table[0].napi); > > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_TX, > > > + &net_device->chan_table[0].napi); > > > > > > /* Connect with the NetVsp */ > > > ret = netvsc_connect_vsp(device, net_device, device_info); > > > @@ -1805,6 +1812,8 @@ struct netvsc_device *netvsc_device_add(struct > > > hv_device *device, > > > > > > close: > > > RCU_INIT_POINTER(net_device_ctx->nvdev, NULL); > > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_TX, NULL); > > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_RX, NULL); > > > napi_disable(&net_device->chan_table[0].napi); > > > > > > /* Now, we can close the channel safely */ > > > diff --git a/drivers/net/hyperv/rndis_filter.c > > > b/drivers/net/hyperv/rndis_filter.c > > > index ecc2128ca9b7..c0ceeef4fcd8 100644 > > > --- a/drivers/net/hyperv/rndis_filter.c > > > +++ b/drivers/net/hyperv/rndis_filter.c > > > @@ -1269,10 +1269,15 @@ static void netvsc_sc_open(struct > vmbus_channel > > > *new_sc) > > > ret = vmbus_open(new_sc, netvsc_ring_bytes, > > > netvsc_ring_bytes, NULL, 0, > > > netvsc_channel_cb, nvchan); > > > - if (ret == 0) > > > + if (ret == 0) { > > > napi_enable(&nvchan->napi); > > > - else > > > + netif_queue_set_napi(ndev, chn_index, > > > NETDEV_QUEUE_TYPE_RX, > > > + &nvchan->napi); > > > + netif_queue_set_napi(ndev, chn_index, > > > NETDEV_QUEUE_TYPE_TX, > > > + &nvchan->napi); > > > + } else { > > > netdev_notice(ndev, "sub channel open failed: %d\n", > > > ret); > > > + } > > > > > > if (atomic_inc_return(&nvscdev->open_chn) == nvscdev- > >num_chn) > > > wake_up(&nvscdev->subchan_open); > > > -- > > > > The code change looks fine to me. > > @Shradha Gupta or @Erni Sri Satya Vennela, Do you have time to test > this? > > > > Thanks, > > - Haiyang > > > > > Hi Joe, Haiyang, > > I have verified the patch on a VM with netvsc interfaces and the seems > to be working as expected > > CLI output after applying the patch: > > [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, > {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'rx'}, > {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'rx'}, > {'id': 4, 'ifindex': 2, 'napi-id': 8197, 'type': 'rx'}, > {'id': 5, 'ifindex': 2, 'napi-id': 8198, 'type': 'rx'}, > {'id': 6, 'ifindex': 2, 'napi-id': 8199, 'type': 'rx'}, > {'id': 7, 'ifindex': 2, 'napi-id': 8200, 'type': 'rx'}, > {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'tx'}, > {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'}, > {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}, > {'id': 4, 'ifindex': 2, 'napi-id': 8197, 'type': 'tx'}, > {'id': 5, 'ifindex': 2, 'napi-id': 8198, 'type': 'tx'}, > {'id': 6, 'ifindex': 2, 'napi-id': 8199, 'type': 'tx'}, > {'id': 7, 'ifindex': 2, 'napi-id': 8200, 'type': 'tx'}] > > The code changes also look good. > > Tested-by: Shradha Gupta <shradhagupta@linux.microsoft.com> > Thank you for the testing! - Haiyang
On Tue, Sep 24, 2024 at 11:48:51PM +0000, Joe Damato wrote: > Use netif_queue_set_napi to link queues to NAPI instances so that they > can be queried with netlink. > > Signed-off-by: Joe Damato <jdamato@fastly.com> > --- > drivers/net/hyperv/netvsc.c | 11 ++++++++++- > drivers/net/hyperv/rndis_filter.c | 9 +++++++-- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > index 2b6ec979a62f..ccaa4690dba0 100644 > --- a/drivers/net/hyperv/netvsc.c > +++ b/drivers/net/hyperv/netvsc.c > @@ -712,8 +712,11 @@ void netvsc_device_remove(struct hv_device *device) > for (i = 0; i < net_device->num_chn; i++) { > /* See also vmbus_reset_channel_cb(). */ > /* only disable enabled NAPI channel */ > - if (i < ndev->real_num_rx_queues) > + if (i < ndev->real_num_rx_queues) { > + netif_queue_set_napi(ndev, i, NETDEV_QUEUE_TYPE_TX, NULL); > + netif_queue_set_napi(ndev, i, NETDEV_QUEUE_TYPE_RX, NULL); Hi Joe, When you post a non-RFC version of this patch, could you consider line-wrapping the above to 80 columns, as is still preferred for Networking code? There is an option to checkpatch that will warn you about this. ...
On Thu, Sep 26, 2024 at 03:34:43AM -0700, Shradha Gupta wrote: > On Wed, Sep 25, 2024 at 07:39:03PM +0000, Haiyang Zhang wrote: > > > > > > > -----Original Message----- > > > From: Joe Damato <jdamato@fastly.com> > > > Sent: Tuesday, September 24, 2024 7:49 PM > > > To: netdev@vger.kernel.org > > > Cc: Joe Damato <jdamato@fastly.com>; KY Srinivasan <kys@microsoft.com>; > > > Haiyang Zhang <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>; > > > Dexuan Cui <decui@microsoft.com>; David S. Miller <davem@davemloft.net>; > > > Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; > > > Paolo Abeni <pabeni@redhat.com>; open list:Hyper-V/Azure CORE AND DRIVERS > > > <linux-hyperv@vger.kernel.org>; open list <linux-kernel@vger.kernel.org> > > > Subject: [RFC net-next 1/1] hv_netvsc: Link queues to NAPIs > > > > > > [You don't often get email from jdamato@fastly.com. Learn why this is > > > important at https://aka.ms/LearnAboutSenderIdentification ] > > > > > > Use netif_queue_set_napi to link queues to NAPI instances so that they > > > can be queried with netlink. > > > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > > --- > > > drivers/net/hyperv/netvsc.c | 11 ++++++++++- > > > drivers/net/hyperv/rndis_filter.c | 9 +++++++-- > > > 2 files changed, 17 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > > > index 2b6ec979a62f..ccaa4690dba0 100644 > > > --- a/drivers/net/hyperv/netvsc.c > > > +++ b/drivers/net/hyperv/netvsc.c > > > @@ -712,8 +712,11 @@ void netvsc_device_remove(struct hv_device *device) > > > for (i = 0; i < net_device->num_chn; i++) { > > > /* See also vmbus_reset_channel_cb(). */ > > > /* only disable enabled NAPI channel */ > > > - if (i < ndev->real_num_rx_queues) > > > + if (i < ndev->real_num_rx_queues) { > > > + netif_queue_set_napi(ndev, i, > > > NETDEV_QUEUE_TYPE_TX, NULL); > > > + netif_queue_set_napi(ndev, i, > > > NETDEV_QUEUE_TYPE_RX, NULL); > > > napi_disable(&net_device->chan_table[i].napi); > > > + } > > > > > > netif_napi_del(&net_device->chan_table[i].napi); > > > } > > > @@ -1787,6 +1790,10 @@ struct netvsc_device *netvsc_device_add(struct > > > hv_device *device, > > > netdev_dbg(ndev, "hv_netvsc channel opened successfully\n"); > > > > > > napi_enable(&net_device->chan_table[0].napi); > > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_RX, > > > + &net_device->chan_table[0].napi); > > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_TX, > > > + &net_device->chan_table[0].napi); > > > > > > /* Connect with the NetVsp */ > > > ret = netvsc_connect_vsp(device, net_device, device_info); > > > @@ -1805,6 +1812,8 @@ struct netvsc_device *netvsc_device_add(struct > > > hv_device *device, > > > > > > close: > > > RCU_INIT_POINTER(net_device_ctx->nvdev, NULL); > > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_TX, NULL); > > > + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_RX, NULL); > > > napi_disable(&net_device->chan_table[0].napi); > > > > > > /* Now, we can close the channel safely */ > > > diff --git a/drivers/net/hyperv/rndis_filter.c > > > b/drivers/net/hyperv/rndis_filter.c > > > index ecc2128ca9b7..c0ceeef4fcd8 100644 > > > --- a/drivers/net/hyperv/rndis_filter.c > > > +++ b/drivers/net/hyperv/rndis_filter.c > > > @@ -1269,10 +1269,15 @@ static void netvsc_sc_open(struct vmbus_channel > > > *new_sc) > > > ret = vmbus_open(new_sc, netvsc_ring_bytes, > > > netvsc_ring_bytes, NULL, 0, > > > netvsc_channel_cb, nvchan); > > > - if (ret == 0) > > > + if (ret == 0) { > > > napi_enable(&nvchan->napi); > > > - else > > > + netif_queue_set_napi(ndev, chn_index, > > > NETDEV_QUEUE_TYPE_RX, > > > + &nvchan->napi); > > > + netif_queue_set_napi(ndev, chn_index, > > > NETDEV_QUEUE_TYPE_TX, > > > + &nvchan->napi); > > > + } else { > > > netdev_notice(ndev, "sub channel open failed: %d\n", > > > ret); > > > + } > > > > > > if (atomic_inc_return(&nvscdev->open_chn) == nvscdev->num_chn) > > > wake_up(&nvscdev->subchan_open); > > > -- > > > > The code change looks fine to me. > > @Shradha Gupta or @Erni Sri Satya Vennela, Do you have time to test this? > > > > Thanks, > > - Haiyang > > > > > Hi Joe, Haiyang, > > I have verified the patch on a VM with netvsc interfaces and the seems > to be working as expected > > CLI output after applying the patch: > > [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, > {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'rx'}, > {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'rx'}, > {'id': 4, 'ifindex': 2, 'napi-id': 8197, 'type': 'rx'}, > {'id': 5, 'ifindex': 2, 'napi-id': 8198, 'type': 'rx'}, > {'id': 6, 'ifindex': 2, 'napi-id': 8199, 'type': 'rx'}, > {'id': 7, 'ifindex': 2, 'napi-id': 8200, 'type': 'rx'}, > {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'tx'}, > {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'}, > {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}, > {'id': 4, 'ifindex': 2, 'napi-id': 8197, 'type': 'tx'}, > {'id': 5, 'ifindex': 2, 'napi-id': 8198, 'type': 'tx'}, > {'id': 6, 'ifindex': 2, 'napi-id': 8199, 'type': 'tx'}, > {'id': 7, 'ifindex': 2, 'napi-id': 8200, 'type': 'tx'}] > > The code changes also look good. > > Tested-by: Shradha Gupta <shradhagupta@linux.microsoft.com> Thank you very much for testing, I will include your tested-by when I resend this next week when net-next is open.
On Thu, Sep 26, 2024 at 04:10:24PM +0100, Simon Horman wrote: > On Tue, Sep 24, 2024 at 11:48:51PM +0000, Joe Damato wrote: > > Use netif_queue_set_napi to link queues to NAPI instances so that they > > can be queried with netlink. > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > --- > > drivers/net/hyperv/netvsc.c | 11 ++++++++++- > > drivers/net/hyperv/rndis_filter.c | 9 +++++++-- > > 2 files changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > > index 2b6ec979a62f..ccaa4690dba0 100644 > > --- a/drivers/net/hyperv/netvsc.c > > +++ b/drivers/net/hyperv/netvsc.c > > @@ -712,8 +712,11 @@ void netvsc_device_remove(struct hv_device *device) > > for (i = 0; i < net_device->num_chn; i++) { > > /* See also vmbus_reset_channel_cb(). */ > > /* only disable enabled NAPI channel */ > > - if (i < ndev->real_num_rx_queues) > > + if (i < ndev->real_num_rx_queues) { > > + netif_queue_set_napi(ndev, i, NETDEV_QUEUE_TYPE_TX, NULL); > > + netif_queue_set_napi(ndev, i, NETDEV_QUEUE_TYPE_RX, NULL); > > Hi Joe, > > When you post a non-RFC version of this patch, could you consider > line-wrapping the above to 80 columns, as is still preferred for > Networking code? > > There is an option to checkpatch that will warn you about this. Thanks for letting me know. I run checkpatch.pl --strict and usually it seems to let me know if I am over 80, but maybe there's another option I need?
On Thu, Sep 26, 2024 at 08:42:32AM -0700, Joe Damato wrote: > On Thu, Sep 26, 2024 at 04:10:24PM +0100, Simon Horman wrote: > > On Tue, Sep 24, 2024 at 11:48:51PM +0000, Joe Damato wrote: > > > Use netif_queue_set_napi to link queues to NAPI instances so that they > > > can be queried with netlink. > > > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > > --- > > > drivers/net/hyperv/netvsc.c | 11 ++++++++++- > > > drivers/net/hyperv/rndis_filter.c | 9 +++++++-- > > > 2 files changed, 17 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > > > index 2b6ec979a62f..ccaa4690dba0 100644 > > > --- a/drivers/net/hyperv/netvsc.c > > > +++ b/drivers/net/hyperv/netvsc.c > > > @@ -712,8 +712,11 @@ void netvsc_device_remove(struct hv_device *device) > > > for (i = 0; i < net_device->num_chn; i++) { > > > /* See also vmbus_reset_channel_cb(). */ > > > /* only disable enabled NAPI channel */ > > > - if (i < ndev->real_num_rx_queues) > > > + if (i < ndev->real_num_rx_queues) { > > > + netif_queue_set_napi(ndev, i, NETDEV_QUEUE_TYPE_TX, NULL); > > > + netif_queue_set_napi(ndev, i, NETDEV_QUEUE_TYPE_RX, NULL); > > > > Hi Joe, > > > > When you post a non-RFC version of this patch, could you consider > > line-wrapping the above to 80 columns, as is still preferred for > > Networking code? > > > > There is an option to checkpatch that will warn you about this. > > Thanks for letting me know. > > I run checkpatch.pl --strict and usually it seems to let me know if > I am over 80, but maybe there's another option I need? Ah, I see: --max-line-length=n set the maximum line length, (default 100) I didn't realize the checkpatch default was 100. Sorry about that, will make sure to pass that flag in the future; thanks for letting me know.
On Wed, Sep 25, 2024 at 07:39:03PM +0000, Haiyang Zhang wrote: > [...] > The code change looks fine to me. > @Shradha Gupta or @Erni Sri Satya Vennela, Do you have time to test this? Haiyang, would you like me to include an acked-by or reviewed-by from you for this patch when I send it when net-next reopens? I've added Shradha's Tested-by.
On Thu, Sep 26, 2024 at 08:42:32AM -0700, Joe Damato wrote: > On Thu, Sep 26, 2024 at 04:10:24PM +0100, Simon Horman wrote: > > On Tue, Sep 24, 2024 at 11:48:51PM +0000, Joe Damato wrote: > > > Use netif_queue_set_napi to link queues to NAPI instances so that they > > > can be queried with netlink. > > > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > > --- > > > drivers/net/hyperv/netvsc.c | 11 ++++++++++- > > > drivers/net/hyperv/rndis_filter.c | 9 +++++++-- > > > 2 files changed, 17 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > > > index 2b6ec979a62f..ccaa4690dba0 100644 > > > --- a/drivers/net/hyperv/netvsc.c > > > +++ b/drivers/net/hyperv/netvsc.c > > > @@ -712,8 +712,11 @@ void netvsc_device_remove(struct hv_device *device) > > > for (i = 0; i < net_device->num_chn; i++) { > > > /* See also vmbus_reset_channel_cb(). */ > > > /* only disable enabled NAPI channel */ > > > - if (i < ndev->real_num_rx_queues) > > > + if (i < ndev->real_num_rx_queues) { > > > + netif_queue_set_napi(ndev, i, NETDEV_QUEUE_TYPE_TX, NULL); > > > + netif_queue_set_napi(ndev, i, NETDEV_QUEUE_TYPE_RX, NULL); > > > > Hi Joe, > > > > When you post a non-RFC version of this patch, could you consider > > line-wrapping the above to 80 columns, as is still preferred for > > Networking code? > > > > There is an option to checkpatch that will warn you about this. > > Thanks for letting me know. > > I run checkpatch.pl --strict and usually it seems to let me know if > I am over 80, but maybe there's another option I need? At some point the default changed from 80 to 100. So these days --max-line-length=80 is needed to detect this. --strict is also good :)
> -----Original Message----- > From: Joe Damato <jdamato@fastly.com> > Sent: Thursday, September 26, 2024 11:53 AM > To: Haiyang Zhang <haiyangz@microsoft.com> > Cc: netdev@vger.kernel.org; Shradha Gupta <shradhagupta@microsoft.com>; > Erni Sri Satya Vennela <ernis@microsoft.com>; KY Srinivasan > <kys@microsoft.com>; Wei Liu <wei.liu@kernel.org>; Dexuan Cui > <decui@microsoft.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; open list:Hyper-V/Azure CORE AND DRIVERS <linux- > hyperv@vger.kernel.org>; open list <linux-kernel@vger.kernel.org> > Subject: Re: [RFC net-next 1/1] hv_netvsc: Link queues to NAPIs > > On Wed, Sep 25, 2024 at 07:39:03PM +0000, Haiyang Zhang wrote: > > > > [...] > > > The code change looks fine to me. > > @Shradha Gupta or @Erni Sri Satya Vennela, Do you have time to test > this? > > Haiyang, would you like me to include an acked-by or reviewed-by > from you for this patch when I send it when net-next reopens? Yes, you can add my reviewed-by. Thanks, - Haiyang
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 2b6ec979a62f..ccaa4690dba0 100644 --- a/drivers/net/hyperv/netvsc.c +++ b/drivers/net/hyperv/netvsc.c @@ -712,8 +712,11 @@ void netvsc_device_remove(struct hv_device *device) for (i = 0; i < net_device->num_chn; i++) { /* See also vmbus_reset_channel_cb(). */ /* only disable enabled NAPI channel */ - if (i < ndev->real_num_rx_queues) + if (i < ndev->real_num_rx_queues) { + netif_queue_set_napi(ndev, i, NETDEV_QUEUE_TYPE_TX, NULL); + netif_queue_set_napi(ndev, i, NETDEV_QUEUE_TYPE_RX, NULL); napi_disable(&net_device->chan_table[i].napi); + } netif_napi_del(&net_device->chan_table[i].napi); } @@ -1787,6 +1790,10 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device, netdev_dbg(ndev, "hv_netvsc channel opened successfully\n"); napi_enable(&net_device->chan_table[0].napi); + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_RX, + &net_device->chan_table[0].napi); + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_TX, + &net_device->chan_table[0].napi); /* Connect with the NetVsp */ ret = netvsc_connect_vsp(device, net_device, device_info); @@ -1805,6 +1812,8 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device, close: RCU_INIT_POINTER(net_device_ctx->nvdev, NULL); + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_TX, NULL); + netif_queue_set_napi(ndev, 0, NETDEV_QUEUE_TYPE_RX, NULL); napi_disable(&net_device->chan_table[0].napi); /* Now, we can close the channel safely */ diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c index ecc2128ca9b7..c0ceeef4fcd8 100644 --- a/drivers/net/hyperv/rndis_filter.c +++ b/drivers/net/hyperv/rndis_filter.c @@ -1269,10 +1269,15 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc) ret = vmbus_open(new_sc, netvsc_ring_bytes, netvsc_ring_bytes, NULL, 0, netvsc_channel_cb, nvchan); - if (ret == 0) + if (ret == 0) { napi_enable(&nvchan->napi); - else + netif_queue_set_napi(ndev, chn_index, NETDEV_QUEUE_TYPE_RX, + &nvchan->napi); + netif_queue_set_napi(ndev, chn_index, NETDEV_QUEUE_TYPE_TX, + &nvchan->napi); + } else { netdev_notice(ndev, "sub channel open failed: %d\n", ret); + } if (atomic_inc_return(&nvscdev->open_chn) == nvscdev->num_chn) wake_up(&nvscdev->subchan_open);
Use netif_queue_set_napi to link queues to NAPI instances so that they can be queried with netlink. Signed-off-by: Joe Damato <jdamato@fastly.com> --- drivers/net/hyperv/netvsc.c | 11 ++++++++++- drivers/net/hyperv/rndis_filter.c | 9 +++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)