Re: [PATCH] xen/smp: Support NULL IPI function pointers

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 18 Nov 2021, at 10:35, Andrew Cooper <amc96@xxxxxxxx> wrote:
> 
> On 18/11/2021 09:58, Jan Beulich wrote:
>> On 17.11.2021 17:48, Andrew Cooper wrote:
>>> There are several cases where the act of interrupting a remote processor has
>>> the required side effect.  Explicitly allow NULL function pointers so the
>>> calling code doesn't have to provide a stub implementation.
>>> 
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> ---
>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> CC: Wei Liu <wl@xxxxxxx>
>>> 
>>> The wait parameter is a little weird.  It serves double duty and will 
>>> confirm
>>> that the IPI has been taken.  All it does is let you control whether you 
>>> also
>>> wait for the handler to complete first.  As such, it is effectively useless
>>> with a stub function.
>>> 
>>> I don't particularly like folding into the .wait() path like that, but I
>>> dislike it less than an if()/else if() and adding a 3rd cpumask_clear_cpu()
>>> into the confusion which is this logic.
>> I can accept this, albeit personally I would have preferred the extra if()
>> over the goto.
> 
> Just so it's been posted.  This is what the if/else looks like:
> 
> diff --git a/xen/common/smp.c b/xen/common/smp.c
> index 79f4ebd14502..ff569bbe9d53 100644
> --- a/xen/common/smp.c
> +++ b/xen/common/smp.c
> @@ -87,7 +87,11 @@ void smp_call_function_interrupt(void)
>  
>      irq_enter();
>  
> -    if ( call_data.wait )
> +    if ( unlikely(!func) )
> +    {
> +        cpumask_clear_cpu(cpu, &call_data.selected);
> +    }
> +    else if ( call_data.wait )
>      {
>          (*func)(info);
>          smp_mb();
> 
> 
> GCC does manage to fold this into the goto version presented originally.
> 
> If everyone else thinks this version is clearer to read then I'll go
> with it.

Hi Andrew,

I would vote for the long version if it’s not a problem.

Cheers,
Luca

> 
> ~Andrew