Re: [PATCH 1/5] xen/domain: Remove function pointers from domain pause helpers

[Jan Beulich <jbeulich@suse.com>]

On 11.11.2021 18:57, Andrew Cooper wrote:
> Retpolines are expensive, and all these do are select between the sync and
> nosync helpers.  Pass a boolean instead, and use direct calls everywhere.
> 
> Pause/unpause operations on behalf of dom0 are not fastpaths, so avoid
> exposing the __domain_pause_by_systemcontroller() internal.
> 
> This actually compiles smaller than before:
> 
>   $ ../scripts/bloat-o-meter xen-syms-before xen-syms-after
>   add/remove: 3/1 grow/shrink: 0/5 up/down: 250/-273 (-23)
>   Function                                     old     new   delta
>   _domain_pause                                  -     115    +115
>   domain_pause_by_systemcontroller               -      69     +69
>   domain_pause_by_systemcontroller_nosync        -      66     +66
>   domain_kill                                  426     398     -28
>   domain_resume                                246     214     -32
>   domain_pause_except_self                     189     141     -48
>   domain_pause                                  59      10     -49
>   domain_pause_nosync                           59       7     -52
>   __domain_pause_by_systemcontroller            64       -     -64
> 
> despite GCC's best efforts.  The new _domain_pause_by_systemcontroller()
> really should not be inlined, considering that the difference is only the
> setup of the sync boolean to pass to _domain_pause(), and there are plenty of
> registers to spare.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit without meaning to override Julien's concerns in any way.

Also a question:

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1234,15 +1234,18 @@ int vcpu_unpause_by_systemcontroller(struct vcpu *v)
>      return 0;
>  }
>  
> -static void do_domain_pause(struct domain *d,
> -                            void (*sleep_fn)(struct vcpu *v))
> +static void _domain_pause(struct domain *d, bool sync /* or nosync */)
>  {
>      struct vcpu *v;
>  
>      atomic_inc(&d->pause_count);
>  
> -    for_each_vcpu( d, v )
> -        sleep_fn(v);
> +    if ( sync )
> +        for_each_vcpu ( d, v )
> +            vcpu_sleep_sync(v);
> +    else
> +        for_each_vcpu ( d, v )
> +            vcpu_sleep_nosync(v);

Is this really better (for whichever reason) than 

    for_each_vcpu ( d, v )
    {
        if ( sync )
            vcpu_sleep_sync(v);
        else
            vcpu_sleep_nosync(v);
    }

?

Jan