On Thu, Aug 01, 2024 at 02:13:18PM +0800, Jason Wang wrote:

On Thu, Aug 1, 2024 at 1:58 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Thu, Aug 01, 2024 at 10:16:00AM +0800, Jason Wang wrote:

quoted

quoted

quoted

@@ -2885,6 +2886,25 @@ static void virtnet_cancel_dim(struct virtnet_info *vi, struct dim *dim)
      net_dim_work_cancel(dim);
 }

+static void virtnet_update_settings(struct virtnet_info *vi)
+{
+     u32 speed;
+     u8 duplex;
+
+     if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX))
+             return;
+
+     virtio_cread_le(vi->vdev, struct virtio_net_config, speed, &speed);
+
+     if (ethtool_validate_speed(speed))
+             vi->speed = speed;
+
+     virtio_cread_le(vi->vdev, struct virtio_net_config, duplex, &duplex);
+
+     if (ethtool_validate_duplex(duplex))
+             vi->duplex = duplex;
+}
+

I already commented on this approach.  This is now invoked on each open,
lots of extra VM exits. No bueno, people are working hard to keep setup
overhead under control. Handle this in the config change interrupt -
your new infrastructure is perfect for this.

No, in this version it doesn't. Config space read only happens if
there's a pending config interrupt during ndo_open:

+       if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
+               if (vi->status & VIRTIO_NET_S_LINK_UP)
+                       netif_carrier_on(vi->dev);
+               virtio_config_driver_enable(vi->vdev);
+       } else {
+               vi->status = VIRTIO_NET_S_LINK_UP;
+               netif_carrier_on(dev);
+               virtnet_update_settings(vi);
+       }

Sorry for being unclear, I was referring to !VIRTIO_NET_F_STATUS.
I do not see why do we need to bother re-reading settings in this case at all,
status is not there, nothing much changes.

Ok, let me remove it from the next version.

quoted

quoted

quoted

quoted

 static int virtnet_open(struct net_device *dev)
 {
      struct virtnet_info *vi = netdev_priv(dev);

@@ -2903,6 +2923,16 @@ static int virtnet_open(struct net_device *dev)
                      goto err_enable_qp;
      }

+     if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
+             if (vi->status & VIRTIO_NET_S_LINK_UP)
+                     netif_carrier_on(vi->dev);
+             virtio_config_driver_enable(vi->vdev);
+     } else {
+             vi->status = VIRTIO_NET_S_LINK_UP;
+             netif_carrier_on(dev);
+             virtnet_update_settings(vi);
+     }
+
      return 0;

 err_enable_qp:

@@ -3381,12 +3411,18 @@ static int virtnet_close(struct net_device *dev)
      disable_delayed_refill(vi);
      /* Make sure refill_work doesn't re-enable napi! */
      cancel_delayed_work_sync(&vi->refill);
+     /* Make sure config notification doesn't schedule config work */

it's clear what this does even without a comment.
what you should comment on, and do not, is *why*.

Well, it just follows the existing style, for example the above said

"/* Make sure refill_work doesn't re-enable napi! */"

only at the grammar level.
you don't see the difference?

        /* Make sure refill_work doesn't re-enable napi! */
        cancel_delayed_work_sync(&vi->refill);

it explains why we cancel: to avoid re-enabling napi.

why do you cancel config callback and work?
comment should say that.

Something like "Prevent the config change callback from changing
carrier after close"?

sounds good.

quoted

quoted

quoted

quoted

+     virtio_config_driver_disable(vi->vdev);
+     /* Make sure status updating is cancelled */

same

also what "status updating"? confuses more than this clarifies.

Does "Make sure the config changed work is cancelled" sounds better?

no, this just repeats what code does.
explain why you cancel it.

Does something like "Make sure carrier changes have been done by the
config change callback" works?

Thanks

I don't understand what this means.

quoted

--
MST