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

[Jason Wang <jasowang@redhat.com>]

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