Re: [PATCH] generic-smp: remove kmalloc()

Peter Zijlstra
2009-02-17T17:41:19Z

On Tue, 2009-02-17 at 18:21 +0100, Oleg Nesterov wrote:
> On 02/17, Peter Zijlstra wrote:
> >
> > Ok, so this is on top of Nick's cleanup from earlier today, and folds
> > everything.
> >
> > No more RCU games as the storage for per-cpu entries is permanent - cpu
> > hotplug should be good because it does a synchronize_sched().
> >
> > What we do play games with is the global list, we can extract entries
> > and place them to the front while its being observed. This means that
> > the list iteration can see some entries twice (not a problem since we
> > remove ourselves from the cpumask), but cannot miss entries.
>
> I think this all is correct.

*phew* :-)

> But I am wondering, don't we have another problem. Before this patch,
> smp_call_function_many(wait => 0) always succeeds, no matter which
> locks the caller holds.
>
> After this patch we can deadlock, csd_lock() can spin forever if the
> caller shares the lock with another func in flight.
>
> IOW,
> void func(void *arg)
> {
> lock(LOCK);
> unlock(LOCK);
> }
>
> CPU 0 does:
>
> smp_call_function(func, NULL, 0);
> lock(LOCK);
> smp_call_function(another_func, NULL, 0);
> unlock(LOCK);
>
> If CPU 0 takes LOCK before CPU 1 calls func, the 2nd smp_call_function()
> hangs in csd_lock().
>
> I am not sure this is the real problem (even if I am right), perhaps
> the answer is "don't do that".
>
> But, otoh, afaics we can tweak generic_smp_call_function_interrupt()
> a bit to avoid this problem. Something like
>
> list_for_each_entry_rcu(data, &call_function.queue, csd.list) {
> void (*func)(void *);
> void *info;
> int refs;
>
> spin_lock(&data->lock);
> if (!cpumask_test_cpu(cpu, data->cpumask)) {
> spin_unlock(&data->lock);
> continue;
> }
> cpumask_clear_cpu(cpu, data->cpumask);
> WARN_ON(data->refs == 0);
> refs = --data->refs;
> func = data->csd.func;
> info = data->csd.info;
> wait = (data->flags & CSD_FLAG_WAIT);
> spin_unlock(&data->lock);
>
> if (!refs) {
> spin_lock(&call_function.lock);
> list_del_rcu(&data->csd.list);
> spin_unlock(&call_function.lock);
> csd_unlock(&data->csd);
> }
>
> func(info);
> if (!refs && wait)
> csd_complete(&data->csd);
> }
>
> I am afraid I missed something, and the code above looks wrong
> because it does csd_unlock() first, then csd_complete().

That does look a bit weird, but

> But if wait == T, then nobody can reuse this per-cpu entry, the
> caller of smp_call_function_many() must spin in csd_wait() on
> the same CPU.

is indeed correct.

> What do you think?

While I would say, don't do that to your deadlock scenario, I do like
the extra freedom this provides, so I'm inclined to go with this. Let me
spin a new patch and build a kernel with it ;-)