* Shrikanth Hegde <sshe...@linux.vnet.ibm.com> [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 <aka...@vmware.com>
> > Cc: Alexey Makhalov <amakha...@vmware.com>
> > Cc: Christophe Leroy <christophe.le...@csgroup.eu>
> > Cc: Juergen Gross <jgr...@suse.com>
> > Cc: linux-ker...@vger.kernel.org
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: Michael Ellerman <m...@ellerman.id.au>
> > Cc: Nicholas Piggin <npig...@gmail.com>
> > Cc: virtualizat...@lists.linux-foundation.org
> > Signed-off-by: Srikar Dronamraju <sri...@linux.vnet.ibm.com>
> > ---
> > 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 <sshe...@linux.vnet.ibm.com>
Thanks Shrikanth.
--
Thanks and Regards
Srikar Dronamraju
