Message-ID: <20240625035746-mutt-send-email-mst@kernel.org>
Date: Tue, 25 Jun 2024 04:04:19 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: xuanzhuo@...ux.alibaba.com, eperezma@...hat.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	virtualization@...ts.linux.dev, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, venkat.x.venkatsubra@...cle.com,
	gia-khanh.nguyen@...cle.com
Subject: Re: [PATCH V2 1/3] virtio: allow nested disabling of the configure
 interrupt

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

Powered by blists - more mailing lists