Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ddf05d4f0f1ffb1a3ff3076e01cc9752a9fd33a8.camel@linux.intel.com>
Subject: Re: [PATCH v3] sched/fair: filter out overloaded cpus in SIS
From:   Tim Chen <tim.c.chen@linux.intel.com>
To:     Abel Wu <wuyun.abel@bytedance.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Mel Gorman <mgorman@suse.de>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Josh Don <joshdon@google.com>
Cc:     linux-kernel@vger.kernel.org
Date:   Thu, 19 May 2022 15:16:31 -0700
In-Reply-To: <20220505122331.42696-1-wuyun.abel@bytedance.com>
References: <20220505122331.42696-1-wuyun.abel@bytedance.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.34.4 (3.34.4-1.fc31) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2022-05-05 at 20:23 +0800, Abel Wu wrote:
> 
> Signed-off-by: Abel Wu <wuyun.abel@bytedance.com>
> ---
>  include/linux/sched/topology.h | 12 ++++++++++
>  kernel/sched/core.c            | 38 ++++++++++++++++++++++++++++++
>  kernel/sched/fair.c            | 43 +++++++++++++++++++++++++++-------
>  kernel/sched/idle.c            |  1 +
>  kernel/sched/sched.h           |  4 ++++
>  kernel/sched/topology.c        |  4 +++-
>  6 files changed, 92 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
> index 56cffe42abbc..95c7ad1e05b5 100644
> --- a/include/linux/sched/topology.h
> +++ b/include/linux/sched/topology.h
> @@ -81,8 +81,20 @@ struct sched_domain_shared {
>  	atomic_t	ref;
>  	atomic_t	nr_busy_cpus;
>  	int		has_idle_cores;
> +
> +	/*
> +	 * Tracking of the overloaded cpus can be heavy, so start
> +	 * a new cacheline to avoid false sharing.
> +	 */
> +	atomic_t	nr_overloaded_cpus ____cacheline_aligned;

Abel,

This is nice work. I have one comment.

The update and reading of nr_overloaded_cpus will incur cache bouncing cost.
As far as I can tell, this counter is used to determine if we should bail out
from the search for an idle CPUs if the system is heavily loaded.  And I
hope we can avoid using atomic counter in these heavily used scheduler paths.  
The logic to filter overloaded CPUs only need overloaded_cpus[] 
mask and not the nr_overloaded_cpus counter.

So I recommend that you break out the logic of using nr_overloaded_cpus
atomic counter to detect LLC has heavy load as a second patch so that
it can be evaluated on its own merit.

That functionality overlaps with Chen Yu's patch to limit search depending
on load, so it will be easier to compare the two approaches if it is separated.

Otherwise, the logic in the patch to use overloaded_cpus[]
mask to filter out the overloaded cpus looks fine and complements
Chen Yu's patch.

Thanks.

Tim

> +	unsigned long	overloaded_cpus[]; /* Must be last */ };
>  
>