Date: Fri, 31 Jul 2015 16:19:42 -0700 (PDT)
From: Vikas Shivappa <vikas.shivappa@intel.com>
To: Peter Zijlstra <peterz@infradead.org>
cc: Vikas Shivappa <vikas.shivappa@linux.intel.com>,
        linux-kernel@vger.kernel.org, vikas.shivappa@intel.com, x86@kernel.org,
        hpa@zytor.com, tglx@linutronix.de, mingo@kernel.org, tj@kernel.org,
        Matt Fleming <matt.fleming@intel.com>,
        "Auld, Will" <will.auld@intel.com>,
        "Williamson, Glenn P" <glenn.p.williamson@intel.com>,
        "Juvva, Kanaka D" <kanaka.d.juvva@intel.com>
Subject: Re: [PATCH 1/9] x86/intel_cqm: Modify hot cpu notification
 handling
In-Reply-To: <20150729164416.GA25159@twins.programming.kicks-ass.net>
Message-ID: <alpine.DEB.2.10.1507301145390.921@vshiva-Udesk>
References: <1435789270-27010-1-git-send-email-vikas.shivappa@linux.intel.com> <1435789270-27010-2-git-send-email-vikas.shivappa@linux.intel.com> <20150729164416.GA25159@twins.programming.kicks-ass.net>
User-Agent: Alpine 2.10 (DEB 1266 2009-07-14)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; format=flowed; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1744
Lines: 42


On Wed, 29 Jul 2015, Peter Zijlstra wrote:

> On Wed, Jul 01, 2015 at 03:21:02PM -0700, Vikas Shivappa wrote:
>> +/*
>> + * Temporary cpumask used during hot cpu notificaiton handling. The usage
>> + * is serialized by hot cpu locks.
>> + */
>> +static cpumask_t tmp_cpumask;
>
> So the problem with this is that its 512 bytes on your general distro
> config. And this patch set includes at least 3 of them
>
> So you've just shot 1k5 bytes of .data for no reason.
>
> I know tglx whacked you over the head for this, but is this really worth
> it? I mean, nobody sane should care about hotplug performance, so who
> cares if we iterate a bunch of cpus on the abysmal slow path called
> hotplug.

We did this so that we dont keep looping on every cpu to check if it 
belongs to a particular package. especially the cost being linear with more and 
more cpus getting added and on large systems.
Would it not make sense to use the mask which would tell you all the
cores on a particular core's package ? I realized to use the mask 
topology_core_cpumask only after seeing tglx's pseudo code because the name is 
definitely confusing and earlier I assumed such mask doesnt exist and hence we 
had to just loop through.
I know you pointed out to not put the mask on the stack , but the static usage 
cost should be  reasonable to avoid the cost of looping through all the 
available cpus..

also it doesnt mean we put more crap when we see  crapy code as per tglx as well 
? so why contradict that.

>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/