linux-kernel - Re: [PATCH 02/10] x86/intel_cqm: Modify hot cpu notification handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.11.1506241003120.4037@nanos>
Date:	Wed, 24 Jun 2015 10:14:01 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Vikas Shivappa <vikas.shivappa@...ux.intel.com>
cc:	linux-kernel@...r.kernel.org, x86@...nel.org, hpa@...or.com,
	mingo@...nel.org, tj@...nel.org, peterz@...radead.org,
	matt.fleming@...el.com, will.auld@...el.com,
	glenn.p.williamson@...el.com, kanaka.d.juvva@...el.com,
	priya.v.autee@...el.com
Subject: Re: [PATCH 02/10] x86/intel_cqm: Modify hot cpu notification
 handling

On Tue, 23 Jun 2015, Vikas Shivappa wrote:

> This patch modifies hot cpu notification handling in Intel cache
> monitoring:
> 
>  - to add a new cpu to the cqm_cpumask(which has one cpu per package)
>   during cpu start, it uses the existing package<->core map instead of
>   looping through all cpus in cqm_cpumask.
>  - to search for the next online sibling during cpu exit, it uses the
>  cpumask_any_online_but instead of looping through all online cpus.  In
>  large systems with large number of cpus the time taken to loop may be
>  expensive and also the time increase linearly.

Of course, you forgot to mention that you added the mutex around it
and changed the hotplug logic by moving the calls to different hotplug
events. That part wants to be a seperate patch anyway.

>  static int intel_cqm_cpu_notifier(struct notifier_block *nb,
> @@ -1288,15 +1291,13 @@ static int intel_cqm_cpu_notifier(struct notifier_block *nb,
>  	unsigned int cpu  = (unsigned long)hcpu;
>  
>  	switch (action & ~CPU_TASKS_FROZEN) {
> -	case CPU_UP_PREPARE:
> +	case CPU_ONLINE:
>  		intel_cqm_cpu_prepare(cpu);

Are you sure, that calling intel_cqm_cpu_prepare() from cpu_online is
sufficient? If yes, please document that in the changelog.

> +		cqm_pick_event_reader(cpu);
>  		break;
> -	case CPU_DOWN_PREPARE:
> +	case CPU_DEAD:
>  		intel_cqm_cpu_exit(cpu);

That means, the CPU is still set in cqm_cpumask when the CPU is
disfunctional already. So the cpu calls in intel_cqm_rmid_stabilize(),
intel_cqm_event_count(), intel_cqm_xchg_rmid() will not execute on the
package to which the dead cpu belongs.

Are you sure, that this is correct? If yes, please document WHY!

Thanks,

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