| Message ID | 20200313130614.27265-4-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/rcu: let rcu work better with core scheduling | expand |
On 13.03.2020 14:06, Juergen Gross wrote: > Xen's RCU implementation relies on no softirq handling taking place > while being in a RCU critical section. Add ASSERT()s in debug builds > in order to catch any violations. > > For that purpose modify rcu_read_[un]lock() to use a dedicated percpu > counter additional to preempt_[en|dis]able() as this enables to test > that condition in __do_softirq() (ASSERT_NOT_IN_ATOMIC() is not > usable there due to __cpu_up() calling process_pending_softirqs() > while holding the cpu hotplug lock). > > While at it switch the rcu_read_[un]lock() implementation to static > inline functions instead of macros. > > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with one remark: > @@ -91,16 +114,23 @@ typedef struct _rcu_read_lock rcu_read_lock_t; > * will be deferred until the outermost RCU read-side critical section > * completes. > * > - * It is illegal to block while in an RCU read-side critical section. > + * It is illegal to process softirqs while in an RCU read-side critical section. The latest with the re-added preempt_disable(), wouldn't this better say "... to process softirqs or block ..."? Jan
On 17.03.20 15:36, Jan Beulich wrote: > On 13.03.2020 14:06, Juergen Gross wrote: >> Xen's RCU implementation relies on no softirq handling taking place >> while being in a RCU critical section. Add ASSERT()s in debug builds >> in order to catch any violations. >> >> For that purpose modify rcu_read_[un]lock() to use a dedicated percpu >> counter additional to preempt_[en|dis]able() as this enables to test >> that condition in __do_softirq() (ASSERT_NOT_IN_ATOMIC() is not >> usable there due to __cpu_up() calling process_pending_softirqs() >> while holding the cpu hotplug lock). >> >> While at it switch the rcu_read_[un]lock() implementation to static >> inline functions instead of macros. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with one remark: > >> @@ -91,16 +114,23 @@ typedef struct _rcu_read_lock rcu_read_lock_t; >> * will be deferred until the outermost RCU read-side critical section >> * completes. >> * >> - * It is illegal to block while in an RCU read-side critical section. >> + * It is illegal to process softirqs while in an RCU read-side critical section. > > The latest with the re-added preempt_disable(), wouldn't this better > say "... to process softirqs or block ..."? I can add this, but OTOH blocking without processing softirqs is not possible, as there is no other (legal) way to enter the scheduler. Juergen
On 18.03.2020 07:26, Jürgen Groß wrote: > On 17.03.20 15:36, Jan Beulich wrote: >> On 13.03.2020 14:06, Juergen Gross wrote: >>> Xen's RCU implementation relies on no softirq handling taking place >>> while being in a RCU critical section. Add ASSERT()s in debug builds >>> in order to catch any violations. >>> >>> For that purpose modify rcu_read_[un]lock() to use a dedicated percpu >>> counter additional to preempt_[en|dis]able() as this enables to test >>> that condition in __do_softirq() (ASSERT_NOT_IN_ATOMIC() is not >>> usable there due to __cpu_up() calling process_pending_softirqs() >>> while holding the cpu hotplug lock). >>> >>> While at it switch the rcu_read_[un]lock() implementation to static >>> inline functions instead of macros. >>> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> with one remark: >> >>> @@ -91,16 +114,23 @@ typedef struct _rcu_read_lock rcu_read_lock_t; >>> * will be deferred until the outermost RCU read-side critical section >>> * completes. >>> * >>> - * It is illegal to block while in an RCU read-side critical section. >>> + * It is illegal to process softirqs while in an RCU read-side critical section. >> >> The latest with the re-added preempt_disable(), wouldn't this better >> say "... to process softirqs or block ..."? > > I can add this, but OTOH blocking without processing softirqs is not > possible, as there is no other (legal) way to enter the scheduler. Sure, but that's still implicit, but could do with saying explicitly. Jan
On 18.03.20 08:37, Jan Beulich wrote: > On 18.03.2020 07:26, Jürgen Groß wrote: >> On 17.03.20 15:36, Jan Beulich wrote: >>> On 13.03.2020 14:06, Juergen Gross wrote: >>>> Xen's RCU implementation relies on no softirq handling taking place >>>> while being in a RCU critical section. Add ASSERT()s in debug builds >>>> in order to catch any violations. >>>> >>>> For that purpose modify rcu_read_[un]lock() to use a dedicated percpu >>>> counter additional to preempt_[en|dis]able() as this enables to test >>>> that condition in __do_softirq() (ASSERT_NOT_IN_ATOMIC() is not >>>> usable there due to __cpu_up() calling process_pending_softirqs() >>>> while holding the cpu hotplug lock). >>>> >>>> While at it switch the rcu_read_[un]lock() implementation to static >>>> inline functions instead of macros. >>>> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> with one remark: >>> >>>> @@ -91,16 +114,23 @@ typedef struct _rcu_read_lock rcu_read_lock_t; >>>> * will be deferred until the outermost RCU read-side critical >>>> section >>>> * completes. >>>> * >>>> - * It is illegal to block while in an RCU read-side critical section. >>>> + * It is illegal to process softirqs while in an RCU read-side >>>> critical section. >>> >>> The latest with the re-added preempt_disable(), wouldn't this better >>> say "... to process softirqs or block ..."? >> >> I can add this, but OTOH blocking without processing softirqs is not >> possible, as there is no other (legal) way to enter the scheduler. > > Sure, but that's still implicit, but could do with saying explicitly. Okay. Juergen
diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c index ed9083d2b2..5e7bd7196f 100644 --- a/xen/common/rcupdate.c +++ b/xen/common/rcupdate.c @@ -46,6 +46,8 @@ #include <xen/cpu.h> #include <xen/stop_machine.h> +DEFINE_PER_CPU(unsigned int, rcu_lock_cnt); + /* Global control variables for rcupdate callback mechanism. */ static struct rcu_ctrlblk { long cur; /* Current batch number. */ diff --git a/xen/common/softirq.c b/xen/common/softirq.c index 00d676b62c..eba65c5fc0 100644 --- a/xen/common/softirq.c +++ b/xen/common/softirq.c @@ -31,6 +31,8 @@ static void __do_softirq(unsigned long ignore_mask) unsigned long pending; bool rcu_allowed = !(ignore_mask & (1ul << RCU_SOFTIRQ)); + ASSERT(!rcu_allowed || rcu_quiesce_allowed()); + for ( ; ; ) { /* @@ -58,7 +60,7 @@ void process_pending_softirqs(void) (1ul << SCHED_SLAVE_SOFTIRQ); /* Block RCU processing in case of rcu_read_lock() held. */ - if ( preempt_count() ) + if ( !rcu_quiesce_allowed() ) ignore_mask |= 1ul << RCU_SOFTIRQ; ASSERT(!in_irq() && local_irq_is_enabled()); diff --git a/xen/include/xen/rcupdate.h b/xen/include/xen/rcupdate.h index 31c8b86d13..d3c2b7b093 100644 --- a/xen/include/xen/rcupdate.h +++ b/xen/include/xen/rcupdate.h @@ -32,12 +32,35 @@ #define __XEN_RCUPDATE_H #include <xen/cache.h> +#include <xen/compiler.h> #include <xen/spinlock.h> #include <xen/cpumask.h> +#include <xen/percpu.h> #include <xen/preempt.h> #define __rcu +DECLARE_PER_CPU(unsigned int, rcu_lock_cnt); + +static inline void rcu_quiesce_disable(void) +{ + preempt_disable(); + this_cpu(rcu_lock_cnt)++; + barrier(); +} + +static inline void rcu_quiesce_enable(void) +{ + barrier(); + this_cpu(rcu_lock_cnt)--; + preempt_enable(); +} + +static inline bool rcu_quiesce_allowed(void) +{ + return !this_cpu(rcu_lock_cnt); +} + /** * struct rcu_head - callback structure for use with RCU * @next: next update requests in a list @@ -91,16 +114,23 @@ typedef struct _rcu_read_lock rcu_read_lock_t; * will be deferred until the outermost RCU read-side critical section * completes. * - * It is illegal to block while in an RCU read-side critical section. + * It is illegal to process softirqs while in an RCU read-side critical section. */ -#define rcu_read_lock(x) ({ ((void)(x)); preempt_disable(); }) +static inline void rcu_read_lock(rcu_read_lock_t *lock) +{ + rcu_quiesce_disable(); +} /** * rcu_read_unlock - marks the end of an RCU read-side critical section. * * See rcu_read_lock() for more information. */ -#define rcu_read_unlock(x) ({ ((void)(x)); preempt_enable(); }) +static inline void rcu_read_unlock(rcu_read_lock_t *lock) +{ + ASSERT(!rcu_quiesce_allowed()); + rcu_quiesce_enable(); +} /* * So where is rcu_write_lock()? It does not exist, as there is no
Xen's RCU implementation relies on no softirq handling taking place while being in a RCU critical section. Add ASSERT()s in debug builds in order to catch any violations. For that purpose modify rcu_read_[un]lock() to use a dedicated percpu counter additional to preempt_[en|dis]able() as this enables to test that condition in __do_softirq() (ASSERT_NOT_IN_ATOMIC() is not usable there due to __cpu_up() calling process_pending_softirqs() while holding the cpu hotplug lock). While at it switch the rcu_read_[un]lock() implementation to static inline functions instead of macros. Signed-off-by: Juergen Gross <jgross@suse.com> --- V3: - add barriers to rcu_[en|dis]able() (Roger Pau Monné) - add rcu_quiesce_allowed() to ASSERT_NOT_IN_ATOMIC (Roger Pau Monné) - convert macros to static inline functions - add sanity check in rcu_read_unlock() V4: - use barrier() in rcu_[en|dis]able() (Julien Grall) V5: - use rcu counter even if not using a debug build V6: - keep preempt_[dis|en]able() calls --- xen/common/rcupdate.c | 2 ++ xen/common/softirq.c | 4 +++- xen/include/xen/rcupdate.h | 36 +++++++++++++++++++++++++++++++++--- 3 files changed, 38 insertions(+), 4 deletions(-)