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

[Chris Metcalf <cmetcalf@ezchip.com>]

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