Re: [PATCH v2 16/35] preempt,rcu: warn on PREEMPT_RCU=n, preempt=full

[Paul E. McKenney]

On Thu, May 30, 2024 at 04:04:41PM -0700, Ankur Arora wrote:
> 
> Peter Zijlstra <peterz@xxxxxxxxxxxxx> writes:
> 
> > On Mon, May 27, 2024 at 05:35:02PM -0700, Ankur Arora wrote:
> >> The combination of PREEMPT_RCU=n and (PREEMPT_AUTO=y, preempt=full)
> >> works at cross purposes: the RCU read side critical sections disable
> >> preemption, while preempt=full schedules eagerly to minimize
> >> latency.
> >>
> >> Warn if the user is switching to full preemption with PREEMPT_RCU=n.
> >>
> >> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> >> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> >> Cc: Juri Lelli <juri.lelli@xxxxxxxxxx>
> >> Cc: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
> >> Suggested-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> >> Link: https://lore.kernel.org/lkml/842f589e-5ea3-4c2b-9376-d718c14fabf5@paulmck-laptop/
> >> Signed-off-by: Ankur Arora <ankur.a.arora@xxxxxxxxxx>
> >> ---
> >>  kernel/sched/core.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> >> index d7804e29182d..df8e333f2d8b 100644
> >> --- a/kernel/sched/core.c
> >> +++ b/kernel/sched/core.c
> >> @@ -8943,6 +8943,10 @@ static void __sched_dynamic_update(int mode)
> >>  		break;
> >>
> >>  	case preempt_dynamic_full:
> >> +		if (!IS_ENABLED(CONFIG_PREEMPT_RCU))
> >> +			pr_warn("%s: preempt=full is not recommended with CONFIG_PREEMPT_RCU=n",
> >> +				PREEMPT_MODE);
> >> +
> >
> > Yeah, so I don't believe this is a viable strategy.
> >
> > Firstly, none of these RCU patches are actually about the whole LAZY
> > preempt scheme, they apply equally well (arguably better) to the
> > existing PREEMPT_DYNAMIC thing.
> 
> Agreed.
> 
> > Secondly, esp. with the LAZY thing, you are effectively running FULL at
> > all times. It's just that some of the preemptions, typically those of
> > the normal scheduling class are somewhat delayed. However RT/DL classes
> > are still insta preempt.
> 
> Also, agreed.
> 
> > Meaning that if you run anything in the realtime classes you're running
> > a fully preemptible kernel. As such, RCU had better be able to deal with
> > it.
> 
> So, RCU can deal with (PREEMPT_RCU=y, PREEMPT_AUTO=y, preempt=none/voluntary/full).
> Since that's basically what PREEMPT_DYNAMIC already works with.
> 
> The other combination, (PREEMPT_RCU=n, PREEMPT_AUTO,
> preempt=none/voluntary) would generally be business as usual, except, as
> you say, it is really PREEMPT_RCU=n, preempt=full in disguise.
> 
> However, as Paul says __rcu_read_lock(), for PREEMPT_RCU=n is defined as:
> 
> static inline void __rcu_read_lock(void)
> {
>         preempt_disable();
> }
> 
> So, this combination -- though non standard -- should also work.
> 
> The reason for adding the warning was because Paul had warned in
> discussions earlier (see here for instance:
> https://lore.kernel.org/lkml/842f589e-5ea3-4c2b-9376-d718c14fabf5@paulmck-laptop/)
> 
> that the PREEMPT_FULL=y and PREEMPT_RCU=n is basically useless. But at
> least in my understanding that's primarily a performance concern not a
> correctness concern. But, Paul can probably speak to that more.
> 
>   "PREEMPT_FULL=y plus PREEMPT_RCU=n appears to be a useless
>   combination.  All of the gains from PREEMPT_FULL=y are more than lost
>   due to PREEMPT_RCU=n, especially when the kernel decides to do something
>   like walk a long task list under RCU protection.  We should not waste
>   people's time getting burned by this combination, nor should we waste
>   cycles testing it."

My selfish motivation here is to avoid testing this combination unless
and until someone actually has a good use for it.  I do not think that
anyone will ever need it, but perhaps I am suffering from a failure
of imagination.  If so, they hit that WARN, complain and explain their
use case, and at that point I start testing it (and fixing whatever bugs
have accumulated in the meantime).  But until that time, I save time by
avoiding testing it.

							Thanx, Paul