Received-SPF: pass (google.com: domain of linux-kernel+bounces-204811-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 6 Jun 2024 19:32:18 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Ankur Arora <ankur.a.arora@oracle.com>
Cc: linux-kernel@vger.kernel.org, tglx@linutronix.de,
	torvalds@linux-foundation.org, paulmck@kernel.org,
	rostedt@goodmis.org, mark.rutland@arm.com, juri.lelli@redhat.com,
	joel@joelfernandes.org, raghavendra.kt@amd.com,
	sshegde@linux.ibm.com, boris.ostrovsky@oracle.com,
	konrad.wilk@oracle.com, Ingo Molnar <mingo@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>
Subject: Re: [PATCH v2 13/35] sched: allow runtime config for PREEMPT_AUTO
Message-ID: <20240606173218.GH8774@noisy.programming.kicks-ass.net>
References: <20240528003521.979836-1-ankur.a.arora@oracle.com>
 <20240528003521.979836-14-ankur.a.arora@oracle.com>
 <20240528162725.GH26599@noisy.programming.kicks-ass.net>
 <87ttif641i.fsf@oracle.com>
 <20240606115101.GC8774@noisy.programming.kicks-ass.net>
 <87ikymw1g2.fsf@oracle.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87ikymw1g2.fsf@oracle.com>

On Thu, Jun 06, 2024 at 08:11:41AM -0700, Ankur Arora wrote:
> 
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > On Thu, May 30, 2024 at 02:29:45AM -0700, Ankur Arora wrote:
> >>
> >> Peter Zijlstra <peterz@infradead.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.