1. Patchew
  2. Xen
  3. [Xen-devel] [PATCH v2 0/4] xen/rcu: let rcu work better with core scheduling
  4. View patch

[Xen-devel] [PATCH v2 4/4] xen/rcu: add assertions to debug build

Juergen Gross posted 4 patches 5 years, 11 months ago
There is a newer version of this series
[Xen-devel] [PATCH v2 4/4] xen/rcu: add assertions to debug build
Posted by Juergen Gross 5 years, 11 months ago 
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 instead of 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).

Dropping the now no longer needed #include of preempt.h in rcupdate.h
requires adding it in some sources.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/multicall.c     |  1 +
 xen/common/rcupdate.c      |  4 ++++
 xen/common/softirq.c       |  2 ++
 xen/common/wait.c          |  1 +
 xen/include/xen/rcupdate.h | 21 +++++++++++++++++----
 5 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/xen/common/multicall.c b/xen/common/multicall.c
index 5a199ebf8f..67f1a23485 100644
--- a/xen/common/multicall.c
+++ b/xen/common/multicall.c
@@ -10,6 +10,7 @@
 #include <xen/multicall.h>
 #include <xen/guest_access.h>
 #include <xen/perfc.h>
+#include <xen/preempt.h>
 #include <xen/trace.h>
 #include <asm/current.h>
 #include <asm/hardirq.h>
diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index e6add0b120..b03f4b44d9 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -46,6 +46,10 @@
 #include <xen/cpu.h>
 #include <xen/stop_machine.h>
 
+#ifndef NDEBUG
+DEFINE_PER_CPU(unsigned int, rcu_lock_cnt);
+#endif
+
 /* 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 3fe75ca3e8..18be8db0c6 100644
--- a/xen/common/softirq.c
+++ b/xen/common/softirq.c
@@ -30,6 +30,8 @@ static void __do_softirq(unsigned long ignore_mask, bool rcu_allowed)
     unsigned int i, cpu;
     unsigned long pending;
 
+    ASSERT(!rcu_allowed || rcu_quiesce_allowed());
+
     for ( ; ; )
     {
         /*
diff --git a/xen/common/wait.c b/xen/common/wait.c
index 24716e7676..9cdb174036 100644
--- a/xen/common/wait.c
+++ b/xen/common/wait.c
@@ -19,6 +19,7 @@
  * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/preempt.h>
 #include <xen/sched.h>
 #include <xen/softirq.h>
 #include <xen/wait.h>
diff --git a/xen/include/xen/rcupdate.h b/xen/include/xen/rcupdate.h
index 87f35b7704..a5ee7fec2b 100644
--- a/xen/include/xen/rcupdate.h
+++ b/xen/include/xen/rcupdate.h
@@ -34,10 +34,23 @@
 #include <xen/cache.h>
 #include <xen/spinlock.h>
 #include <xen/cpumask.h>
-#include <xen/preempt.h>
+#include <xen/percpu.h>
 
 #define __rcu
 
+#ifndef NDEBUG
+DECLARE_PER_CPU(unsigned int, rcu_lock_cnt);
+
+#define rcu_quiesce_disable() (this_cpu(rcu_lock_cnt))++
+#define rcu_quiesce_enable()  (this_cpu(rcu_lock_cnt))--
+#define rcu_quiesce_allowed() (!this_cpu(rcu_lock_cnt))
+
+#else
+#define rcu_quiesce_disable() ((void)0)
+#define rcu_quiesce_enable()  ((void)0)
+#define rcu_quiesce_allowed() true
+#endif
+
 /**
  * struct rcu_head - callback structure for use with RCU
  * @next: next update requests in a list
@@ -90,16 +103,16 @@ 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(); })
+#define rcu_read_lock(x)       ({ ((void)(x)); 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(); })
+#define rcu_read_unlock(x)     ({ ((void)(x)); 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
Re: [Xen-devel] [PATCH v2 4/4] xen/rcu: add assertions to debug build
Posted by Roger Pau Monné 5 years, 11 months ago 
On Tue, Feb 18, 2020 at 01:21:14PM +0100, 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 instead of 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).
> 
> Dropping the now no longer needed #include of preempt.h in rcupdate.h
> requires adding it in some sources.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/common/multicall.c     |  1 +
>  xen/common/rcupdate.c      |  4 ++++
>  xen/common/softirq.c       |  2 ++
>  xen/common/wait.c          |  1 +
>  xen/include/xen/rcupdate.h | 21 +++++++++++++++++----
>  5 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/multicall.c b/xen/common/multicall.c
> index 5a199ebf8f..67f1a23485 100644
> --- a/xen/common/multicall.c
> +++ b/xen/common/multicall.c
> @@ -10,6 +10,7 @@
>  #include <xen/multicall.h>
>  #include <xen/guest_access.h>
>  #include <xen/perfc.h>
> +#include <xen/preempt.h>
>  #include <xen/trace.h>
>  #include <asm/current.h>
>  #include <asm/hardirq.h>
> diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
> index e6add0b120..b03f4b44d9 100644
> --- a/xen/common/rcupdate.c
> +++ b/xen/common/rcupdate.c
> @@ -46,6 +46,10 @@
>  #include <xen/cpu.h>
>  #include <xen/stop_machine.h>
>  
> +#ifndef NDEBUG
> +DEFINE_PER_CPU(unsigned int, rcu_lock_cnt);
> +#endif
> +
>  /* 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 3fe75ca3e8..18be8db0c6 100644
> --- a/xen/common/softirq.c
> +++ b/xen/common/softirq.c
> @@ -30,6 +30,8 @@ static void __do_softirq(unsigned long ignore_mask, bool rcu_allowed)
>      unsigned int i, cpu;
>      unsigned long pending;
>  
> +    ASSERT(!rcu_allowed || rcu_quiesce_allowed());
> +
>      for ( ; ; )
>      {
>          /*
> diff --git a/xen/common/wait.c b/xen/common/wait.c
> index 24716e7676..9cdb174036 100644
> --- a/xen/common/wait.c
> +++ b/xen/common/wait.c
> @@ -19,6 +19,7 @@
>   * along with this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <xen/preempt.h>
>  #include <xen/sched.h>
>  #include <xen/softirq.h>
>  #include <xen/wait.h>
> diff --git a/xen/include/xen/rcupdate.h b/xen/include/xen/rcupdate.h
> index 87f35b7704..a5ee7fec2b 100644
> --- a/xen/include/xen/rcupdate.h
> +++ b/xen/include/xen/rcupdate.h
> @@ -34,10 +34,23 @@
>  #include <xen/cache.h>
>  #include <xen/spinlock.h>
>  #include <xen/cpumask.h>
> -#include <xen/preempt.h>
> +#include <xen/percpu.h>
>  
>  #define __rcu
>  
> +#ifndef NDEBUG
> +DECLARE_PER_CPU(unsigned int, rcu_lock_cnt);
> +
> +#define rcu_quiesce_disable() (this_cpu(rcu_lock_cnt))++
> +#define rcu_quiesce_enable()  (this_cpu(rcu_lock_cnt))--

I think you need a barrier here like it's currently used in
preempt_{enabled/disable}, or use arch_lock_{acquire/release}_barrier
which would be better IMO.

> +#define rcu_quiesce_allowed() (!this_cpu(rcu_lock_cnt))

ASSERT_NOT_IN_ATOMIC should be expanded to also assert
!this_cpu(rcu_lock_cnt), or else missing pairs of
rcu_read_{lock/unlock} would be undetected.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 4/4] xen/rcu: add assertions to debug build
Posted by Jürgen Groß 5 years, 11 months ago 
On 24.02.20 12:31, Roger Pau Monné wrote:
> On Tue, Feb 18, 2020 at 01:21:14PM +0100, 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 instead of 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).
>>
>> Dropping the now no longer needed #include of preempt.h in rcupdate.h
>> requires adding it in some sources.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   xen/common/multicall.c     |  1 +
>>   xen/common/rcupdate.c      |  4 ++++
>>   xen/common/softirq.c       |  2 ++
>>   xen/common/wait.c          |  1 +
>>   xen/include/xen/rcupdate.h | 21 +++++++++++++++++----
>>   5 files changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/common/multicall.c b/xen/common/multicall.c
>> index 5a199ebf8f..67f1a23485 100644
>> --- a/xen/common/multicall.c
>> +++ b/xen/common/multicall.c
>> @@ -10,6 +10,7 @@
>>   #include <xen/multicall.h>
>>   #include <xen/guest_access.h>
>>   #include <xen/perfc.h>
>> +#include <xen/preempt.h>
>>   #include <xen/trace.h>
>>   #include <asm/current.h>
>>   #include <asm/hardirq.h>
>> diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
>> index e6add0b120..b03f4b44d9 100644
>> --- a/xen/common/rcupdate.c
>> +++ b/xen/common/rcupdate.c
>> @@ -46,6 +46,10 @@
>>   #include <xen/cpu.h>
>>   #include <xen/stop_machine.h>
>>   
>> +#ifndef NDEBUG
>> +DEFINE_PER_CPU(unsigned int, rcu_lock_cnt);
>> +#endif
>> +
>>   /* 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 3fe75ca3e8..18be8db0c6 100644
>> --- a/xen/common/softirq.c
>> +++ b/xen/common/softirq.c
>> @@ -30,6 +30,8 @@ static void __do_softirq(unsigned long ignore_mask, bool rcu_allowed)
>>       unsigned int i, cpu;
>>       unsigned long pending;
>>   
>> +    ASSERT(!rcu_allowed || rcu_quiesce_allowed());
>> +
>>       for ( ; ; )
>>       {
>>           /*
>> diff --git a/xen/common/wait.c b/xen/common/wait.c
>> index 24716e7676..9cdb174036 100644
>> --- a/xen/common/wait.c
>> +++ b/xen/common/wait.c
>> @@ -19,6 +19,7 @@
>>    * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>    */
>>   
>> +#include <xen/preempt.h>
>>   #include <xen/sched.h>
>>   #include <xen/softirq.h>
>>   #include <xen/wait.h>
>> diff --git a/xen/include/xen/rcupdate.h b/xen/include/xen/rcupdate.h
>> index 87f35b7704..a5ee7fec2b 100644
>> --- a/xen/include/xen/rcupdate.h
>> +++ b/xen/include/xen/rcupdate.h
>> @@ -34,10 +34,23 @@
>>   #include <xen/cache.h>
>>   #include <xen/spinlock.h>
>>   #include <xen/cpumask.h>
>> -#include <xen/preempt.h>
>> +#include <xen/percpu.h>
>>   
>>   #define __rcu
>>   
>> +#ifndef NDEBUG
>> +DECLARE_PER_CPU(unsigned int, rcu_lock_cnt);
>> +
>> +#define rcu_quiesce_disable() (this_cpu(rcu_lock_cnt))++
>> +#define rcu_quiesce_enable()  (this_cpu(rcu_lock_cnt))--
> 
> I think you need a barrier here like it's currently used in
> preempt_{enabled/disable}, or use arch_lock_{acquire/release}_barrier
> which would be better IMO.

Thanks, will do that.

> 
>> +#define rcu_quiesce_allowed() (!this_cpu(rcu_lock_cnt))
> 
> ASSERT_NOT_IN_ATOMIC should be expanded to also assert
> !this_cpu(rcu_lock_cnt), or else missing pairs of
> rcu_read_{lock/unlock} would be undetected.

Good idea.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.