Re: [PATCH] powerpc/paravirt: Improve vcpu_is_preempted

[Srikar Dronamraju]

* Shrikanth Hegde <sshegde@xxxxxxxxxxxxxxxxxx> [2023-10-11 14:33:34]:
> On 10/9/23 10:47 AM, Srikar Dronamraju wrote:
> 
> Hi Srikar. This is an interesting patch. 
> 
> > PowerVM Hypervisor dispatches on a whole core basis. In a shared LPAR, a
> s/whole/big 
> 
> Can we mention that a big core consist of two small cores. and w.r.t
> linux a core is at small core. Hence there is mismatch. 

PowerVM currently always schedules at a Big core granularity. And we would
want to transparent about it even if it changes.

> > CPU from a core that is preempted may have a larger latency. In
> > such a scenario, its preferable to choose a different CPU to run.
> > 
> > If one of the CPUs in the core is active, i.e neither CEDED nor
> > preempted, then consider this CPU as not preempted
> > 
> > Also if any of the CPUs in the core has yielded but OS has not requested
> > CEDE or CONFER, then consider this CPU to be preempted.
> > 
> 
> This is because an idle CPU cannot be preempted. Right?

If a CPU from the same SMT8 core has been preempted, we should consider this CPU
also has been preempted.

> 
> This patch should help address the has_idle_core functionality and ttwu path 
> in powerpc SPLPAR based on powerVM. Currently they are not correct.  
> 
> when the all the CPU's are idle, __update_idle_core will not set has_idle_core
>  which is functionally not right. That is one example, there are other places where correct 
> functionality of vcpu_is_preempted is crucial as well. 
> 

Right, its a crucial from a functionality perspective on shared LPARs.
The Dedicated ones dont have this issue.

> 
> > Cc: Ajay Kaher <akaher@xxxxxxxxxx>
> > Cc: Alexey Makhalov <amakhalov@xxxxxxxxxx>
> > Cc: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> > Cc: Juergen Gross <jgross@xxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: linuxppc-dev@xxxxxxxxxxxxxxxx
> > Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> > Cc: Nicholas Piggin <npiggin@xxxxxxxxx>
> > Cc: virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Srikar Dronamraju <srikar@xxxxxxxxxxxxxxxxxx>
> > ---
> >  arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++++++---
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
> > index e08513d73119..a980756f58df 100644
> > --- a/arch/powerpc/include/asm/paravirt.h
> > +++ b/arch/powerpc/include/asm/paravirt.h
> > @@ -121,9 +121,19 @@ static inline bool vcpu_is_preempted(int cpu)
> >  	if (!is_shared_processor())
> >  		return false;
> > 
> > +	if (!(yield_count_of(cpu) & 1))
> > +		return false;
> > +
> > +	/*
> > +	 * If CPU has yielded but OS has not requested idle then this CPU is
> 
> nit: can it be "if CPU is in hypervisor but OS has not requested ..." ?

Ok, will take it.

> 
> > +	 * definitely preempted.
> > +	 */
> > +	if (!lppaca_of(cpu).idle)
> > +		return true;
> > +
> >  #ifdef CONFIG_PPC_SPLPAR
> >  	if (!is_kvm_guest()) {
> > -		int first_cpu;
> > +		int first_cpu, i;
> > 
> >  		/*
> >  		 * The result of vcpu_is_preempted() is used in a
> > @@ -149,11 +159,28 @@ static inline bool vcpu_is_preempted(int cpu)
> >  		 */
> >  		if (cpu_first_thread_sibling(cpu) == first_cpu)
> >  			return false;
> > +
> > +		/*
> > +		 * If any of the threads of this core is not preempted or
> > +		 * ceded, then consider this CPU to be non-preempted
> > +		 */
> > +		first_cpu = cpu_first_thread_sibling(cpu);
> > +		for (i = first_cpu; i < first_cpu + threads_per_core; i++) {
> > +			if (i == cpu)
> > +				continue;
> > +			if (!(yield_count_of(i) & 1))
> > +				return false;
> > +			if (!lppaca_of(i).idle)
> > +				return true;
> > +		}
> >  	}
> >  #endif
> > 
> > -	if (yield_count_of(cpu) & 1)
> > -		return true;
> > +	/*
> > +	 * None of the threads in this thread group are running but none of
> > +	 * them were preempted too. Hence assume the thread to be
> > +	 * non-preempted.
> > +	 */
> 
> That comment is bit confusing. instead of threads it would be better say CPUs
> 
> "None of the CPUs in this Big Core are running but none of them were preempted too. Hence assume the 
> the CPU to be non-preempted."
> 
> 
> >  	return false;
> >  }
> > 
> 
> Otherwise LGTM
> Reviewed-by: Shrikanth Hegde <sshegde@xxxxxxxxxxxxxxxxxx>


Thanks Shrikanth.

-- 
Thanks and Regards
Srikar Dronamraju