Re: [PATCH v2 0/6] RCU changes for PREEMPT_LAZY

[Ankur Arora <ankur.a.arora@oracle.com>]

Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes:

> On 2024-11-06 12:17:52 [-0800], Ankur Arora wrote:
>> This series adds RCU and some leftover scheduler bits for lazy
>> preemption.
>
> This is not critical for the current implementation. The way I
> understand is that you make a change in 3/6 and then all other patches
> in this series are required to deal with this.
>
> For bisect reasons it would make sense to have 3/6 last in the series
> and to the "fixes" first before the code is enabled. I mean if you apply
> 3/6 first then you get build failures without 1/6. But with 3/6 before
> 5/6 you should get runtime errors, right?

That's a good point. Will reorder.

>> The main problem addressed in the RCU related patches is that before
>> PREEMPT_LAZY, PREEMPTION=y implied PREEMPT_RCU=y. With PREEMPT_LAZY,
>> that's no longer true.
>
> No, you want to make PREEMPTION=y + PREEMPT_RCU=n + PREEMPT_LAZY=y
> possible. This is different. Your wording makes it sound like there _is_
> an actual problem.

That's too literal a reading. It's just the problem ("matter or
situation that is unwelcome" to quote from a dictionary) addressed in
the patches.

>> That's because PREEMPT_RCU makes some trade-offs to optimize for
>> latency as opposed to throughput, and configurations with limited
>> preemption might prefer the stronger forward-progress guarantees of
>> PREEMPT_RCU=n.
>>
>> Accordingly, with standalone PREEMPT_LAZY (much like PREEMPT_NONE,
>> PREEMPT_VOLUNTARY) we want to use PREEMPT_RCU=n. And, when used in
>> conjunction with PREEMPT_DYNAMIC, we continue to use PREEMPT_RCU=y.
>>
>> Patches 1 and 2 are cleanup patches:
>>   "rcu: fix header guard for rcu_all_qs()"
>>   "rcu: rename PREEMPT_AUTO to PREEMPT_LAZY"
>>
>> Patch 3, "rcu: limit PREEMPT_RCU configurations", explicitly limits
>> PREEMPT_RCU=y to the PREEMPT_DYNAMIC or the latency oriented models.
>>
>> Patches 4 and 5,
>>   "rcu: handle quiescent states for PREEMPT_RCU=n, PREEMPT_COUNT=y"
>>   "osnoise: handle quiescent states for PREEMPT_RCU=n, PREEMPTION=y"
>>
>> handle quiescent states for the (PREEMPT_LAZY=y, PREEMPT_RCU=n)
>> configuration.
>
> I was briefly thinking about
>
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5646,8 +5646,11 @@ void sched_tick(void)
>  	hw_pressure = arch_scale_hw_pressure(cpu_of(rq));
>  	update_hw_load_avg(rq_clock_task(rq), rq, hw_pressure);
>
> -	if (dynamic_preempt_lazy() && tif_test_bit(TIF_NEED_RESCHED_LAZY))
> +	if (dynamic_preempt_lazy() && tif_test_bit(TIF_NEED_RESCHED_LAZY)) {
>  		resched_curr(rq);
> +		if (!IS_ENABLED(CONFIG_PREEMPT_RCU))
> +			rcu_all_qs();
> +	}
>
>  	donor->sched_class->task_tick(rq, donor, 0);
>  	if (sched_feat(LATENCY_WARN))
>
> which should make #4+ #5 obsolete. But I think it is nicer to have the
> change in #4 since it extends the check to cover all cases. And then
> we would do it twice just for osnoise.

Yeah, exactly. The check here only deals with this specific case
while the one in rcu_flavor_sched_clock_irq() can handle that more
generally.


Thanks.

--
ankur