linux-kernel - Re: [PATCH v8 1/3] smpboot: allow excluding cpus from the smpboot threads

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]

Date:	Thu, 16 Apr 2015 11:50:06 -0400
From:	Chris Metcalf <cmetcalf@...hip.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
CC:	Don Zickus <dzickus@...hat.com>, Ingo Molnar <mingo@...nel.org>,
	"Andrew Morton" <akpm@...ux-foundation.org>,
	Andrew Jones <drjones@...hat.com>,
	"chai wen" <chaiw.fnst@...fujitsu.com>,
	Ulrich Obergfell <uobergfe@...hat.com>,
	Fabian Frederick <fabf@...net.be>,
	Aaron Tomlin <atomlin@...hat.com>,
	"Ben Zhang" <benzh@...omium.org>, Christoph Lameter <cl@...ux.com>,
	"Gilad Ben-Yossef" <gilad@...yossef.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	<linux-kernel@...r.kernel.org>, Jonathan Corbet <corbet@....net>,
	<linux-doc@...r.kernel.org>, Thomas Gleixner <tglx@...utronix.de>,
	"Peter Zijlstra" <peterz@...radead.org>
Subject: Re: [PATCH v8 1/3] smpboot: allow excluding cpus from the smpboot
 threads

On 4/16/2015 11:28 AM, Frederic Weisbecker wrote:
>> +	/* Unpark any threads that were voluntarily parked. */
>> >+	if (ht->cpumask) {
>> >+		cpumask_andnot(&tmp_mask, cpu_online_mask, ht->cpumask);
>> >+		for_each_cpu(cpu, &tmp_mask) {
>> >+			struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu);
>> >+			if (tsk)
>> >+				kthread_unpark(tsk);
>> >+		}
>> >+	}
> Why do you need to do that? smpboot_destroy_threads() doesn't work on parked threads?
> But kthread_stop() does an explicit unparking.

Yes, this part left me scratching my head.  Experimentally, this was necessary.
I saw the unpark in kthread_stop() but it didn't make things work properly.
Currently it looks like parked threads are only in that state while cores are
being offlined, and then they are killed individually, so it seems likely that
this particular path hasn't been tested before.

> +/* Statically allocated and used under smpboot_threads_lock. */
> +static struct cpumask tmp_mask;
> +
> Better allocate the cpumask on need rather than have it resident on memory.
> struct cpumask can be large. Plus we need to worry about locking it.
>

I was trying to avoid the need to make functions return errors for the
extremely unlikely case of ENOMEM.  No one is going to check that error
return in practice anyway; programmers are lazy.  It seemed easy to
allocate one mask statically and use it under the lock; even large systems aren't
likely to burn more than a couple hundred bytes of .bss for this.

But, if you'd prefer using allocation and the error-return model, I can
certainly change the code to do that.

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

--
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/