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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Andrew,

> -----Original Message-----
> From: Andrew Cooper <amc96@xxxxxxxx>
> Sent: 2021年11月18日 18:35
> To: Jan Beulich <jbeulich@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Wei Chen <Wei.Chen@xxxxxxx>
> Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Xen-
> devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH] xen/smp: Support NULL IPI function pointers
> 
> 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.

This version is much clearer to read. But if there will be some code
comments in goto version to make it easy to read. I am ok for either.

> 
> ~Andrew