Re: [PATCH 00/30] PREEMPT_AUTO: support lazy rescheduling

[Shrikanth Hegde <sshegde@linux.ibm.com>]

On 5/8/24 10:48 AM, Ankur Arora wrote:
> 
> Shrikanth Hegde <sshegde@linux.ibm.com> writes:
> 
>> On 4/27/24 12:30 AM, Ankur Arora wrote:
>>>
>>
>> Hi Ankur,
>>
>> Sorry for the delay, I was on leave last week.
>> There might be a delay in response as I am recovering from fever.
> 
> Please take your your time.
> 
>>> Great. I'm guessing these tests are when running in voluntary preemption
>>> mode (under PREEMPT_AUTO).
>>>
>>
>> It was run under preempt=none.
>>
>>> If you haven't, could you also try full preemption? There you should see
>>> identical results unless something is horribly wrong.
>>
>> I tried preempt=full with patch you provided below. ran the hackbench for much longer
>> with 100000 loops. I don't see any regression on the larger system.
>> I see slight improvement in some cases.  I dont see any major regression with 10k ops
>> which was tried earlier as well.
> 
> Great, so no surprises with preempt=full.

Right. 

> 
>> ==========================================================
>> 1L ops.
>> ==========================================================
>> Process 10 groups          :       9.85,       9.87(-0.20)
>> Process 20 groups          :      17.69,      17.32(2.09)
>> Process 30 groups          :      25.89,      25.96(-0.27)
>> Process 40 groups          :      34.70,      34.61(0.26)
>> Process 50 groups          :      44.02,      43.79(0.52)
>> Process 60 groups          :      52.72,      52.10(1.18)
>> Thread  10 groups          :      10.50,      10.52(-0.19)
>> Thread  20 groups          :      18.79,      18.60(1.01)
>> Process(Pipe) 10 groups    :      10.39,      10.37(0.19)
>> Process(Pipe) 20 groups    :      18.45,      18.54(-0.49)
>> Process(Pipe) 30 groups    :      25.63,      25.92(-1.13)
>> Process(Pipe) 40 groups    :      33.79,      33.49(0.89)
>> Process(Pipe) 50 groups    :      43.15,      41.83(3.06)
>> Process(Pipe) 60 groups    :      51.94,      50.32(3.12)
>> Thread(Pipe)  10 groups    :      10.73,      10.85(-1.12)
>> Thread(Pipe)  20 groups    :      19.24,      19.35(-0.57)
> 
> I presume the ones on the left are the baseline numbers with
> PREEMPT_AUTO on the right?
> 
> Also, these are with preempt=none/voluntary/full?
> 
One on the left is 6.9 baseline (preempt=full) and one on the right is preempt auto. (preempt=full)
These numbers are with preempt=full. 


>>>> However, I still see 20-50%
>>>> regression on the larger system(320 CPUS). I will continue to debug why.
>>>
>>> Could you try this patch? This is needed because PREEMPT_AUTO turns on
>>> CONFIG_PREEMPTION, but not CONFIG_PREEMPT:
> 
> Just wanted to check if the regression you were seeing with preempt=none
> is fixed?

I ran the tests again today with this patch for preempt=none. 
Unfortunately, the numbers indicate that there is regression with preempt=none on the 
large system. I will continue to debug why.

				6.9 		preempt_auto
				preempt=none	preempt=none

Process 10 groups          :       9.66,       9.75(-0.93)
Process 20 groups          :      16.82,      17.21(-2.32)
Process 30 groups          :      24.56,      25.51(-3.87)
Process 40 groups          :      31.79,      33.99(-6.92)
Process 50 groups          :      39.55,      42.85(-8.34)
Process 60 groups          :      47.39,      51.34(-8.34)
thread 10 groups           :      10.21,      10.36(-1.47)
thread 20 groups           :      18.00,      18.39(-2.17)
Process(Pipe) 10 groups    :       8.78,       8.71(0.80)
Process(Pipe) 20 groups    :      12.19,      13.42(-10.09)
Process(Pipe) 30 groups    :      15.82,      18.32(-15.80)
Process(Pipe) 40 groups    :      19.15,      22.04(-15.09)
Process(Pipe) 50 groups    :      22.23,      27.40(-23.26)
Process(Pipe) 60 groups    :      25.35,      31.37(-23.75)
thread(Pipe) 10 groups     :       8.84,       8.72(1.36)
thread(Pipe) 20 groups     :      12.90,      13.45(-4.26)



> 
>> This patch can be considered as the enablement patch for Powerpc for preempt_auto.
>> Michael, Nick, Do you see any concerns?
>>
>> Ankur, Could you please add this patch, if there are no objections.
> 
> Of course. Thanks for the patch.
> 
> The patch overall looks good. A minor comment below.
> 
>  Will add it to v2.

FWIW, we can add this support for PowerPC and fix the issues along the way. 

> 
>> ---
>> From 878a5a7c990e3459758a5d19d7697b07d8d27d0f Mon Sep 17 00:00:00 2001
>> From: Shrikanth Hegde <sshegde@linux.ibm.com>
>> Date: Tue, 7 May 2024 04:42:04 -0500
>> Subject: [PATCH] powerpc: add support for preempt_auto
>>
>> Add PowerPC arch support for PREEMPT_AUTO by defining LAZY bits.
>>
>> Since PowerPC doesn't use generic exit to functions, Add
>> NR_LAZY check in exit to user and exit to kernel from interrupt
>> routines.
>>
>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> ---
>>  arch/powerpc/Kconfig                   |  1 +
>>  arch/powerpc/include/asm/thread_info.h | 11 ++++++++++-
>>  arch/powerpc/kernel/interrupt.c        |  6 ++++--
>>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> ...
> 
>> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
>> index eca293794a1e..0c0b7010995a 100644
>> --- a/arch/powerpc/kernel/interrupt.c
>> +++ b/arch/powerpc/kernel/interrupt.c
>> @@ -185,7 +185,7 @@ interrupt_exit_user_prepare_main(unsigned long ret, struct pt_regs *regs)
>>  	ti_flags = read_thread_flags();
>>  	while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
>>  		local_irq_enable();
>> -		if (ti_flags & _TIF_NEED_RESCHED) {
>> +		if (ti_flags & (_TIF_NEED_RESCHED | _TIF_NEED_RESCHED_LAZY)) {
>>  			schedule();
>>  		} else {
>>  			/*
>> @@ -396,7 +396,9 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs)
>>  		/* Returning to a kernel context with local irqs enabled. */
>>  		WARN_ON_ONCE(!(regs->msr & MSR_EE));
>>  again:
>> -		if (IS_ENABLED(CONFIG_PREEMPT)) {
>> +
>> +		if ((IS_ENABLED(CONFIG_PREEMPT_AUTO) && IS_ENABLED(CONFIG_PREEMPTION)) ||
>> +		    (!IS_ENABLED(CONFIG_PREEMPT_AUTO) && IS_ENABLED(CONFIG_PREEMPT))) {
> 
> Seems to me, this could just be:
> 
>         if (IS_EANBLED(CONFIG_PREEMPTION))

You are right, that should be fine. Since CONFIG_PREEMPTION is enabled 
iff preempt=full or preempt_auto. So its the same thing. 

Maybe putting a comment around it to just describe is good enough. 

> 
> But, maybe the intent is to exclude support for, say
> (PREEMPT_DYNAMIC && PREEMPT_NONE), then maybe it is best to explicitly
> encode that?
> 
> Alternately, a comment on the disparate treatment for PREEMPT_AUTO and
> !PREEMPT_AUTO might be helpful.
> 
> 
> Thanks
> Ankur
> 
>>  			/* Return to preemptible kernel context */
>>  			if (unlikely(read_thread_flags() & _TIF_NEED_RESCHED)) {
>>  				if (preempt_count() == 0)