[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/2] rcu: Introduce force_rcu notifier
From: |
Greg Kurz |
Subject: |
Re: [PATCH v2 1/2] rcu: Introduce force_rcu notifier |
Date: |
Wed, 20 Oct 2021 09:59:26 +0200 |
On Tue, 19 Oct 2021 13:26:25 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 19/10/21 07:56, Greg Kurz wrote:
> > The drain_rcu_call() function can be blocked as long as an RCU reader
> > stays in a read-side critical section. This is typically what happens
> > when a TCG vCPU is executing a busy loop. It can deadlock the QEMU
> > monitor as reported in https://gitlab.com/qemu-project/qemu/-/issues/650 .
> >
> > This can be avoided by allowing drain_rcu_call() to enforce an RCU grace
> > period. Since each reader might need to do specific actions to end a
> > read-side critical section, do it with notifiers.
> >
> > Prepare ground for this by adding a notifier list to the RCU reader
> > struct and use it in wait_for_readers() if drain_rcu_call() is in
> > progress. An API is added for readers to register their notifiers.
> >
> > This is largely based on a draft from Paolo Bonzini.
> >
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > include/qemu/rcu.h | 16 ++++++++++++++++
> > util/rcu.c | 22 +++++++++++++++++++++-
> > 2 files changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> > index 515d327cf11c..d8c4fd8686b4 100644
> > --- a/include/qemu/rcu.h
> > +++ b/include/qemu/rcu.h
> > @@ -27,6 +27,7 @@
> > #include "qemu/thread.h"
> > #include "qemu/queue.h"
> > #include "qemu/atomic.h"
> > +#include "qemu/notify.h"
> > #include "qemu/sys_membarrier.h"
> >
> > #ifdef __cplusplus
> > @@ -66,6 +67,14 @@ struct rcu_reader_data {
> >
> > /* Data used for registry, protected by rcu_registry_lock */
> > QLIST_ENTRY(rcu_reader_data) node;
> > +
> > + /*
> > + * NotifierList used to force an RCU grace period. Accessed under
> > + * rcu_registry_lock. Note that the notifier is called _outside_
> > + * the thread!
> > + */
> > + NotifierList force_rcu;
> > + void *force_rcu_data;
>
> This is a bit ugly because the force_rcu_data is shared across all
> notifiers. Sure right now we have only one, but still the data argument
> should be in rcu_register_thread rather than rcu_add_force_rcu_notifier.
>
I don't quite see why we'd need more than one notifier, but indeed
this isn't conceptually correct.
> It's a pity because I liked the Notifier local variable... But after
> thinking about it more and deleting some suggestions that won't work,
> it's just easiest to have the notifier in CPUState.
>
Agreed.
> Maybe even move the unregistration to the existing function
> tcg_cpus_destroy, and add tcg_cpus_init that calls tcg_register_thread()
> and rcu_add_force_rcu_notifier(). This way you don't have to export
> tcg_cpus_force_rcu, and the tcg-accel-ops.h APIs are a bit more tidy.
>
I don't think we can do that because of round-robin : we only have one
thread in this case but tcg_cpus_destroy() must still be called for all
vCPUs. Also, a single notifier will work just fine no matter which
vCPU is running when wait_for_readers() is called if I understand
correctly how round-robin works.
> Paolo
>
> > +void rcu_add_force_rcu_notifier(Notifier *n, void *data)
> > +{
> > + qemu_mutex_lock(&rcu_registry_lock);
> > + notifier_list_add(&rcu_reader.force_rcu, n);
> > + rcu_reader.force_rcu_data = data;
> > + qemu_mutex_unlock(&rcu_registry_lock);
> > +}
> > +
> > +void rcu_remove_force_rcu_notifier(Notifier *n)
> > +{
> > + qemu_mutex_lock(&rcu_registry_lock);
> > + rcu_reader.force_rcu_data = NULL;
> > + notifier_remove(n);
> > + qemu_mutex_unlock(&rcu_registry_lock);
> > +}
> > +
> > static void rcu_init_complete(void)
> > {
> > QemuThread thread;
> >
>