Re: [Xen-devel] [PATCH v6 3/4] xen/rcu: add assertions to debug build
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 Reviewed-by: Jan Beulich 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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6 3/4] xen/rcu: add assertions to debug build
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 Reviewed-by: Jan Beulich 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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6 3/4] xen/rcu: add assertions to debug build
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 Reviewed-by: Jan Beulich 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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6 3/4] xen/rcu: add assertions to debug build
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 Reviewed-by: Jan Beulich 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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v6 3/4] xen/rcu: add assertions to debug build
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
---
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(-)
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
#include
+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
+#include
#include
#include
+#include
#include
#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
--
2.16.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
