* [PATCH V3 net-next 0/2] virtio-net: don't busy poll for cvq command @ 2023-05-24 8:18 Jason Wang 2023-05-24 8:18 ` [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue Jason Wang 2023-05-24 8:18 ` [PATCH V3 net-next 2/2] virtio-net: add cond_resched() to the command waiting loop Jason Wang 0 siblings, 2 replies; 12+ messages in thread From: Jason Wang @ 2023-05-24 8:18 UTC (permalink / raw) To: mst, jasowang, xuanzhuo, davem, edumazet, kuba, pabeni Cc: netdev, linux-kernel, virtualization Hi all: The code used to busy poll for cvq command which turns out to have several side effects: 1) infinite poll for buggy devices 2) bad interaction with scheduler So this series tries to use cond_resched() in the waiting loop. Before doing this we need first make sure the cvq command is not executed in atomic environment, so we need first convert rx mode handling to a workqueue. Please review. Thanks Changes since V2: - Don't use interrupt but cond_resched() Changes since V1: - use RTNL to synchronize rx mode worker - use completion for simplicity - don't try to harden CVQ command Changes since RFC: - switch to use BAD_RING in virtio_break_device() - check virtqueue_is_broken() after being woken up - use more_used() instead of virtqueue_get_buf() to allow caller to get buffers afterwards - break the virtio-net device when timeout - get buffer manually since the virtio core check more_used() instead Jason Wang (2): virtio-net: convert rx mode setting to use workqueue virtio-net: add cond_resched() to the command waiting loop drivers/net/virtio_net.c | 59 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 55 insertions(+), 4 deletions(-) -- 2.25.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue 2023-05-24 8:18 [PATCH V3 net-next 0/2] virtio-net: don't busy poll for cvq command Jason Wang @ 2023-05-24 8:18 ` Jason Wang 2023-05-24 9:15 ` Michael S. Tsirkin 2023-05-24 8:18 ` [PATCH V3 net-next 2/2] virtio-net: add cond_resched() to the command waiting loop Jason Wang 1 sibling, 1 reply; 12+ messages in thread From: Jason Wang @ 2023-05-24 8:18 UTC (permalink / raw) To: mst, jasowang, xuanzhuo, davem, edumazet, kuba, pabeni Cc: netdev, linux-kernel, virtualization This patch convert rx mode setting to be done in a workqueue, this is a must for allow to sleep when waiting for the cvq command to response since current code is executed under addr spin lock. Signed-off-by: Jason Wang <jasowang@redhat.com> --- Changes since V1: - use RTNL to synchronize rx mode worker --- drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 3 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 56ca1d270304..5d2f1da4eaa0 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -265,6 +265,12 @@ struct virtnet_info { /* Work struct for config space updates */ struct work_struct config_work; + /* Work struct for config rx mode */ + struct work_struct rx_mode_work; + + /* Is rx mode work enabled? */ + bool rx_mode_work_enabled; + /* Does the affinity hint is set for virtqueues? */ bool affinity_hint_set; @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) spin_unlock_bh(&vi->refill_lock); } +static void enable_rx_mode_work(struct virtnet_info *vi) +{ + rtnl_lock(); + vi->rx_mode_work_enabled = true; + rtnl_unlock(); +} + +static void disable_rx_mode_work(struct virtnet_info *vi) +{ + rtnl_lock(); + vi->rx_mode_work_enabled = false; + rtnl_unlock(); +} + static void virtqueue_napi_schedule(struct napi_struct *napi, struct virtqueue *vq) { @@ -2341,9 +2361,11 @@ static int virtnet_close(struct net_device *dev) return 0; } -static void virtnet_set_rx_mode(struct net_device *dev) +static void virtnet_rx_mode_work(struct work_struct *work) { - struct virtnet_info *vi = netdev_priv(dev); + struct virtnet_info *vi = + container_of(work, struct virtnet_info, rx_mode_work); + struct net_device *dev = vi->dev; struct scatterlist sg[2]; struct virtio_net_ctrl_mac *mac_data; struct netdev_hw_addr *ha; @@ -2356,6 +2378,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) return; + rtnl_lock(); + vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); @@ -2373,14 +2397,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", vi->ctrl->allmulti ? "en" : "dis"); + netif_addr_lock_bh(dev); + uc_count = netdev_uc_count(dev); mc_count = netdev_mc_count(dev); /* MAC filter - use one buffer for both lists */ buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + (2 * sizeof(mac_data->entries)), GFP_ATOMIC); mac_data = buf; - if (!buf) + if (!buf) { + netif_addr_unlock_bh(dev); + rtnl_unlock(); return; + } sg_init_table(sg, 2); @@ -2401,6 +2430,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) netdev_for_each_mc_addr(ha, dev) memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); + netif_addr_unlock_bh(dev); + sg_set_buf(&sg[1], mac_data, sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); @@ -2408,9 +2439,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); + rtnl_unlock(); + kfree(buf); } +static void virtnet_set_rx_mode(struct net_device *dev) +{ + struct virtnet_info *vi = netdev_priv(dev); + + if (vi->rx_mode_work_enabled) + schedule_work(&vi->rx_mode_work); +} + static int virtnet_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid) { @@ -3181,6 +3222,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) /* Make sure no work handler is accessing the device */ flush_work(&vi->config_work); + disable_rx_mode_work(vi); + flush_work(&vi->rx_mode_work); netif_tx_lock_bh(vi->dev); netif_device_detach(vi->dev); @@ -3203,6 +3246,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) virtio_device_ready(vdev); enable_delayed_refill(vi); + enable_rx_mode_work(vi); if (netif_running(vi->dev)) { err = virtnet_open(vi->dev); @@ -4002,6 +4046,7 @@ static int virtnet_probe(struct virtio_device *vdev) vdev->priv = vi; INIT_WORK(&vi->config_work, virtnet_config_changed_work); + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); spin_lock_init(&vi->refill_lock); if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { @@ -4110,6 +4155,8 @@ static int virtnet_probe(struct virtio_device *vdev) if (vi->has_rss || vi->has_rss_hash_report) virtnet_init_default_rss(vi); + enable_rx_mode_work(vi); + /* serialize netdev register + virtio_device_ready() with ndo_open() */ rtnl_lock(); @@ -4207,6 +4254,8 @@ static void virtnet_remove(struct virtio_device *vdev) /* Make sure no work handler is accessing the device. */ flush_work(&vi->config_work); + disable_rx_mode_work(vi); + flush_work(&vi->rx_mode_work); unregister_netdev(vi->dev); -- 2.25.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue 2023-05-24 8:18 ` [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue Jason Wang @ 2023-05-24 9:15 ` Michael S. Tsirkin 2023-05-25 3:43 ` Jason Wang 0 siblings, 1 reply; 12+ messages in thread From: Michael S. Tsirkin @ 2023-05-24 9:15 UTC (permalink / raw) To: Jason Wang Cc: xuanzhuo, netdev, linux-kernel, virtualization, edumazet, kuba, pabeni, davem On Wed, May 24, 2023 at 04:18:41PM +0800, Jason Wang wrote: > This patch convert rx mode setting to be done in a workqueue, this is > a must for allow to sleep when waiting for the cvq command to > response since current code is executed under addr spin lock. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > Changes since V1: > - use RTNL to synchronize rx mode worker > --- > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 52 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 56ca1d270304..5d2f1da4eaa0 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -265,6 +265,12 @@ struct virtnet_info { > /* Work struct for config space updates */ > struct work_struct config_work; > > + /* Work struct for config rx mode */ With a bit less abbreviation maybe? setting rx mode? > + struct work_struct rx_mode_work; > + > + /* Is rx mode work enabled? */ Ugh not a great comment. > + bool rx_mode_work_enabled; > + > /* Does the affinity hint is set for virtqueues? */ > bool affinity_hint_set; > > @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) > spin_unlock_bh(&vi->refill_lock); > } > > +static void enable_rx_mode_work(struct virtnet_info *vi) > +{ > + rtnl_lock(); > + vi->rx_mode_work_enabled = true; > + rtnl_unlock(); > +} > + > +static void disable_rx_mode_work(struct virtnet_info *vi) > +{ > + rtnl_lock(); > + vi->rx_mode_work_enabled = false; > + rtnl_unlock(); > +} > + > static void virtqueue_napi_schedule(struct napi_struct *napi, > struct virtqueue *vq) > { > @@ -2341,9 +2361,11 @@ static int virtnet_close(struct net_device *dev) > return 0; > } > > -static void virtnet_set_rx_mode(struct net_device *dev) > +static void virtnet_rx_mode_work(struct work_struct *work) > { > - struct virtnet_info *vi = netdev_priv(dev); > + struct virtnet_info *vi = > + container_of(work, struct virtnet_info, rx_mode_work); > + struct net_device *dev = vi->dev; > struct scatterlist sg[2]; > struct virtio_net_ctrl_mac *mac_data; > struct netdev_hw_addr *ha; > @@ -2356,6 +2378,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > return; > > + rtnl_lock(); > + > vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > @@ -2373,14 +2397,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > vi->ctrl->allmulti ? "en" : "dis"); > > + netif_addr_lock_bh(dev); > + > uc_count = netdev_uc_count(dev); > mc_count = netdev_mc_count(dev); > /* MAC filter - use one buffer for both lists */ > buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + > (2 * sizeof(mac_data->entries)), GFP_ATOMIC); > mac_data = buf; > - if (!buf) > + if (!buf) { > + netif_addr_unlock_bh(dev); > + rtnl_unlock(); > return; > + } > > sg_init_table(sg, 2); > > @@ -2401,6 +2430,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > netdev_for_each_mc_addr(ha, dev) > memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); > > + netif_addr_unlock_bh(dev); > + > sg_set_buf(&sg[1], mac_data, > sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); > > @@ -2408,9 +2439,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) > dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); > > + rtnl_unlock(); > + > kfree(buf); > } > > +static void virtnet_set_rx_mode(struct net_device *dev) > +{ > + struct virtnet_info *vi = netdev_priv(dev); > + > + if (vi->rx_mode_work_enabled) > + schedule_work(&vi->rx_mode_work); > +} > + > static int virtnet_vlan_rx_add_vid(struct net_device *dev, > __be16 proto, u16 vid) > { > @@ -3181,6 +3222,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > > /* Make sure no work handler is accessing the device */ > flush_work(&vi->config_work); > + disable_rx_mode_work(vi); > + flush_work(&vi->rx_mode_work); > > netif_tx_lock_bh(vi->dev); > netif_device_detach(vi->dev); Hmm so queued rx mode work will just get skipped and on restore we get a wrong rx mode. Any way to make this more robust? > @@ -3203,6 +3246,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) > virtio_device_ready(vdev); > > enable_delayed_refill(vi); > + enable_rx_mode_work(vi); > > if (netif_running(vi->dev)) { > err = virtnet_open(vi->dev); > @@ -4002,6 +4046,7 @@ static int virtnet_probe(struct virtio_device *vdev) > vdev->priv = vi; > > INIT_WORK(&vi->config_work, virtnet_config_changed_work); > + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); > spin_lock_init(&vi->refill_lock); > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { > @@ -4110,6 +4155,8 @@ static int virtnet_probe(struct virtio_device *vdev) > if (vi->has_rss || vi->has_rss_hash_report) > virtnet_init_default_rss(vi); > > + enable_rx_mode_work(vi); > + > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > rtnl_lock(); > > @@ -4207,6 +4254,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > /* Make sure no work handler is accessing the device. */ > flush_work(&vi->config_work); > + disable_rx_mode_work(vi); > + flush_work(&vi->rx_mode_work); > > unregister_netdev(vi->dev); > > -- > 2.25.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue 2023-05-24 9:15 ` Michael S. Tsirkin @ 2023-05-25 3:43 ` Jason Wang 2023-05-25 7:41 ` Michael S. Tsirkin 0 siblings, 1 reply; 12+ messages in thread From: Jason Wang @ 2023-05-25 3:43 UTC (permalink / raw) To: Michael S. Tsirkin Cc: xuanzhuo, netdev, linux-kernel, virtualization, edumazet, kuba, pabeni, davem On Wed, May 24, 2023 at 5:15 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, May 24, 2023 at 04:18:41PM +0800, Jason Wang wrote: > > This patch convert rx mode setting to be done in a workqueue, this is > > a must for allow to sleep when waiting for the cvq command to > > response since current code is executed under addr spin lock. > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > --- > > Changes since V1: > > - use RTNL to synchronize rx mode worker > > --- > > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 52 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 56ca1d270304..5d2f1da4eaa0 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -265,6 +265,12 @@ struct virtnet_info { > > /* Work struct for config space updates */ > > struct work_struct config_work; > > > > + /* Work struct for config rx mode */ > > With a bit less abbreviation maybe? setting rx mode? That's fine. > > > + struct work_struct rx_mode_work; > > + > > + /* Is rx mode work enabled? */ > > Ugh not a great comment. Any suggestions for this. E.g we had: /* Is delayed refill enabled? */ > > > + bool rx_mode_work_enabled; > > + > > > > > /* Does the affinity hint is set for virtqueues? */ > > bool affinity_hint_set; > > > > @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) > > spin_unlock_bh(&vi->refill_lock); > > } > > > > +static void enable_rx_mode_work(struct virtnet_info *vi) > > +{ > > + rtnl_lock(); > > + vi->rx_mode_work_enabled = true; > > + rtnl_unlock(); > > +} > > + > > +static void disable_rx_mode_work(struct virtnet_info *vi) > > +{ > > + rtnl_lock(); > > + vi->rx_mode_work_enabled = false; > > + rtnl_unlock(); > > +} > > + > > static void virtqueue_napi_schedule(struct napi_struct *napi, > > struct virtqueue *vq) > > { > > @@ -2341,9 +2361,11 @@ static int virtnet_close(struct net_device *dev) > > return 0; > > } > > > > -static void virtnet_set_rx_mode(struct net_device *dev) > > +static void virtnet_rx_mode_work(struct work_struct *work) > > { > > - struct virtnet_info *vi = netdev_priv(dev); > > + struct virtnet_info *vi = > > + container_of(work, struct virtnet_info, rx_mode_work); > > + struct net_device *dev = vi->dev; > > struct scatterlist sg[2]; > > struct virtio_net_ctrl_mac *mac_data; > > struct netdev_hw_addr *ha; > > @@ -2356,6 +2378,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > > return; > > > > + rtnl_lock(); > > + > > vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > > vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > > > @@ -2373,14 +2397,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > > vi->ctrl->allmulti ? "en" : "dis"); > > > > + netif_addr_lock_bh(dev); > > + > > uc_count = netdev_uc_count(dev); > > mc_count = netdev_mc_count(dev); > > /* MAC filter - use one buffer for both lists */ > > buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + > > (2 * sizeof(mac_data->entries)), GFP_ATOMIC); > > mac_data = buf; > > - if (!buf) > > + if (!buf) { > > + netif_addr_unlock_bh(dev); > > + rtnl_unlock(); > > return; > > + } > > > > sg_init_table(sg, 2); > > > > @@ -2401,6 +2430,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > netdev_for_each_mc_addr(ha, dev) > > memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); > > > > + netif_addr_unlock_bh(dev); > > + > > sg_set_buf(&sg[1], mac_data, > > sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); > > > > @@ -2408,9 +2439,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) > > dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); > > > > + rtnl_unlock(); > > + > > kfree(buf); > > } > > > > +static void virtnet_set_rx_mode(struct net_device *dev) > > +{ > > + struct virtnet_info *vi = netdev_priv(dev); > > + > > + if (vi->rx_mode_work_enabled) > > + schedule_work(&vi->rx_mode_work); > > +} > > + > > > static int virtnet_vlan_rx_add_vid(struct net_device *dev, > > __be16 proto, u16 vid) > > { > > @@ -3181,6 +3222,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > > > > /* Make sure no work handler is accessing the device */ > > flush_work(&vi->config_work); > > + disable_rx_mode_work(vi); > > + flush_work(&vi->rx_mode_work); > > > > netif_tx_lock_bh(vi->dev); > > netif_device_detach(vi->dev); > > Hmm so queued rx mode work will just get skipped > and on restore we get a wrong rx mode. > Any way to make this more robust? It could be done by scheduling a work on restore. Thanks > > > > @@ -3203,6 +3246,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) > > virtio_device_ready(vdev); > > > > enable_delayed_refill(vi); > > + enable_rx_mode_work(vi); > > > > if (netif_running(vi->dev)) { > > err = virtnet_open(vi->dev); > > @@ -4002,6 +4046,7 @@ static int virtnet_probe(struct virtio_device *vdev) > > vdev->priv = vi; > > > > INIT_WORK(&vi->config_work, virtnet_config_changed_work); > > + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); > > spin_lock_init(&vi->refill_lock); > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { > > @@ -4110,6 +4155,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > if (vi->has_rss || vi->has_rss_hash_report) > > virtnet_init_default_rss(vi); > > > > + enable_rx_mode_work(vi); > > + > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > rtnl_lock(); > > > > @@ -4207,6 +4254,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > > > /* Make sure no work handler is accessing the device. */ > > flush_work(&vi->config_work); > > + disable_rx_mode_work(vi); > > + flush_work(&vi->rx_mode_work); > > > > unregister_netdev(vi->dev); > > > > -- > > 2.25.1 > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue 2023-05-25 3:43 ` Jason Wang @ 2023-05-25 7:41 ` Michael S. Tsirkin 2023-05-26 1:31 ` Jason Wang 0 siblings, 1 reply; 12+ messages in thread From: Michael S. Tsirkin @ 2023-05-25 7:41 UTC (permalink / raw) To: Jason Wang Cc: xuanzhuo, netdev, linux-kernel, virtualization, edumazet, kuba, pabeni, davem On Thu, May 25, 2023 at 11:43:34AM +0800, Jason Wang wrote: > On Wed, May 24, 2023 at 5:15 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Wed, May 24, 2023 at 04:18:41PM +0800, Jason Wang wrote: > > > This patch convert rx mode setting to be done in a workqueue, this is > > > a must for allow to sleep when waiting for the cvq command to > > > response since current code is executed under addr spin lock. > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > --- > > > Changes since V1: > > > - use RTNL to synchronize rx mode worker > > > --- > > > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 52 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 56ca1d270304..5d2f1da4eaa0 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -265,6 +265,12 @@ struct virtnet_info { > > > /* Work struct for config space updates */ > > > struct work_struct config_work; > > > > > > + /* Work struct for config rx mode */ > > > > With a bit less abbreviation maybe? setting rx mode? > > That's fine. > > > > > > + struct work_struct rx_mode_work; > > > + > > > + /* Is rx mode work enabled? */ > > > > Ugh not a great comment. > > Any suggestions for this. E.g we had: > > /* Is delayed refill enabled? */ /* OK to queue work setting RX mode? */ > > > > > + bool rx_mode_work_enabled; > > > + > > > > > > > > > /* Does the affinity hint is set for virtqueues? */ > > > bool affinity_hint_set; > > > > > > @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) > > > spin_unlock_bh(&vi->refill_lock); > > > } > > > > > > +static void enable_rx_mode_work(struct virtnet_info *vi) > > > +{ > > > + rtnl_lock(); > > > + vi->rx_mode_work_enabled = true; > > > + rtnl_unlock(); > > > +} > > > + > > > +static void disable_rx_mode_work(struct virtnet_info *vi) > > > +{ > > > + rtnl_lock(); > > > + vi->rx_mode_work_enabled = false; > > > + rtnl_unlock(); > > > +} > > > + > > > static void virtqueue_napi_schedule(struct napi_struct *napi, > > > struct virtqueue *vq) > > > { > > > @@ -2341,9 +2361,11 @@ static int virtnet_close(struct net_device *dev) > > > return 0; > > > } > > > > > > -static void virtnet_set_rx_mode(struct net_device *dev) > > > +static void virtnet_rx_mode_work(struct work_struct *work) > > > { > > > - struct virtnet_info *vi = netdev_priv(dev); > > > + struct virtnet_info *vi = > > > + container_of(work, struct virtnet_info, rx_mode_work); > > > + struct net_device *dev = vi->dev; > > > struct scatterlist sg[2]; > > > struct virtio_net_ctrl_mac *mac_data; > > > struct netdev_hw_addr *ha; > > > @@ -2356,6 +2378,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > > > return; > > > > > > + rtnl_lock(); > > > + > > > vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > > > vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > > > > > @@ -2373,14 +2397,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > > > vi->ctrl->allmulti ? "en" : "dis"); > > > > > > + netif_addr_lock_bh(dev); > > > + > > > uc_count = netdev_uc_count(dev); > > > mc_count = netdev_mc_count(dev); > > > /* MAC filter - use one buffer for both lists */ > > > buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + > > > (2 * sizeof(mac_data->entries)), GFP_ATOMIC); > > > mac_data = buf; > > > - if (!buf) > > > + if (!buf) { > > > + netif_addr_unlock_bh(dev); > > > + rtnl_unlock(); > > > return; > > > + } > > > > > > sg_init_table(sg, 2); > > > > > > @@ -2401,6 +2430,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > netdev_for_each_mc_addr(ha, dev) > > > memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); > > > > > > + netif_addr_unlock_bh(dev); > > > + > > > sg_set_buf(&sg[1], mac_data, > > > sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); > > > > > > @@ -2408,9 +2439,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) > > > dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); > > > > > > + rtnl_unlock(); > > > + > > > kfree(buf); > > > } > > > > > > +static void virtnet_set_rx_mode(struct net_device *dev) > > > +{ > > > + struct virtnet_info *vi = netdev_priv(dev); > > > + > > > + if (vi->rx_mode_work_enabled) > > > + schedule_work(&vi->rx_mode_work); > > > +} > > > + > > > > > static int virtnet_vlan_rx_add_vid(struct net_device *dev, > > > __be16 proto, u16 vid) > > > { > > > @@ -3181,6 +3222,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > > > > > > /* Make sure no work handler is accessing the device */ > > > flush_work(&vi->config_work); > > > + disable_rx_mode_work(vi); > > > + flush_work(&vi->rx_mode_work); > > > > > > netif_tx_lock_bh(vi->dev); > > > netif_device_detach(vi->dev); > > > > Hmm so queued rx mode work will just get skipped > > and on restore we get a wrong rx mode. > > Any way to make this more robust? > > It could be done by scheduling a work on restore. > > Thanks > > > > > > > @@ -3203,6 +3246,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) > > > virtio_device_ready(vdev); > > > > > > enable_delayed_refill(vi); > > > + enable_rx_mode_work(vi); > > > > > > if (netif_running(vi->dev)) { > > > err = virtnet_open(vi->dev); > > > @@ -4002,6 +4046,7 @@ static int virtnet_probe(struct virtio_device *vdev) > > > vdev->priv = vi; > > > > > > INIT_WORK(&vi->config_work, virtnet_config_changed_work); > > > + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); > > > spin_lock_init(&vi->refill_lock); > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { > > > @@ -4110,6 +4155,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > if (vi->has_rss || vi->has_rss_hash_report) > > > virtnet_init_default_rss(vi); > > > > > > + enable_rx_mode_work(vi); > > > + > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > > rtnl_lock(); > > > > > > @@ -4207,6 +4254,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > > > > > /* Make sure no work handler is accessing the device. */ > > > flush_work(&vi->config_work); > > > + disable_rx_mode_work(vi); > > > + flush_work(&vi->rx_mode_work); > > > > > > unregister_netdev(vi->dev); > > > > > > -- > > > 2.25.1 > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue 2023-05-25 7:41 ` Michael S. Tsirkin @ 2023-05-26 1:31 ` Jason Wang 2023-05-28 11:39 ` Michael S. Tsirkin 0 siblings, 1 reply; 12+ messages in thread From: Jason Wang @ 2023-05-26 1:31 UTC (permalink / raw) To: Michael S. Tsirkin Cc: xuanzhuo, netdev, linux-kernel, virtualization, edumazet, kuba, pabeni, davem On Thu, May 25, 2023 at 3:41 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, May 25, 2023 at 11:43:34AM +0800, Jason Wang wrote: > > On Wed, May 24, 2023 at 5:15 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Wed, May 24, 2023 at 04:18:41PM +0800, Jason Wang wrote: > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > a must for allow to sleep when waiting for the cvq command to > > > > response since current code is executed under addr spin lock. > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > --- > > > > Changes since V1: > > > > - use RTNL to synchronize rx mode worker > > > > --- > > > > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- > > > > 1 file changed, 52 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > index 56ca1d270304..5d2f1da4eaa0 100644 > > > > --- a/drivers/net/virtio_net.c > > > > +++ b/drivers/net/virtio_net.c > > > > @@ -265,6 +265,12 @@ struct virtnet_info { > > > > /* Work struct for config space updates */ > > > > struct work_struct config_work; > > > > > > > > + /* Work struct for config rx mode */ > > > > > > With a bit less abbreviation maybe? setting rx mode? > > > > That's fine. > > > > > > > > > + struct work_struct rx_mode_work; > > > > + > > > > + /* Is rx mode work enabled? */ > > > > > > Ugh not a great comment. > > > > Any suggestions for this. E.g we had: > > > > /* Is delayed refill enabled? */ > > /* OK to queue work setting RX mode? */ Ok. > > > > > > > > > + bool rx_mode_work_enabled; > > > > + > > > > > > > > > > > > > /* Does the affinity hint is set for virtqueues? */ > > > > bool affinity_hint_set; > > > > > > > > @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) > > > > spin_unlock_bh(&vi->refill_lock); > > > > } > > > > > > > > +static void enable_rx_mode_work(struct virtnet_info *vi) > > > > +{ > > > > + rtnl_lock(); > > > > + vi->rx_mode_work_enabled = true; > > > > + rtnl_unlock(); > > > > +} > > > > + > > > > +static void disable_rx_mode_work(struct virtnet_info *vi) > > > > +{ > > > > + rtnl_lock(); > > > > + vi->rx_mode_work_enabled = false; > > > > + rtnl_unlock(); > > > > +} > > > > + > > > > static void virtqueue_napi_schedule(struct napi_struct *napi, > > > > struct virtqueue *vq) > > > > { > > > > @@ -2341,9 +2361,11 @@ static int virtnet_close(struct net_device *dev) > > > > return 0; > > > > } > > > > > > > > -static void virtnet_set_rx_mode(struct net_device *dev) > > > > +static void virtnet_rx_mode_work(struct work_struct *work) > > > > { > > > > - struct virtnet_info *vi = netdev_priv(dev); > > > > + struct virtnet_info *vi = > > > > + container_of(work, struct virtnet_info, rx_mode_work); > > > > + struct net_device *dev = vi->dev; > > > > struct scatterlist sg[2]; > > > > struct virtio_net_ctrl_mac *mac_data; > > > > struct netdev_hw_addr *ha; > > > > @@ -2356,6 +2378,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > > > > return; > > > > > > > > + rtnl_lock(); > > > > + > > > > vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > > > > vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > > > > > > > @@ -2373,14 +2397,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > > > > vi->ctrl->allmulti ? "en" : "dis"); > > > > > > > > + netif_addr_lock_bh(dev); > > > > + > > > > uc_count = netdev_uc_count(dev); > > > > mc_count = netdev_mc_count(dev); > > > > /* MAC filter - use one buffer for both lists */ > > > > buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + > > > > (2 * sizeof(mac_data->entries)), GFP_ATOMIC); > > > > mac_data = buf; > > > > - if (!buf) > > > > + if (!buf) { > > > > + netif_addr_unlock_bh(dev); > > > > + rtnl_unlock(); > > > > return; > > > > + } > > > > > > > > sg_init_table(sg, 2); > > > > > > > > @@ -2401,6 +2430,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > netdev_for_each_mc_addr(ha, dev) > > > > memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); > > > > > > > > + netif_addr_unlock_bh(dev); > > > > + > > > > sg_set_buf(&sg[1], mac_data, > > > > sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); > > > > > > > > @@ -2408,9 +2439,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) > > > > dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); > > > > > > > > + rtnl_unlock(); > > > > + > > > > kfree(buf); > > > > } > > > > > > > > +static void virtnet_set_rx_mode(struct net_device *dev) > > > > +{ > > > > + struct virtnet_info *vi = netdev_priv(dev); > > > > + > > > > + if (vi->rx_mode_work_enabled) > > > > + schedule_work(&vi->rx_mode_work); > > > > +} > > > > + > > > > > > > static int virtnet_vlan_rx_add_vid(struct net_device *dev, > > > > __be16 proto, u16 vid) > > > > { > > > > @@ -3181,6 +3222,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > > > > > > > > /* Make sure no work handler is accessing the device */ > > > > flush_work(&vi->config_work); > > > > + disable_rx_mode_work(vi); > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > netif_tx_lock_bh(vi->dev); > > > > netif_device_detach(vi->dev); > > > > > > Hmm so queued rx mode work will just get skipped > > > and on restore we get a wrong rx mode. > > > Any way to make this more robust? > > > > It could be done by scheduling a work on restore. Rethink this, I think we don't need to care about this case since the user processes should have been frozened. And that the reason we don't even need to hold RTNL here. Thanks > > > > Thanks > > > > > > > > > > > > @@ -3203,6 +3246,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) > > > > virtio_device_ready(vdev); > > > > > > > > enable_delayed_refill(vi); > > > > + enable_rx_mode_work(vi); > > > > > > > > if (netif_running(vi->dev)) { > > > > err = virtnet_open(vi->dev); > > > > @@ -4002,6 +4046,7 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > vdev->priv = vi; > > > > > > > > INIT_WORK(&vi->config_work, virtnet_config_changed_work); > > > > + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); > > > > spin_lock_init(&vi->refill_lock); > > > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { > > > > @@ -4110,6 +4155,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > if (vi->has_rss || vi->has_rss_hash_report) > > > > virtnet_init_default_rss(vi); > > > > > > > > + enable_rx_mode_work(vi); > > > > + > > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > > > rtnl_lock(); > > > > > > > > @@ -4207,6 +4254,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > > > > > > > /* Make sure no work handler is accessing the device. */ > > > > flush_work(&vi->config_work); > > > > + disable_rx_mode_work(vi); > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > unregister_netdev(vi->dev); > > > > > > > > -- > > > > 2.25.1 > > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue 2023-05-26 1:31 ` Jason Wang @ 2023-05-28 11:39 ` Michael S. Tsirkin 2023-05-29 1:21 ` Jason Wang 0 siblings, 1 reply; 12+ messages in thread From: Michael S. Tsirkin @ 2023-05-28 11:39 UTC (permalink / raw) To: Jason Wang Cc: xuanzhuo, netdev, linux-kernel, virtualization, edumazet, kuba, pabeni, davem On Fri, May 26, 2023 at 09:31:34AM +0800, Jason Wang wrote: > On Thu, May 25, 2023 at 3:41 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Thu, May 25, 2023 at 11:43:34AM +0800, Jason Wang wrote: > > > On Wed, May 24, 2023 at 5:15 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Wed, May 24, 2023 at 04:18:41PM +0800, Jason Wang wrote: > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > --- > > > > > Changes since V1: > > > > > - use RTNL to synchronize rx mode worker > > > > > --- > > > > > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- > > > > > 1 file changed, 52 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > index 56ca1d270304..5d2f1da4eaa0 100644 > > > > > --- a/drivers/net/virtio_net.c > > > > > +++ b/drivers/net/virtio_net.c > > > > > @@ -265,6 +265,12 @@ struct virtnet_info { > > > > > /* Work struct for config space updates */ > > > > > struct work_struct config_work; > > > > > > > > > > + /* Work struct for config rx mode */ > > > > > > > > With a bit less abbreviation maybe? setting rx mode? > > > > > > That's fine. > > > > > > > > > > > > + struct work_struct rx_mode_work; > > > > > + > > > > > + /* Is rx mode work enabled? */ > > > > > > > > Ugh not a great comment. > > > > > > Any suggestions for this. E.g we had: > > > > > > /* Is delayed refill enabled? */ > > > > /* OK to queue work setting RX mode? */ > > Ok. > > > > > > > > > > > > > > + bool rx_mode_work_enabled; > > > > > + > > > > > > > > > > > > > > > > > /* Does the affinity hint is set for virtqueues? */ > > > > > bool affinity_hint_set; > > > > > > > > > > @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) > > > > > spin_unlock_bh(&vi->refill_lock); > > > > > } > > > > > > > > > > +static void enable_rx_mode_work(struct virtnet_info *vi) > > > > > +{ > > > > > + rtnl_lock(); > > > > > + vi->rx_mode_work_enabled = true; > > > > > + rtnl_unlock(); > > > > > +} > > > > > + > > > > > +static void disable_rx_mode_work(struct virtnet_info *vi) > > > > > +{ > > > > > + rtnl_lock(); > > > > > + vi->rx_mode_work_enabled = false; > > > > > + rtnl_unlock(); > > > > > +} > > > > > + > > > > > static void virtqueue_napi_schedule(struct napi_struct *napi, > > > > > struct virtqueue *vq) > > > > > { > > > > > @@ -2341,9 +2361,11 @@ static int virtnet_close(struct net_device *dev) > > > > > return 0; > > > > > } > > > > > > > > > > -static void virtnet_set_rx_mode(struct net_device *dev) > > > > > +static void virtnet_rx_mode_work(struct work_struct *work) > > > > > { > > > > > - struct virtnet_info *vi = netdev_priv(dev); > > > > > + struct virtnet_info *vi = > > > > > + container_of(work, struct virtnet_info, rx_mode_work); > > > > > + struct net_device *dev = vi->dev; > > > > > struct scatterlist sg[2]; > > > > > struct virtio_net_ctrl_mac *mac_data; > > > > > struct netdev_hw_addr *ha; > > > > > @@ -2356,6 +2378,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > > > > > return; > > > > > > > > > > + rtnl_lock(); > > > > > + > > > > > vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > > > > > vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > > > > > > > > > @@ -2373,14 +2397,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > > > > > vi->ctrl->allmulti ? "en" : "dis"); > > > > > > > > > > + netif_addr_lock_bh(dev); > > > > > + > > > > > uc_count = netdev_uc_count(dev); > > > > > mc_count = netdev_mc_count(dev); > > > > > /* MAC filter - use one buffer for both lists */ > > > > > buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + > > > > > (2 * sizeof(mac_data->entries)), GFP_ATOMIC); > > > > > mac_data = buf; > > > > > - if (!buf) > > > > > + if (!buf) { > > > > > + netif_addr_unlock_bh(dev); > > > > > + rtnl_unlock(); > > > > > return; > > > > > + } > > > > > > > > > > sg_init_table(sg, 2); > > > > > > > > > > @@ -2401,6 +2430,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > netdev_for_each_mc_addr(ha, dev) > > > > > memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); > > > > > > > > > > + netif_addr_unlock_bh(dev); > > > > > + > > > > > sg_set_buf(&sg[1], mac_data, > > > > > sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); > > > > > > > > > > @@ -2408,9 +2439,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) > > > > > dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); > > > > > > > > > > + rtnl_unlock(); > > > > > + > > > > > kfree(buf); > > > > > } > > > > > > > > > > +static void virtnet_set_rx_mode(struct net_device *dev) > > > > > +{ > > > > > + struct virtnet_info *vi = netdev_priv(dev); > > > > > + > > > > > + if (vi->rx_mode_work_enabled) > > > > > + schedule_work(&vi->rx_mode_work); > > > > > +} > > > > > + > > > > > > > > > static int virtnet_vlan_rx_add_vid(struct net_device *dev, > > > > > __be16 proto, u16 vid) > > > > > { > > > > > @@ -3181,6 +3222,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > > > > > > > > > > /* Make sure no work handler is accessing the device */ > > > > > flush_work(&vi->config_work); > > > > > + disable_rx_mode_work(vi); > > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > > > netif_tx_lock_bh(vi->dev); > > > > > netif_device_detach(vi->dev); > > > > > > > > Hmm so queued rx mode work will just get skipped > > > > and on restore we get a wrong rx mode. > > > > Any way to make this more robust? > > > > > > It could be done by scheduling a work on restore. > > Rethink this, I think we don't need to care about this case since the > user processes should have been frozened. Yes but not the workqueue. Want to switch to system_freezable_wq? > And that the reason we don't > even need to hold RTNL here. > > Thanks > > > > > > > Thanks > > > > > > > > > > > > > > > > > @@ -3203,6 +3246,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) > > > > > virtio_device_ready(vdev); > > > > > > > > > > enable_delayed_refill(vi); > > > > > + enable_rx_mode_work(vi); > > > > > > > > > > if (netif_running(vi->dev)) { > > > > > err = virtnet_open(vi->dev); > > > > > @@ -4002,6 +4046,7 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > vdev->priv = vi; > > > > > > > > > > INIT_WORK(&vi->config_work, virtnet_config_changed_work); > > > > > + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); > > > > > spin_lock_init(&vi->refill_lock); > > > > > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { > > > > > @@ -4110,6 +4155,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > if (vi->has_rss || vi->has_rss_hash_report) > > > > > virtnet_init_default_rss(vi); > > > > > > > > > > + enable_rx_mode_work(vi); > > > > > + > > > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > > > > rtnl_lock(); > > > > > > > > > > @@ -4207,6 +4254,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > > > > > > > > > /* Make sure no work handler is accessing the device. */ > > > > > flush_work(&vi->config_work); > > > > > + disable_rx_mode_work(vi); > > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > > > unregister_netdev(vi->dev); > > > > > > > > > > -- > > > > > 2.25.1 > > > > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue 2023-05-28 11:39 ` Michael S. Tsirkin @ 2023-05-29 1:21 ` Jason Wang 2023-05-31 1:07 ` Jason Wang 0 siblings, 1 reply; 12+ messages in thread From: Jason Wang @ 2023-05-29 1:21 UTC (permalink / raw) To: Michael S. Tsirkin Cc: xuanzhuo, netdev, linux-kernel, virtualization, edumazet, kuba, pabeni, davem On Sun, May 28, 2023 at 7:39 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, May 26, 2023 at 09:31:34AM +0800, Jason Wang wrote: > > On Thu, May 25, 2023 at 3:41 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Thu, May 25, 2023 at 11:43:34AM +0800, Jason Wang wrote: > > > > On Wed, May 24, 2023 at 5:15 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > On Wed, May 24, 2023 at 04:18:41PM +0800, Jason Wang wrote: > > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > --- > > > > > > Changes since V1: > > > > > > - use RTNL to synchronize rx mode worker > > > > > > --- > > > > > > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- > > > > > > 1 file changed, 52 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > > index 56ca1d270304..5d2f1da4eaa0 100644 > > > > > > --- a/drivers/net/virtio_net.c > > > > > > +++ b/drivers/net/virtio_net.c > > > > > > @@ -265,6 +265,12 @@ struct virtnet_info { > > > > > > /* Work struct for config space updates */ > > > > > > struct work_struct config_work; > > > > > > > > > > > > + /* Work struct for config rx mode */ > > > > > > > > > > With a bit less abbreviation maybe? setting rx mode? > > > > > > > > That's fine. > > > > > > > > > > > > > > > + struct work_struct rx_mode_work; > > > > > > + > > > > > > + /* Is rx mode work enabled? */ > > > > > > > > > > Ugh not a great comment. > > > > > > > > Any suggestions for this. E.g we had: > > > > > > > > /* Is delayed refill enabled? */ > > > > > > /* OK to queue work setting RX mode? */ > > > > Ok. > > > > > > > > > > > > > > > > > > > + bool rx_mode_work_enabled; > > > > > > + > > > > > > > > > > > > > > > > > > > > > /* Does the affinity hint is set for virtqueues? */ > > > > > > bool affinity_hint_set; > > > > > > > > > > > > @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) > > > > > > spin_unlock_bh(&vi->refill_lock); > > > > > > } > > > > > > > > > > > > +static void enable_rx_mode_work(struct virtnet_info *vi) > > > > > > +{ > > > > > > + rtnl_lock(); > > > > > > + vi->rx_mode_work_enabled = true; > > > > > > + rtnl_unlock(); > > > > > > +} > > > > > > + > > > > > > +static void disable_rx_mode_work(struct virtnet_info *vi) > > > > > > +{ > > > > > > + rtnl_lock(); > > > > > > + vi->rx_mode_work_enabled = false; > > > > > > + rtnl_unlock(); > > > > > > +} > > > > > > + > > > > > > static void virtqueue_napi_schedule(struct napi_struct *napi, > > > > > > struct virtqueue *vq) > > > > > > { > > > > > > @@ -2341,9 +2361,11 @@ static int virtnet_close(struct net_device *dev) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > -static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > +static void virtnet_rx_mode_work(struct work_struct *work) > > > > > > { > > > > > > - struct virtnet_info *vi = netdev_priv(dev); > > > > > > + struct virtnet_info *vi = > > > > > > + container_of(work, struct virtnet_info, rx_mode_work); > > > > > > + struct net_device *dev = vi->dev; > > > > > > struct scatterlist sg[2]; > > > > > > struct virtio_net_ctrl_mac *mac_data; > > > > > > struct netdev_hw_addr *ha; > > > > > > @@ -2356,6 +2378,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > > > > > > return; > > > > > > > > > > > > + rtnl_lock(); > > > > > > + > > > > > > vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > > > > > > vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > > > > > > > > > > > @@ -2373,14 +2397,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > > > > > > vi->ctrl->allmulti ? "en" : "dis"); > > > > > > > > > > > > + netif_addr_lock_bh(dev); > > > > > > + > > > > > > uc_count = netdev_uc_count(dev); > > > > > > mc_count = netdev_mc_count(dev); > > > > > > /* MAC filter - use one buffer for both lists */ > > > > > > buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + > > > > > > (2 * sizeof(mac_data->entries)), GFP_ATOMIC); > > > > > > mac_data = buf; > > > > > > - if (!buf) > > > > > > + if (!buf) { > > > > > > + netif_addr_unlock_bh(dev); > > > > > > + rtnl_unlock(); > > > > > > return; > > > > > > + } > > > > > > > > > > > > sg_init_table(sg, 2); > > > > > > > > > > > > @@ -2401,6 +2430,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > netdev_for_each_mc_addr(ha, dev) > > > > > > memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); > > > > > > > > > > > > + netif_addr_unlock_bh(dev); > > > > > > + > > > > > > sg_set_buf(&sg[1], mac_data, > > > > > > sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); > > > > > > > > > > > > @@ -2408,9 +2439,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) > > > > > > dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); > > > > > > > > > > > > + rtnl_unlock(); > > > > > > + > > > > > > kfree(buf); > > > > > > } > > > > > > > > > > > > +static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > +{ > > > > > > + struct virtnet_info *vi = netdev_priv(dev); > > > > > > + > > > > > > + if (vi->rx_mode_work_enabled) > > > > > > + schedule_work(&vi->rx_mode_work); > > > > > > +} > > > > > > + > > > > > > > > > > > static int virtnet_vlan_rx_add_vid(struct net_device *dev, > > > > > > __be16 proto, u16 vid) > > > > > > { > > > > > > @@ -3181,6 +3222,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > > > > > > > > > > > > /* Make sure no work handler is accessing the device */ > > > > > > flush_work(&vi->config_work); > > > > > > + disable_rx_mode_work(vi); > > > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > > > > > netif_tx_lock_bh(vi->dev); > > > > > > netif_device_detach(vi->dev); > > > > > > > > > > Hmm so queued rx mode work will just get skipped > > > > > and on restore we get a wrong rx mode. > > > > > Any way to make this more robust? > > > > > > > > It could be done by scheduling a work on restore. > > > > Rethink this, I think we don't need to care about this case since the > > user processes should have been frozened. > > Yes but not the workqueue. Want to switch to system_freezable_wq? Yes, I will do it in v2. Thanks > > > And that the reason we don't > > even need to hold RTNL here. > > > > Thanks > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > @@ -3203,6 +3246,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) > > > > > > virtio_device_ready(vdev); > > > > > > > > > > > > enable_delayed_refill(vi); > > > > > > + enable_rx_mode_work(vi); > > > > > > > > > > > > if (netif_running(vi->dev)) { > > > > > > err = virtnet_open(vi->dev); > > > > > > @@ -4002,6 +4046,7 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > > vdev->priv = vi; > > > > > > > > > > > > INIT_WORK(&vi->config_work, virtnet_config_changed_work); > > > > > > + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); > > > > > > spin_lock_init(&vi->refill_lock); > > > > > > > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { > > > > > > @@ -4110,6 +4155,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > > if (vi->has_rss || vi->has_rss_hash_report) > > > > > > virtnet_init_default_rss(vi); > > > > > > > > > > > > + enable_rx_mode_work(vi); > > > > > > + > > > > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > > > > > rtnl_lock(); > > > > > > > > > > > > @@ -4207,6 +4254,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > > > > > > > > > > > /* Make sure no work handler is accessing the device. */ > > > > > > flush_work(&vi->config_work); > > > > > > + disable_rx_mode_work(vi); > > > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > > > > > unregister_netdev(vi->dev); > > > > > > > > > > > > -- > > > > > > 2.25.1 > > > > > > > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue 2023-05-29 1:21 ` Jason Wang @ 2023-05-31 1:07 ` Jason Wang 2023-06-28 13:33 ` Michael S. Tsirkin 0 siblings, 1 reply; 12+ messages in thread From: Jason Wang @ 2023-05-31 1:07 UTC (permalink / raw) To: Michael S. Tsirkin Cc: xuanzhuo, netdev, linux-kernel, virtualization, edumazet, kuba, pabeni, davem On Mon, May 29, 2023 at 9:21 AM Jason Wang <jasowang@redhat.com> wrote: > > On Sun, May 28, 2023 at 7:39 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Fri, May 26, 2023 at 09:31:34AM +0800, Jason Wang wrote: > > > On Thu, May 25, 2023 at 3:41 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Thu, May 25, 2023 at 11:43:34AM +0800, Jason Wang wrote: > > > > > On Wed, May 24, 2023 at 5:15 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > On Wed, May 24, 2023 at 04:18:41PM +0800, Jason Wang wrote: > > > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > --- > > > > > > > Changes since V1: > > > > > > > - use RTNL to synchronize rx mode worker > > > > > > > --- > > > > > > > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- > > > > > > > 1 file changed, 52 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > > > index 56ca1d270304..5d2f1da4eaa0 100644 > > > > > > > --- a/drivers/net/virtio_net.c > > > > > > > +++ b/drivers/net/virtio_net.c > > > > > > > @@ -265,6 +265,12 @@ struct virtnet_info { > > > > > > > /* Work struct for config space updates */ > > > > > > > struct work_struct config_work; > > > > > > > > > > > > > > + /* Work struct for config rx mode */ > > > > > > > > > > > > With a bit less abbreviation maybe? setting rx mode? > > > > > > > > > > That's fine. > > > > > > > > > > > > > > > > > > + struct work_struct rx_mode_work; > > > > > > > + > > > > > > > + /* Is rx mode work enabled? */ > > > > > > > > > > > > Ugh not a great comment. > > > > > > > > > > Any suggestions for this. E.g we had: > > > > > > > > > > /* Is delayed refill enabled? */ > > > > > > > > /* OK to queue work setting RX mode? */ > > > > > > Ok. > > > > > > > > > > > > > > > > > > > > > > > > + bool rx_mode_work_enabled; > > > > > > > + > > > > > > > > > > > > > > > > > > > > > > > > > /* Does the affinity hint is set for virtqueues? */ > > > > > > > bool affinity_hint_set; > > > > > > > > > > > > > > @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) > > > > > > > spin_unlock_bh(&vi->refill_lock); > > > > > > > } > > > > > > > > > > > > > > +static void enable_rx_mode_work(struct virtnet_info *vi) > > > > > > > +{ > > > > > > > + rtnl_lock(); > > > > > > > + vi->rx_mode_work_enabled = true; > > > > > > > + rtnl_unlock(); > > > > > > > +} > > > > > > > + > > > > > > > +static void disable_rx_mode_work(struct virtnet_info *vi) > > > > > > > +{ > > > > > > > + rtnl_lock(); > > > > > > > + vi->rx_mode_work_enabled = false; > > > > > > > + rtnl_unlock(); > > > > > > > +} > > > > > > > + > > > > > > > static void virtqueue_napi_schedule(struct napi_struct *napi, > > > > > > > struct virtqueue *vq) > > > > > > > { > > > > > > > @@ -2341,9 +2361,11 @@ static int virtnet_close(struct net_device *dev) > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > -static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > +static void virtnet_rx_mode_work(struct work_struct *work) > > > > > > > { > > > > > > > - struct virtnet_info *vi = netdev_priv(dev); > > > > > > > + struct virtnet_info *vi = > > > > > > > + container_of(work, struct virtnet_info, rx_mode_work); > > > > > > > + struct net_device *dev = vi->dev; > > > > > > > struct scatterlist sg[2]; > > > > > > > struct virtio_net_ctrl_mac *mac_data; > > > > > > > struct netdev_hw_addr *ha; > > > > > > > @@ -2356,6 +2378,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > > > > > > > return; > > > > > > > > > > > > > > + rtnl_lock(); > > > > > > > + > > > > > > > vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > > > > > > > vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > > > > > > > > > > > > > @@ -2373,14 +2397,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > > > > > > > vi->ctrl->allmulti ? "en" : "dis"); > > > > > > > > > > > > > > + netif_addr_lock_bh(dev); > > > > > > > + > > > > > > > uc_count = netdev_uc_count(dev); > > > > > > > mc_count = netdev_mc_count(dev); > > > > > > > /* MAC filter - use one buffer for both lists */ > > > > > > > buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + > > > > > > > (2 * sizeof(mac_data->entries)), GFP_ATOMIC); > > > > > > > mac_data = buf; > > > > > > > - if (!buf) > > > > > > > + if (!buf) { > > > > > > > + netif_addr_unlock_bh(dev); > > > > > > > + rtnl_unlock(); > > > > > > > return; > > > > > > > + } > > > > > > > > > > > > > > sg_init_table(sg, 2); > > > > > > > > > > > > > > @@ -2401,6 +2430,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > netdev_for_each_mc_addr(ha, dev) > > > > > > > memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); > > > > > > > > > > > > > > + netif_addr_unlock_bh(dev); > > > > > > > + > > > > > > > sg_set_buf(&sg[1], mac_data, > > > > > > > sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); > > > > > > > > > > > > > > @@ -2408,9 +2439,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) > > > > > > > dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); > > > > > > > > > > > > > > + rtnl_unlock(); > > > > > > > + > > > > > > > kfree(buf); > > > > > > > } > > > > > > > > > > > > > > +static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > +{ > > > > > > > + struct virtnet_info *vi = netdev_priv(dev); > > > > > > > + > > > > > > > + if (vi->rx_mode_work_enabled) > > > > > > > + schedule_work(&vi->rx_mode_work); > > > > > > > +} > > > > > > > + > > > > > > > > > > > > > static int virtnet_vlan_rx_add_vid(struct net_device *dev, > > > > > > > __be16 proto, u16 vid) > > > > > > > { > > > > > > > @@ -3181,6 +3222,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > > > > > > > > > > > > > > /* Make sure no work handler is accessing the device */ > > > > > > > flush_work(&vi->config_work); > > > > > > > + disable_rx_mode_work(vi); > > > > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > > > > > > > netif_tx_lock_bh(vi->dev); > > > > > > > netif_device_detach(vi->dev); > > > > > > > > > > > > Hmm so queued rx mode work will just get skipped > > > > > > and on restore we get a wrong rx mode. > > > > > > Any way to make this more robust? > > > > > > > > > > It could be done by scheduling a work on restore. > > > > > > Rethink this, I think we don't need to care about this case since the > > > user processes should have been frozened. > > > > Yes but not the workqueue. Want to switch to system_freezable_wq? > > Yes, I will do it in v2. Actually, this doesn't work. Freezable workqueue can only guarantee when being freezed the new work will be queued and not scheduled until thaw. So the ktrhead that is executing the workqueue is not freezable. The busy loop (even with cond_resched()) will force suspend in this case. I wonder if we should switch to using a dedicated kthread for virtio-net then we can allow it to be frozen. Thanks > > Thanks > > > > > > And that the reason we don't > > > even need to hold RTNL here. > > > > > > Thanks > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -3203,6 +3246,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) > > > > > > > virtio_device_ready(vdev); > > > > > > > > > > > > > > enable_delayed_refill(vi); > > > > > > > + enable_rx_mode_work(vi); > > > > > > > > > > > > > > if (netif_running(vi->dev)) { > > > > > > > err = virtnet_open(vi->dev); > > > > > > > @@ -4002,6 +4046,7 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > > > vdev->priv = vi; > > > > > > > > > > > > > > INIT_WORK(&vi->config_work, virtnet_config_changed_work); > > > > > > > + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); > > > > > > > spin_lock_init(&vi->refill_lock); > > > > > > > > > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { > > > > > > > @@ -4110,6 +4155,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > > > if (vi->has_rss || vi->has_rss_hash_report) > > > > > > > virtnet_init_default_rss(vi); > > > > > > > > > > > > > > + enable_rx_mode_work(vi); > > > > > > > + > > > > > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > > > > > > rtnl_lock(); > > > > > > > > > > > > > > @@ -4207,6 +4254,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > > > > > > > > > > > > > /* Make sure no work handler is accessing the device. */ > > > > > > > flush_work(&vi->config_work); > > > > > > > + disable_rx_mode_work(vi); > > > > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > > > > > > > unregister_netdev(vi->dev); > > > > > > > > > > > > > > -- > > > > > > > 2.25.1 > > > > > > > > > > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue 2023-05-31 1:07 ` Jason Wang @ 2023-06-28 13:33 ` Michael S. Tsirkin 2023-06-29 3:20 ` Jason Wang 0 siblings, 1 reply; 12+ messages in thread From: Michael S. Tsirkin @ 2023-06-28 13:33 UTC (permalink / raw) To: Jason Wang Cc: xuanzhuo, netdev, linux-kernel, virtualization, edumazet, kuba, pabeni, davem On Wed, May 31, 2023 at 09:07:25AM +0800, Jason Wang wrote: > On Mon, May 29, 2023 at 9:21 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Sun, May 28, 2023 at 7:39 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Fri, May 26, 2023 at 09:31:34AM +0800, Jason Wang wrote: > > > > On Thu, May 25, 2023 at 3:41 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > On Thu, May 25, 2023 at 11:43:34AM +0800, Jason Wang wrote: > > > > > > On Wed, May 24, 2023 at 5:15 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > On Wed, May 24, 2023 at 04:18:41PM +0800, Jason Wang wrote: > > > > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > --- > > > > > > > > Changes since V1: > > > > > > > > - use RTNL to synchronize rx mode worker > > > > > > > > --- > > > > > > > > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- > > > > > > > > 1 file changed, 52 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > > > > index 56ca1d270304..5d2f1da4eaa0 100644 > > > > > > > > --- a/drivers/net/virtio_net.c > > > > > > > > +++ b/drivers/net/virtio_net.c > > > > > > > > @@ -265,6 +265,12 @@ struct virtnet_info { > > > > > > > > /* Work struct for config space updates */ > > > > > > > > struct work_struct config_work; > > > > > > > > > > > > > > > > + /* Work struct for config rx mode */ > > > > > > > > > > > > > > With a bit less abbreviation maybe? setting rx mode? > > > > > > > > > > > > That's fine. > > > > > > > > > > > > > > > > > > > > > + struct work_struct rx_mode_work; > > > > > > > > + > > > > > > > > + /* Is rx mode work enabled? */ > > > > > > > > > > > > > > Ugh not a great comment. > > > > > > > > > > > > Any suggestions for this. E.g we had: > > > > > > > > > > > > /* Is delayed refill enabled? */ > > > > > > > > > > /* OK to queue work setting RX mode? */ > > > > > > > > Ok. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + bool rx_mode_work_enabled; > > > > > > > > + > > > > > > > > > > > > > > > > > > > > > > > > > > > > > /* Does the affinity hint is set for virtqueues? */ > > > > > > > > bool affinity_hint_set; > > > > > > > > > > > > > > > > @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) > > > > > > > > spin_unlock_bh(&vi->refill_lock); > > > > > > > > } > > > > > > > > > > > > > > > > +static void enable_rx_mode_work(struct virtnet_info *vi) > > > > > > > > +{ > > > > > > > > + rtnl_lock(); > > > > > > > > + vi->rx_mode_work_enabled = true; > > > > > > > > + rtnl_unlock(); > > > > > > > > +} > > > > > > > > + > > > > > > > > +static void disable_rx_mode_work(struct virtnet_info *vi) > > > > > > > > +{ > > > > > > > > + rtnl_lock(); > > > > > > > > + vi->rx_mode_work_enabled = false; > > > > > > > > + rtnl_unlock(); > > > > > > > > +} > > > > > > > > + > > > > > > > > static void virtqueue_napi_schedule(struct napi_struct *napi, > > > > > > > > struct virtqueue *vq) > > > > > > > > { > > > > > > > > @@ -2341,9 +2361,11 @@ static int virtnet_close(struct net_device *dev) > > > > > > > > return 0; > > > > > > > > } > > > > > > > > > > > > > > > > -static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > > +static void virtnet_rx_mode_work(struct work_struct *work) > > > > > > > > { > > > > > > > > - struct virtnet_info *vi = netdev_priv(dev); > > > > > > > > + struct virtnet_info *vi = > > > > > > > > + container_of(work, struct virtnet_info, rx_mode_work); > > > > > > > > + struct net_device *dev = vi->dev; > > > > > > > > struct scatterlist sg[2]; > > > > > > > > struct virtio_net_ctrl_mac *mac_data; > > > > > > > > struct netdev_hw_addr *ha; > > > > > > > > @@ -2356,6 +2378,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > > > > > > > > return; > > > > > > > > > > > > > > > > + rtnl_lock(); > > > > > > > > + > > > > > > > > vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > > > > > > > > vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > > > > > > > > > > > > > > > @@ -2373,14 +2397,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > > > > > > > > vi->ctrl->allmulti ? "en" : "dis"); > > > > > > > > > > > > > > > > + netif_addr_lock_bh(dev); > > > > > > > > + > > > > > > > > uc_count = netdev_uc_count(dev); > > > > > > > > mc_count = netdev_mc_count(dev); > > > > > > > > /* MAC filter - use one buffer for both lists */ > > > > > > > > buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + > > > > > > > > (2 * sizeof(mac_data->entries)), GFP_ATOMIC); > > > > > > > > mac_data = buf; > > > > > > > > - if (!buf) > > > > > > > > + if (!buf) { > > > > > > > > + netif_addr_unlock_bh(dev); > > > > > > > > + rtnl_unlock(); > > > > > > > > return; > > > > > > > > + } > > > > > > > > > > > > > > > > sg_init_table(sg, 2); > > > > > > > > > > > > > > > > @@ -2401,6 +2430,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > > netdev_for_each_mc_addr(ha, dev) > > > > > > > > memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); > > > > > > > > > > > > > > > > + netif_addr_unlock_bh(dev); > > > > > > > > + > > > > > > > > sg_set_buf(&sg[1], mac_data, > > > > > > > > sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); > > > > > > > > > > > > > > > > @@ -2408,9 +2439,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > > VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) > > > > > > > > dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); > > > > > > > > > > > > > > > > + rtnl_unlock(); > > > > > > > > + > > > > > > > > kfree(buf); > > > > > > > > } > > > > > > > > > > > > > > > > +static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > > +{ > > > > > > > > + struct virtnet_info *vi = netdev_priv(dev); > > > > > > > > + > > > > > > > > + if (vi->rx_mode_work_enabled) > > > > > > > > + schedule_work(&vi->rx_mode_work); > > > > > > > > +} > > > > > > > > + > > > > > > > > > > > > > > > static int virtnet_vlan_rx_add_vid(struct net_device *dev, > > > > > > > > __be16 proto, u16 vid) > > > > > > > > { > > > > > > > > @@ -3181,6 +3222,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > > > > > > > > > > > > > > > > /* Make sure no work handler is accessing the device */ > > > > > > > > flush_work(&vi->config_work); > > > > > > > > + disable_rx_mode_work(vi); > > > > > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > > > > > > > > > netif_tx_lock_bh(vi->dev); > > > > > > > > netif_device_detach(vi->dev); > > > > > > > > > > > > > > Hmm so queued rx mode work will just get skipped > > > > > > > and on restore we get a wrong rx mode. > > > > > > > Any way to make this more robust? > > > > > > > > > > > > It could be done by scheduling a work on restore. > > > > > > > > Rethink this, I think we don't need to care about this case since the > > > > user processes should have been frozened. > > > > > > Yes but not the workqueue. Want to switch to system_freezable_wq? > > > > Yes, I will do it in v2. > > Actually, this doesn't work. Freezable workqueue can only guarantee > when being freezed the new work will be queued and not scheduled until > thaw. So the ktrhead that is executing the workqueue is not freezable. > The busy loop (even with cond_resched()) will force suspend in this > case. > > I wonder if we should switch to using a dedicated kthread for > virtio-net then we can allow it to be frozen. > > Thanks > So what's the plan then? > > > > Thanks > > > > > > > > > And that the reason we don't > > > > even need to hold RTNL here. > > > > > > > > Thanks > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -3203,6 +3246,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) > > > > > > > > virtio_device_ready(vdev); > > > > > > > > > > > > > > > > enable_delayed_refill(vi); > > > > > > > > + enable_rx_mode_work(vi); > > > > > > > > > > > > > > > > if (netif_running(vi->dev)) { > > > > > > > > err = virtnet_open(vi->dev); > > > > > > > > @@ -4002,6 +4046,7 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > > > > vdev->priv = vi; > > > > > > > > > > > > > > > > INIT_WORK(&vi->config_work, virtnet_config_changed_work); > > > > > > > > + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); > > > > > > > > spin_lock_init(&vi->refill_lock); > > > > > > > > > > > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { > > > > > > > > @@ -4110,6 +4155,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > > > > if (vi->has_rss || vi->has_rss_hash_report) > > > > > > > > virtnet_init_default_rss(vi); > > > > > > > > > > > > > > > > + enable_rx_mode_work(vi); > > > > > > > > + > > > > > > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > > > > > > > rtnl_lock(); > > > > > > > > > > > > > > > > @@ -4207,6 +4254,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > > > > > > > > > > > > > > > /* Make sure no work handler is accessing the device. */ > > > > > > > > flush_work(&vi->config_work); > > > > > > > > + disable_rx_mode_work(vi); > > > > > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > > > > > > > > > unregister_netdev(vi->dev); > > > > > > > > > > > > > > > > -- > > > > > > > > 2.25.1 > > > > > > > > > > > > > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue 2023-06-28 13:33 ` Michael S. Tsirkin @ 2023-06-29 3:20 ` Jason Wang 0 siblings, 0 replies; 12+ messages in thread From: Jason Wang @ 2023-06-29 3:20 UTC (permalink / raw) To: Michael S. Tsirkin Cc: xuanzhuo, netdev, linux-kernel, virtualization, edumazet, kuba, pabeni, davem On Wed, Jun 28, 2023 at 9:34 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, May 31, 2023 at 09:07:25AM +0800, Jason Wang wrote: > > On Mon, May 29, 2023 at 9:21 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > On Sun, May 28, 2023 at 7:39 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Fri, May 26, 2023 at 09:31:34AM +0800, Jason Wang wrote: > > > > > On Thu, May 25, 2023 at 3:41 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > On Thu, May 25, 2023 at 11:43:34AM +0800, Jason Wang wrote: > > > > > > > On Wed, May 24, 2023 at 5:15 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > > > On Wed, May 24, 2023 at 04:18:41PM +0800, Jason Wang wrote: > > > > > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > --- > > > > > > > > > Changes since V1: > > > > > > > > > - use RTNL to synchronize rx mode worker > > > > > > > > > --- > > > > > > > > > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- > > > > > > > > > 1 file changed, 52 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > > > > > index 56ca1d270304..5d2f1da4eaa0 100644 > > > > > > > > > --- a/drivers/net/virtio_net.c > > > > > > > > > +++ b/drivers/net/virtio_net.c > > > > > > > > > @@ -265,6 +265,12 @@ struct virtnet_info { > > > > > > > > > /* Work struct for config space updates */ > > > > > > > > > struct work_struct config_work; > > > > > > > > > > > > > > > > > > + /* Work struct for config rx mode */ > > > > > > > > > > > > > > > > With a bit less abbreviation maybe? setting rx mode? > > > > > > > > > > > > > > That's fine. > > > > > > > > > > > > > > > > > > > > > > > > + struct work_struct rx_mode_work; > > > > > > > > > + > > > > > > > > > + /* Is rx mode work enabled? */ > > > > > > > > > > > > > > > > Ugh not a great comment. > > > > > > > > > > > > > > Any suggestions for this. E.g we had: > > > > > > > > > > > > > > /* Is delayed refill enabled? */ > > > > > > > > > > > > /* OK to queue work setting RX mode? */ > > > > > > > > > > Ok. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + bool rx_mode_work_enabled; > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > /* Does the affinity hint is set for virtqueues? */ > > > > > > > > > bool affinity_hint_set; > > > > > > > > > > > > > > > > > > @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) > > > > > > > > > spin_unlock_bh(&vi->refill_lock); > > > > > > > > > } > > > > > > > > > > > > > > > > > > +static void enable_rx_mode_work(struct virtnet_info *vi) > > > > > > > > > +{ > > > > > > > > > + rtnl_lock(); > > > > > > > > > + vi->rx_mode_work_enabled = true; > > > > > > > > > + rtnl_unlock(); > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +static void disable_rx_mode_work(struct virtnet_info *vi) > > > > > > > > > +{ > > > > > > > > > + rtnl_lock(); > > > > > > > > > + vi->rx_mode_work_enabled = false; > > > > > > > > > + rtnl_unlock(); > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > static void virtqueue_napi_schedule(struct napi_struct *napi, > > > > > > > > > struct virtqueue *vq) > > > > > > > > > { > > > > > > > > > @@ -2341,9 +2361,11 @@ static int virtnet_close(struct net_device *dev) > > > > > > > > > return 0; > > > > > > > > > } > > > > > > > > > > > > > > > > > > -static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > > > +static void virtnet_rx_mode_work(struct work_struct *work) > > > > > > > > > { > > > > > > > > > - struct virtnet_info *vi = netdev_priv(dev); > > > > > > > > > + struct virtnet_info *vi = > > > > > > > > > + container_of(work, struct virtnet_info, rx_mode_work); > > > > > > > > > + struct net_device *dev = vi->dev; > > > > > > > > > struct scatterlist sg[2]; > > > > > > > > > struct virtio_net_ctrl_mac *mac_data; > > > > > > > > > struct netdev_hw_addr *ha; > > > > > > > > > @@ -2356,6 +2378,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > > > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > > > > > > > > > return; > > > > > > > > > > > > > > > > > > + rtnl_lock(); > > > > > > > > > + > > > > > > > > > vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > > > > > > > > > vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > > > > > > > > > > > > > > > > > @@ -2373,14 +2397,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > > > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > > > > > > > > > vi->ctrl->allmulti ? "en" : "dis"); > > > > > > > > > > > > > > > > > > + netif_addr_lock_bh(dev); > > > > > > > > > + > > > > > > > > > uc_count = netdev_uc_count(dev); > > > > > > > > > mc_count = netdev_mc_count(dev); > > > > > > > > > /* MAC filter - use one buffer for both lists */ > > > > > > > > > buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + > > > > > > > > > (2 * sizeof(mac_data->entries)), GFP_ATOMIC); > > > > > > > > > mac_data = buf; > > > > > > > > > - if (!buf) > > > > > > > > > + if (!buf) { > > > > > > > > > + netif_addr_unlock_bh(dev); > > > > > > > > > + rtnl_unlock(); > > > > > > > > > return; > > > > > > > > > + } > > > > > > > > > > > > > > > > > > sg_init_table(sg, 2); > > > > > > > > > > > > > > > > > > @@ -2401,6 +2430,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > > > netdev_for_each_mc_addr(ha, dev) > > > > > > > > > memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); > > > > > > > > > > > > > > > > > > + netif_addr_unlock_bh(dev); > > > > > > > > > + > > > > > > > > > sg_set_buf(&sg[1], mac_data, > > > > > > > > > sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); > > > > > > > > > > > > > > > > > > @@ -2408,9 +2439,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > > > VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) > > > > > > > > > dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); > > > > > > > > > > > > > > > > > > + rtnl_unlock(); > > > > > > > > > + > > > > > > > > > kfree(buf); > > > > > > > > > } > > > > > > > > > > > > > > > > > > +static void virtnet_set_rx_mode(struct net_device *dev) > > > > > > > > > +{ > > > > > > > > > + struct virtnet_info *vi = netdev_priv(dev); > > > > > > > > > + > > > > > > > > > + if (vi->rx_mode_work_enabled) > > > > > > > > > + schedule_work(&vi->rx_mode_work); > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > > > > > > > > > static int virtnet_vlan_rx_add_vid(struct net_device *dev, > > > > > > > > > __be16 proto, u16 vid) > > > > > > > > > { > > > > > > > > > @@ -3181,6 +3222,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > > > > > > > > > > > > > > > > > > /* Make sure no work handler is accessing the device */ > > > > > > > > > flush_work(&vi->config_work); > > > > > > > > > + disable_rx_mode_work(vi); > > > > > > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > > > > > > > > > > > netif_tx_lock_bh(vi->dev); > > > > > > > > > netif_device_detach(vi->dev); > > > > > > > > > > > > > > > > Hmm so queued rx mode work will just get skipped > > > > > > > > and on restore we get a wrong rx mode. > > > > > > > > Any way to make this more robust? > > > > > > > > > > > > > > It could be done by scheduling a work on restore. > > > > > > > > > > Rethink this, I think we don't need to care about this case since the > > > > > user processes should have been frozened. > > > > > > > > Yes but not the workqueue. Want to switch to system_freezable_wq? > > > > > > Yes, I will do it in v2. > > > > Actually, this doesn't work. Freezable workqueue can only guarantee > > when being freezed the new work will be queued and not scheduled until > > thaw. So the ktrhead that is executing the workqueue is not freezable. > > The busy loop (even with cond_resched()) will force suspend in this > > case. > > > > I wonder if we should switch to using a dedicated kthread for > > virtio-net then we can allow it to be frozen. > > > > Thanks > > > > So what's the plan then? I plan to send a new version that doesn't take special care to freeze. And address the freeze on top, probably with a dedicated kthread that can be frozen and respond to things like SIGKILL (which is somehow similar to what we want to solve for vhost). Thanks > > > > > > > Thanks > > > > > > > > > > > > And that the reason we don't > > > > > even need to hold RTNL here. > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -3203,6 +3246,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) > > > > > > > > > virtio_device_ready(vdev); > > > > > > > > > > > > > > > > > > enable_delayed_refill(vi); > > > > > > > > > + enable_rx_mode_work(vi); > > > > > > > > > > > > > > > > > > if (netif_running(vi->dev)) { > > > > > > > > > err = virtnet_open(vi->dev); > > > > > > > > > @@ -4002,6 +4046,7 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > > > > > vdev->priv = vi; > > > > > > > > > > > > > > > > > > INIT_WORK(&vi->config_work, virtnet_config_changed_work); > > > > > > > > > + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); > > > > > > > > > spin_lock_init(&vi->refill_lock); > > > > > > > > > > > > > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { > > > > > > > > > @@ -4110,6 +4155,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > > > > > if (vi->has_rss || vi->has_rss_hash_report) > > > > > > > > > virtnet_init_default_rss(vi); > > > > > > > > > > > > > > > > > > + enable_rx_mode_work(vi); > > > > > > > > > + > > > > > > > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > > > > > > > > rtnl_lock(); > > > > > > > > > > > > > > > > > > @@ -4207,6 +4254,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > > > > > > > > > > > > > > > > > /* Make sure no work handler is accessing the device. */ > > > > > > > > > flush_work(&vi->config_work); > > > > > > > > > + disable_rx_mode_work(vi); > > > > > > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > > > > > > > > > > > unregister_netdev(vi->dev); > > > > > > > > > > > > > > > > > > -- > > > > > > > > > 2.25.1 > > > > > > > > > > > > > > > > > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V3 net-next 2/2] virtio-net: add cond_resched() to the command waiting loop 2023-05-24 8:18 [PATCH V3 net-next 0/2] virtio-net: don't busy poll for cvq command Jason Wang 2023-05-24 8:18 ` [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue Jason Wang @ 2023-05-24 8:18 ` Jason Wang 1 sibling, 0 replies; 12+ messages in thread From: Jason Wang @ 2023-05-24 8:18 UTC (permalink / raw) To: mst, jasowang, xuanzhuo, davem, edumazet, kuba, pabeni Cc: netdev, linux-kernel, virtualization Adding cond_resched() to the command waiting loop for a better co-operation with the scheduler. This allows to give CPU a breath to run other task(workqueue) instead of busy looping when preemption is not allowed on a device whose CVQ might be slow. Signed-off-by: Jason Wang <jasowang@redhat.com> --- drivers/net/virtio_net.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 5d2f1da4eaa0..de498dbbf0d4 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -2207,8 +2207,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, * into the hypervisor, so the request should be handled immediately. */ while (!virtqueue_get_buf(vi->cvq, &tmp) && - !virtqueue_is_broken(vi->cvq)) + !virtqueue_is_broken(vi->cvq)) { + cond_resched(); cpu_relax(); + } return vi->ctrl->status == VIRTIO_NET_OK; } -- 2.25.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-06-29 3:20 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-05-24 8:18 [PATCH V3 net-next 0/2] virtio-net: don't busy poll for cvq command Jason Wang 2023-05-24 8:18 ` [PATCH V3 net-next 1/2] virtio-net: convert rx mode setting to use workqueue Jason Wang 2023-05-24 9:15 ` Michael S. Tsirkin 2023-05-25 3:43 ` Jason Wang 2023-05-25 7:41 ` Michael S. Tsirkin 2023-05-26 1:31 ` Jason Wang 2023-05-28 11:39 ` Michael S. Tsirkin 2023-05-29 1:21 ` Jason Wang 2023-05-31 1:07 ` Jason Wang 2023-06-28 13:33 ` Michael S. Tsirkin 2023-06-29 3:20 ` Jason Wang 2023-05-24 8:18 ` [PATCH V3 net-next 2/2] virtio-net: add cond_resched() to the command waiting loop Jason Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).