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

[Jens Axboe]

On Tue, Apr 22 2008, Linus Torvalds wrote:
> 
> 
> On Tue, 22 Apr 2008, Jens Axboe wrote:
> > > 
> > > You forgot to free the "data" here? The waiter must also free the object, 
> > > since now the callee does not.
> > 
> > The ipi interrupt handler does that, see kfree() in
> > generic_smp_call_function_single_interrupt() or call_func_data_free() in
> > generic_smp_call_function_interrupt().
> 
> Hell no, it does *not*.
> 
> Doing that for the waiting case would be a *huge* bug, since the waiter 
> needs to wait until the flag is clear - and if the waitee free's the 
> allocation, that will never happen.
> 
> So the rule *must* be:
>  - waiter frees
>  - ipi interrupt frees non-waiting ones.
> because anything else cannot work.
> 
> And you must have known that, because the code you pointed me to does 
> *not* free the data at all. It just clears the FLAG_WAIT flag:
> 
> 	+               if (data->csd.flags & CSD_FLAG_WAIT) {
> 	+                       smp_wmb();
> 	+                       data->csd.flags &= ~CSD_FLAG_WAIT;
> 	+               } else
> 	+                       call_func_data_free(data);
> 
> So please think about this some more. 

Yep, sorry, pre-dinner sugar low. So how about this, it implements what
I/you detailed:

- If wait == 1, then we always use on-stack allocation
- If wait == 0 and alloc fails, set wait = 1 and repeat.
- For wait == 1, there's never anything to free.
- For wait == 0, the ipi interrupt handler frees the structure if
  CSD_FLAG_ALLOC is set. This COULD be an else without checking
  CSD_FLAG_ALLOC for smp_call_function_mask(), only
  smp_call_function_single() really needs the check. But I left it there
  in case we allow pre-allocated insert for that path as well in the
  future.

OK?