| Message ID | 20200511160951.8733-3-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread | expand |
On Mon, May 11, 2020 at 07:09:46PM +0300, Maxim Levitsky wrote: > /* The operands of the minus operator must have the same type, > * which must be the one that we specify in the cast. > diff --git a/qdev-monitor.c b/qdev-monitor.c > index 56cee1483f..70877840a2 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) > return; > } > dev = qdev_device_add(opts, &local_err); > + drain_call_rcu(); Please include comments explaining what each drain waits for. Without comments we'll quickly lose track of why drain_call_rcu() calls are necessary (similar to documenting memory barrier or refcount inc/dec pairing). > diff --git a/util/rcu.c b/util/rcu.c > index 60a37f72c3..e8b1c4d6c5 100644 > --- a/util/rcu.c > +++ b/util/rcu.c > @@ -293,6 +293,39 @@ void call_rcu1(struct rcu_head *node, void (*func)(struct rcu_head *node)) > qemu_event_set(&rcu_call_ready_event); > } > > + > +struct rcu_drain { > + struct rcu_head rcu; > + QemuEvent drain_complete_event; > +}; > + > +static void drain_rcu_callback(struct rcu_head *node) > +{ > + struct rcu_drain *event = (struct rcu_drain *)node; > + qemu_event_set(&event->drain_complete_event); A comment would be nice explaining that callbacks are invoked in sequence so we're sure that all previously scheduled callbacks have completed when drain_rcu_callback() is invoked. > +} > + > +void drain_call_rcu(void) Please document that the main loop mutex is dropped if it's held. This will prevent surprises and allow callers to think about thread-safety across this call. Aside from the comment requests: Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Wed, 2020-05-27 at 14:11 +0100, Stefan Hajnoczi wrote: > On Mon, May 11, 2020 at 07:09:46PM +0300, Maxim Levitsky wrote: > > /* The operands of the minus operator must have the same type, > > * which must be the one that we specify in the cast. > > diff --git a/qdev-monitor.c b/qdev-monitor.c > > index 56cee1483f..70877840a2 100644 > > --- a/qdev-monitor.c > > +++ b/qdev-monitor.c > > @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) > > return; > > } > > dev = qdev_device_add(opts, &local_err); > > + drain_call_rcu(); > > Please include comments explaining what each drain waits for. Without > comments we'll quickly lose track of why drain_call_rcu() calls are > necessary (similar to documenting memory barrier or refcount inc/dec > pairing). > > > diff --git a/util/rcu.c b/util/rcu.c > > index 60a37f72c3..e8b1c4d6c5 100644 > > --- a/util/rcu.c > > +++ b/util/rcu.c > > @@ -293,6 +293,39 @@ void call_rcu1(struct rcu_head *node, void (*func)(struct rcu_head *node)) > > qemu_event_set(&rcu_call_ready_event); > > } > > > > + > > +struct rcu_drain { > > + struct rcu_head rcu; > > + QemuEvent drain_complete_event; > > +}; > > + > > +static void drain_rcu_callback(struct rcu_head *node) > > +{ > > + struct rcu_drain *event = (struct rcu_drain *)node; > > + qemu_event_set(&event->drain_complete_event); > > A comment would be nice explaining that callbacks are invoked in > sequence so we're sure that all previously scheduled callbacks have > completed when drain_rcu_callback() is invoked. > > > +} > > + > > +void drain_call_rcu(void) > > Please document that the main loop mutex is dropped if it's held. This > will prevent surprises and allow callers to think about thread-safety > across this call. Done. > > Aside from the comment requests: > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Best regards, Maxim levitsky
Maxim Levitsky <mlevitsk@redhat.com> writes: > This allows to preserve the semantics of hmp_device_del, > that the device is deleted immediatly which was changed by previos > patch that delayed this to RCU callback > > Suggested-by: Stefan Hajnoczi <stefanha@gmail.com> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > include/qemu/rcu.h | 1 + > qdev-monitor.c | 3 +++ > util/rcu.c | 33 +++++++++++++++++++++++++++++++++ > 3 files changed, 37 insertions(+) > > diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h > index 570aa603eb..0e375ebe13 100644 > --- a/include/qemu/rcu.h > +++ b/include/qemu/rcu.h > @@ -133,6 +133,7 @@ struct rcu_head { > }; > > extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func); > +extern void drain_call_rcu(void); > > /* The operands of the minus operator must have the same type, > * which must be the one that we specify in the cast. > diff --git a/qdev-monitor.c b/qdev-monitor.c > index 56cee1483f..70877840a2 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) > return; > } > dev = qdev_device_add(opts, &local_err); > + drain_call_rcu(); > + > if (!dev) { > error_propagate(errp, local_err); > qemu_opts_del(opts); > @@ -904,6 +906,7 @@ void qmp_device_del(const char *id, Error **errp) > } > > qdev_unplug(dev, errp); > + drain_call_rcu(); > } > } > Subject claims "in hmp_device_del", code has it in qmp_device_add() and qmp_device_del(). Please advise. [...]
On Thu, 2020-07-09 at 13:42 +0200, Markus Armbruster wrote: > Maxim Levitsky <mlevitsk@redhat.com> writes: > > > This allows to preserve the semantics of hmp_device_del, > > that the device is deleted immediatly which was changed by previos > > patch that delayed this to RCU callback > > > > Suggested-by: Stefan Hajnoczi <stefanha@gmail.com> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > include/qemu/rcu.h | 1 + > > qdev-monitor.c | 3 +++ > > util/rcu.c | 33 +++++++++++++++++++++++++++++++++ > > 3 files changed, 37 insertions(+) > > > > diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h > > index 570aa603eb..0e375ebe13 100644 > > --- a/include/qemu/rcu.h > > +++ b/include/qemu/rcu.h > > @@ -133,6 +133,7 @@ struct rcu_head { > > }; > > > > extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func); > > +extern void drain_call_rcu(void); > > > > /* The operands of the minus operator must have the same type, > > * which must be the one that we specify in the cast. > > diff --git a/qdev-monitor.c b/qdev-monitor.c > > index 56cee1483f..70877840a2 100644 > > --- a/qdev-monitor.c > > +++ b/qdev-monitor.c > > @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) > > return; > > } > > dev = qdev_device_add(opts, &local_err); > > + drain_call_rcu(); > > + > > if (!dev) { > > error_propagate(errp, local_err); > > qemu_opts_del(opts); > > @@ -904,6 +906,7 @@ void qmp_device_del(const char *id, Error **errp) > > } > > > > qdev_unplug(dev, errp); > > + drain_call_rcu(); > > } > > } > > > > Subject claims "in hmp_device_del", code has it in qmp_device_add() and > qmp_device_del(). Please advise. I added it in both, because addition of a device can fail and trigger removal, which can also be now delayed due to RCU. Since both device_add and device_del aren't used often, the overhead won't be a problem IMHO. Best regards, Maxim Levitsky > > [...]
On 09/07/20 13:56, Maxim Levitsky wrote: > On Thu, 2020-07-09 at 13:42 +0200, Markus Armbruster wrote: >> Maxim Levitsky <mlevitsk@redhat.com> writes: >> >>> This allows to preserve the semantics of hmp_device_del, >>> that the device is deleted immediatly which was changed by previos >>> patch that delayed this to RCU callback >>> >>> Suggested-by: Stefan Hajnoczi <stefanha@gmail.com> >>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> >>> --- >>> include/qemu/rcu.h | 1 + >>> qdev-monitor.c | 3 +++ >>> util/rcu.c | 33 +++++++++++++++++++++++++++++++++ >>> 3 files changed, 37 insertions(+) >>> >>> diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h >>> index 570aa603eb..0e375ebe13 100644 >>> --- a/include/qemu/rcu.h >>> +++ b/include/qemu/rcu.h >>> @@ -133,6 +133,7 @@ struct rcu_head { >>> }; >>> >>> extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func); >>> +extern void drain_call_rcu(void); >>> >>> /* The operands of the minus operator must have the same type, >>> * which must be the one that we specify in the cast. >>> diff --git a/qdev-monitor.c b/qdev-monitor.c >>> index 56cee1483f..70877840a2 100644 >>> --- a/qdev-monitor.c >>> +++ b/qdev-monitor.c >>> @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) >>> return; >>> } >>> dev = qdev_device_add(opts, &local_err); >>> + drain_call_rcu(); >>> + >>> if (!dev) { >>> error_propagate(errp, local_err); >>> qemu_opts_del(opts); >>> @@ -904,6 +906,7 @@ void qmp_device_del(const char *id, Error **errp) >>> } >>> >>> qdev_unplug(dev, errp); >>> + drain_call_rcu(); >>> } >>> } >>> >> >> Subject claims "in hmp_device_del", code has it in qmp_device_add() and >> qmp_device_del(). Please advise. > > I added it in both, because addition of a device can fail and trigger removal, > which can also be now delayed due to RCU. > Since both device_add and device_del aren't used often, the overhead won't > be a problem IMHO. Ok, just mention this in the commit message. It may also be a good idea to move it from qmp_device_add to the error-propagation section of qdev_device_add. Paolo
diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h index 570aa603eb..0e375ebe13 100644 --- a/include/qemu/rcu.h +++ b/include/qemu/rcu.h @@ -133,6 +133,7 @@ struct rcu_head { }; extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func); +extern void drain_call_rcu(void); /* The operands of the minus operator must have the same type, * which must be the one that we specify in the cast. diff --git a/qdev-monitor.c b/qdev-monitor.c index 56cee1483f..70877840a2 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) return; } dev = qdev_device_add(opts, &local_err); + drain_call_rcu(); + if (!dev) { error_propagate(errp, local_err); qemu_opts_del(opts); @@ -904,6 +906,7 @@ void qmp_device_del(const char *id, Error **errp) } qdev_unplug(dev, errp); + drain_call_rcu(); } } diff --git a/util/rcu.c b/util/rcu.c index 60a37f72c3..e8b1c4d6c5 100644 --- a/util/rcu.c +++ b/util/rcu.c @@ -293,6 +293,39 @@ void call_rcu1(struct rcu_head *node, void (*func)(struct rcu_head *node)) qemu_event_set(&rcu_call_ready_event); } + +struct rcu_drain { + struct rcu_head rcu; + QemuEvent drain_complete_event; +}; + +static void drain_rcu_callback(struct rcu_head *node) +{ + struct rcu_drain *event = (struct rcu_drain *)node; + qemu_event_set(&event->drain_complete_event); +} + +void drain_call_rcu(void) +{ + struct rcu_drain rcu_drain; + bool locked = qemu_mutex_iothread_locked(); + + memset(&rcu_drain, 0, sizeof(struct rcu_drain)); + + if (locked) { + qemu_mutex_unlock_iothread(); + } + + qemu_event_init(&rcu_drain.drain_complete_event, false); + call_rcu1(&rcu_drain.rcu, drain_rcu_callback); + qemu_event_wait(&rcu_drain.drain_complete_event); + + if (locked) { + qemu_mutex_lock_iothread(); + } + +} + void rcu_register_thread(void) { assert(rcu_reader.ctr == 0);
This allows to preserve the semantics of hmp_device_del, that the device is deleted immediatly which was changed by previos patch that delayed this to RCU callback Suggested-by: Stefan Hajnoczi <stefanha@gmail.com> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- include/qemu/rcu.h | 1 + qdev-monitor.c | 3 +++ util/rcu.c | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+)