Messages in this thread | | | Date | Wed, 11 Oct 2023 17:46:23 +0530 | From | Srikar Dronamraju <> | Subject | Re: [PATCH] powerpc/paravirt: Improve vcpu_is_preempted |
| |
* Shrikanth Hegde <sshegde@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 <akaher@vmware.com> > > Cc: Alexey Makhalov <amakhalov@vmware.com> > > Cc: Christophe Leroy <christophe.leroy@csgroup.eu> > > Cc: Juergen Gross <jgross@suse.com> > > Cc: linux-kernel@vger.kernel.org > > Cc: linuxppc-dev@lists.ozlabs.org > > Cc: Michael Ellerman <mpe@ellerman.id.au> > > Cc: Nicholas Piggin <npiggin@gmail.com> > > Cc: virtualization@lists.linux-foundation.org > > Signed-off-by: Srikar Dronamraju <srikar@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 <sshegde@linux.vnet.ibm.com>
Thanks Shrikanth.
-- Thanks and Regards Srikar Dronamraju
| |