Date: Wed, 12 Jun 2024 11:10:54 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Ankur Arora <ankur.a.arora@...cle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, linux-kernel@...r.kernel.org,
	tglx@...utronix.de, torvalds@...ux-foundation.org,
	rostedt@...dmis.org, mark.rutland@....com, juri.lelli@...hat.com,
	joel@...lfernandes.org, raghavendra.kt@....com,
	sshegde@...ux.ibm.com, boris.ostrovsky@...cle.com,
	konrad.wilk@...cle.com, Ingo Molnar <mingo@...hat.com>,
	Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH v2 13/35] sched: allow runtime config for PREEMPT_AUTO

On Sat, Jun 08, 2024 at 05:46:26PM -0700, Ankur Arora wrote:
> Peter Zijlstra <peterz@...radead.org> writes:
> > On Thu, Jun 06, 2024 at 08:11:41AM -0700, Ankur Arora wrote:
> >> Peter Zijlstra <peterz@...radead.org> writes:
> >> > On Thu, May 30, 2024 at 02:29:45AM -0700, Ankur Arora wrote:
> >> >> Peter Zijlstra <peterz@...radead.org> writes:
> >> >> > On Mon, May 27, 2024 at 05:34:59PM -0700, Ankur Arora wrote:
> >> >> >> Reuse sched_dynamic_update() and related logic to enable choosing
> >> >> >> the preemption model at boot or runtime for PREEMPT_AUTO.
> >> >> >>
> >> >> >> The interface is identical to PREEMPT_DYNAMIC.
> >> >> >
> >> >> > Colour me confused, why?!? What are you doing and why aren't just just
> >> >> > adding AUTO to the existing DYNAMIC thing?
> >> >>
> >> >> You mean have a single __sched_dynamic_update()? AUTO doesn't use any
> >> >> of the static_call/static_key stuff so I'm not sure how that would work.
> >> >
> >> > *sigh*... see the below, seems to work.
> >>
> >> Sorry, didn't mean for you to have to do all that work to prove the
> >> point.
> >
> > Well, for a large part it was needed for me to figure out what your
> > patches were actually doing anyway. Peel away all the layers and this is
> > what remains.
> >
> >> I phrased it badly. I do understand how lazy can be folded in as
> >> you do here:
> >>
> >> > +	case preempt_dynamic_lazy:
> >> > +		if (!klp_override)
> >> > +			preempt_dynamic_disable(cond_resched);
> >> > +		preempt_dynamic_disable(might_resched);
> >> > +		preempt_dynamic_enable(preempt_schedule);
> >> > +		preempt_dynamic_enable(preempt_schedule_notrace);
> >> > +		preempt_dynamic_enable(irqentry_exit_cond_resched);
> >> > +		preempt_dynamic_key_enable(preempt_lazy);
> >> > +		if (mode != preempt_dynamic_mode)
> >> > +			pr_info("Dynamic Preempt: lazy\n");
> >> > +		break;
> >> >  	}
> >>
> >> But, if the long term goal (at least as I understand it) is to get rid
> >> of cond_resched() -- to allow optimizations that needing to call cond_resched()
> >> makes impossible -- does it make sense to pull all of these together?
> >
> > It certainly doesn't make sense to add yet another configurable thing. We
> > have one, so yes add it here.
> >
> >> Say, eventually preempt_dynamic_lazy and preempt_dynamic_full are the
> >> only two models left. Then we will have (modulo figuring out how to
> >> switch over klp from cond_resched() to a different unwinding technique):
> >>
> >> static void __sched_dynamic_update(int mode)
> >> {
> >>         preempt_dynamic_enable(preempt_schedule);
> >>         preempt_dynamic_enable(preempt_schedule_notrace);
> >>         preempt_dynamic_enable(irqentry_exit_cond_resched);
> >>
> >>         switch (mode) {
> >>         case preempt_dynamic_full:
> >>                 preempt_dynamic_key_disable(preempt_lazy);
> >>                 if (mode != preempt_dynamic_mode)
> >>                         pr_info("%s: full\n", PREEMPT_MODE);
> >>                 break;
> >>
> >> 	case preempt_dynamic_lazy:
> >> 		preempt_dynamic_key_enable(preempt_lazy);
> >> 		if (mode != preempt_dynamic_mode)
> >> 			pr_info("Dynamic Preempt: lazy\n");
> >> 		break;
> >>         }
> >>
> >>         preempt_dynamic_mode = mode;
> >> }
> >>
> >> Which is pretty similar to what the PREEMPT_AUTO code was doing.
> >
> > Right, but without duplicating all that stuff in the interim.
> 
> Yeah, that makes sense. Joel had suggested something on these lines
> earlier [1], to which I was resistant.
> 
> However, the duplication (and the fact that the voluntary model
> was quite thin) should have told me that (AUTO, preempt=voluntary)
> should just be folded under PREEMPT_DYNAMIC.
> 
> I'll rework the series to do that.
> 
> That should also simplify RCU related choices which I think Paul will
> like. Given that the lazy model is meant to eventually replace
> none/voluntary, so PREEMPT_RCU configuration can just be:
> 
> --- a/kernel/rcu/Kconfig
> +++ b/kernel/rcu/Kconfig
> @@ -18,7 +18,7 @@ config TREE_RCU
> 
>  config PREEMPT_RCU
>         bool
> -       default y if PREEMPTION
> +       default y if PREEMPTION && !PREEMPT_LAZY

Given that PREEMPT_DYNAMIC selects PREEMPT_BUILD which in turn selects
PREEMPTION, this should work.

> Or, maybe we should instead have this:
> 
> --- a/kernel/rcu/Kconfig
> +++ b/kernel/rcu/Kconfig
> @@ -18,7 +18,7 @@ config TREE_RCU
> 
>  config PREEMPT_RCU
>         bool
> -       default y if PREEMPTION
> +       default y if PREEMPT || PREEMPT_RT
>         select TREE_RCU
> 
> Though this would be a change in behaviour for current PREEMPT_DYNAMIC
> users.

Which I believe to be a no-go.  I believe that PREEMPT_DYNAMIC users
really need their preemptible kernels to include preemptible RCU.

If PREEMPT_LAZY causes PREEMPT_DYNAMIC non-preemptible kernels to become
lazily preemptible, that is a topic to discuss with PREEMPT_DYNAMIC users.
On the other hand, if PREEMPT_LAZY does not cause PREEMPT_DYNAMIC
kernels to become lazily preemptible, then I would expect there to be
hard questions about removing cond_resched() and might_sleep(), or,
for that matter changing their semantics.  Which I again must leave
to PREEMPT_DYNAMIC users.

							Thanx, Paul

> [1] https://lore.kernel.org/lkml/fd48ea5c-bc74-4914-a621-d12c9741c014@joelfernandes.org/
> 
> Thanks
> --
> ankur

Powered by blists - more mailing lists