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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@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 */)
{
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 */);
}
void domain_pause_nosync(struct domain *d)
{
- do_domain_pause(d, vcpu_sleep_nosync);
+ _domain_pause(d, false /* nosync */);
}
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 */)
{
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 */);
+}
+
+int domain_pause_by_systemcontroller_nosync(struct domain *d)
+{
+ return _domain_pause_by_systemcontroller(d, false /* nosync */);
+}
+
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
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
On 12/11/2021 09:57, Jan Beulich wrote:
> On 11.11.2021 18:57, Andrew Cooper wrote:
>> --- 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);
> }
>
> ?
Yes. For cases where it can't be optimised out via constant
propagation, it removes a conditional branch from the middle of a loop.
I forget what the name for the compiler pass which does this is, but it
makes a big difference given the way that L0 instruction caches and
loop-stream-detectors/etc are build.
~Andrew
Hi Andrew,
> On 11 Nov 2021, at 17:57, Andrew Cooper <andrew.cooper3@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.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@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
>
On 15.11.2021 11:13, Bertrand Marquis wrote:
>> On 11 Nov 2021, at 17:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- 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.
While I agree the comment here isn't overly useful, I think ...
>> @@ -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.
... comments like this one are pretty useful to disambiguate the plain
"true" or "false" (without the reader needing to go look at the function
declaration or definition).
Jan
Hi Jan,
> On 15 Nov 2021, at 10:20, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.11.2021 11:13, Bertrand Marquis wrote:
>>> On 11 Nov 2021, at 17:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> --- 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.
>
> While I agree the comment here isn't overly useful, I think ...
>
>>> @@ -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.
>
> ... comments like this one are pretty useful to disambiguate the plain
> "true" or "false" (without the reader needing to go look at the function
> declaration or definition).
I agree with that but the comment here is useful, it could be added before
the call instead of inside it.
Bertrand
>
> Jan
>
On 15.11.2021 11:23, Bertrand Marquis wrote:
> Hi Jan,
>
>> On 15 Nov 2021, at 10:20, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 15.11.2021 11:13, Bertrand Marquis wrote:
>>>> On 11 Nov 2021, at 17:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> --- 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.
>>
>> While I agree the comment here isn't overly useful, I think ...
>>
>>>> @@ -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.
>>
>> ... comments like this one are pretty useful to disambiguate the plain
>> "true" or "false" (without the reader needing to go look at the function
>> declaration or definition).
>
> I agree with that but the comment here is useful, it could be added before
> the call instead of inside it.
Except the form Andrew has used is the one we've been using elsewhere
for some time.
Jan
Hi Jan,
> On 15 Nov 2021, at 10:55, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.11.2021 11:23, Bertrand Marquis wrote:
>> Hi Jan,
>>
>>> On 15 Nov 2021, at 10:20, Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 15.11.2021 11:13, Bertrand Marquis wrote:
>>>>> On 11 Nov 2021, at 17:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>>> --- 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.
>>>
>>> While I agree the comment here isn't overly useful, I think ...
>>>
>>>>> @@ -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.
>>>
>>> ... comments like this one are pretty useful to disambiguate the plain
>>> "true" or "false" (without the reader needing to go look at the function
>>> declaration or definition).
>>
>> I agree with that but the comment here is useful, it could be added before
>> the call instead of inside it.
>
> Except the form Andrew has used is the one we've been using elsewhere
> for some time.
I know I found some other examples and that why I say “should” and not must.
If other consider that this is the right way to go and should not be changed this
is ok with me but I wanted to make the comment as this could ease the work
with FuSa and Misra compliance in the future.
Bertrand
>
> Jan
Hi Bertrand,
On 15/11/2021 11:23, Bertrand Marquis wrote:
>> On 15 Nov 2021, at 10:55, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 15.11.2021 11:23, Bertrand Marquis wrote:
>>> Hi Jan,
>>>
>>>> On 15 Nov 2021, at 10:20, Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 15.11.2021 11:13, Bertrand Marquis wrote:
>>>>>> On 11 Nov 2021, at 17:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>>>> --- 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.
>>>>
>>>> While I agree the comment here isn't overly useful, I think ...
>>>>
>>>>>> @@ -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.
>>>>
>>>> ... comments like this one are pretty useful to disambiguate the plain
>>>> "true" or "false" (without the reader needing to go look at the function
>>>> declaration or definition).
>>>
>>> I agree with that but the comment here is useful, it could be added before
>>> the call instead of inside it.
>>
>> Except the form Andrew has used is the one we've been using elsewhere
>> for some time.
>
> I know I found some other examples and that why I say “should” and not must.
> If other consider that this is the right way to go and should not be changed this
> is ok with me
Adding the comment after the parameter is a lot easier to read.
What is Misra/FuSA trying to solve by preventing to comment just after
the parameters?
> but I wanted to make the comment as this could ease the work
> with FuSa and Misra compliance in the future.
This will need to be part of a larger discussion on how the community
want to integrate FuSa/Misra rules. I do expect a few of the rules may
be quite controversial to adopt (like the one above) and therefore we
would need to discuss the pros/cons of them.
Cheers,
--
Julien Grall
Hi,
> On 15 Nov 2021, at 14:11, Julien Grall <julien@xen.org> wrote:
>
> Hi Bertrand,
>
> On 15/11/2021 11:23, Bertrand Marquis wrote:
>>> On 15 Nov 2021, at 10:55, Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 15.11.2021 11:23, Bertrand Marquis wrote:
>>>> Hi Jan,
>>>>
>>>>> On 15 Nov 2021, at 10:20, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>
>>>>> On 15.11.2021 11:13, Bertrand Marquis wrote:
>>>>>>> On 11 Nov 2021, at 17:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>>>>> --- 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.
>>>>>
>>>>> While I agree the comment here isn't overly useful, I think ...
>>>>>
>>>>>>> @@ -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.
>>>>>
>>>>> ... comments like this one are pretty useful to disambiguate the plain
>>>>> "true" or "false" (without the reader needing to go look at the function
>>>>> declaration or definition).
>>>>
>>>> I agree with that but the comment here is useful, it could be added before
>>>> the call instead of inside it.
>>>
>>> Except the form Andrew has used is the one we've been using elsewhere
>>> for some time.
>> I know I found some other examples and that why I say “should” and not must.
>> If other consider that this is the right way to go and should not be changed this
>> is ok with me
>
> Adding the comment after the parameter is a lot easier to read.
>
> What is Misra/FuSA trying to solve by preventing to comment just after the parameters?
I do not think Misra is always trying to prevent “bugs”, sometimes it is just trying to make
the code easier to read and review by making it always look the same. Here is to saying that
this should not be done but that comment should be written one or several lines using /* */ form.
>
>> but I wanted to make the comment as this could ease the work
>> with FuSa and Misra compliance in the future.
>
> This will need to be part of a larger discussion on how the community want to integrate FuSa/Misra rules. I do expect a few of the rules may be quite controversial to adopt (like the one above) and therefore we would need to discuss the pros/cons of them.
I definitely think this will require a discussion and maybe some more explanation from us on why.
Your comment asking “What is prevented” is interesting and I will try to keep that in mind when we start the discussion.
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
On Mon, 15 Nov 2021, Jan Beulich wrote:
> On 15.11.2021 11:23, Bertrand Marquis wrote:
> > Hi Jan,
> >
> >> On 15 Nov 2021, at 10:20, Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 15.11.2021 11:13, Bertrand Marquis wrote:
> >>>> On 11 Nov 2021, at 17:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >>>> --- 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.
> >>
> >> While I agree the comment here isn't overly useful, I think ...
> >>
> >>>> @@ -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.
> >>
> >> ... comments like this one are pretty useful to disambiguate the plain
> >> "true" or "false" (without the reader needing to go look at the function
> >> declaration or definition).
> >
> > I agree with that but the comment here is useful, it could be added before
> > the call instead of inside it.
>
> Except the form Andrew has used is the one we've been using elsewhere
> for some time.
Bertrand's comment about MISRA aside, this style of comments doesn't
seem to be covered/allowed by our current CODING_STYLE. It would be good
to keep the CODING_STYLE document up to date: not only it helps us
during reviews, it also helps contributors making sure they don't
violate our guidelines. Moreover CODING_STYLE will certainly turn out
useful when we are going to have MISRA discussion so that we can have an
up-to-date reference of what we currently do and support.
On 16.11.2021 01:41, Stefano Stabellini wrote: > Bertrand's comment about MISRA aside, this style of comments doesn't > seem to be covered/allowed by our current CODING_STYLE. It would be good > to keep the CODING_STYLE document up to date: not only it helps us > during reviews, it also helps contributors making sure they don't > violate our guidelines. Moreover CODING_STYLE will certainly turn out > useful when we are going to have MISRA discussion so that we can have an > up-to-date reference of what we currently do and support. Well, yes. My track record of getting changes to this file accepted is so poor that I'm afraid I'm not willing to make a patch. Jan
~/works/oss/linux/scripts/bloat-o-meter xen-syms-old xen-syms On 11/11/2021 17: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. To be honest, I much prefer to read the old code. I am totally not against the change but I can see how I would be ready to introduce new function pointers use in the future. So I think we need some guidelines on when to use function pointers in Xen. The more... > > 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: ... the code doesn't really compile smaller on Arm: 42sh> ../scripts/bloat-o-meter xen-syms-old xen-syms add/remove: 4/2 grow/shrink: 0/6 up/down: 272/-252 (20) Function old new delta _domain_pause - 136 +136 _domain_pause_by_systemcontroller - 120 +120 domain_pause_by_systemcontroller_nosync - 8 +8 domain_pause_by_systemcontroller - 8 +8 domain_resume 136 132 -4 domain_pause_nosync 12 8 -4 domain_pause 12 8 -4 domain_pause_except_self 188 180 -8 do_domctl 5480 5472 -8 domain_kill 372 356 -16 do_domain_pause 88 - -88 __domain_pause_by_systemcontroller 120 - -120 Total: Before=966919, After=966939, chg +0.00% > > $ ../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. Cheers, -- Julien Grall
On 12/11/2021 09:36, Julien Grall wrote: > On 11/11/2021 17: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. > > To be honest, I much prefer to read the old code. I am totally not > against the change but I can see how I would be ready to introduce new > function pointers use in the future. Really? The only reason there are function points to begin with was because of a far more naive (and far pre-spectre) me fixing a reference counting mess in 2014 by consolidating the logic. My mistake was not spotting that the function pointers weren't actually necessary in the first place. > So I think we need some guidelines on when to use function pointers in > Xen. It's easy. If you are in any way unsure, they're probably the wrong answer. (Ok - I'm being a little facetious) There are concrete security improvements from not using function pointers, demonstrated by fact that JOP/COP attacks are so pervasive that all major hardware and software vendors are working on techniques (both hardware and software) to prevent forward-edge control flow integrity violations. (The mandate from the NSA to make this happen certainly helped spur things on, too.) There are also concrete performance improvements too. All competitive processors in the market today can cope with direct branches more efficiently than indirect branches, and a key principle behind profile-guided-optimsiation is to rearrange your `ptr()` function pointer call into `if ( ptr == a ) a(); else if ( ptr == b ) b(); else ptr()` based on the frequency of destinations, because this really does make orders of magnitude improvements in some cases. We have some shockingly inappropriate uses of function pointers in Xen right now (patches 4 and 5 in particular, and "x86/hvm: Remove callback from paging->flush_tlb() hook" posted today). While this specific example doesn't fall into shockingly inappropriate in my books, it is firmly in the "not appropriate" category. > The more... > >> >> 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: > > ... the code doesn't really compile smaller on Arm: > > 42sh> ../scripts/bloat-o-meter xen-syms-old xen-syms > > add/remove: 4/2 grow/shrink: 0/6 up/down: 272/-252 (20) > Function old new delta > _domain_pause - 136 +136 > _domain_pause_by_systemcontroller - 120 +120 > domain_pause_by_systemcontroller_nosync - 8 +8 > domain_pause_by_systemcontroller - 8 +8 > domain_resume 136 132 -4 > domain_pause_nosync 12 8 -4 > domain_pause 12 8 -4 > domain_pause_except_self 188 180 -8 > do_domctl 5480 5472 -8 > domain_kill 372 356 -16 > do_domain_pause 88 - -88 > __domain_pause_by_systemcontroller 120 - -120 > Total: Before=966919, After=966939, chg +0.00% ARM, like x86, compiles for speed, not size. "it got a bit larger" is generally not as interesting as "it got smaller, despite everything the compiler would normally do in the opposite direction". Obviously, if the build of Xen got 10x larger then we'd have a problem, but it hasn't. Conversely, if we were compiling for size not speed, then "it got smaller" is the uninteresting direction. The truth is that Xen (both x86 and ARM) is a couple of megabytes in RAM and this literally doesn't matter these days where RAM is measured in GB and TB. We care to an extent for efficient use of the cache/etc, but noone would bat an eyelid at several MB more for a want-to-have feature. Here, you're saying that for an added 5 instructions, totalling 0.00% delta in the size of the hypervisor, you've got some reasonably frequent codepaths which will execute faster, and cannot be hijacked with a JOP/COP attack. That's a clear all-around improvement on ARM, just like it is on x86. ~Andrew
Hi Andrew, On 18/11/2021 01:47, Andrew Cooper wrote: > On 12/11/2021 09:36, Julien Grall wrote: >> On 11/11/2021 17: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. >> >> To be honest, I much prefer to read the old code. I am totally not >> against the change but I can see how I would be ready to introduce new >> function pointers use in the future. > > Really? The only reason there are function points to begin with was > because of a far more naive (and far pre-spectre) me fixing a reference > counting mess in 2014 by consolidating the logic. My mistake was not > spotting that the function pointers weren't actually necessary in the > first place. > >> So I think we need some guidelines on when to use function pointers in >> Xen. > > It's easy. If you are in any way unsure, they're probably the wrong > answer. (Ok - I'm being a little facetious) > > There are concrete security improvements from not using function > pointers, demonstrated by fact that JOP/COP attacks are so pervasive > that all major hardware and software vendors are working on techniques > (both hardware and software) to prevent forward-edge control flow > integrity violations. (The mandate from the NSA to make this happen > certainly helped spur things on, too.) > > There are also concrete performance improvements too. All competitive > processors in the market today can cope with direct branches more > efficiently than indirect branches, and a key principle behind > profile-guided-optimsiation is to rearrange your `ptr()` function > pointer call into `if ( ptr == a ) a(); else if ( ptr == b ) b(); else > ptr()` based on the frequency of destinations, because this really does > make orders of magnitude improvements in some cases. > > We have some shockingly inappropriate uses of function pointers in Xen > right now (patches 4 and 5 in particular, and "x86/hvm: Remove callback > from paging->flush_tlb() hook" posted today). While this specific > example doesn't fall into shockingly inappropriate in my books, it is > firmly in the "not appropriate" category. Thanks for the full explanation. What I am looking for is an update of CODING_STYLE to make clear the function pointers should be avoided in Xen and when we would accept them. >>> This actually compiles smaller than before: >> >> ... the code doesn't really compile smaller on Arm: >> >> 42sh> ../scripts/bloat-o-meter xen-syms-old xen-syms >> >> add/remove: 4/2 grow/shrink: 0/6 up/down: 272/-252 (20) >> Function old new delta >> _domain_pause - 136 +136 >> _domain_pause_by_systemcontroller - 120 +120 >> domain_pause_by_systemcontroller_nosync - 8 +8 >> domain_pause_by_systemcontroller - 8 +8 >> domain_resume 136 132 -4 >> domain_pause_nosync 12 8 -4 >> domain_pause 12 8 -4 >> domain_pause_except_self 188 180 -8 >> do_domctl 5480 5472 -8 >> domain_kill 372 356 -16 >> do_domain_pause 88 - -88 >> __domain_pause_by_systemcontroller 120 - -120 >> Total: Before=966919, After=966939, chg +0.00% > > > ARM, like x86, compiles for speed, not size. "it got a bit larger" is > generally not as interesting as "it got smaller, despite everything the > compiler would normally do in the opposite direction". My point is you have a generic section "this compiles smaller" section in your commit message when in fact this was only tested with one x86 compiler version. So at the minimum you should specify the version/architecture because otherwise this sounds like you claim smaller Xen for everyone. But to be honest, I don't really see the value to mention them as this is depending on your compiler (e.g. it may be bigger or smaller) and as you wrote it yourself "you're saying that for an removed 5 instructions". Cheers, -- Julien Grall
© 2016 - 2026 Red Hat, Inc.