Re: [PATCH v5 05/18] qdev: unplug blocker for devices

From: Stefan Hajnoczi <stefanha@redhat.com>
To: Jag Raman <jag.raman@oracle.com>
Cc: "eduardo@habkost.net" <eduardo@habkost.net>,
	"Elena Ufimtseva" <elena.ufimtseva@oracle.com>,
	"John Johnson" <john.g.johnson@oracle.com>,
	"berrange@redhat.com" <berrange@redhat.com>,
	"bleal@redhat.com" <bleal@redhat.com>,
	"john.levon@nutanix.com" <john.levon@nutanix.com>,
	"mst@redhat.com" <mst@redhat.com>,
	"armbru@redhat.com" <armbru@redhat.com>,
	"quintela@redhat.com" <quintela@redhat.com>,
	qemu-devel <qemu-devel@nongnu.org>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Marc-André Lureau" <marcandre.lureau@gmail.com>,
	"thanos.makatos@nutanix.com" <thanos.makatos@nutanix.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"eblake@redhat.com" <eblake@redhat.com>,
	"dgilbert@redhat.com" <dgilbert@redhat.com>
Subject: Re: [PATCH v5 05/18] qdev: unplug blocker for devices
Date: Wed, 26 Jan 2022 09:32:40 +0000	[thread overview]
Message-ID: <YfEVOKTeMgaD/Nh1@stefanha-x1.localdomain> (raw)
In-Reply-To: <4B7C4ECE-8A8B-486B-A66A-983FDD2D6223@oracle.com>

[-- Attachment #1: Type: text/plain, Size: 3150 bytes --]

On Tue, Jan 25, 2022 at 02:43:33PM +0000, Jag Raman wrote:
> 
> 
> > On Jan 25, 2022, at 5:27 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > 
> > On Wed, Jan 19, 2022 at 04:41:54PM -0500, Jagannathan Raman wrote:
> >> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> >> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> >> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> >> ---
> >> include/hw/qdev-core.h |  5 +++++
> >> softmmu/qdev-monitor.c | 35 +++++++++++++++++++++++++++++++++++
> >> 2 files changed, 40 insertions(+)
> >> 
> >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> >> index eed2983072..67df5e0081 100644
> >> --- a/include/hw/qdev-core.h
> >> +++ b/include/hw/qdev-core.h
> >> @@ -193,6 +193,7 @@ struct DeviceState {
> >>     int instance_id_alias;
> >>     int alias_required_for_version;
> >>     ResettableState reset;
> >> +    GSList *unplug_blockers;
> >> };
> >> 
> >> struct DeviceListener {
> >> @@ -433,6 +434,10 @@ typedef bool (QDevPutBusFunc)(BusState *bus, Error **errp);
> >> bool qdev_set_bus_cbs(QDevGetBusFunc *get_bus, QDevPutBusFunc *put_bus,
> >>                       Error **errp);
> >> 
> >> +int qdev_add_unplug_blocker(DeviceState *dev, Error *reason, Error **errp);
> >> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason);
> >> +bool qdev_unplug_blocked(DeviceState *dev, Error **errp);
> >> +
> >> /**
> >>  * GpioPolarity: Polarity of a GPIO line
> >>  *
> >> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> >> index 7306074019..1a169f89a2 100644
> >> --- a/softmmu/qdev-monitor.c
> >> +++ b/softmmu/qdev-monitor.c
> >> @@ -978,10 +978,45 @@ void qmp_device_del(const char *id, Error **errp)
> >>             return;
> >>         }
> >> 
> >> +        if (qdev_unplug_blocked(dev, errp)) {
> >> +            return;
> >> +        }
> >> +
> >>         qdev_unplug(dev, errp);
> >>     }
> >> }
> >> 
> >> +int qdev_add_unplug_blocker(DeviceState *dev, Error *reason, Error **errp)
> >> +{
> >> +    ERRP_GUARD();
> >> +
> >> +    if (!migration_is_idle()) {
> >> +        error_setg(errp, "migration is in progress");
> >> +        return -EBUSY;
> >> +    }
> > 
> > Why can this function not be called during migration?
> 
> Since ‘unplug_blockers' is a member of the device, I thought it wouldn’t be correct to
> allow changes to the device's state during migration.
> 
> I did weigh the following reasons against adding this check:
>   - unplug_blockers is not migrated to the destination anyway, so it doesn’t matter if
>     it changes after migration starts

Yes.

>   - whichever code/object that needs to add the blocker could add it at the destination
>     if needed

Yes.

> However, unlike qmp_device_add(), qmp_object_add() doesn’t reject during
> migration. As such, an object could add a blocker for the device when migration is
> in progress.
> 
> Would you prefer to throw a warning, or fully remove this test?

Adding an unplug blocker during migration seems safe to me. I would
remove this test.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]