[PATCH] rcu: remove unnecessary check cpu_no_qs.norm on rcu_report_qs_rdp

[PATCH] rcu: remove unnecessary check cpu_no_qs.norm on rcu_report_qs_rdp

[Levi Yun]

Wherever rcu_report_qs_rdp is called, cpu_no_qs.norm value is false.
Therefore, Remove unnecessary check in rcu_report_qs_rdp.

Signed-off-by: Levi Yun <ppbuk5246@gmail.com>
---
 kernel/rcu/tree.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 1449cb69a0e0..d840596e9903 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -1962,8 +1962,7 @@ rcu_report_qs_rdp(struct rcu_data *rdp)
 	WARN_ON_ONCE(rdp->cpu != smp_processor_id());
 	rnp = rdp->mynode;
 	raw_spin_lock_irqsave_rcu_node(rnp, flags);
-	if (rdp->cpu_no_qs.b.norm || rdp->gp_seq != rnp->gp_seq ||
-	    rdp->gpwrap) {
+	if (rdp->gp_seq != rnp->gp_seq || rdp->gpwrap) {

 		/*
 		 * The grace period in which this quiescent state was
--
2.37.2

Re: [PATCH] rcu: remove unnecessary check cpu_no_qs.norm on rcu_report_qs_rdp

[Paul E. McKenney]

On Fri, Jul 21, 2023 at 01:15:33PM +0100, Levi Yun wrote:
> Wherever rcu_report_qs_rdp is called, cpu_no_qs.norm value is false.
> Therefore, Remove unnecessary check in rcu_report_qs_rdp.
> 
> Signed-off-by: Levi Yun <ppbuk5246@gmail.com>

Why not start with something like this?

	if (!WARN_ON_ONCE(!rdp->cpu_no_qs.b.norm) ||
	    rdp->gp_seq != rnp->gp_seq || rdp->gpwrap) {

Except that rcu_report_qs_rdp() is invoked with interrupts enabled,
which means that there is some possibility of state changes up to the
raw_spin_lock_irqsave_rcu_node(rnp, flags) statement.

So, did you check whether RCU's interrupt paths change this state?

						Thanx, Paul

> ---
>  kernel/rcu/tree.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 1449cb69a0e0..d840596e9903 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1962,8 +1962,7 @@ rcu_report_qs_rdp(struct rcu_data *rdp)
>  	WARN_ON_ONCE(rdp->cpu != smp_processor_id());
>  	rnp = rdp->mynode;
>  	raw_spin_lock_irqsave_rcu_node(rnp, flags);
> -	if (rdp->cpu_no_qs.b.norm || rdp->gp_seq != rnp->gp_seq ||
> -	    rdp->gpwrap) {
> +	if (rdp->gp_seq != rnp->gp_seq || rdp->gpwrap) {
> 
>  		/*
>  		 * The grace period in which this quiescent state was
> --
> 2.37.2

Re: [PATCH] rcu: remove unnecessary check cpu_no_qs.norm on rcu_report_qs_rdp

[Yun Levi]

Hi, Paul.

Thanks for looking into this :)


> Except that rcu_report_qs_rdp() is invoked with interrupts enabled,
> which means that there is some possibility of state changes up to the
> raw_spin_lock_irqsave_rcu_node(rnp, flags) statement.
>
> So, did you check whether RCU's interrupt paths change this state?

In my narrow view,
only a new gp started, cpu_no_qs.b.norm changes as true in the path of
rcu_sched_clock_irq.
But in that case, rcu_report_qs_rdp isn't called.

Did I understand your question well and are there any missed paths I didn't see?

> Why not start with something like this?
>
>         if (!WARN_ON_ONCE(!rdp->cpu_no_qs.b.norm) ||
>             rdp->gp_seq != rnp->gp_seq || rdp->gpwrap) {
>

Yes. but with different message

Re: [PATCH] rcu: remove unnecessary check cpu_no_qs.norm on rcu_report_qs_rdp

[Paul E. McKenney]

On Sat, Jul 22, 2023 at 10:23:26AM +0100, Yun Levi wrote:
> Hi, Paul.
> 
> Thanks for looking into this :)
> 
> 
> > Except that rcu_report_qs_rdp() is invoked with interrupts enabled,
> > which means that there is some possibility of state changes up to the
> > raw_spin_lock_irqsave_rcu_node(rnp, flags) statement.
> >
> > So, did you check whether RCU's interrupt paths change this state?
> 
> In my narrow view,
> only a new gp started, cpu_no_qs.b.norm changes as true in the path of
> rcu_sched_clock_irq.
> But in that case, rcu_report_qs_rdp isn't called.
> 
> Did I understand your question well and are there any missed paths I didn't see?

Suppose that the scheduler-clock interrupt invoking rcu_sched_clock_irq()
happened just before the lock was acquired in rcu_report_qs_rdp().
Suppose further that the RCU grace-period kthread started a new grace
period just before that interrupt occurred.  Then mightn't that interrupt
notice the new grace period and set ->cpu_no_qs.b.norm to true before
fully returning?

							Thanx, Paul

> > Why not start with something like this?
> >
> >         if (!WARN_ON_ONCE(!rdp->cpu_no_qs.b.norm) ||
> >             rdp->gp_seq != rnp->gp_seq || rdp->gpwrap) {
> >
> 
> Yes. but with different message

Re: [PATCH] rcu: remove unnecessary check cpu_no_qs.norm on rcu_report_qs_rdp

[Yun Levi]

Hi Paul.

> Suppose that the scheduler-clock interrupt invoking rcu_sched_clock_irq()
> happened just before the lock was acquired in rcu_report_qs_rdp().
> Suppose further that the RCU grace-period kthread started a new grace
> period just before that interrupt occurred.  Then mightn't that interrupt
> notice the new grace period and set ->cpu_no_qs.b.norm to true before
> fully returning?

IIUC, RCU grace-period kthread couldn't start new grace period
because the interrupted cpu don't report qs to rnp  via rcu_report_qs_rdp.
That situation is listened like new gp could be started thou all cpus
doesn't enter yet.
That's is the reason why it's better to use WARN_ON_ONCE as you suggest
to notice if the buggy situation happens

Am I missing something or wrong?

Thanks.

--------
Sincerely,
Levi.

Re: [PATCH] rcu: remove unnecessary check cpu_no_qs.norm on rcu_report_qs_rdp

[Paul E. McKenney]

On Sat, Jul 22, 2023 at 09:14:28PM +0100, Yun Levi wrote:
> Hi Paul.
> 
> > Suppose that the scheduler-clock interrupt invoking rcu_sched_clock_irq()
> > happened just before the lock was acquired in rcu_report_qs_rdp().
> > Suppose further that the RCU grace-period kthread started a new grace
> > period just before that interrupt occurred.  Then mightn't that interrupt
> > notice the new grace period and set ->cpu_no_qs.b.norm to true before
> > fully returning?
> 
> IIUC, RCU grace-period kthread couldn't start new grace period
> because the interrupted cpu don't report qs to rnp  via rcu_report_qs_rdp.
> That situation is listened like new gp could be started thou all cpus
> doesn't enter yet.
> That's is the reason why it's better to use WARN_ON_ONCE as you suggest
> to notice if the buggy situation happens

And try testing with CONFIG_RCU_STRICT_GRACE_PERIOD=y and CONFIG_PREEMPT=n.
Though there might be better Kconfig options to use.  Those two come
immediately to mind.

> Am I missing something or wrong?

I cannot see into your head, so I cannot say.

But one critical piece is that softirq handlers, including the RCU_SOFTIRQ
handler rcu_core_si(), can be invoked upon return from interrupts.
Another critical piece is that if a CPU is idle during any part of a
grace period, the grace-period kthread can report a quiescent state on
its behalf.

Does that help?

							Thanx, Paul

Re: [PATCH] rcu: remove unnecessary check cpu_no_qs.norm on rcu_report_qs_rdp

[Yun Levi]

Thanks for replying to reply Paul :)

> And try testing with CONFIG_RCU_STRICT_GRACE_PERIOD=y and CONFIG_PREEMPT=n.
> Though there might be better Kconfig options to use.  Those two come
> immediately to mind.

I've tested with this option via rcu torture.
and it doesn't report any problems.
and after commit 6d60ea03ac2d3 ("rcu: Report QS for outermost
PREEMPT=n rcu_read_unlock() for strict GPs")
it always makes cpu_no_qs.b.norm false whenever it calls
rcu_report_qs_rdp in rcu_read_unlock.

> But one critical piece is that softirq handlers, including the RCU_SOFTIRQ
> handler rcu_core_si(), can be invoked upon return from interrupts.

I think in that case, no problem because if it reports qs already,
rcu_check_quiescent_state wouldn't call rcu_report_qs_rdp again.
And if RCU_SOFTIRQ is called as soon as RCU interrupt is finished,
it seems the fastest one to notify qs to related tree.

> Another critical piece is that if a CPU is idle during any part of a
> grace period, the grace-period kthread can report a quiescent state on
> its behalf.

I think
    1) If timer interrupt is still programed,
          - when rcu_sched_clock_irq first reports qs, no problem
          - If qs is reported via grace period thread first,
note_gp_chagned in rcu interrupt
            will recognize this situation by setting core_needs_qs as false,
            it doesn't call rcu_report_qs_rdp thou cpu_no_qs.b.norm is true.

     2) If the timer interrupt isn't programmed,
          - rcu_gp_kthreaad reports its qs, no problem.

So, I think there's no problem removing
      "rdp->cpu_no_qs.b.norm" check in rcu_report_qs_rdp.
or wrap this condition check as WARN_ON_ONCE.

> Does that help?
Many thanks always :)

--------
SIncerely,
Levi.

Re: [PATCH] rcu: remove unnecessary check cpu_no_qs.norm on rcu_report_qs_rdp

[Paul E. McKenney]

On Sun, Jul 23, 2023 at 07:23:00AM +0100, Yun Levi wrote:
> Thanks for replying to reply Paul :)
> 
> > And try testing with CONFIG_RCU_STRICT_GRACE_PERIOD=y and CONFIG_PREEMPT=n.
> > Though there might be better Kconfig options to use.  Those two come
> > immediately to mind.
> 
> I've tested with this option via rcu torture.
> and it doesn't report any problems.
> and after commit 6d60ea03ac2d3 ("rcu: Report QS for outermost
> PREEMPT=n rcu_read_unlock() for strict GPs")
> it always makes cpu_no_qs.b.norm false whenever it calls
> rcu_report_qs_rdp in rcu_read_unlock.

Again, the concern is that an interrupt and softirq handler at that
point changes the grace period.  What else could you do to check that
this sequence of events actually occurred?  That is, that your testing
didn't just get lucky and not actually hit this condition?

Interrupts are enabled at that point, so there is nothing that prevents
that condition, after all.

> > But one critical piece is that softirq handlers, including the RCU_SOFTIRQ
> > handler rcu_core_si(), can be invoked upon return from interrupts.
> 
> I think in that case, no problem because if it reports qs already,
> rcu_check_quiescent_state wouldn't call rcu_report_qs_rdp again.
> And if RCU_SOFTIRQ is called as soon as RCU interrupt is finished,
> it seems the fastest one to notify qs to related tree.
> 
> > Another critical piece is that if a CPU is idle during any part of a
> > grace period, the grace-period kthread can report a quiescent state on
> > its behalf.
> 
> I think
>     1) If timer interrupt is still programed,
>           - when rcu_sched_clock_irq first reports qs, no problem
>           - If qs is reported via grace period thread first,
> note_gp_chagned in rcu interrupt
>             will recognize this situation by setting core_needs_qs as false,
>             it doesn't call rcu_report_qs_rdp thou cpu_no_qs.b.norm is true.
> 
>      2) If the timer interrupt isn't programmed,
>           - rcu_gp_kthreaad reports its qs, no problem.
> 
> So, I think there's no problem removing
>       "rdp->cpu_no_qs.b.norm" check in rcu_report_qs_rdp.
> or wrap this condition check as WARN_ON_ONCE.

One additional concern is that a quiescent state detected in an old
grace period will be incorrectly reported for a new grace period.

							Thanx, Paul

> > Does that help?
> Many thanks always :)
> 
> --------
> SIncerely,
> Levi.

Re: [PATCH] rcu: remove unnecessary check cpu_no_qs.norm on rcu_report_qs_rdp

[Z qiang]

>
> Thanks for replying to reply Paul :)
>
> > And try testing with CONFIG_RCU_STRICT_GRACE_PERIOD=y and CONFIG_PREEMPT=n.
> > Though there might be better Kconfig options to use.  Those two come
> > immediately to mind.
>
> I've tested with this option via rcu torture.
> and it doesn't report any problems.
> and after commit 6d60ea03ac2d3 ("rcu: Report QS for outermost
> PREEMPT=n rcu_read_unlock() for strict GPs")
> it always makes cpu_no_qs.b.norm false whenever it calls
> rcu_report_qs_rdp in rcu_read_unlock.
>
> > But one critical piece is that softirq handlers, including the RCU_SOFTIRQ
> > handler rcu_core_si(), can be invoked upon return from interrupts.
>
> I think in that case, no problem because if it reports qs already,
> rcu_check_quiescent_state wouldn't call rcu_report_qs_rdp again.
> And if RCU_SOFTIRQ is called as soon as RCU interrupt is finished,
> it seems the fastest one to notify qs to related tree.
>
> > Another critical piece is that if a CPU is idle during any part of a
> > grace period, the grace-period kthread can report a quiescent state on
> > its behalf.
>
> I think
>     1) If timer interrupt is still programed,
>           - when rcu_sched_clock_irq first reports qs, no problem
>           - If qs is reported via grace period thread first,
> note_gp_chagned in rcu interrupt
>             will recognize this situation by setting core_needs_qs as false,
>             it doesn't call rcu_report_qs_rdp thou cpu_no_qs.b.norm is true.
>
>      2) If the timer interrupt isn't programmed,
>           - rcu_gp_kthreaad reports its qs, no problem.
>
> So, I think there's no problem removing
>       "rdp->cpu_no_qs.b.norm" check in rcu_report_qs_rdp.
> or wrap this condition check as WARN_ON_ONCE.
>
> > Does that help?
> Many thanks always :)
>


Hi Levi

For built with CONFIG_RCU_STRICT_GRACE_PERIOD=y and CONFIG_PREEMPT=n kernels
Consider the following scenario:

__rcu_read_unlock()
   -> rcu_read_unlock_strict()
        ->rdp = this_cpu_ptr(&rcu_data);
        ->rdp->cpu_no_qs.b.norm = false;

                 by interrupt and return invoke rcu_core():
                 ->rcu_check_quiescent_state()
                      ->rdp = raw_cpu_ptr(&rcu_data);
                      -> rcu_check_quiescent_state(rdp);
                            ->note_gp_changes(rdp);
                                -> __note_gp_changes(rnp, rdp)
                                start new gp
                                ->rdp->cpu_no_qs.b.norm = true;

        ->rcu_report_qs_rdp(rdp);
           ->if (rdp->cpu_no_qs.b.norm || ...)


Thanks
Zqiang


>
> --------
> SIncerely,
> Levi.

Re: [PATCH] rcu: remove unnecessary check cpu_no_qs.norm on rcu_report_qs_rdp

[Yun Levi]

Hi Z qiang!

Thanks for replying. But I'm pinned on something wrong..!

> For built with CONFIG_RCU_STRICT_GRACE_PERIOD=y and CONFIG_PREEMPT=n kernels
> Consider the following scenario:
>
> __rcu_read_unlock()
>    -> rcu_read_unlock_strict()
>         ->rdp = this_cpu_ptr(&rcu_data);
>         ->rdp->cpu_no_qs.b.norm = false;
>
>                  by interrupt and return invoke rcu_core():
>                  ->rcu_check_quiescent_state()
>                       ->rdp = raw_cpu_ptr(&rcu_data);
>                       -> rcu_check_quiescent_state(rdp);
>                             ->note_gp_changes(rdp);
>                                 -> __note_gp_changes(rnp, rdp)
>                                 start new gp
>                                 ->rdp->cpu_no_qs.b.norm = true;
>
>         ->rcu_report_qs_rdp(rdp);
>            ->if (rdp->cpu_no_qs.b.norm || ...)

I've already seen this scenario, But I think something is missing in my view.
What I couldn't catch is

                  ->rcu_check_quiescent_state()
                       ->rdp = raw_cpu_ptr(&rcu_data);
                       -> rcu_check_quiescent_state(rdp);
                             ->note_gp_changes(rdp);
                                 -> __note_gp_changes(rnp, rdp)
                                 start new gp

the new gp is started.

to set cpu_no_qs.b.norm as true, below condition should be true

1201 >---if (rcu_seq_new_gp(rdp->gp_seq, rnp->gp_seq) ||
  1202 >---    unlikely(READ_ONCE(rdp->gpwrap))) {

Here,
How rcu_seq_new_gp could return true and new gp already started via
rcu_gp_kthread.
IIUC, because rcu_gp_fqs_loop couldn't see the root rnp->qsmask is
zero, it couldn't call rcu_gp_init.

Sorry to make noise, but would you correct me what I'm thinking wrong?

Many thanks..!

-----------
Sincerely,
Levi.

On Mon, Jul 24, 2023 at 4:21 AM Z qiang <qiang.zhang1211@gmail.com> wrote:
>
> >
> > Thanks for replying to reply Paul :)
> >
> > > And try testing with CONFIG_RCU_STRICT_GRACE_PERIOD=y and CONFIG_PREEMPT=n.
> > > Though there might be better Kconfig options to use.  Those two come
> > > immediately to mind.
> >
> > I've tested with this option via rcu torture.
> > and it doesn't report any problems.
> > and after commit 6d60ea03ac2d3 ("rcu: Report QS for outermost
> > PREEMPT=n rcu_read_unlock() for strict GPs")
> > it always makes cpu_no_qs.b.norm false whenever it calls
> > rcu_report_qs_rdp in rcu_read_unlock.
> >
> > > But one critical piece is that softirq handlers, including the RCU_SOFTIRQ
> > > handler rcu_core_si(), can be invoked upon return from interrupts.
> >
> > I think in that case, no problem because if it reports qs already,
> > rcu_check_quiescent_state wouldn't call rcu_report_qs_rdp again.
> > And if RCU_SOFTIRQ is called as soon as RCU interrupt is finished,
> > it seems the fastest one to notify qs to related tree.
> >
> > > Another critical piece is that if a CPU is idle during any part of a
> > > grace period, the grace-period kthread can report a quiescent state on
> > > its behalf.
> >
> > I think
> >     1) If timer interrupt is still programed,
> >           - when rcu_sched_clock_irq first reports qs, no problem
> >           - If qs is reported via grace period thread first,
> > note_gp_chagned in rcu interrupt
> >             will recognize this situation by setting core_needs_qs as false,
> >             it doesn't call rcu_report_qs_rdp thou cpu_no_qs.b.norm is true.
> >
> >      2) If the timer interrupt isn't programmed,
> >           - rcu_gp_kthreaad reports its qs, no problem.
> >
> > So, I think there's no problem removing
> >       "rdp->cpu_no_qs.b.norm" check in rcu_report_qs_rdp.
> > or wrap this condition check as WARN_ON_ONCE.
> >
> > > Does that help?
> > Many thanks always :)
> >
>
>
> Hi Levi
>
> For built with CONFIG_RCU_STRICT_GRACE_PERIOD=y and CONFIG_PREEMPT=n kernels
> Consider the following scenario:
>
> __rcu_read_unlock()
>    -> rcu_read_unlock_strict()
>         ->rdp = this_cpu_ptr(&rcu_data);
>         ->rdp->cpu_no_qs.b.norm = false;
>
>                  by interrupt and return invoke rcu_core():
>                  ->rcu_check_quiescent_state()
>                       ->rdp = raw_cpu_ptr(&rcu_data);
>                       -> rcu_check_quiescent_state(rdp);
>                             ->note_gp_changes(rdp);
>                                 -> __note_gp_changes(rnp, rdp)
>                                 start new gp
>                                 ->rdp->cpu_no_qs.b.norm = true;
>
>         ->rcu_report_qs_rdp(rdp);
>            ->if (rdp->cpu_no_qs.b.norm || ...)
>
>
> Thanks
> Zqiang
>
>
> >
> > --------
> > SIncerely,
> > Levi.

Re: [PATCH] rcu: remove unnecessary check cpu_no_qs.norm on rcu_report_qs_rdp

[Z qiang]

>
> Hi Z qiang!
>
> Thanks for replying. But I'm pinned on something wrong..!
>
> > For built with CONFIG_RCU_STRICT_GRACE_PERIOD=y and CONFIG_PREEMPT=n kernels
> > Consider the following scenario:
> >
> > __rcu_read_unlock()
> >    -> rcu_read_unlock_strict()

note that as soon as we exit the rcu read critical section
the rcu_read_unlock_strict() will be called, even if the RCU is idle
at this time


> >         ->rdp = this_cpu_ptr(&rcu_data);
> >         ->rdp->cpu_no_qs.b.norm = false;
> >
> >                  by interrupt and return invoke rcu_core():
> >                  ->rcu_check_quiescent_state()
> >                       ->rdp = raw_cpu_ptr(&rcu_data);
> >                       -> rcu_check_quiescent_state(rdp);
> >                             ->note_gp_changes(rdp);
> >                                 -> __note_gp_changes(rnp, rdp)
> >                                 start new gp
> >                                 ->rdp->cpu_no_qs.b.norm = true;
> >
> >         ->rcu_report_qs_rdp(rdp);
> >            ->if (rdp->cpu_no_qs.b.norm || ...)
>
> I've already seen this scenario, But I think something is missing in my view.
> What I couldn't catch is
>
>                   ->rcu_check_quiescent_state()
>                        ->rdp = raw_cpu_ptr(&rcu_data);
>                        -> rcu_check_quiescent_state(rdp);
>                              ->note_gp_changes(rdp);
>                                  -> __note_gp_changes(rnp, rdp)
>                                  start new gp
>
> the new gp is started.
>
>

Maybe this "start new gp"  note misunderstood you.
For built with CONFIG_RCU_STRICT_GRACE_PERIOD=y and CONFIG_PREEMPT=n kernels，
if the gp kthread start a new GP before we exit the RCU read critical section,
and just before we call rcu_report_qs_rdp() in
rcu_read_unlock_strict(), at this time if the clock irq
happens and find that the "rcu_seq_current(&rnp->gp_seq) !=
rdp->gp_seq" is true in rcu_pening(),
will trigger RCU softirq and find that the rcu_seq_new_gp(rdp->gp_seq,
rnp->gp_seq) is true,
will  set rdp->cpu_no_qs.b.norm is true. when we return from the
softirq and call rcu_report_qs_rdp()
in rcu_read_unlock_strict(), find that the rdp->cpu_no_qs.b.norm is true.
so there is a situation where the rdp->cpu_no_qs.b.norm is true.


>
> to set cpu_no_qs.b.norm as true, below condition should be true
>
> 1201 >---if (rcu_seq_new_gp(rdp->gp_seq, rnp->gp_seq) ||
>   1202 >---    unlikely(READ_ONCE(rdp->gpwrap))) {
>
> Here,
> How rcu_seq_new_gp could return true and new gp already started via
> rcu_gp_kthread.
> IIUC, because rcu_gp_fqs_loop couldn't see the root rnp->qsmask is
> zero, it couldn't call rcu_gp_init.
>
>
> Sorry to make noise, but would you correct me what I'm thinking wrong?
>
> Many thanks..!
>
> -----------
> Sincerely,
> Levi.
>
> On Mon, Jul 24, 2023 at 4:21 AM Z qiang <qiang.zhang1211@gmail.com> wrote:
> >
> > >
> > > Thanks for replying to reply Paul :)
> > >
> > > > And try testing with CONFIG_RCU_STRICT_GRACE_PERIOD=y and CONFIG_PREEMPT=n.
> > > > Though there might be better Kconfig options to use.  Those two come
> > > > immediately to mind.
> > >
> > > I've tested with this option via rcu torture.
> > > and it doesn't report any problems.
> > > and after commit 6d60ea03ac2d3 ("rcu: Report QS for outermost
> > > PREEMPT=n rcu_read_unlock() for strict GPs")
> > > it always makes cpu_no_qs.b.norm false whenever it calls
> > > rcu_report_qs_rdp in rcu_read_unlock.
> > >
> > > > But one critical piece is that softirq handlers, including the RCU_SOFTIRQ
> > > > handler rcu_core_si(), can be invoked upon return from interrupts.
> > >
> > > I think in that case, no problem because if it reports qs already,
> > > rcu_check_quiescent_state wouldn't call rcu_report_qs_rdp again.
> > > And if RCU_SOFTIRQ is called as soon as RCU interrupt is finished,
> > > it seems the fastest one to notify qs to related tree.
> > >
> > > > Another critical piece is that if a CPU is idle during any part of a
> > > > grace period, the grace-period kthread can report a quiescent state on
> > > > its behalf.
> > >
> > > I think
> > >     1) If timer interrupt is still programed,
> > >           - when rcu_sched_clock_irq first reports qs, no problem
> > >           - If qs is reported via grace period thread first,
> > > note_gp_chagned in rcu interrupt
> > >             will recognize this situation by setting core_needs_qs as false,
> > >             it doesn't call rcu_report_qs_rdp thou cpu_no_qs.b.norm is true.
> > >
> > >      2) If the timer interrupt isn't programmed,
> > >           - rcu_gp_kthreaad reports its qs, no problem.
> > >
> > > So, I think there's no problem removing
> > >       "rdp->cpu_no_qs.b.norm" check in rcu_report_qs_rdp.
> > > or wrap this condition check as WARN_ON_ONCE.
> > >
> > > > Does that help?
> > > Many thanks always :)
> > >
> >
> >
> > Hi Levi
> >
> > For built with CONFIG_RCU_STRICT_GRACE_PERIOD=y and CONFIG_PREEMPT=n kernels
> > Consider the following scenario:
> >
> > __rcu_read_unlock()
> >    -> rcu_read_unlock_strict()
> >         ->rdp = this_cpu_ptr(&rcu_data);
> >         ->rdp->cpu_no_qs.b.norm = false;
> >
> >                  by interrupt and return invoke rcu_core():
> >                  ->rcu_check_quiescent_state()
> >                       ->rdp = raw_cpu_ptr(&rcu_data);
> >                       -> rcu_check_quiescent_state(rdp);
> >                             ->note_gp_changes(rdp);
> >                                 -> __note_gp_changes(rnp, rdp)
> >                                 start new gp
> >                                 ->rdp->cpu_no_qs.b.norm = true;
> >
> >         ->rcu_report_qs_rdp(rdp);
> >            ->if (rdp->cpu_no_qs.b.norm || ...)
> >
> >
> > Thanks
> > Zqiang
> >
> >
> > >
> > > --------
> > > SIncerely,
> > > Levi.

Re: [PATCH] rcu: remove unnecessary check cpu_no_qs.norm on rcu_report_qs_rdp

[Yun Levi]

Hi, Z qiang.

> Maybe this "start new gp"  note misunderstood you.
> For built with CONFIG_RCU_STRICT_GRACE_PERIOD=y and CONFIG_PREEMPT=n kernels，
> if the gp kthread start a new GP before we exit the RCU read critical section,
> and just before we call rcu_report_qs_rdp() in
> rcu_read_unlock_strict(), at this time if the clock irq
> happens and find that the "rcu_seq_current(&rnp->gp_seq) !=
> rdp->gp_seq" is true in rcu_pening(),
> will trigger RCU softirq and find that the rcu_seq_new_gp(rdp->gp_seq,
> rnp->gp_seq) is true,
> will  set rdp->cpu_no_qs.b.norm is true. when we return from the
> softirq and call rcu_report_qs_rdp()
> in rcu_read_unlock_strict(), find that the rdp->cpu_no_qs.b.norm is true.
> so there is a situation where the rdp->cpu_no_qs.b.norm is true.

Thanks for making me out from my misunderstanding..!
and even the not in STRICT_GRACE_PERIOD,
old grace period's qs could be reported though rcu_gp_kthread start new gp
by first rcu_report_qs_rdp, will be clear soon by continuous rcu_report_qs_rdp
which is triggered by interrupting before the first rcu_report_qs_rdp.

Many thanks..!

And sorry to make noise...!