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

[Jürgen Groß <jgross@xxxxxxxx>]

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@xxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel