Messages in this thread | | | Date | Fri, 31 Jul 2015 16:19:42 -0700 (PDT) | From | Vikas Shivappa <> | Subject | Re: [PATCH 1/9] x86/intel_cqm: Modify hot cpu notification handling |
| |
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.
>
| |