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

On Thu, Jun 06, 2024 at 08:22:13AM +0800, Jason Wang wrote:

quoted

On Fri, May 31, 2024 at 8:18 AM Jason Wang [off-list ref] wrote:

quoted

On Thu, May 30, 2024 at 9:09 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Thu, May 30, 2024 at 06:29:51PM +0800, Jason Wang wrote:

quoted

On Thu, May 30, 2024 at 2:10 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Thu, May 30, 2024 at 11:20:55AM +0800, Jason Wang wrote:

quoted

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 <redacted>
Cc: Gia-Khanh Nguyen <redacted>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <redacted>
---
Changes since V1:
- rebase
- add ack/review tags

quoted

---
 drivers/net/virtio_net.c | 94 +++++++++++++++++++++++++++-------------
 1 file changed, 63 insertions(+), 31 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4a802c0ea2cb..69e4ae353c51 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -433,6 +433,12 @@ struct virtnet_info {
      /* The lock to synchronize the access to refill_enabled */
      spinlock_t refill_lock;

+     /* Is config change enabled? */
+     bool config_change_enabled;
+
+     /* The lock to synchronize the access to config_change_enabled */
+     spinlock_t config_change_lock;
+
      /* Work struct for config space updates */
      struct work_struct config_work;

But we already have dev->config_lock and dev->config_enabled.

And it actually works better - instead of discarding config
change events it defers them until enabled.

Yes but then both virtio-net driver and virtio core can ask to enable
and disable and then we need some kind of synchronization which is
non-trivial.

Well for core it happens on bring up path before driver works
and later on tear down after it is gone.
So I do not think they ever do it at the same time.

For example, there could be a suspend/resume when the admin state is down.

quoted

quoted

And device enabling on the core is different from bringing the device
up in the networking subsystem. Here we just delay to deal with the
config change interrupt on ndo_open(). (E.g try to ack announce is
meaningless when the device is down).

Thanks

another thing is that it is better not to re-read all config
on link up if there was no config interrupt - less vm exits.

Yes, but it should not matter much as it's done in the ndo_open().

Michael, any more comments on this?

Please confirm if this patch is ok or not. If you prefer to reuse the
config_disable() I can change it from a boolean to a counter that
allows to be nested.

Thanks

I think doing this in core and re-using config_lock is a cleaner approach for sure, yes.

For remove we do not need stacking. But yes we need it for freeze.

I am not sure how will a counter work. Let's see the patch.


It might be easier, besides config_enabled, to add config_disabled,
document that  config_enabled is controlled by core,
config_disabled by driver.
And we'd have 2 APIs virtio_config_disable_core and
virtio_config_disable_driver.


But I leave that decision in your capable hands.

Let me try to switch to using a counter for config_enabled first.

Thanks

quoted

quoted

Thanks

quoted

--
MST