[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 4/7] device-core: use atomic_set on .realized property
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH v2 4/7] device-core: use atomic_set on .realized property |
Date: |
Wed, 27 May 2020 16:00:53 +0100 |
On Mon, May 11, 2020 at 07:09:48PM +0300, Maxim Levitsky wrote:
> Some code might race with placement of new devices on a bus.
> We currently first place a (unrealized) device on the bus
> and then realize it.
>
> As a workaround, users that scan the child device list, can
> check the realized property to see if it is safe to access such a device.
> Use an atomic write here too to aid with this.
>
> A separate discussion is what to do with devices that are unrealized:
> It looks like for this case we only call the hotplug handler's unplug
> callback and its up to it to unrealize the device.
> An atomic operation doesn't cause harm for this code path though.
>
> Signed-off-by: Maxim Levitsky <address@hidden>
> ---
> hw/core/qdev.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
Please add a comment to struct DeviceState saying the realized field
must be accessed with atomic_load_acquire() when used outside the QEMU
global mutex.
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 732789e2b7..d530c5922f 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -964,7 +964,20 @@ static void device_set_realized(Object *obj, bool value,
> Error **errp)
> }
> }
>
> + atomic_store_release(&dev->realized, value);
> +
> } else if (!value && dev->realized) {
> +
> + /*
> + * Change the value so that any concurrent users are aware
> + * that the device is going to be unrealized
> + *
> + * TODO: change .realized property to enum that states
> + * each phase of the device realization/unrealization
> + */
> +
> + atomic_store_release(&dev->realized, value);
I'm not sure if atomic_store_release() is strong enough in the true ->
false case:
Operations coming after ``atomic_store_release()`` can still be
reordered before it.
A reader may already seen changes made to unrealize the DeviceState even
though realized still appears to be true. A full write memory barrier
seems safer here.
signature.asc
Description: PGP signature
- [PATCH v2 0/7] RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, Maxim Levitsky, 2020/05/11
- [PATCH v2 1/7] scsi/scsi_bus: switch search direction in scsi_device_find, Maxim Levitsky, 2020/05/11
- [PATCH v2 2/7] Implement drain_call_rcu and use it in hmp_device_del, Maxim Levitsky, 2020/05/11
- [PATCH v2 3/7] device-core: use RCU for list of childs of a bus, Maxim Levitsky, 2020/05/11
- [PATCH v2 4/7] device-core: use atomic_set on .realized property, Maxim Levitsky, 2020/05/11
- Re: [PATCH v2 4/7] device-core: use atomic_set on .realized property,
Stefan Hajnoczi <=
- [PATCH v2 5/7] virtio-scsi: don't touch scsi devices that are not yet realized or about to be un-realized, Maxim Levitsky, 2020/05/11
- [PATCH v2 6/7] scsi: Add scsi_device_get, Maxim Levitsky, 2020/05/11
- [PATCH v2 7/7] virtio-scsi: use scsi_device_get, Maxim Levitsky, 2020/05/11
- Re: [PATCH v2 0/7] RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, no-reply, 2020/05/11
- Re: [PATCH v2 0/7] RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, no-reply, 2020/05/11