On Tue, Jun 25, 2024 at 3:11 PM Michael S. Tsirkin [off-list ref] wrote:

On Tue, Jun 25, 2024 at 09:27:04AM +0800, Jason Wang wrote:

quoted

On Mon, Jun 24, 2024 at 5:59 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Mon, Jun 24, 2024 at 10:45:21AM +0800, Jason Wang wrote:

quoted

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)

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

Thanks

quoted

quoted

quoted

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