Jason Wang
2023-Apr-17 03:40 UTC
[PATCH net-next V2 1/2] virtio-net: convert rx mode setting to use workqueue
On Fri, Apr 14, 2023 at 3:21?PM Michael S. Tsirkin <mst at redhat.com> wrote:> > On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: > > Forget to cc netdev, adding. > > > > On Fri, Apr 14, 2023 at 12:25?AM Michael S. Tsirkin <mst at redhat.com> wrote: > > > > > > On Thu, Apr 13, 2023 at 02:40:26PM +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 at redhat.com> > > > > > > I don't like this frankly. This means that setting RX mode which would > > > previously be reliable, now becomes unreliable. > > > > It is "unreliable" by design: > > > > void (*ndo_set_rx_mode)(struct net_device *dev); > > > > > - first of all configuration is no longer immediate > > > > Is immediate a hard requirement? I can see a workqueue is used at least: > > > > mlx5e, ipoib, efx, ... > > > > > and there is no way for driver to find out when > > > it actually took effect > > > > But we know rx mode is best effort e.g it doesn't support vhost and we > > survive from this for years. > > > > > - second, if device fails command, this is also not > > > propagated to driver, again no way for driver to find out > > > > > > VDUSE needs to be fixed to do tricks to fix this > > > without breaking normal drivers. > > > > It's not specific to VDUSE. For example, when using virtio-net in the > > UP environment with any software cvq (like mlx5 via vDPA or cma > > transport). > > > > Thanks > > Hmm. Can we differentiate between these use-cases?It doesn't look easy since we are drivers for virtio bus. Underlayer details were hidden from virtio-net. Or do you have any ideas on this? Thanks> > > > > > > > > > > --- > > > > 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 e2560b6f7980..2e56bbf86894 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) > > > > { > > > > @@ -2310,9 +2330,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; > > > > @@ -2325,6 +2347,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); > > > > > > > > @@ -2342,14 +2366,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); > > > > > > > > @@ -2370,6 +2399,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)); > > > > > > > > @@ -2377,9 +2408,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) > > > > { > > > > @@ -3150,6 +3191,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); > > > > > > So now configuration is not propagated to device. > > > Won't device later wake up in wrong state? > > > > > > > > > > @@ -3172,6 +3215,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); > > > > @@ -3969,6 +4013,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)) { > > > > @@ -4077,6 +4122,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(); > > > > > > > > @@ -4174,6 +4221,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 > > > >
Jason Wang
2023-May-05 03:46 UTC
[PATCH net-next V2 1/2] virtio-net: convert rx mode setting to use workqueue
? 2023/4/17 11:40, Jason Wang ??:> On Fri, Apr 14, 2023 at 3:21?PM Michael S. Tsirkin <mst at redhat.com> wrote: >> On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: >>> Forget to cc netdev, adding. >>> >>> On Fri, Apr 14, 2023 at 12:25?AM Michael S. Tsirkin <mst at redhat.com> wrote: >>>> On Thu, Apr 13, 2023 at 02:40:26PM +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 at redhat.com> >>>> I don't like this frankly. This means that setting RX mode which would >>>> previously be reliable, now becomes unreliable. >>> It is "unreliable" by design: >>> >>> void (*ndo_set_rx_mode)(struct net_device *dev); >>> >>>> - first of all configuration is no longer immediate >>> Is immediate a hard requirement? I can see a workqueue is used at least: >>> >>> mlx5e, ipoib, efx, ... >>> >>>> and there is no way for driver to find out when >>>> it actually took effect >>> But we know rx mode is best effort e.g it doesn't support vhost and we >>> survive from this for years. >>> >>>> - second, if device fails command, this is also not >>>> propagated to driver, again no way for driver to find out >>>> >>>> VDUSE needs to be fixed to do tricks to fix this >>>> without breaking normal drivers. >>> It's not specific to VDUSE. For example, when using virtio-net in the >>> UP environment with any software cvq (like mlx5 via vDPA or cma >>> transport). >>> >>> Thanks >> Hmm. Can we differentiate between these use-cases? > It doesn't look easy since we are drivers for virtio bus. Underlayer > details were hidden from virtio-net. > > Or do you have any ideas on this?Michael, any thought on this? Thanks> > Thanks > >>>> >>>>> --- >>>>> 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 e2560b6f7980..2e56bbf86894 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) >>>>> { >>>>> @@ -2310,9 +2330,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; >>>>> @@ -2325,6 +2347,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); >>>>> >>>>> @@ -2342,14 +2366,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); >>>>> >>>>> @@ -2370,6 +2399,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)); >>>>> >>>>> @@ -2377,9 +2408,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) >>>>> { >>>>> @@ -3150,6 +3191,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); >>>> So now configuration is not propagated to device. >>>> Won't device later wake up in wrong state? >>>> >>>> >>>>> @@ -3172,6 +3215,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); >>>>> @@ -3969,6 +4013,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)) { >>>>> @@ -4077,6 +4122,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(); >>>>> >>>>> @@ -4174,6 +4221,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
Michael S. Tsirkin
2023-May-10 05:32 UTC
[PATCH net-next V2 1/2] virtio-net: convert rx mode setting to use workqueue
On Mon, Apr 17, 2023 at 11:40:58AM +0800, Jason Wang wrote:> On Fri, Apr 14, 2023 at 3:21?PM Michael S. Tsirkin <mst at redhat.com> wrote: > > > > On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: > > > Forget to cc netdev, adding. > > > > > > On Fri, Apr 14, 2023 at 12:25?AM Michael S. Tsirkin <mst at redhat.com> wrote: > > > > > > > > On Thu, Apr 13, 2023 at 02:40:26PM +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 at redhat.com> > > > > > > > > I don't like this frankly. This means that setting RX mode which would > > > > previously be reliable, now becomes unreliable. > > > > > > It is "unreliable" by design: > > > > > > void (*ndo_set_rx_mode)(struct net_device *dev); > > > > > > > - first of all configuration is no longer immediate > > > > > > Is immediate a hard requirement? I can see a workqueue is used at least: > > > > > > mlx5e, ipoib, efx, ... > > > > > > > and there is no way for driver to find out when > > > > it actually took effect > > > > > > But we know rx mode is best effort e.g it doesn't support vhost and we > > > survive from this for years. > > > > > > > - second, if device fails command, this is also not > > > > propagated to driver, again no way for driver to find out > > > > > > > > VDUSE needs to be fixed to do tricks to fix this > > > > without breaking normal drivers. > > > > > > It's not specific to VDUSE. For example, when using virtio-net in the > > > UP environment with any software cvq (like mlx5 via vDPA or cma > > > transport). > > > > > > Thanks > > > > Hmm. Can we differentiate between these use-cases? > > It doesn't look easy since we are drivers for virtio bus. Underlayer > details were hidden from virtio-net. > > Or do you have any ideas on this? > > ThanksI don't know, pass some kind of flag in struct virtqueue? "bool slow; /* This vq can be very slow sometimes. Don't wait for it! */" ? -- MST
Reasonably Related Threads
- [PATCH net-next V2 1/2] virtio-net: convert rx mode setting to use workqueue
- [PATCH v1 0/2] vduse: add support for networking devices
- [PATCH v1 0/2] vduse: add support for networking devices
- [PATCH v1 0/2] vduse: add support for networking devices
- [PATCH net-next V2 0/2] virtio-net: don't busy poll for cvq command