Re: [PATCH v6 3/4] sched/fair: Introduce SIS_CORE

From: Abel Wu
Date: Fri Oct 21 2022 - 05:35:48 EST


On 10/21/22 12:34 PM, Chen Yu wrote:
On 2022-10-21 at 12:30:56 +0800, Abel Wu wrote:
Hi Chen, thanks for your reviewing!

On 10/21/22 12:03 PM, Chen Yu wrote:
On 2022-10-19 at 20:28:58 +0800, Abel Wu wrote:
[cut]
A major concern is the accuracy of the idle cpumask. A cpu present
in the mask might not be idle any more, which is called the false
positive cpu. Such cpus will negate lots of benefit this feature
brings. The strategy against the false positives will be introduced
in next patch.

I was thinking that, if patch[3/4] needs [4/4] to fix the false positives,
maybe SIS_CORE could be disabled by default in 3/4 but enabled
in 4/4? So this might facilicate git bisect in case of any regression
check?

Agreed. Will fix in next version.

[cut]
+ * To honor the rule of CORE granule update, set this cpu to the LLC idle
+ * cpumask only if there is no cpu of this core showed up in the cpumask.
+ */
+static void update_idle_cpu(int cpu)
+{
+ struct sched_domain_shared *sds;
+
+ if (!sched_feat(SIS_CORE))
+ return;
+
+ sds = rcu_dereference(per_cpu(sd_llc_shared, cpu));
+ if (sds) {
+ struct cpumask *icpus = to_cpumask(sds->icpus);
+
+ /*
+ * This is racy against clearing in select_idle_cpu(),
+ * and can lead to idle cpus miss the chance to be set to
+ * the idle cpumask, thus the idle cpus are temporarily
+ * out of reach in SIS domain scan. But it should be rare
+ * and we still have ILB to kick them working.
+ */
+ if (!cpumask_intersects(cpu_smt_mask(cpu), icpus))
+ cpumask_set_cpu(cpu, icpus);
Maybe I miss something, here we only set one CPU in the icpus, but
when we reach update_idle_cpu(), all SMT siblings of 'cpu' are idle,
is this intended for 'CORE granule update'?

The __update_idle_core() is called by all the cpus that need to go idle
to update has_idle_core if necessary, and update_idle_cpu() is called
before that check.

I see.

Since __update_idle_core() has checked all SMT siblings of 'cpu' if
they are idle, can that information also be updated to icpus?

I think this will simply fallback to the original per-cpu proposal and
lose the opportunity to spread tasks to different cores.