[PATCH V2 0/3] virtio-net: synchronize operstate with admin state on up/down

[PATCH V2 0/3] virtio-net: synchronize operstate with admin state on up/down

[Jason Wang]

Hi All:

Currently, we don't synchronize the opersteate with the admin state
currently when prevent the link status to be propagated to the stacked
device on top of the virtio-net.

This patch fixes this issue.

Changes since V1:
- Try to reuse the virtio core facility to enable and disable the
  configure interrupt

Jason Wang (3):
  virtio: allow nested disabling of the configure interrupt
  virtio: export virtio_config_{enable|disable}()
  virtio-net: synchronize operstate with admin state on up/down

 drivers/net/virtio_net.c | 72 +++++++++++++++++++++++-----------------
 drivers/virtio/virtio.c  | 26 ++++++++++-----
 include/linux/virtio.h   |  5 ++-
 3 files changed, 63 insertions(+), 40 deletions(-)

-- 
2.31.1

[PATCH V2 1/3] virtio: allow nested disabling of the configure interrupt

[Jason Wang]

Somtime driver may want to enable or disable the config callback. This
requires a synchronization with the core. So this patch change the
config_enabled to be a integer counter. This allows the toggling of
the config_enable to be synchronized between the virtio core and the
virtio driver.

The counter is not allowed to be increased greater than one, this
simplifies the logic where the interrupt could be disabled immediately
without extra synchronization between driver and core.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio.c | 20 +++++++++++++-------
 include/linux/virtio.h  |  2 +-
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index b968b2aa5f4d..d3aa74b8ae5d 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -127,7 +127,7 @@ static void __virtio_config_changed(struct virtio_device *dev)
 {
 	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
 
-	if (!dev->config_enabled)
+	if (dev->config_enabled < 1)
 		dev->config_change_pending = true;
 	else if (drv && drv->config_changed)
 		drv->config_changed(dev);
@@ -146,17 +146,23 @@ EXPORT_SYMBOL_GPL(virtio_config_changed);
 static void virtio_config_disable(struct virtio_device *dev)
 {
 	spin_lock_irq(&dev->config_lock);
-	dev->config_enabled = false;
+	--dev->config_enabled;
 	spin_unlock_irq(&dev->config_lock);
 }
 
 static void virtio_config_enable(struct virtio_device *dev)
 {
 	spin_lock_irq(&dev->config_lock);
-	dev->config_enabled = true;
-	if (dev->config_change_pending)
-		__virtio_config_changed(dev);
-	dev->config_change_pending = false;
+
+	if (dev->config_enabled < 1) {
+		++dev->config_enabled;
+		if (dev->config_enabled == 1 &&
+		    dev->config_change_pending) {
+			__virtio_config_changed(dev);
+			dev->config_change_pending = false;
+		}
+	}
+
 	spin_unlock_irq(&dev->config_lock);
 }
 
@@ -455,7 +461,7 @@ int register_virtio_device(struct virtio_device *dev)
 		goto out_ida_remove;
 
 	spin_lock_init(&dev->config_lock);
-	dev->config_enabled = false;
+	dev->config_enabled = 0;
 	dev->config_change_pending = false;
 
 	INIT_LIST_HEAD(&dev->vqs);
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 96fea920873b..4496f9ba5d82 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -132,7 +132,7 @@ struct virtio_admin_cmd {
 struct virtio_device {
 	int index;
 	bool failed;
-	bool config_enabled;
+	int config_enabled;
 	bool config_change_pending;
 	spinlock_t config_lock;
 	spinlock_t vqs_list_lock;
-- 
2.31.1

[PATCH V2 2/3] virtio: export virtio_config_{enable|disable}()

[Jason Wang]

This patch exports virtio_config_enable() and virtio_config_disable()
for drivers to disable config callbacks.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio.c | 6 ++++--
 include/linux/virtio.h  | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index d3aa74b8ae5d..a4622a62aac3 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -143,14 +143,15 @@ void virtio_config_changed(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(virtio_config_changed);
 
-static void virtio_config_disable(struct virtio_device *dev)
+void virtio_config_disable(struct virtio_device *dev)
 {
 	spin_lock_irq(&dev->config_lock);
 	--dev->config_enabled;
 	spin_unlock_irq(&dev->config_lock);
 }
+EXPORT_SYMBOL_GPL(virtio_config_disable);
 
-static void virtio_config_enable(struct virtio_device *dev)
+void virtio_config_enable(struct virtio_device *dev)
 {
 	spin_lock_irq(&dev->config_lock);
 
@@ -165,6 +166,7 @@ static void virtio_config_enable(struct virtio_device *dev)
 
 	spin_unlock_irq(&dev->config_lock);
 }
+EXPORT_SYMBOL_GPL(virtio_config_enable);
 
 void virtio_add_status(struct virtio_device *dev, unsigned int status)
 {
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 4496f9ba5d82..bc5e408582c9 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -163,6 +163,9 @@ void __virtqueue_break(struct virtqueue *_vq);
 void __virtqueue_unbreak(struct virtqueue *_vq);
 
 void virtio_config_changed(struct virtio_device *dev);
+
+void virtio_config_disable(struct virtio_device *dev);
+void virtio_config_enable(struct virtio_device *dev);
 #ifdef CONFIG_PM_SLEEP
 int virtio_device_freeze(struct virtio_device *dev);
 int virtio_device_restore(struct virtio_device *dev);
-- 
2.31.1

[PATCH V2 3/3] virtio-net: synchronize operstate with admin state on up/down

[Jason Wang]

This patch synchronize operstate with admin state per RFC2863.

This is done by trying to toggle the carrier upon open/close and
synchronize with the config change work. This allows propagate status
correctly to stacked devices like:

ip link add link enp0s3 macvlan0 type macvlan
ip link set link enp0s3 down
ip link show

Before this patch:

3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
    link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
......
5: macvlan0@enp0s3: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff

After this patch:

3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
    link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
...
5: macvlan0@enp0s3: <NO-CARRIER,BROADCAST,MULTICAST,UP,M-DOWN> mtu 1500 qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
    link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff

Cc: Venkat Venkatsubra <venkat.x.venkatsubra@oracle.com>
Cc: Gia-Khanh Nguyen <gia-khanh.nguyen@oracle.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c | 72 +++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 30 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b1f8b720733e..eff3ad3d6bcc 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2468,6 +2468,25 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
 	return err;
 }
 
+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;
+}
+
 static int virtnet_open(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -2486,6 +2505,22 @@ static int virtnet_open(struct net_device *dev)
 			goto err_enable_qp;
 	}
 
+	/* Assume link up if device can't report link status,
+	   otherwise get link status from config. */
+	netif_carrier_off(dev);
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
+		virtio_config_enable(vi->vdev);
+		/* We are not sure if config interrupt is disabled by
+		 * core or not, so we can't schedule config_work by
+		 * ourselves.
+		 */
+		virtio_config_changed(vi->vdev);
+	} else {
+		vi->status = VIRTIO_NET_S_LINK_UP;
+		virtnet_update_settings(vi);
+		netif_carrier_on(dev);
+	}
+
 	return 0;
 
 err_enable_qp:
@@ -2928,12 +2963,19 @@ 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 */
+	virtio_config_disable(vi->vdev);
+	/* Make sure status updating is cancelled */
+	cancel_work_sync(&vi->config_work);
 
 	for (i = 0; i < vi->max_queue_pairs; i++) {
 		virtnet_disable_queue_pair(vi, i);
 		cancel_work_sync(&vi->rq[i].dim.work);
 	}
 
+	vi->status &= ~VIRTIO_NET_S_LINK_UP;
+	netif_carrier_off(dev);
+
 	return 0;
 }
 
@@ -4632,25 +4674,6 @@ static void virtnet_init_settings(struct net_device *dev)
 	vi->duplex = DUPLEX_UNKNOWN;
 }
 
-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;
-}
-
 static u32 virtnet_get_rxfh_key_size(struct net_device *dev)
 {
 	return ((struct virtnet_info *)netdev_priv(dev))->rss_key_size;
@@ -5958,17 +5981,6 @@ static int virtnet_probe(struct virtio_device *vdev)
 		goto free_unregister_netdev;
 	}
 
-	/* Assume link up if device can't report link status,
-	   otherwise get link status from config. */
-	netif_carrier_off(dev);
-	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
-		schedule_work(&vi->config_work);
-	} else {
-		vi->status = VIRTIO_NET_S_LINK_UP;
-		virtnet_update_settings(vi);
-		netif_carrier_on(dev);
-	}
-
 	for (i = 0; i < ARRAY_SIZE(guest_offloads); i++)
 		if (virtio_has_feature(vi->vdev, guest_offloads[i]))
 			set_bit(guest_offloads[i], &vi->guest_offloads);
-- 
2.31.1

Re: [PATCH V2 1/3] virtio: allow nested disabling of the configure interrupt

[Michael S. Tsirkin]

On Mon, Jun 24, 2024 at 10:45:21AM +0800, Jason Wang wrote:
> Somtime driver may want to enable or disable the config callback. This
> requires a synchronization with the core. So this patch change the
> config_enabled to be a integer counter. This allows the toggling of
> the config_enable to be synchronized between the virtio core and the
> virtio driver.
> 
> The counter is not allowed to be increased greater than one, this
> simplifies the logic where the interrupt could be disabled immediately
> without extra synchronization between driver and core.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/virtio/virtio.c | 20 +++++++++++++-------
>  include/linux/virtio.h  |  2 +-
>  2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index b968b2aa5f4d..d3aa74b8ae5d 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -127,7 +127,7 @@ static void __virtio_config_changed(struct virtio_device *dev)
>  {
>  	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>  
> -	if (!dev->config_enabled)
> +	if (dev->config_enabled < 1)
>  		dev->config_change_pending = true;
>  	else if (drv && drv->config_changed)
>  		drv->config_changed(dev);
> @@ -146,17 +146,23 @@ EXPORT_SYMBOL_GPL(virtio_config_changed);
>  static void virtio_config_disable(struct virtio_device *dev)
>  {
>  	spin_lock_irq(&dev->config_lock);
> -	dev->config_enabled = false;
> +	--dev->config_enabled;
>  	spin_unlock_irq(&dev->config_lock);
>  }
>  
>  static void virtio_config_enable(struct virtio_device *dev)
>  {
>  	spin_lock_irq(&dev->config_lock);
> -	dev->config_enabled = true;
> -	if (dev->config_change_pending)
> -		__virtio_config_changed(dev);
> -	dev->config_change_pending = false;
> +
> +	if (dev->config_enabled < 1) {
> +		++dev->config_enabled;
> +		if (dev->config_enabled == 1 &&
> +		    dev->config_change_pending) {
> +			__virtio_config_changed(dev);
> +			dev->config_change_pending = false;
> +		}
> +	}
> +
>  	spin_unlock_irq(&dev->config_lock);
>  }
>

So every disable decrements the counter. Enable only increments it up to 1.
You seem to be making some very specific assumptions
about how this API will be used. Any misuse will lead to under/overflow
eventually ...



My suggestion would be to
1. rename config_enabled to config_core_enabled
2. rename virtio_config_enable/disable to virtio_config_core_enable/disable
3. add bool config_driver_disabled and make virtio_config_enable/disable
   switch that.
4. Change logic from dev->config_enabled to
   dev->config_core_enabled && !dev->config_driver_disabled



  
> @@ -455,7 +461,7 @@ int register_virtio_device(struct virtio_device *dev)
>  		goto out_ida_remove;
>  
>  	spin_lock_init(&dev->config_lock);
> -	dev->config_enabled = false;
> +	dev->config_enabled = 0;
>  	dev->config_change_pending = false;
>  
>  	INIT_LIST_HEAD(&dev->vqs);
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 96fea920873b..4496f9ba5d82 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -132,7 +132,7 @@ struct virtio_admin_cmd {
>  struct virtio_device {
>  	int index;
>  	bool failed;
> -	bool config_enabled;
> +	int config_enabled;
>  	bool config_change_pending;
>  	spinlock_t config_lock;
>  	spinlock_t vqs_list_lock;
> -- 
> 2.31.1

Re: [PATCH V2 3/3] virtio-net: synchronize operstate with admin state on up/down

[Michael S. Tsirkin]

On Mon, Jun 24, 2024 at 10:45:23AM +0800, Jason Wang wrote:
> This patch synchronize operstate with admin state per RFC2863.
> 
> This is done by trying to toggle the carrier upon open/close and
> synchronize with the config change work. This allows propagate status
> correctly to stacked devices like:
> 
> ip link add link enp0s3 macvlan0 type macvlan
> ip link set link enp0s3 down
> ip link show
> 
> Before this patch:
> 
> 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
>     link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> ......
> 5: macvlan0@enp0s3: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>     link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> 
> After this patch:
> 
> 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
>     link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> ...
> 5: macvlan0@enp0s3: <NO-CARRIER,BROADCAST,MULTICAST,UP,M-DOWN> mtu 1500 qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
>     link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> 
> Cc: Venkat Venkatsubra <venkat.x.venkatsubra@oracle.com>
> Cc: Gia-Khanh Nguyen <gia-khanh.nguyen@oracle.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/net/virtio_net.c | 72 +++++++++++++++++++++++-----------------
>  1 file changed, 42 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b1f8b720733e..eff3ad3d6bcc 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2468,6 +2468,25 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
>  	return err;
>  }
>  
> +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;
> +}
> +
>  static int virtnet_open(struct net_device *dev)
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
> @@ -2486,6 +2505,22 @@ static int virtnet_open(struct net_device *dev)
>  			goto err_enable_qp;
>  	}
>  
> +	/* Assume link up if device can't report link status,
> +	   otherwise get link status from config. */
> +	netif_carrier_off(dev);
> +	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> +		virtio_config_enable(vi->vdev);
> +		/* We are not sure if config interrupt is disabled by
> +		 * core or not, so we can't schedule config_work by
> +		 * ourselves.
> +		 */

This comment confuses more than it explains.
You seem to be arguing about some alternative design
you had in mind, but readers don't have it in mind.


Please just explain what this does and why.
For what: something like "Trigger re-read of config - same
as we'd do if config changed".

Now, please do what you don't do here: explain the why:


why do we want all these VM
exits on each open/close as opposed to once on probe and later on
config changed interrupt.


> +		virtio_config_changed(vi->vdev);
> +	} else {
> +		vi->status = VIRTIO_NET_S_LINK_UP;
> +		virtnet_update_settings(vi);
> +		netif_carrier_on(dev);
> +	}
> +
>  	return 0;
>  
>  err_enable_qp:
> @@ -2928,12 +2963,19 @@ 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 */
> +	virtio_config_disable(vi->vdev);
> +	/* Make sure status updating is cancelled */
> +	cancel_work_sync(&vi->config_work);
>  
>  	for (i = 0; i < vi->max_queue_pairs; i++) {
>  		virtnet_disable_queue_pair(vi, i);
>  		cancel_work_sync(&vi->rq[i].dim.work);
>  	}
>  
> +	vi->status &= ~VIRTIO_NET_S_LINK_UP;
> +	netif_carrier_off(dev);
> +
>  	return 0;
>  }
>  
> @@ -4632,25 +4674,6 @@ static void virtnet_init_settings(struct net_device *dev)
>  	vi->duplex = DUPLEX_UNKNOWN;
>  }
>  
> -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;
> -}
> -
>  static u32 virtnet_get_rxfh_key_size(struct net_device *dev)
>  {
>  	return ((struct virtnet_info *)netdev_priv(dev))->rss_key_size;
> @@ -5958,17 +5981,6 @@ static int virtnet_probe(struct virtio_device *vdev)
>  		goto free_unregister_netdev;
>  	}
>  
> -	/* Assume link up if device can't report link status,
> -	   otherwise get link status from config. */
> -	netif_carrier_off(dev);
> -	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> -		schedule_work(&vi->config_work);
> -	} else {
> -		vi->status = VIRTIO_NET_S_LINK_UP;
> -		virtnet_update_settings(vi);
> -		netif_carrier_on(dev);
> -	}
> -
>  	for (i = 0; i < ARRAY_SIZE(guest_offloads); i++)
>  		if (virtio_has_feature(vi->vdev, guest_offloads[i]))
>  			set_bit(guest_offloads[i], &vi->guest_offloads);
> -- 
> 2.31.1

Re: [PATCH V2 1/3] virtio: allow nested disabling of the configure interrupt

[Jason Wang]

On Mon, Jun 24, 2024 at 5:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jun 24, 2024 at 10:45:21AM +0800, Jason Wang wrote:
> > Somtime driver may want to enable or disable the config callback. This
> > requires a synchronization with the core. So this patch change the
> > config_enabled to be a integer counter. This allows the toggling of
> > the config_enable to be synchronized between the virtio core and the
> > virtio driver.
> >
> > The counter is not allowed to be increased greater than one, this
> > simplifies the logic where the interrupt could be disabled immediately
> > without extra synchronization between driver and core.
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  drivers/virtio/virtio.c | 20 +++++++++++++-------
> >  include/linux/virtio.h  |  2 +-
> >  2 files changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index b968b2aa5f4d..d3aa74b8ae5d 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -127,7 +127,7 @@ static void __virtio_config_changed(struct virtio_device *dev)
> >  {
> >       struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> >
> > -     if (!dev->config_enabled)
> > +     if (dev->config_enabled < 1)
> >               dev->config_change_pending = true;
> >       else if (drv && drv->config_changed)
> >               drv->config_changed(dev);
> > @@ -146,17 +146,23 @@ EXPORT_SYMBOL_GPL(virtio_config_changed);
> >  static void virtio_config_disable(struct virtio_device *dev)
> >  {
> >       spin_lock_irq(&dev->config_lock);
> > -     dev->config_enabled = false;
> > +     --dev->config_enabled;
> >       spin_unlock_irq(&dev->config_lock);
> >  }
> >
> >  static void virtio_config_enable(struct virtio_device *dev)
> >  {
> >       spin_lock_irq(&dev->config_lock);
> > -     dev->config_enabled = true;
> > -     if (dev->config_change_pending)
> > -             __virtio_config_changed(dev);
> > -     dev->config_change_pending = false;
> > +
> > +     if (dev->config_enabled < 1) {
> > +             ++dev->config_enabled;
> > +             if (dev->config_enabled == 1 &&
> > +                 dev->config_change_pending) {
> > +                     __virtio_config_changed(dev);
> > +                     dev->config_change_pending = false;
> > +             }
> > +     }
> > +
> >       spin_unlock_irq(&dev->config_lock);
> >  }
> >
>
> So every disable decrements the counter. Enable only increments it up to 1.
> You seem to be making some very specific assumptions
> about how this API will be used. Any misuse will lead to under/overflow
> eventually ...
>

Well, a counter gives us more information than a boolean. With
boolean, misuse is even harder to be noticed.

>
>
> My suggestion would be to
> 1. rename config_enabled to config_core_enabled
> 2. rename virtio_config_enable/disable to virtio_config_core_enable/disable
> 3. add bool config_driver_disabled and make virtio_config_enable/disable
>    switch that.
> 4. Change logic from dev->config_enabled to
>    dev->config_core_enabled && !dev->config_driver_disabled

If we make config_driver_disabled by default true, we need someone to
enable it explicitly. If it's core, it breaks the semantic that it is
under the control of the driver (or needs to synchronize with the
driver). If it's a driver, each driver needs to enable it at some time
which can be easily forgotten. And if we end up with workarounds like:

        /* If probe didn't do it, mark device DRIVER_OK ourselves. */
        if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
                virtio_device_ready(dev);

It's another break of the semantics. And actually the above is also racy.

It seems the only choice is to make config_driver_disabled by default
false. But the driver needs to be aware of this and take extra care
when calling virtio_device_ready() which is also tricky.

So in conclusion, two booleans seems sut-optimal than a counter. For
example we can use different bits for the counter as preempt_count
did. With counter(s), core and driver don't need any implicit/explicit
synchronization.

Thanks





>
>
>
>
> > @@ -455,7 +461,7 @@ int register_virtio_device(struct virtio_device *dev)
> >               goto out_ida_remove;
> >
> >       spin_lock_init(&dev->config_lock);
> > -     dev->config_enabled = false;
> > +     dev->config_enabled = 0;
> >       dev->config_change_pending = false;
> >
> >       INIT_LIST_HEAD(&dev->vqs);
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index 96fea920873b..4496f9ba5d82 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -132,7 +132,7 @@ struct virtio_admin_cmd {
> >  struct virtio_device {
> >       int index;
> >       bool failed;
> > -     bool config_enabled;
> > +     int config_enabled;
> >       bool config_change_pending;
> >       spinlock_t config_lock;
> >       spinlock_t vqs_list_lock;
> > --
> > 2.31.1
>

Re: [PATCH V2 3/3] virtio-net: synchronize operstate with admin state on up/down

[Jason Wang]

On Mon, Jun 24, 2024 at 6:07 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jun 24, 2024 at 10:45:23AM +0800, Jason Wang wrote:
> > This patch synchronize operstate with admin state per RFC2863.
> >
> > This is done by trying to toggle the carrier upon open/close and
> > synchronize with the config change work. This allows propagate status
> > correctly to stacked devices like:
> >
> > ip link add link enp0s3 macvlan0 type macvlan
> > ip link set link enp0s3 down
> > ip link show
> >
> > Before this patch:
> >
> > 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
> >     link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > ......
> > 5: macvlan0@enp0s3: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> >     link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> >
> > After this patch:
> >
> > 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
> >     link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > ...
> > 5: macvlan0@enp0s3: <NO-CARRIER,BROADCAST,MULTICAST,UP,M-DOWN> mtu 1500 qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
> >     link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> >
> > Cc: Venkat Venkatsubra <venkat.x.venkatsubra@oracle.com>
> > Cc: Gia-Khanh Nguyen <gia-khanh.nguyen@oracle.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  drivers/net/virtio_net.c | 72 +++++++++++++++++++++++-----------------
> >  1 file changed, 42 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index b1f8b720733e..eff3ad3d6bcc 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2468,6 +2468,25 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
> >       return err;
> >  }
> >
> > +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;
> > +}
> > +
> >  static int virtnet_open(struct net_device *dev)
> >  {
> >       struct virtnet_info *vi = netdev_priv(dev);
> > @@ -2486,6 +2505,22 @@ static int virtnet_open(struct net_device *dev)
> >                       goto err_enable_qp;
> >       }
> >
> > +     /* Assume link up if device can't report link status,
> > +        otherwise get link status from config. */
> > +     netif_carrier_off(dev);
> > +     if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> > +             virtio_config_enable(vi->vdev);
> > +             /* We are not sure if config interrupt is disabled by
> > +              * core or not, so we can't schedule config_work by
> > +              * ourselves.
> > +              */
>
> This comment confuses more than it explains.
> You seem to be arguing about some alternative design
> you had in mind, but readers don't have it in mind.
>
>
> Please just explain what this does and why.
> For what: something like "Trigger re-read of config - same
> as we'd do if config changed".
>
> Now, please do what you don't do here: explain the why:
>
>
> why do we want all these VM
> exits on each open/close as opposed to once on probe and later on
> config changed interrupt.

Fine, the main reason is that a config interrupt might be pending
during ifdown and core may disable configure interrupt due to several
reasons.

Thanks


>
>
> > +             virtio_config_changed(vi->vdev);
> > +     } else {
> > +             vi->status = VIRTIO_NET_S_LINK_UP;
> > +             virtnet_update_settings(vi);
> > +             netif_carrier_on(dev);
> > +     }
> > +
> >       return 0;
> >
> >  err_enable_qp:
> > @@ -2928,12 +2963,19 @@ 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 */
> > +     virtio_config_disable(vi->vdev);
> > +     /* Make sure status updating is cancelled */
> > +     cancel_work_sync(&vi->config_work);
> >
> >       for (i = 0; i < vi->max_queue_pairs; i++) {
> >               virtnet_disable_queue_pair(vi, i);
> >               cancel_work_sync(&vi->rq[i].dim.work);
> >       }
> >
> > +     vi->status &= ~VIRTIO_NET_S_LINK_UP;
> > +     netif_carrier_off(dev);
> > +
> >       return 0;
> >  }
> >
> > @@ -4632,25 +4674,6 @@ static void virtnet_init_settings(struct net_device *dev)
> >       vi->duplex = DUPLEX_UNKNOWN;
> >  }
> >
> > -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;
> > -}
> > -
> >  static u32 virtnet_get_rxfh_key_size(struct net_device *dev)
> >  {
> >       return ((struct virtnet_info *)netdev_priv(dev))->rss_key_size;
> > @@ -5958,17 +5981,6 @@ static int virtnet_probe(struct virtio_device *vdev)
> >               goto free_unregister_netdev;
> >       }
> >
> > -     /* Assume link up if device can't report link status,
> > -        otherwise get link status from config. */
> > -     netif_carrier_off(dev);
> > -     if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> > -             schedule_work(&vi->config_work);
> > -     } else {
> > -             vi->status = VIRTIO_NET_S_LINK_UP;
> > -             virtnet_update_settings(vi);
> > -             netif_carrier_on(dev);
> > -     }
> > -
> >       for (i = 0; i < ARRAY_SIZE(guest_offloads); i++)
> >               if (virtio_has_feature(vi->vdev, guest_offloads[i]))
> >                       set_bit(guest_offloads[i], &vi->guest_offloads);
> > --
> > 2.31.1
>

Re: [PATCH V2 1/3] virtio: allow nested disabling of the configure interrupt

[Michael S. Tsirkin]

On Tue, Jun 25, 2024 at 09:27:04AM +0800, Jason Wang wrote:
> On Mon, Jun 24, 2024 at 5:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Jun 24, 2024 at 10:45:21AM +0800, Jason Wang wrote:
> > > Somtime driver may want to enable or disable the config callback. This
> > > requires a synchronization with the core. So this patch change the
> > > config_enabled to be a integer counter. This allows the toggling of
> > > the config_enable to be synchronized between the virtio core and the
> > > virtio driver.
> > >
> > > The counter is not allowed to be increased greater than one, this
> > > simplifies the logic where the interrupt could be disabled immediately
> > > without extra synchronization between driver and core.
> > >
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  drivers/virtio/virtio.c | 20 +++++++++++++-------
> > >  include/linux/virtio.h  |  2 +-
> > >  2 files changed, 14 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > index b968b2aa5f4d..d3aa74b8ae5d 100644
> > > --- a/drivers/virtio/virtio.c
> > > +++ b/drivers/virtio/virtio.c
> > > @@ -127,7 +127,7 @@ static void __virtio_config_changed(struct virtio_device *dev)
> > >  {
> > >       struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> > >
> > > -     if (!dev->config_enabled)
> > > +     if (dev->config_enabled < 1)
> > >               dev->config_change_pending = true;
> > >       else if (drv && drv->config_changed)
> > >               drv->config_changed(dev);
> > > @@ -146,17 +146,23 @@ EXPORT_SYMBOL_GPL(virtio_config_changed);
> > >  static void virtio_config_disable(struct virtio_device *dev)
> > >  {
> > >       spin_lock_irq(&dev->config_lock);
> > > -     dev->config_enabled = false;
> > > +     --dev->config_enabled;
> > >       spin_unlock_irq(&dev->config_lock);
> > >  }
> > >
> > >  static void virtio_config_enable(struct virtio_device *dev)
> > >  {
> > >       spin_lock_irq(&dev->config_lock);
> > > -     dev->config_enabled = true;
> > > -     if (dev->config_change_pending)
> > > -             __virtio_config_changed(dev);
> > > -     dev->config_change_pending = false;
> > > +
> > > +     if (dev->config_enabled < 1) {
> > > +             ++dev->config_enabled;
> > > +             if (dev->config_enabled == 1 &&
> > > +                 dev->config_change_pending) {
> > > +                     __virtio_config_changed(dev);
> > > +                     dev->config_change_pending = false;
> > > +             }
> > > +     }
> > > +
> > >       spin_unlock_irq(&dev->config_lock);
> > >  }
> > >
> >
> > So every disable decrements the counter. Enable only increments it up to 1.
> > You seem to be making some very specific assumptions
> > about how this API will be used. Any misuse will lead to under/overflow
> > eventually ...
> >
> 
> Well, a counter gives us more information than a boolean. With
> boolean, misuse is even harder to be noticed.

With boolean we can prevent misuse easily because previous state
is known exactly. E.g.:

static void virtio_config_driver_disable(struct virtio_device *dev)
{
	BUG_ON(dev->config_driver_disabled);
	dev->config_driver_disabled = true;
}



static void virtio_config_driver_enable(struct virtio_device *dev)
{
	BUG_ON(!dev->config_driver_disabled);
	dev->config_driver_disabled = false;
}


Does not work with integer you simply have no idea what the value
should be at point of call.


> >
> >
> > My suggestion would be to
> > 1. rename config_enabled to config_core_enabled
> > 2. rename virtio_config_enable/disable to virtio_config_core_enable/disable
> > 3. add bool config_driver_disabled and make virtio_config_enable/disable
> >    switch that.
> > 4. Change logic from dev->config_enabled to
> >    dev->config_core_enabled && !dev->config_driver_disabled
> 
> If we make config_driver_disabled by default true,

No, we make it false by default.

> we need someone to
> enable it explicitly. If it's core, it breaks the semantic that it is
> under the control of the driver (or needs to synchronize with the
> driver). If it's a driver, each driver needs to enable it at some time
> which can be easily forgotten. And if we end up with workarounds like:
> 
>         /* If probe didn't do it, mark device DRIVER_OK ourselves. */
>         if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
>                 virtio_device_ready(dev);
> 
> It's another break of the semantics. And actually the above is also racy.
> 
> It seems the only choice is to make config_driver_disabled by default
> false. But the driver needs to be aware of this and take extra care
> when calling virtio_device_ready() which is also tricky.


No, false by default simply means no change to semantics.


> 
> So in conclusion, two booleans seems sut-optimal than a counter. For
> example we can use different bits for the counter as preempt_count
> did. With counter(s), core and driver don't need any implicit/explicit
> synchronization.
> 
> Thanks
> 

We have a simple problem, we can solve it simply. reference counting
is tricky to get right and hard to debug, if we don't need it let us
not go there.



But in conclusion ;) if you don't like my suggestion do something else
but make the APIs make sense, at least do better than +5
on Rusty's interface design scale.

> 
> 
> 
> >
> >
> >
> >
> > > @@ -455,7 +461,7 @@ int register_virtio_device(struct virtio_device *dev)
> > >               goto out_ida_remove;
> > >
> > >       spin_lock_init(&dev->config_lock);
> > > -     dev->config_enabled = false;
> > > +     dev->config_enabled = 0;
> > >       dev->config_change_pending = false;
> > >
> > >       INIT_LIST_HEAD(&dev->vqs);
> > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > index 96fea920873b..4496f9ba5d82 100644
> > > --- a/include/linux/virtio.h
> > > +++ b/include/linux/virtio.h
> > > @@ -132,7 +132,7 @@ struct virtio_admin_cmd {
> > >  struct virtio_device {
> > >       int index;
> > >       bool failed;
> > > -     bool config_enabled;
> > > +     int config_enabled;
> > >       bool config_change_pending;
> > >       spinlock_t config_lock;
> > >       spinlock_t vqs_list_lock;
> > > --
> > > 2.31.1
> >

Re: [PATCH V2 3/3] virtio-net: synchronize operstate with admin state on up/down

[Michael S. Tsirkin]

On Tue, Jun 25, 2024 at 09:27:38AM +0800, Jason Wang wrote:
> On Mon, Jun 24, 2024 at 6:07 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Jun 24, 2024 at 10:45:23AM +0800, Jason Wang wrote:
> > > This patch synchronize operstate with admin state per RFC2863.
> > >
> > > This is done by trying to toggle the carrier upon open/close and
> > > synchronize with the config change work. This allows propagate status
> > > correctly to stacked devices like:
> > >
> > > ip link add link enp0s3 macvlan0 type macvlan
> > > ip link set link enp0s3 down
> > > ip link show
> > >
> > > Before this patch:
> > >
> > > 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
> > >     link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > ......
> > > 5: macvlan0@enp0s3: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> > >     link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > >
> > > After this patch:
> > >
> > > 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
> > >     link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > ...
> > > 5: macvlan0@enp0s3: <NO-CARRIER,BROADCAST,MULTICAST,UP,M-DOWN> mtu 1500 qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
> > >     link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > >
> > > Cc: Venkat Venkatsubra <venkat.x.venkatsubra@oracle.com>
> > > Cc: Gia-Khanh Nguyen <gia-khanh.nguyen@oracle.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  drivers/net/virtio_net.c | 72 +++++++++++++++++++++++-----------------
> > >  1 file changed, 42 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index b1f8b720733e..eff3ad3d6bcc 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -2468,6 +2468,25 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
> > >       return err;
> > >  }
> > >
> > > +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;
> > > +}
> > > +
> > >  static int virtnet_open(struct net_device *dev)
> > >  {
> > >       struct virtnet_info *vi = netdev_priv(dev);
> > > @@ -2486,6 +2505,22 @@ static int virtnet_open(struct net_device *dev)
> > >                       goto err_enable_qp;
> > >       }
> > >
> > > +     /* Assume link up if device can't report link status,
> > > +        otherwise get link status from config. */
> > > +     netif_carrier_off(dev);
> > > +     if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> > > +             virtio_config_enable(vi->vdev);
> > > +             /* We are not sure if config interrupt is disabled by
> > > +              * core or not, so we can't schedule config_work by
> > > +              * ourselves.
> > > +              */
> >
> > This comment confuses more than it explains.
> > You seem to be arguing about some alternative design
> > you had in mind, but readers don't have it in mind.
> >
> >
> > Please just explain what this does and why.
> > For what: something like "Trigger re-read of config - same
> > as we'd do if config changed".
> >
> > Now, please do what you don't do here: explain the why:
> >
> >
> > why do we want all these VM
> > exits on each open/close as opposed to once on probe and later on
> > config changed interrupt.
> 
> Fine, the main reason is that a config interrupt might be pending
> during ifdown and core may disable configure interrupt due to several
> reasons.
> 
> Thanks

If the config changes exactly as command is executing?
Then we'll get an interrupt later and update.
You can't always win this race, even if you read it can
change right after.


> 
> >
> >
> > > +             virtio_config_changed(vi->vdev);
> > > +     } else {
> > > +             vi->status = VIRTIO_NET_S_LINK_UP;
> > > +             virtnet_update_settings(vi);
> > > +             netif_carrier_on(dev);
> > > +     }
> > > +
> > >       return 0;
> > >
> > >  err_enable_qp:
> > > @@ -2928,12 +2963,19 @@ 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 */
> > > +     virtio_config_disable(vi->vdev);
> > > +     /* Make sure status updating is cancelled */
> > > +     cancel_work_sync(&vi->config_work);
> > >
> > >       for (i = 0; i < vi->max_queue_pairs; i++) {
> > >               virtnet_disable_queue_pair(vi, i);
> > >               cancel_work_sync(&vi->rq[i].dim.work);
> > >       }
> > >
> > > +     vi->status &= ~VIRTIO_NET_S_LINK_UP;
> > > +     netif_carrier_off(dev);
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -4632,25 +4674,6 @@ static void virtnet_init_settings(struct net_device *dev)
> > >       vi->duplex = DUPLEX_UNKNOWN;
> > >  }
> > >
> > > -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;
> > > -}
> > > -
> > >  static u32 virtnet_get_rxfh_key_size(struct net_device *dev)
> > >  {
> > >       return ((struct virtnet_info *)netdev_priv(dev))->rss_key_size;
> > > @@ -5958,17 +5981,6 @@ static int virtnet_probe(struct virtio_device *vdev)
> > >               goto free_unregister_netdev;
> > >       }
> > >
> > > -     /* Assume link up if device can't report link status,
> > > -        otherwise get link status from config. */
> > > -     netif_carrier_off(dev);
> > > -     if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> > > -             schedule_work(&vi->config_work);
> > > -     } else {
> > > -             vi->status = VIRTIO_NET_S_LINK_UP;
> > > -             virtnet_update_settings(vi);
> > > -             netif_carrier_on(dev);
> > > -     }
> > > -
> > >       for (i = 0; i < ARRAY_SIZE(guest_offloads); i++)
> > >               if (virtio_has_feature(vi->vdev, guest_offloads[i]))
> > >                       set_bit(guest_offloads[i], &vi->guest_offloads);
> > > --
> > > 2.31.1
> >

Re: [PATCH V2 3/3] virtio-net: synchronize operstate with admin state on up/down

[Jason Wang]

On Tue, Jun 25, 2024 at 3:16 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Jun 25, 2024 at 09:27:38AM +0800, Jason Wang wrote:
> > On Mon, Jun 24, 2024 at 6:07 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Jun 24, 2024 at 10:45:23AM +0800, Jason Wang wrote:
> > > > This patch synchronize operstate with admin state per RFC2863.
> > > >
> > > > This is done by trying to toggle the carrier upon open/close and
> > > > synchronize with the config change work. This allows propagate status
> > > > correctly to stacked devices like:
> > > >
> > > > ip link add link enp0s3 macvlan0 type macvlan
> > > > ip link set link enp0s3 down
> > > > ip link show
> > > >
> > > > Before this patch:
> > > >
> > > > 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
> > > >     link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > > ......
> > > > 5: macvlan0@enp0s3: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> > > >     link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > > >
> > > > After this patch:
> > > >
> > > > 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
> > > >     link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > > ...
> > > > 5: macvlan0@enp0s3: <NO-CARRIER,BROADCAST,MULTICAST,UP,M-DOWN> mtu 1500 qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
> > > >     link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > > >
> > > > Cc: Venkat Venkatsubra <venkat.x.venkatsubra@oracle.com>
> > > > Cc: Gia-Khanh Nguyen <gia-khanh.nguyen@oracle.com>
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > >  drivers/net/virtio_net.c | 72 +++++++++++++++++++++++-----------------
> > > >  1 file changed, 42 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index b1f8b720733e..eff3ad3d6bcc 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -2468,6 +2468,25 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
> > > >       return err;
> > > >  }
> > > >
> > > > +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;
> > > > +}
> > > > +
> > > >  static int virtnet_open(struct net_device *dev)
> > > >  {
> > > >       struct virtnet_info *vi = netdev_priv(dev);
> > > > @@ -2486,6 +2505,22 @@ static int virtnet_open(struct net_device *dev)
> > > >                       goto err_enable_qp;
> > > >       }
> > > >
> > > > +     /* Assume link up if device can't report link status,
> > > > +        otherwise get link status from config. */
> > > > +     netif_carrier_off(dev);
> > > > +     if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> > > > +             virtio_config_enable(vi->vdev);
> > > > +             /* We are not sure if config interrupt is disabled by
> > > > +              * core or not, so we can't schedule config_work by
> > > > +              * ourselves.
> > > > +              */
> > >
> > > This comment confuses more than it explains.
> > > You seem to be arguing about some alternative design
> > > you had in mind, but readers don't have it in mind.
> > >
> > >
> > > Please just explain what this does and why.
> > > For what: something like "Trigger re-read of config - same
> > > as we'd do if config changed".
> > >
> > > Now, please do what you don't do here: explain the why:
> > >
> > >
> > > why do we want all these VM
> > > exits on each open/close as opposed to once on probe and later on
> > > config changed interrupt.
> >
> > Fine, the main reason is that a config interrupt might be pending
> > during ifdown and core may disable configure interrupt due to several
> > reasons.
> >
> > Thanks
>
> If the config changes exactly as command is executing?
> Then we'll get an interrupt later and update.

Workqueue is used to serialize those so we won't lose any change.

Currently the interrupt is only used to:

1) update link status, so according to rfc2863, no need to do that
when the interface is admin down
2) announce the link, it's also meaningless to announce the link when
the interface is down.

Or anything I miss here?

> You can't always win this race, even if you read it can
> change right after.

Thanks

>
>
> >
> > >
> > >
> > > > +             virtio_config_changed(vi->vdev);
> > > > +     } else {
> > > > +             vi->status = VIRTIO_NET_S_LINK_UP;
> > > > +             virtnet_update_settings(vi);
> > > > +             netif_carrier_on(dev);
> > > > +     }
> > > > +
> > > >       return 0;
> > > >
> > > >  err_enable_qp:
> > > > @@ -2928,12 +2963,19 @@ 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 */
> > > > +     virtio_config_disable(vi->vdev);
> > > > +     /* Make sure status updating is cancelled */
> > > > +     cancel_work_sync(&vi->config_work);
> > > >
> > > >       for (i = 0; i < vi->max_queue_pairs; i++) {
> > > >               virtnet_disable_queue_pair(vi, i);
> > > >               cancel_work_sync(&vi->rq[i].dim.work);
> > > >       }
> > > >
> > > > +     vi->status &= ~VIRTIO_NET_S_LINK_UP;
> > > > +     netif_carrier_off(dev);
> > > > +
> > > >       return 0;
> > > >  }
> > > >
> > > > @@ -4632,25 +4674,6 @@ static void virtnet_init_settings(struct net_device *dev)
> > > >       vi->duplex = DUPLEX_UNKNOWN;
> > > >  }
> > > >
> > > > -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;
> > > > -}
> > > > -
> > > >  static u32 virtnet_get_rxfh_key_size(struct net_device *dev)
> > > >  {
> > > >       return ((struct virtnet_info *)netdev_priv(dev))->rss_key_size;
> > > > @@ -5958,17 +5981,6 @@ static int virtnet_probe(struct virtio_device *vdev)
> > > >               goto free_unregister_netdev;
> > > >       }
> > > >
> > > > -     /* Assume link up if device can't report link status,
> > > > -        otherwise get link status from config. */
> > > > -     netif_carrier_off(dev);
> > > > -     if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> > > > -             schedule_work(&vi->config_work);
> > > > -     } else {
> > > > -             vi->status = VIRTIO_NET_S_LINK_UP;
> > > > -             virtnet_update_settings(vi);
> > > > -             netif_carrier_on(dev);
> > > > -     }
> > > > -
> > > >       for (i = 0; i < ARRAY_SIZE(guest_offloads); i++)
> > > >               if (virtio_has_feature(vi->vdev, guest_offloads[i]))
> > > >                       set_bit(guest_offloads[i], &vi->guest_offloads);
> > > > --
> > > > 2.31.1
> > >
>

Re: [PATCH V2 1/3] virtio: allow nested disabling of the configure interrupt

[Jason Wang]

On Tue, Jun 25, 2024 at 3:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Jun 25, 2024 at 09:27:04AM +0800, Jason Wang wrote:
> > On Mon, Jun 24, 2024 at 5:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Jun 24, 2024 at 10:45:21AM +0800, Jason Wang wrote:
> > > > Somtime driver may want to enable or disable the config callback. This
> > > > requires a synchronization with the core. So this patch change the
> > > > config_enabled to be a integer counter. This allows the toggling of
> > > > the config_enable to be synchronized between the virtio core and the
> > > > virtio driver.
> > > >
> > > > The counter is not allowed to be increased greater than one, this
> > > > simplifies the logic where the interrupt could be disabled immediately
> > > > without extra synchronization between driver and core.
> > > >
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > >  drivers/virtio/virtio.c | 20 +++++++++++++-------
> > > >  include/linux/virtio.h  |  2 +-
> > > >  2 files changed, 14 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > index b968b2aa5f4d..d3aa74b8ae5d 100644
> > > > --- a/drivers/virtio/virtio.c
> > > > +++ b/drivers/virtio/virtio.c
> > > > @@ -127,7 +127,7 @@ static void __virtio_config_changed(struct virtio_device *dev)
> > > >  {
> > > >       struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> > > >
> > > > -     if (!dev->config_enabled)
> > > > +     if (dev->config_enabled < 1)
> > > >               dev->config_change_pending = true;
> > > >       else if (drv && drv->config_changed)
> > > >               drv->config_changed(dev);
> > > > @@ -146,17 +146,23 @@ EXPORT_SYMBOL_GPL(virtio_config_changed);
> > > >  static void virtio_config_disable(struct virtio_device *dev)
> > > >  {
> > > >       spin_lock_irq(&dev->config_lock);
> > > > -     dev->config_enabled = false;
> > > > +     --dev->config_enabled;
> > > >       spin_unlock_irq(&dev->config_lock);
> > > >  }
> > > >
> > > >  static void virtio_config_enable(struct virtio_device *dev)
> > > >  {
> > > >       spin_lock_irq(&dev->config_lock);
> > > > -     dev->config_enabled = true;
> > > > -     if (dev->config_change_pending)
> > > > -             __virtio_config_changed(dev);
> > > > -     dev->config_change_pending = false;
> > > > +
> > > > +     if (dev->config_enabled < 1) {
> > > > +             ++dev->config_enabled;
> > > > +             if (dev->config_enabled == 1 &&
> > > > +                 dev->config_change_pending) {
> > > > +                     __virtio_config_changed(dev);
> > > > +                     dev->config_change_pending = false;
> > > > +             }
> > > > +     }
> > > > +
> > > >       spin_unlock_irq(&dev->config_lock);
> > > >  }
> > > >
> > >
> > > So every disable decrements the counter. Enable only increments it up to 1.
> > > You seem to be making some very specific assumptions
> > > about how this API will be used. Any misuse will lead to under/overflow
> > > eventually ...
> > >
> >
> > Well, a counter gives us more information than a boolean. With
> > boolean, misuse is even harder to be noticed.
>
> With boolean we can prevent misuse easily because previous state
> is known exactly. E.g.:
>
> static void virtio_config_driver_disable(struct virtio_device *dev)
> {
>         BUG_ON(dev->config_driver_disabled);
>         dev->config_driver_disabled = true;
> }
>
>
>
> static void virtio_config_driver_enable(struct virtio_device *dev)
> {
>         BUG_ON(!dev->config_driver_disabled);
>         dev->config_driver_disabled = false;
> }
>
>
> Does not work with integer you simply have no idea what the value
> should be at point of call.

Yes but I meant if we want the config could be disabled by different
parties (core, driver and others)

>
>
> > >
> > >
> > > My suggestion would be to
> > > 1. rename config_enabled to config_core_enabled
> > > 2. rename virtio_config_enable/disable to virtio_config_core_enable/disable
> > > 3. add bool config_driver_disabled and make virtio_config_enable/disable
> > >    switch that.
> > > 4. Change logic from dev->config_enabled to
> > >    dev->config_core_enabled && !dev->config_driver_disabled
> >
> > If we make config_driver_disabled by default true,
>
> No, we make it false by default.
>
> > we need someone to
> > enable it explicitly. If it's core, it breaks the semantic that it is
> > under the control of the driver (or needs to synchronize with the
> > driver). If it's a driver, each driver needs to enable it at some time
> > which can be easily forgotten. And if we end up with workarounds like:
> >
> >         /* If probe didn't do it, mark device DRIVER_OK ourselves. */
> >         if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
> >                 virtio_device_ready(dev);
> >
> > It's another break of the semantics. And actually the above is also racy.
> >
> > It seems the only choice is to make config_driver_disabled by default
> > false. But the driver needs to be aware of this and take extra care
> > when calling virtio_device_ready() which is also tricky.
>
>
> No, false by default simply means no change to semantics.

No change to current semantics, probably. But we need to document

1) driver config is enabled by default
2) no nested enabling and disabling

If you think they are all fine, I can go with that way.

>
>
> >
> > So in conclusion, two booleans seems sut-optimal than a counter. For
> > example we can use different bits for the counter as preempt_count
> > did. With counter(s), core and driver don't need any implicit/explicit
> > synchronization.
> >
> > Thanks
> >
>
> We have a simple problem, we can solve it simply. reference counting
> is tricky to get right and hard to debug, if we don't need it let us
> not go there.

I fully agree, and that's why I limit the change to virtio-net driver
in the first version.

>
>
>
> But in conclusion ;) if you don't like my suggestion do something else
> but make the APIs make sense,

I don't say I don't like it:)

Limiting it to virtio-net seems to be the most easy way. And if we
want to do it in the core, I just want to make nesting to be supported
which might not be necessary now.

> at least do better than +5
> on Rusty's interface design scale.
>
> >

Thanks


> >
> >
> > >
> > >
> > >
> > >
> > > > @@ -455,7 +461,7 @@ int register_virtio_device(struct virtio_device *dev)
> > > >               goto out_ida_remove;
> > > >
> > > >       spin_lock_init(&dev->config_lock);
> > > > -     dev->config_enabled = false;
> > > > +     dev->config_enabled = 0;
> > > >       dev->config_change_pending = false;
> > > >
> > > >       INIT_LIST_HEAD(&dev->vqs);
> > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > > index 96fea920873b..4496f9ba5d82 100644
> > > > --- a/include/linux/virtio.h
> > > > +++ b/include/linux/virtio.h
> > > > @@ -132,7 +132,7 @@ struct virtio_admin_cmd {
> > > >  struct virtio_device {
> > > >       int index;
> > > >       bool failed;
> > > > -     bool config_enabled;
> > > > +     int config_enabled;
> > > >       bool config_change_pending;
> > > >       spinlock_t config_lock;
> > > >       spinlock_t vqs_list_lock;
> > > > --
> > > > 2.31.1
> > >
>

Re: [PATCH V2 3/3] virtio-net: synchronize operstate with admin state on up/down

[Michael S. Tsirkin]

On Tue, Jun 25, 2024 at 03:46:44PM +0800, Jason Wang wrote:
> Workqueue is used to serialize those so we won't lose any change.

So we don't need to re-read then?

Re: [PATCH V2 1/3] virtio: allow nested disabling of the configure interrupt

[Michael S. Tsirkin]

On Tue, Jun 25, 2024 at 03:50:30PM +0800, Jason Wang wrote:
> On Tue, Jun 25, 2024 at 3:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Jun 25, 2024 at 09:27:04AM +0800, Jason Wang wrote:
> > > On Mon, Jun 24, 2024 at 5:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Mon, Jun 24, 2024 at 10:45:21AM +0800, Jason Wang wrote:
> > > > > Somtime driver may want to enable or disable the config callback. This
> > > > > requires a synchronization with the core. So this patch change the
> > > > > config_enabled to be a integer counter. This allows the toggling of
> > > > > the config_enable to be synchronized between the virtio core and the
> > > > > virtio driver.
> > > > >
> > > > > The counter is not allowed to be increased greater than one, this
> > > > > simplifies the logic where the interrupt could be disabled immediately
> > > > > without extra synchronization between driver and core.
> > > > >
> > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > ---
> > > > >  drivers/virtio/virtio.c | 20 +++++++++++++-------
> > > > >  include/linux/virtio.h  |  2 +-
> > > > >  2 files changed, 14 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > index b968b2aa5f4d..d3aa74b8ae5d 100644
> > > > > --- a/drivers/virtio/virtio.c
> > > > > +++ b/drivers/virtio/virtio.c
> > > > > @@ -127,7 +127,7 @@ static void __virtio_config_changed(struct virtio_device *dev)
> > > > >  {
> > > > >       struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> > > > >
> > > > > -     if (!dev->config_enabled)
> > > > > +     if (dev->config_enabled < 1)
> > > > >               dev->config_change_pending = true;
> > > > >       else if (drv && drv->config_changed)
> > > > >               drv->config_changed(dev);
> > > > > @@ -146,17 +146,23 @@ EXPORT_SYMBOL_GPL(virtio_config_changed);
> > > > >  static void virtio_config_disable(struct virtio_device *dev)
> > > > >  {
> > > > >       spin_lock_irq(&dev->config_lock);
> > > > > -     dev->config_enabled = false;
> > > > > +     --dev->config_enabled;
> > > > >       spin_unlock_irq(&dev->config_lock);
> > > > >  }
> > > > >
> > > > >  static void virtio_config_enable(struct virtio_device *dev)
> > > > >  {
> > > > >       spin_lock_irq(&dev->config_lock);
> > > > > -     dev->config_enabled = true;
> > > > > -     if (dev->config_change_pending)
> > > > > -             __virtio_config_changed(dev);
> > > > > -     dev->config_change_pending = false;
> > > > > +
> > > > > +     if (dev->config_enabled < 1) {
> > > > > +             ++dev->config_enabled;
> > > > > +             if (dev->config_enabled == 1 &&
> > > > > +                 dev->config_change_pending) {
> > > > > +                     __virtio_config_changed(dev);
> > > > > +                     dev->config_change_pending = false;
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > >       spin_unlock_irq(&dev->config_lock);
> > > > >  }
> > > > >
> > > >
> > > > So every disable decrements the counter. Enable only increments it up to 1.
> > > > You seem to be making some very specific assumptions
> > > > about how this API will be used. Any misuse will lead to under/overflow
> > > > eventually ...
> > > >
> > >
> > > Well, a counter gives us more information than a boolean. With
> > > boolean, misuse is even harder to be noticed.
> >
> > With boolean we can prevent misuse easily because previous state
> > is known exactly. E.g.:
> >
> > static void virtio_config_driver_disable(struct virtio_device *dev)
> > {
> >         BUG_ON(dev->config_driver_disabled);
> >         dev->config_driver_disabled = true;
> > }
> >
> >
> >
> > static void virtio_config_driver_enable(struct virtio_device *dev)
> > {
> >         BUG_ON(!dev->config_driver_disabled);
> >         dev->config_driver_disabled = false;
> > }
> >
> >
> > Does not work with integer you simply have no idea what the value
> > should be at point of call.
> 
> Yes but I meant if we want the config could be disabled by different
> parties (core, driver and others)

For now, we don't have others ;)

> >
> >
> > > >
> > > >
> > > > My suggestion would be to
> > > > 1. rename config_enabled to config_core_enabled
> > > > 2. rename virtio_config_enable/disable to virtio_config_core_enable/disable
> > > > 3. add bool config_driver_disabled and make virtio_config_enable/disable
> > > >    switch that.
> > > > 4. Change logic from dev->config_enabled to
> > > >    dev->config_core_enabled && !dev->config_driver_disabled
> > >
> > > If we make config_driver_disabled by default true,
> >
> > No, we make it false by default.
> >
> > > we need someone to
> > > enable it explicitly. If it's core, it breaks the semantic that it is
> > > under the control of the driver (or needs to synchronize with the
> > > driver). If it's a driver, each driver needs to enable it at some time
> > > which can be easily forgotten. And if we end up with workarounds like:
> > >
> > >         /* If probe didn't do it, mark device DRIVER_OK ourselves. */
> > >         if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > >                 virtio_device_ready(dev);
> > >
> > > It's another break of the semantics. And actually the above is also racy.
> > >
> > > It seems the only choice is to make config_driver_disabled by default
> > > false. But the driver needs to be aware of this and take extra care
> > > when calling virtio_device_ready() which is also tricky.
> >
> >
> > No, false by default simply means no change to semantics.
> 
> No change to current semantics, probably. But we need to document
> 
> 1) driver config is enabled by default
> 2) no nested enabling and disabling
> 
> If you think they are all fine, I can go with that way.

yes, a good idea to document this.


> >
> >
> > >
> > > So in conclusion, two booleans seems sut-optimal than a counter. For
> > > example we can use different bits for the counter as preempt_count
> > > did. With counter(s), core and driver don't need any implicit/explicit
> > > synchronization.
> > >
> > > Thanks
> > >
> >
> > We have a simple problem, we can solve it simply. reference counting
> > is tricky to get right and hard to debug, if we don't need it let us
> > not go there.
> 
> I fully agree, and that's why I limit the change to virtio-net driver
> in the first version.

I got that. I didn't like the code duplication though.

> >
> >
> >
> > But in conclusion ;) if you don't like my suggestion do something else
> > but make the APIs make sense,
> 
> I don't say I don't like it:)
> 
> Limiting it to virtio-net seems to be the most easy way. And if we
> want to do it in the core, I just want to make nesting to be supported
> which might not be necessary now.

I feel limiting it to a single driver strikes the right balance ATM.

> 
> > at least do better than +5
> > on Rusty's interface design scale.
> >
> > >
> 
> Thanks
> 
> 
> > >
> > >
> > > >
> > > >
> > > >
> > > >
> > > > > @@ -455,7 +461,7 @@ int register_virtio_device(struct virtio_device *dev)
> > > > >               goto out_ida_remove;
> > > > >
> > > > >       spin_lock_init(&dev->config_lock);
> > > > > -     dev->config_enabled = false;
> > > > > +     dev->config_enabled = 0;
> > > > >       dev->config_change_pending = false;
> > > > >
> > > > >       INIT_LIST_HEAD(&dev->vqs);
> > > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > > > index 96fea920873b..4496f9ba5d82 100644
> > > > > --- a/include/linux/virtio.h
> > > > > +++ b/include/linux/virtio.h
> > > > > @@ -132,7 +132,7 @@ struct virtio_admin_cmd {
> > > > >  struct virtio_device {
> > > > >       int index;
> > > > >       bool failed;
> > > > > -     bool config_enabled;
> > > > > +     int config_enabled;
> > > > >       bool config_change_pending;
> > > > >       spinlock_t config_lock;
> > > > >       spinlock_t vqs_list_lock;
> > > > > --
> > > > > 2.31.1
> > > >
> >

Re: [PATCH V2 3/3] virtio-net: synchronize operstate with admin state on up/down

[Jason Wang]

On Tue, Jun 25, 2024 at 3:57 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Jun 25, 2024 at 03:46:44PM +0800, Jason Wang wrote:
> > Workqueue is used to serialize those so we won't lose any change.
>
> So we don't need to re-read then?
>

We might have to re-read but I don't get why it is a problem for us.

Thanks

Re: [PATCH V2 1/3] virtio: allow nested disabling of the configure interrupt

[Jason Wang]

On Tue, Jun 25, 2024 at 4:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Jun 25, 2024 at 03:50:30PM +0800, Jason Wang wrote:
> > On Tue, Jun 25, 2024 at 3:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Jun 25, 2024 at 09:27:04AM +0800, Jason Wang wrote:
> > > > On Mon, Jun 24, 2024 at 5:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Mon, Jun 24, 2024 at 10:45:21AM +0800, Jason Wang wrote:
> > > > > > Somtime driver may want to enable or disable the config callback. This
> > > > > > requires a synchronization with the core. So this patch change the
> > > > > > config_enabled to be a integer counter. This allows the toggling of
> > > > > > the config_enable to be synchronized between the virtio core and the
> > > > > > virtio driver.
> > > > > >
> > > > > > The counter is not allowed to be increased greater than one, this
> > > > > > simplifies the logic where the interrupt could be disabled immediately
> > > > > > without extra synchronization between driver and core.
> > > > > >
> > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > ---
> > > > > >  drivers/virtio/virtio.c | 20 +++++++++++++-------
> > > > > >  include/linux/virtio.h  |  2 +-
> > > > > >  2 files changed, 14 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > > > index b968b2aa5f4d..d3aa74b8ae5d 100644
> > > > > > --- a/drivers/virtio/virtio.c
> > > > > > +++ b/drivers/virtio/virtio.c
> > > > > > @@ -127,7 +127,7 @@ static void __virtio_config_changed(struct virtio_device *dev)
> > > > > >  {
> > > > > >       struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> > > > > >
> > > > > > -     if (!dev->config_enabled)
> > > > > > +     if (dev->config_enabled < 1)
> > > > > >               dev->config_change_pending = true;
> > > > > >       else if (drv && drv->config_changed)
> > > > > >               drv->config_changed(dev);
> > > > > > @@ -146,17 +146,23 @@ EXPORT_SYMBOL_GPL(virtio_config_changed);
> > > > > >  static void virtio_config_disable(struct virtio_device *dev)
> > > > > >  {
> > > > > >       spin_lock_irq(&dev->config_lock);
> > > > > > -     dev->config_enabled = false;
> > > > > > +     --dev->config_enabled;
> > > > > >       spin_unlock_irq(&dev->config_lock);
> > > > > >  }
> > > > > >
> > > > > >  static void virtio_config_enable(struct virtio_device *dev)
> > > > > >  {
> > > > > >       spin_lock_irq(&dev->config_lock);
> > > > > > -     dev->config_enabled = true;
> > > > > > -     if (dev->config_change_pending)
> > > > > > -             __virtio_config_changed(dev);
> > > > > > -     dev->config_change_pending = false;
> > > > > > +
> > > > > > +     if (dev->config_enabled < 1) {
> > > > > > +             ++dev->config_enabled;
> > > > > > +             if (dev->config_enabled == 1 &&
> > > > > > +                 dev->config_change_pending) {
> > > > > > +                     __virtio_config_changed(dev);
> > > > > > +                     dev->config_change_pending = false;
> > > > > > +             }
> > > > > > +     }
> > > > > > +
> > > > > >       spin_unlock_irq(&dev->config_lock);
> > > > > >  }
> > > > > >
> > > > >
> > > > > So every disable decrements the counter. Enable only increments it up to 1.
> > > > > You seem to be making some very specific assumptions
> > > > > about how this API will be used. Any misuse will lead to under/overflow
> > > > > eventually ...
> > > > >
> > > >
> > > > Well, a counter gives us more information than a boolean. With
> > > > boolean, misuse is even harder to be noticed.
> > >
> > > With boolean we can prevent misuse easily because previous state
> > > is known exactly. E.g.:
> > >
> > > static void virtio_config_driver_disable(struct virtio_device *dev)
> > > {
> > >         BUG_ON(dev->config_driver_disabled);
> > >         dev->config_driver_disabled = true;
> > > }
> > >
> > >
> > >
> > > static void virtio_config_driver_enable(struct virtio_device *dev)
> > > {
> > >         BUG_ON(!dev->config_driver_disabled);
> > >         dev->config_driver_disabled = false;
> > > }
> > >
> > >
> > > Does not work with integer you simply have no idea what the value
> > > should be at point of call.
> >
> > Yes but I meant if we want the config could be disabled by different
> > parties (core, driver and others)
>
> For now, we don't have others ;)
>
> > >
> > >
> > > > >
> > > > >
> > > > > My suggestion would be to
> > > > > 1. rename config_enabled to config_core_enabled
> > > > > 2. rename virtio_config_enable/disable to virtio_config_core_enable/disable
> > > > > 3. add bool config_driver_disabled and make virtio_config_enable/disable
> > > > >    switch that.
> > > > > 4. Change logic from dev->config_enabled to
> > > > >    dev->config_core_enabled && !dev->config_driver_disabled
> > > >
> > > > If we make config_driver_disabled by default true,
> > >
> > > No, we make it false by default.
> > >
> > > > we need someone to
> > > > enable it explicitly. If it's core, it breaks the semantic that it is
> > > > under the control of the driver (or needs to synchronize with the
> > > > driver). If it's a driver, each driver needs to enable it at some time
> > > > which can be easily forgotten. And if we end up with workarounds like:
> > > >
> > > >         /* If probe didn't do it, mark device DRIVER_OK ourselves. */
> > > >         if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > > >                 virtio_device_ready(dev);
> > > >
> > > > It's another break of the semantics. And actually the above is also racy.
> > > >
> > > > It seems the only choice is to make config_driver_disabled by default
> > > > false. But the driver needs to be aware of this and take extra care
> > > > when calling virtio_device_ready() which is also tricky.
> > >
> > >
> > > No, false by default simply means no change to semantics.
> >
> > No change to current semantics, probably. But we need to document
> >
> > 1) driver config is enabled by default
> > 2) no nested enabling and disabling
> >
> > If you think they are all fine, I can go with that way.
>
> yes, a good idea to document this.
>
>

[...]

> > >
> > > We have a simple problem, we can solve it simply. reference counting
> > > is tricky to get right and hard to debug, if we don't need it let us
> > > not go there.
> >
> > I fully agree, and that's why I limit the change to virtio-net driver
> > in the first version.
>
> I got that. I didn't like the code duplication though.
>
> > >
> > >
> > >
> > > But in conclusion ;) if you don't like my suggestion do something else
> > > but make the APIs make sense,
> >
> > I don't say I don't like it:)
> >
> > Limiting it to virtio-net seems to be the most easy way. And if we
> > want to do it in the core, I just want to make nesting to be supported
> > which might not be necessary now.
>
> I feel limiting it to a single driver strikes the right balance ATM.

Just to make sure I understand here, should we go back to v1 or go
with the config_driver_disabled?

Thanks

>
> >
> > > at least do better than +5
> > > on Rusty's interface design scale.
> > >
> > > >
> >
> > Thanks
> >
> >
> > > >
> > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > > @@ -455,7 +461,7 @@ int register_virtio_device(struct virtio_device *dev)
> > > > > >               goto out_ida_remove;
> > > > > >
> > > > > >       spin_lock_init(&dev->config_lock);
> > > > > > -     dev->config_enabled = false;
> > > > > > +     dev->config_enabled = 0;
> > > > > >       dev->config_change_pending = false;
> > > > > >
> > > > > >       INIT_LIST_HEAD(&dev->vqs);
> > > > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > > > > index 96fea920873b..4496f9ba5d82 100644
> > > > > > --- a/include/linux/virtio.h
> > > > > > +++ b/include/linux/virtio.h
> > > > > > @@ -132,7 +132,7 @@ struct virtio_admin_cmd {
> > > > > >  struct virtio_device {
> > > > > >       int index;
> > > > > >       bool failed;
> > > > > > -     bool config_enabled;
> > > > > > +     int config_enabled;
> > > > > >       bool config_change_pending;
> > > > > >       spinlock_t config_lock;
> > > > > >       spinlock_t vqs_list_lock;
> > > > > > --
> > > > > > 2.31.1
> > > > >
> > >
>

Re: [PATCH V2 1/3] virtio: allow nested disabling of the configure interrupt

[Michael S. Tsirkin]

On Tue, Jun 25, 2024 at 04:18:00PM +0800, Jason Wang wrote:
> > > >
> > > >
> > > >
> > > > But in conclusion ;) if you don't like my suggestion do something else
> > > > but make the APIs make sense,
> > >
> > > I don't say I don't like it:)
> > >
> > > Limiting it to virtio-net seems to be the most easy way. And if we
> > > want to do it in the core, I just want to make nesting to be supported
> > > which might not be necessary now.
> >
> > I feel limiting it to a single driver strikes the right balance ATM.
> 
> Just to make sure I understand here, should we go back to v1 or go
> with the config_driver_disabled?
> 
> Thanks


I still like config_driver_disabled.


> >
> > >
> > > > at least do better than +5
> > > > on Rusty's interface design scale.
> > > >
> > > > >
> > >
> > > Thanks
> > >
> > >
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > > @@ -455,7 +461,7 @@ int register_virtio_device(struct virtio_device *dev)
> > > > > > >               goto out_ida_remove;
> > > > > > >
> > > > > > >       spin_lock_init(&dev->config_lock);
> > > > > > > -     dev->config_enabled = false;
> > > > > > > +     dev->config_enabled = 0;
> > > > > > >       dev->config_change_pending = false;
> > > > > > >
> > > > > > >       INIT_LIST_HEAD(&dev->vqs);
> > > > > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > > > > > index 96fea920873b..4496f9ba5d82 100644
> > > > > > > --- a/include/linux/virtio.h
> > > > > > > +++ b/include/linux/virtio.h
> > > > > > > @@ -132,7 +132,7 @@ struct virtio_admin_cmd {
> > > > > > >  struct virtio_device {
> > > > > > >       int index;
> > > > > > >       bool failed;
> > > > > > > -     bool config_enabled;
> > > > > > > +     int config_enabled;
> > > > > > >       bool config_change_pending;
> > > > > > >       spinlock_t config_lock;
> > > > > > >       spinlock_t vqs_list_lock;
> > > > > > > --
> > > > > > > 2.31.1
> > > > > >
> > > >
> >

Re: [PATCH V2 3/3] virtio-net: synchronize operstate with admin state on up/down

[Michael S. Tsirkin]

On Tue, Jun 25, 2024 at 04:11:05PM +0800, Jason Wang wrote:
> On Tue, Jun 25, 2024 at 3:57 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Jun 25, 2024 at 03:46:44PM +0800, Jason Wang wrote:
> > > Workqueue is used to serialize those so we won't lose any change.
> >
> > So we don't need to re-read then?
> >
> 
> We might have to re-read but I don't get why it is a problem for us.
> 
> Thanks

I don't think each ethtool command should force a full config read,
is what I mean. Only do it if really needed.

-- 
MST

Re: [PATCH V2 3/3] virtio-net: synchronize operstate with admin state on up/down

[Jason Wang]

On Tue, Jun 25, 2024 at 4:32 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Jun 25, 2024 at 04:11:05PM +0800, Jason Wang wrote:
> > On Tue, Jun 25, 2024 at 3:57 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Jun 25, 2024 at 03:46:44PM +0800, Jason Wang wrote:
> > > > Workqueue is used to serialize those so we won't lose any change.
> > >
> > > So we don't need to re-read then?
> > >
> >
> > We might have to re-read but I don't get why it is a problem for us.
> >
> > Thanks
>
> I don't think each ethtool command should force a full config read,
> is what I mean. Only do it if really needed.

We don't, as we will check config_pending there.

Thanks

>
> --
> MST
>

Re: [PATCH V2 3/3] virtio-net: synchronize operstate with admin state on up/down

[Michael S. Tsirkin]

On Wed, Jun 26, 2024 at 09:58:32AM +0800, Jason Wang wrote:
> On Tue, Jun 25, 2024 at 4:32 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Jun 25, 2024 at 04:11:05PM +0800, Jason Wang wrote:
> > > On Tue, Jun 25, 2024 at 3:57 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, Jun 25, 2024 at 03:46:44PM +0800, Jason Wang wrote:
> > > > > Workqueue is used to serialize those so we won't lose any change.
> > > >
> > > > So we don't need to re-read then?
> > > >
> > >
> > > We might have to re-read but I don't get why it is a problem for us.
> > >
> > > Thanks
> >
> > I don't think each ethtool command should force a full config read,
> > is what I mean. Only do it if really needed.
> 
> We don't, as we will check config_pending there.
> 
> Thanks

And config_pending set from an interrupt? That's fine.
But it's not what this patch does, right?

> >
> > --
> > MST
> >