Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 27 Jun 2019 08:51:52 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     subhra mazumdar <subhra.mazumdar@oracle.com>
cc:     linux-kernel@vger.kernel.org, peterz@infradead.org,
        mingo@redhat.com, steven.sistare@oracle.com,
        dhaval.giani@oracle.com, daniel.lezcano@linaro.org,
        vincent.guittot@linaro.org, viresh.kumar@linaro.org,
        tim.c.chen@linux.intel.com, mgorman@techsingularity.net
Subject: Re: [PATCH v3 6/7] x86/smpboot: introduce per-cpu variable for HT
 siblings
In-Reply-To: <20190627012919.4341-7-subhra.mazumdar@oracle.com>
Message-ID: <alpine.DEB.2.21.1906270844500.32342@nanos.tec.linutronix.de>
References: <20190627012919.4341-1-subhra.mazumdar@oracle.com> <20190627012919.4341-7-subhra.mazumdar@oracle.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 26 Jun 2019, subhra mazumdar wrote:

> Introduce a per-cpu variable to keep the number of HT siblings of a cpu.
> This will be used for quick lookup in select_idle_cpu to determine the
> limits of search.

Why? The number of siblings is constant at least today unless you play
silly cpu hotplug games. A bit more justification for adding yet another
random storage would be appreciated.

> This patch does it only for x86.

# grep 'This patch' Documentation/process/submitting-patches.rst

IOW, we all know already that this is a patch and from the subject prefix
and the diffstat it's pretty obvious that this is x86 only.

So instead of documenting the obvious, please add proper context to justify
the change.
 
> +/* representing number of HT siblings of each CPU */
> +DEFINE_PER_CPU_READ_MOSTLY(unsigned int, cpumask_weight_sibling);
> +EXPORT_PER_CPU_SYMBOL(cpumask_weight_sibling);

Why does this need an export? No module has any reason to access this.

>  /* representing HT and core siblings of each logical CPU */
>  DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_map);
>  EXPORT_PER_CPU_SYMBOL(cpu_core_map);
> @@ -520,6 +524,8 @@ void set_cpu_sibling_map(int cpu)
>  
>  	if (!has_mp) {
>  		cpumask_set_cpu(cpu, topology_sibling_cpumask(cpu));
> +		per_cpu(cpumask_weight_sibling, cpu) =
> +		    cpumask_weight(topology_sibling_cpumask(cpu));
>  		cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu));
>  		cpumask_set_cpu(cpu, topology_core_cpumask(cpu));
>  		c->booted_cores = 1;
> @@ -529,8 +535,12 @@ void set_cpu_sibling_map(int cpu)
>  	for_each_cpu(i, cpu_sibling_setup_mask) {
>  		o = &cpu_data(i);
>  
> -		if ((i == cpu) || (has_smt && match_smt(c, o)))
> +		if ((i == cpu) || (has_smt && match_smt(c, o))) {
>  			link_mask(topology_sibling_cpumask, cpu, i);
> +			threads = cpumask_weight(topology_sibling_cpumask(cpu));
> +			per_cpu(cpumask_weight_sibling, cpu) = threads;
> +			per_cpu(cpumask_weight_sibling, i) = threads;

This only works for SMT=2, but fails to update the rest for SMT=4.

> @@ -1482,6 +1494,8 @@ static void remove_siblinginfo(int cpu)
>  
>  	for_each_cpu(sibling, topology_core_cpumask(cpu)) {
>  		cpumask_clear_cpu(cpu, topology_core_cpumask(sibling));
> +		per_cpu(cpumask_weight_sibling, sibling) =
> +		    cpumask_weight(topology_sibling_cpumask(sibling));

While remove does the right thing.

Thanks,

	tglx