diff mbox series

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

Message ID 5ad3f47f9ed507211fa8db5b8c36b9d32413bbbb.1642626515.git.jag.raman@oracle.com (mailing list archive)
State New, archived
Headers show
Series vfio-user server in QEMU | expand

Commit Message

Jag Raman Jan. 19, 2022, 9:41 p.m. UTC 
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(+)

Comments

Stefan Hajnoczi Jan. 25, 2022, 10:27 a.m. UTC | #1 
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?

> +
> +    dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason);
> +
> +    return 0;
> +}
> +
> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason)
> +{
> +    dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason);
> +}
> +
> +bool qdev_unplug_blocked(DeviceState *dev, Error **errp)
> +{
> +    ERRP_GUARD();
> +
> +    if (dev->unplug_blockers) {
> +        error_propagate(errp, error_copy(dev->unplug_blockers->data));
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>  void hmp_device_add(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> -- 
> 2.20.1
>
Jag Raman Jan. 25, 2022, 2:43 p.m. UTC | #2 
> 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
  - whichever code/object that needs to add the blocker could add it at the destination
    if needed

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?

--
Jag 

> 
>> +
>> +    dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason);
>> +
>> +    return 0;
>> +}
>> +
>> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason)
>> +{
>> +    dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason);
>> +}
>> +
>> +bool qdev_unplug_blocked(DeviceState *dev, Error **errp)
>> +{
>> +    ERRP_GUARD();
>> +
>> +    if (dev->unplug_blockers) {
>> +        error_propagate(errp, error_copy(dev->unplug_blockers->data));
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> void hmp_device_add(Monitor *mon, const QDict *qdict)
>> {
>>     Error *err = NULL;
>> -- 
>> 2.20.1
>>
Stefan Hajnoczi Jan. 26, 2022, 9:32 a.m. UTC | #3 
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
Jag Raman Jan. 26, 2022, 3:13 p.m. UTC | #4 
> On Jan 26, 2022, at 4:32 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> 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.

OK, will do.

Thank you!
--
Jag

> 
> Stefan
diff mbox series

Patch

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;
+    }
+
+    dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason);
+
+    return 0;
+}
+
+void qdev_del_unplug_blocker(DeviceState *dev, Error *reason)
+{
+    dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason);
+}
+
+bool qdev_unplug_blocked(DeviceState *dev, Error **errp)
+{
+    ERRP_GUARD();
+
+    if (dev->unplug_blockers) {
+        error_propagate(errp, error_copy(dev->unplug_blockers->data));
+        return true;
+    }
+
+    return false;
+}
+
 void hmp_device_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;