Re: rcu: WARNING in rcu_seq_end

[Boqun Feng]

On Tue, Mar 07, 2017 at 08:05:19AM +0100, Dmitry Vyukov wrote:
[...]
> >>
> >> What is that mutex? And what locks/unlocks provide synchronization? I
> >> see that one uses exp_mutex and another -- exp_wake_mutex.
> >
> > Both of them.
> >
> > ->exp_mutex is acquired by the task requesting the grace period, and
> > the counter's first increment is done by that task under that mutex.
> > This task then schedules a workqueue, which drives forward the grace
> > period.  Upon grace-period completion, the workqueue handler does the
> > second increment (the one that your patch addressed).  The workqueue
> > handler then acquires ->exp_wake_mutex and wakes the task that holds
> > ->exp_mutex (along with all other tasks waiting for this grace period),
> > and that task releases ->exp_mutex, which allows the next grace period to
> > start (and the first increment for that next grace period to be carried
> > out under that lock).  The workqueue handler releases ->exp_wake_mutex
> > after finishing its wakeups.
> 
> 
> Then we need the following for the case when task requesting the grace
> period does not block, right?
> 

Won't be necessary I think, as the smp_mb() in rcu_seq_end() and the
smp_mb__before_atomic() in sync_exp_work_done() already provide the
required ordering, no?

Regards,
Boqun

> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index d80c2587bed8..aa7ba83f6a56 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3534,7 +3534,7 @@ static void rcu_seq_start(unsigned long *sp)
>  static void rcu_seq_end(unsigned long *sp)
>  {
>         smp_mb(); /* Ensure update-side operation before counter increment. */
> -       WRITE_ONCE(*sp, *sp + 1);
> +       smp_store_release(sp, *sp + 1);
>         WARN_ON_ONCE(*sp & 0x1);
>  }
> 
> @@ -3554,7 +3554,7 @@ static unsigned long rcu_seq_snap(unsigned long *sp)
>   */
>  static bool rcu_seq_done(unsigned long *sp, unsigned long s)
>  {
> -       return ULONG_CMP_GE(READ_ONCE(*sp), s);
> +       return ULONG_CMP_GE(smp_load_acquire(sp), s);
> 
>  }

Attachment: signature.asc
Description: PGP signature