Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <1ebf2b9d-f496-565c-bc00-4fee9cb11b0b@linux.vnet.ibm.com>
Date:   Wed, 11 Oct 2023 14:33:34 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH] powerpc/paravirt: Improve vcpu_is_preempted
Content-Language: en-US
To:     Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc:     Juergen Gross <jgross@suse.com>, linux-kernel@vger.kernel.org,
        virtualization@lists.linux-foundation.org,
        Ajay Kaher <akaher@vmware.com>,
        Nicholas Piggin <npiggin@gmail.com>,
        Alexey Makhalov <amakhalov@vmware.com>,
        linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
        shrikanth hegde <sshegde@linux.vnet.ibm.com>,
        Michael Ellerman <mpe@ellerman.id.au>
References: <20231009051740.17683-1-srikar@linux.vnet.ibm.com>
From:   Shrikanth Hegde <sshegde@linux.vnet.ibm.com>
In-Reply-To: <20231009051740.17683-1-srikar@linux.vnet.ibm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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. 
> 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?

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. 


> 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 ..." ?

> +	 * 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>