On Tue, Jun 25, 2024 at 4:04 PM Michael S. Tsirkin <mst@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > > > ---
> > > > > > 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
> > > > >
> > >
>
