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: <3e4d2594-f678-b77a-4883-0b893daf19f6@bytedance.com>
Date:   Mon, 27 Jun 2022 18:13:23 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.10.0
Subject: Re: [PATCH v4 6/7] sched/fair: skip busy cores in SIS search
Content-Language: en-US
To:     Chen Yu <yu.c.chen@intel.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Mel Gorman <mgorman@suse.de>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Josh Don <joshdon@google.com>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        K Prateek Nayak <kprateek.nayak@amd.com>,
        "Gautham R . Shenoy" <gautham.shenoy@amd.com>,
        linux-kernel@vger.kernel.org
References: <20220619120451.95251-1-wuyun.abel@bytedance.com>
 <20220619120451.95251-7-wuyun.abel@bytedance.com>
 <20220621181442.GA37168@chenyu5-mobl1>
 <543d55e1-fad8-3df3-8bae-d79c0c8d8340@bytedance.com>
 <20220624033032.GA14945@chenyu5-mobl1>
From:   Abel Wu <wuyun.abel@bytedance.com>
In-Reply-To: <20220624033032.GA14945@chenyu5-mobl1>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 6/24/22 11:30 AM, Chen Yu Wrote:
>> ...
>>>> @@ -9273,8 +9319,40 @@ find_idlest_group(struct sched_domain *sd, struct task_struct *p, int this_cpu)
>>>>    static void sd_update_state(struct lb_env *env, struct sd_lb_stats *sds)
>>>>    {
>>>> -	if (sds->sd_state == sd_has_icpus && !test_idle_cpus(env->dst_cpu))
>>>> -		set_idle_cpus(env->dst_cpu, true);
>>>> +	struct sched_domain_shared *sd_smt_shared = env->sd->shared;
>>>> +	enum sd_state new = sds->sd_state;
>>>> +	int this = env->dst_cpu;
>>>> +
>>>> +	/*
>>>> +	 * Parallel updating can hardly contribute accuracy to
>>>> +	 * the filter, besides it can be one of the burdens on
>>>> +	 * cache traffic.
>>>> +	 */
>>>> +	if (cmpxchg(&sd_smt_shared->updating, 0, 1))
>>>> +		return;
>>>> +
>>>> +	/*
>>>> +	 * There is at least one unoccupied cpu available, so
>>>> +	 * propagate it to the filter to avoid false negative
>>>> +	 * issue which could result in lost tracking of some
>>>> +	 * idle cpus thus throughupt downgraded.
>>>> +	 */
>>>> +	if (new != sd_is_busy) {
>>>> +		if (!test_idle_cpus(this))
>>>> +			set_idle_cpus(this, true);
>>>> +	} else {
>>>> +		/*
>>>> +		 * Nothing changes so nothing to update or
>>>> +		 * propagate.
>>>> +		 */
>>>> +		if (sd_smt_shared->state == sd_is_busy)
>>>> +			goto out;
>>>> +	}
>>>> +
>>>> +	sd_update_icpus(this, sds->idle_cpu);
>>> I wonder if we could further enhance it to facilitate idle CPU scan.
>>> For example, can we propagate the idle CPUs in smt domain, to its parent
>>> domain in a hierarchic sequence, and finally to the LLC domain. If there is
>>
>> In fact, it was my first try to cache the unoccupied cpus in SMT
>> shared domain, but the overhead of cpumask ops seems like a major
>> stumbling block.
>>
>>> a cluster domain between SMT and LLC domain, the cluster domain idle CPU filter
>>> could benefit from this mechanism.
>>> https://lore.kernel.org/lkml/20220609120622.47724-3-yangyicong@hisilicon.com/
>>
>> Putting SIS into a hierarchical pattern is good for cache locality.
>> But I don't think multi-level filter is appropriate since it could
>> bring too much cache traffic in SIS,
> Could you please elaborate a little more about the cache traffic? I thought we
> don't save the unoccupied cpus in SMT shared domain, but to store it in middle
> layer shared domain, say, cluster->idle_cpus, this would reduce cache write
> contention compared to writing to llc->idle_cpus directly, because a smaller
> set of CPUs share the idle_cpus filter. Similarly, SIS can only scan the cluster->idle_cpus
> first, without having to query the llc->idle_cpus. This looks like splitting
> a big lock into fine grain small lock.

I'm afraid I didn't quite follow.. Did you mean replace the LLC filter
with multiple cluster filters? Then I agree with what you suggested
that the contention would be reduced. But there are other concerns:

   a. Is it appropriate to fake an intermediate sched_domain if
      cluster level doesn't available? How to identify the proper
      size of the faked sched_domain?

   b. The SIS path might touch more cachelines (multiple cluster
      filters). I'm not sure how much is the impact.

Whatever, this seems worth a try. :)

>> and it could be expected to be
>> a disaster for netperf/tbench or the workloads suffering frequent
>> context switches.
>>
> So this overhead comes from the NEWLY_IDLE case?
> 

Yes, I think it's the main cause to rise the contention to new heights.
But it's also important to keep the filter fresh.

Thanks & BR,
Abel