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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Andrew,

> On 11 Nov 2021, at 17:57, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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.

To add an argument to the discussion I would say that preventing to use function
pointers is something that is good for FuSa so I am in favour here.

> 
> 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>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
> xen/common/domain.c     | 31 ++++++++++++++++++++++---------
> xen/include/xen/sched.h | 15 +++++----------
> 2 files changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 56d47dd66478..562872cdf87a 100644
> --- 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 */)

Here you use comments inside the function definition.
I think this is something that should be avoided and in this specific case a
boolean sync is clear enough not to need to explain that false is nosing.

> {
>     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);
> 
>     arch_domain_pause(d);
> }
> @@ -1250,12 +1253,12 @@ static void do_domain_pause(struct domain *d,
> void domain_pause(struct domain *d)
> {
>     ASSERT(d != current->domain);
> -    do_domain_pause(d, vcpu_sleep_sync);
> +    _domain_pause(d, true /* sync */);
Same here.

> }
> 
> void domain_pause_nosync(struct domain *d)
> {
> -    do_domain_pause(d, vcpu_sleep_nosync);
> +    _domain_pause(d, false /* nosync */);
Same here.

> }
> 
> void domain_unpause(struct domain *d)
> @@ -1269,8 +1272,8 @@ void domain_unpause(struct domain *d)
>             vcpu_wake(v);
> }
> 
> -int __domain_pause_by_systemcontroller(struct domain *d,
> -                                       void (*pause_fn)(struct domain *d))
> +static int _domain_pause_by_systemcontroller(
> +    struct domain *d, bool sync /* or nosync */)
Same here.

> {
>     int old, new, prev = d->controller_pause_count;
> 
> @@ -1289,11 +1292,21 @@ int __domain_pause_by_systemcontroller(struct domain 
> *d,
>         prev = cmpxchg(&d->controller_pause_count, old, new);
>     } while ( prev != old );
> 
> -    pause_fn(d);
> +    _domain_pause(d, sync);
> 
>     return 0;
> }
> 
> +int domain_pause_by_systemcontroller(struct domain *d)
> +{
> +    return _domain_pause_by_systemcontroller(d, true /* sync */);
Same here.

> +}
> +
> +int domain_pause_by_systemcontroller_nosync(struct domain *d)
> +{
> +    return _domain_pause_by_systemcontroller(d, false /* nosync */);
Same here.

Cheers
Bertrand

> +}
> +
> int domain_unpause_by_systemcontroller(struct domain *d)
> {
>     int old, new, prev = d->controller_pause_count;
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 28146ee404e6..37f78cc4c4c9 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -920,26 +920,21 @@ static inline bool vcpu_cpu_dirty(const struct vcpu *v)
> 
> void vcpu_block(void);
> void vcpu_unblock(struct vcpu *v);
> +
> void vcpu_pause(struct vcpu *v);
> void vcpu_pause_nosync(struct vcpu *v);
> void vcpu_unpause(struct vcpu *v);
> +
> int vcpu_pause_by_systemcontroller(struct vcpu *v);
> int vcpu_unpause_by_systemcontroller(struct vcpu *v);
> 
> void domain_pause(struct domain *d);
> void domain_pause_nosync(struct domain *d);
> void domain_unpause(struct domain *d);
> +
> +int domain_pause_by_systemcontroller(struct domain *d);
> +int domain_pause_by_systemcontroller_nosync(struct domain *d);
> int domain_unpause_by_systemcontroller(struct domain *d);
> -int __domain_pause_by_systemcontroller(struct domain *d,
> -                                       void (*pause_fn)(struct domain *d));
> -static inline int domain_pause_by_systemcontroller(struct domain *d)
> -{
> -    return __domain_pause_by_systemcontroller(d, domain_pause);
> -}
> -static inline int domain_pause_by_systemcontroller_nosync(struct domain *d)
> -{
> -    return __domain_pause_by_systemcontroller(d, domain_pause_nosync);
> -}
> 
> /* domain_pause() but safe against trying to pause current. */
> int __must_check domain_pause_except_self(struct domain *d);
> -- 
> 2.11.0
>