在 2023/4/17 11:40, Jason Wang 写道:

On Fri, Apr 14, 2023 at 3:21 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote:

quoted

Forget to cc netdev, adding.

On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote:

quoted

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 <redacted>

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);

quoted

- 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, ...

quoted

   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.

quoted

- 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

quoted

quoted

quoted

quoted

---
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?

quoted

@@ -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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization