Messages in this thread | | | Date | Wed, 28 Jan 2009 14:07:25 -0800 | From | Andrew Morton <> | Subject | Re: Buggy IPI and MTRR code on low memory |
| |
On Wed, 28 Jan 2009 16:23:32 -0500 (EST) Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Wed, 28 Jan 2009, Andrew Morton wrote: > > > On Wed, 28 Jan 2009 13:12:02 -0800 > > Andrew Morton <akpm@linux-foundation.org> wrote: > > > > > Thought: do we need to do the kmalloc at all? Perhaps we can instead > > > use a statically allocated per-cpu call_single_data local to > > > kernel/smp.c? It would need a spinlock or something to protect it... > > > > (not a spinlock - get_cpu_var/put_cpu_var will suffice) > > Is that enough? > > The calling IPIs may process the data after smp_call_function is made. > What happens if two smp_call_functions are executed one after the other? > The second one may corrupt the data if the IPI function has not executed > yet. > > We may still need that "RELEASE" flag for that case. >
Good point.
Forget I said that - smp_call_function_single() is calling a function on a *different* CPU, so data which is local to the calling CPU is of course in the wrong place.
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?
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.
| |