Re: [PATCH 1/10] Add generic helpers for arch IPI function calls

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Fri, May 02, 2008 at 07:21:39AM -0700, Paul E. McKenney wrote:
> On Fri, May 02, 2008 at 02:59:29PM +0200, Peter Zijlstra wrote:
> > On Fri, 2008-05-02 at 05:42 -0700, Paul E. McKenney wrote:
> > 
> > > And here is one scenario that makes me doubt that my imagination is
> > > faulty:
> > > 
> > > 1.	CPU 0 disables irqs.
> > > 
> > > 2.	CPU 1 disables irqs.
> > > 
> > > 3.	CPU 0 invokes smp_call_function().  But CPU 1 will never respond
> > > 	because its irqs are disabled.
> > > 
> > > 4.	CPU 1 invokes smp_call_function().  But CPU 0 will never respond
> > > 	because its irqs are disabled.
> > > 
> > > Looks like inherent deadlock to me, requiring that smp_call_function()
> > > be invoked with irqs enabled.
> > > 
> > > So, what am I missing here?
> > 
> > The wish to do it anyway ;-)
> > 
> > I can imagine some situations where I'd like to try anyway and fall back
> > to a slower path when failing.
> > 
> > With the initial design we would simply allocate data, stick it on the
> > queue and call the ipi (when needed).
> > 
> > This is perfectly deadlock free when wait=0 and it just returns -ENOMEM
> > on allocation failure.
> > 
> > It it doesn't return -ENOMEM I know its been queued and will be
> > processed at some point, if it does fail, I can deal with it in another
> > way.
> > 
> > I know I'd like to do that and I suspect Nick has a few use cases up his
> > sleeve as well.
> 
> OK, so one approach would be to check for irqs being disabled,
> perhaps as follows, on top of my previous patch:
> 
> 		struct call_single_data *data = NULL;
> 
> 		if (!wait) {
> 			data = kmalloc(sizeof(*data), GFP_ATOMIC);
> 			if (data)
> 				data->flags = CSD_FLAG_ALLOC;
> 		}
> 		if (!data) {
> 			if (unlikely(irqs_disabled())) {
> 				put_cpu();
> 				return -ENOMEM;
> 			}
> 			data = &d;
> 			data->flags = CSD_FLAG_WAIT;
> 		}
> 
> 		data->func = func;
> 		data->info = info;
> 		generic_exec_single(cpu, data);
> 
> That would prevent -ENOMEM unless you invoked the function with irqs
> disabled.  So normal callers would still see the current failure-free
> semantics -- you really don't want to be inflicting failure when not
> necessary, right?
> 
> There could only be one irq-disabled caller at a time, which could be
> handled using a trylock, returning -EBUSY if the lock is already held.
> Otherwise, you end up with the scenario called out above (which Keith
> Ownens pointed out some years ago).
> 
> Does this approach make sense?

Actually, no...  The irq-disabled callers would need to acquire the
spinlock -before- disabling irqs, otherwise we end up right back in
the deadlock scenario.

						Thanx, Paul