Received-SPF: pass (google.com: domain of linux-kernel+bounces-21171-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <5c357634-c9c5-4796-9f5c-df00a4a5a523@linux.vnet.ibm.com>
Date: Tue, 9 Jan 2024 22:56:46 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 7/8] sched/core: boost/unboost in guest scheduler
Content-Language: en-US
To: "Vineeth Pillai (Google)" <vineeth@bitbyteword.org>,
        Joel Fernandes <joel@joelfernandes.org>
Cc: Suleiman Souhlal <suleiman@google.com>,
        Masami Hiramatsu <mhiramat@google.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, x86@kernel.org,
        Ben Segall
 <bsegall@google.com>, Borislav Petkov <bp@alien8.de>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        "H . Peter Anvin"
 <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>, Mel Gorman <mgorman@suse.de>,
        Paolo Bonzini <pbonzini@redhat.com>, Andy Lutomirski <luto@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Sean Christopherson <seanjc@google.com>,
        Steven Rostedt
 <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Valentin Schneider <vschneid@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        shrikanth hegde <sshegde@linux.vnet.ibm.com>,
        Srikar Dronamraju <srikar@linux.vnet.ibm.com>,
        svaidy@linux.vnet.ibm.com
References: <20231214024727.3503870-1-vineeth@bitbyteword.org>
 <20231214024727.3503870-8-vineeth@bitbyteword.org>
From: Shrikanth Hegde <sshegde@linux.vnet.ibm.com>
In-Reply-To: <20231214024727.3503870-8-vineeth@bitbyteword.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 12/14/23 8:17 AM, Vineeth Pillai (Google) wrote:
> RT or higher priority tasks in guest is considered a critical workload
> and guest scheduler can request boost/unboost on a task switch and/or a
> task wakeup. Also share the preempt status of guest vcpu with the host
> so that host can take decision on boot/unboost.
> 
> CONFIG_TRACE_PREEMPT_TOGGLE is enabled for using the function
> equivalent of preempt_count_{add,sub} to update the shared memory.
> Another option is to update the preempt_count_{add,sub} macros, but
> it will be more code churn and complex.
> 
> Boost request is lazy, but unboost request is synchronous.
> 
> Detect the feature in guest from cpuid flags and use the MSR to pass the
> GPA of memory location for sharing scheduling information.
> 
> Co-developed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> Signed-off-by: Vineeth Pillai (Google) <vineeth@bitbyteword.org>
> ---
>  arch/x86/Kconfig                | 13 +++++
>  arch/x86/include/asm/kvm_para.h |  7 +++
>  arch/x86/kernel/kvm.c           | 16 ++++++
>  include/linux/sched.h           | 21 ++++++++
>  kernel/entry/common.c           |  9 ++++
>  kernel/sched/core.c             | 93 ++++++++++++++++++++++++++++++++-
>  6 files changed, 158 insertions(+), 1 deletion(-)
> 


Wish you all happy new year!
Sorry for the late reply. Took a while to go through this. 

[...]
>  /*
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b47f72b6595f..57f211f1b3d7 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -151,6 +151,71 @@ const_debug unsigned int sysctl_sched_nr_migrate = SCHED_NR_MIGRATE_BREAK;
>  
>  __read_mostly int scheduler_running;
>  
> +#ifdef CONFIG_PARAVIRT_SCHED
> +#include <linux/kvm_para.h>
> +
> +DEFINE_STATIC_KEY_FALSE(__pv_sched_enabled);
> +
> +DEFINE_PER_CPU_DECRYPTED(struct pv_sched_data, pv_sched) __aligned(64);
> +
> +unsigned long pv_sched_pa(void)
> +{
> +	return slow_virt_to_phys(this_cpu_ptr(&pv_sched));
> +}
> +
> +bool pv_sched_vcpu_boosted(void)
> +{
> +	return (this_cpu_read(pv_sched.boost_status) == VCPU_BOOST_BOOSTED);
> +}
> +
> +void pv_sched_boost_vcpu_lazy(void)
> +{
> +	this_cpu_write(pv_sched.schedinfo.boost_req, VCPU_REQ_BOOST);
> +}
> +
> +void pv_sched_unboost_vcpu_lazy(void)
> +{
> +	this_cpu_write(pv_sched.schedinfo.boost_req, VCPU_REQ_UNBOOST);
> +}
> +
> +void pv_sched_boost_vcpu(void)
> +{
> +	pv_sched_boost_vcpu_lazy();
> +	/*
> +	 * XXX: there could be a race between the boost_status check
> +	 *      and hypercall.
> +	 */
> +	if (this_cpu_read(pv_sched.boost_status) == VCPU_BOOST_NORMAL)
> +		kvm_pv_sched_notify_host();
> +}
> +
> +void pv_sched_unboost_vcpu(void)
> +{
> +	pv_sched_unboost_vcpu_lazy();
> +	/*
> +	 * XXX: there could be a race between the boost_status check
> +	 *      and hypercall.
> +	 */
> +	if (this_cpu_read(pv_sched.boost_status) == VCPU_BOOST_BOOSTED &&
> +			!preempt_count())
> +		kvm_pv_sched_notify_host();
> +}
> +
> +/*
> + * Share the preemption enabled/disabled status with host. This will not incur a
> + * VMEXIT and acts as a lazy boost/unboost mechanism - host will check this on
> + * the next VMEXIT for boost/unboost decisions.
> + * XXX: Lazy unboosting may allow cfs tasks to run on RT vcpu till next VMEXIT.
> + */
> +static inline void pv_sched_update_preempt_status(bool preempt_disabled)
> +{
> +	if (pv_sched_enabled())
> +		this_cpu_write(pv_sched.schedinfo.preempt_disabled, preempt_disabled);
> +}
> +#else
> +static inline void pv_sched_update_preempt_status(bool preempt_disabled) {}
> +#endif
> +
>  #ifdef CONFIG_SCHED_CORE
>  

Wouldn't it be better to define a arch hook for this instead? implementation then could 
follow depending on the architecture. This boosting for vcpu tasks in host may be of 
interest to other hypervisors as well. 


Currently I see there are two places where interaction is taking place for paravirt. 
1. steal time accounting 
2. vcpu_is_preempted  
each architecture seems do this in their own way. So one option is an arch hook for 
other paravirt interfaces as well. Other option is probably what was discussed. i.e 
define a framework for paravirt interfaces from the ground up. 


We are working on resource limiting aspect of para virtualization on powerVM. 
Interface for that could be done via hypercall or via VPA (he VPA is a memory structure shared 
between the hypervisor and OS, defined by PAPR). That would be one more paravirt interface. 
As you mentioned in the cover letter, I am curious to know if you have tried the resource limiting, 
since you have overcommit of vCPUs.  

>  DEFINE_STATIC_KEY_FALSE(__sched_core_enabled);
> @@ -2070,6 +2135,19 @@ unsigned long get_wchan(struct task_struct *p)
>  
>  static inline void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
>  {
> +#ifdef CONFIG_PARAVIRT_SCHED
> +	/*
> +	 * TODO: currently request for boosting remote vcpus is not implemented. So
> +	 * we boost only if this enqueue happens for this cpu.
> +	 * This is not a big problem though, target cpu gets an IPI and then gets
> +	 * boosted by the host. Posted interrupts is an exception where target vcpu
> +	 * will not get boosted immediately, but on the next schedule().
> +	 */
> +	if (pv_sched_enabled() && this_rq() == rq &&
> +			sched_class_above(p->sched_class, &fair_sched_class))
> +		pv_sched_boost_vcpu_lazy();
> +#endif
> +
>  	if (!(flags & ENQUEUE_NOCLOCK))
>  		update_rq_clock(rq);
>  
> @@ -5835,6 +5913,8 @@ static inline void preempt_latency_start(int val)
>  #ifdef CONFIG_DEBUG_PREEMPT
>  		current->preempt_disable_ip = ip;
>  #endif
> +		pv_sched_update_preempt_status(true);
> +
>  		trace_preempt_off(CALLER_ADDR0, ip);
>  	}
>  }
> @@ -5867,8 +5947,10 @@ NOKPROBE_SYMBOL(preempt_count_add);
>   */
>  static inline void preempt_latency_stop(int val)
>  {
> -	if (preempt_count() == val)
> +	if (preempt_count() == val) {
> +		pv_sched_update_preempt_status(false);
>  		trace_preempt_on(CALLER_ADDR0, get_lock_parent_ip());
> +	}
>  }
>  
>  void preempt_count_sub(int val)
> @@ -6678,6 +6760,15 @@ static void __sched notrace __schedule(unsigned int sched_mode)
>  	rq->last_seen_need_resched_ns = 0;
>  #endif
>  
> +#ifdef CONFIG_PARAVIRT_SCHED
> +	if (pv_sched_enabled()) {
> +		if (sched_class_above(next->sched_class, &fair_sched_class))
> +			pv_sched_boost_vcpu_lazy();
> +		else if (next->sched_class == &fair_sched_class)
> +			pv_sched_unboost_vcpu();
> +	}
> +#endif
> +
>  	if (likely(prev != next)) {
>  		rq->nr_switches++;
>  		/*