Re: Buggy IPI and MTRR code on low memory

[Andrew Morton]

On Wed, 28 Jan 2009 17:47:55 -0500 (EST)
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:

> 
> On Wed, 28 Jan 2009, Andrew Morton wrote:
> > 
> > So if we're going to use per-cpu data then we'd need to protect it with
> > a lock.  We could (should?) have a separate lock for each destination
> > CPU.
> > 
> > We could make smp_call_function_single() block until the IPI handler
> > has consumed the call_single_data, in which case we might as well put
> > the call_single_data, onto the caller's stack, as you've done.
> > 
> > Or we could take the per-cpu spinlock in smp_call_function_single(),
> > and release it in the IPI handler, after the call_single_data has been
> > consumed, which is a bit more efficient.  But I have a suspicion that
> > this is AB/BA deadlockable.
> > 
> > <tries to think of any scenarios>
> > 
> > <fails>
> > 
> > So we have
> > 
> > smp_call_function_single(int cpu)
> > {
> > 	spin_lock(per_cpu(cpu, locks));
> > 	per_cpu(cpu, call_single_data) = <stuff>
> > 	send_ipi(cpu);
> > 	return;
> > }
> > 
> > ipi_handler(...)
> > {
> > 	int cpu = smp_processor_id();
> > 	call_single_data csd = per_cpu(cpu, call_single_data);
> > 
> > 	spin_unlock(per_cpu(cpu, locks));
> > 	use(csd);
> > }
> > 
> > does that work?
> 
> I don't think so. With ticket spinlocks and such, that just looks like it 
> is destine to crash. Also, spinlocks disable preemption, and we would need 
> to enable it again otherwise we have a dangling preempt_disable.

Well that sucks.

I think the model of taking a lock, passing the data across to another
thread of control and having that thread of control release the lock
once the data has been consumed is a good one.  It's faster and in this
case it means that we can now (maybe) perform (wait=0) IPI calls with local
interrupts disabled, which wasn't the case before.

It's a shame that irrelevant-other-stuff prevents us from implementing
such a thing.  Of course, we can still do it, via some ad-hoc locking
thing using raw_spin_lock/atomic_t's/bitops/cpu_relax/etc.

> > 
> > Dunno if it's any better than what you have now.  It does however
> > remove the unpleasant "try kmalloc and if that failed, try something
> > else" mess.
> 
> We could have a percpu for single data and still use the lock. Keep the 
> method of the IPI handler signaling to the caller that they copied it to 
> their stack. Then the caller could release the lock.

Yeah, spose so.  That separates the signalling from the locking, rather
than using the lock as the signal.  But it's an inferior solution
because it a) makes the caller hang around until the consumer has
executed and it b) requires that callers still have local interrupts
enabled.

Now, maybe we can never implement b) at all - maybe we'll still require
that callers have local interrupts enabled.  Because there might be
arch_send_call_function_single_ipi() which has to loop around until it
sees that the target CPU has accepted the interrupt, dunno.

> We can keep this only for the non wait case. Most users set the wait bit, 
> so it will only slow down the non waiters.

It would be good to avoid separate sync and async code paths as much
as possible, of course.

> Of course this will only work with the single cpu function callers. I 
> think the all cpu function callers still need to alloc.

I haven't really thought about the smp_call_function_many()
common case.  
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/