Hi Andrew,
> On 11 Nov 2021, at 17:57, Andrew Cooper <andrew.coop...@citrix.com> 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.coop...@citrix.com>
> ---
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Roger Pau Monné <roger....@citrix.com>
> CC: Wei Liu <w...@xen.org>
> CC: Stefano Stabellini <sstabell...@kernel.org>
> CC: Julien Grall <jul...@xen.org>
> CC: Volodymyr Babchuk <volodymyr_babc...@epam.com>
> CC: Bertrand Marquis <bertrand.marq...@arm.com>
> ---
> 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
>
