* [Xen-devel] [PATCH] xen/grant: Fix signed/unsigned comparisons issues
@ 2020-02-27 18:46 Andrew Cooper
2020-02-28 8:41 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2020-02-27 18:46 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Jan Beulich
Each function takes an unsigned count, and loops based on a signed i. This
causes problems when between 2 and 4 billion operations are requested.
In practice, signed-ness issues aren't possible because count exceeding
INT_MAX is excluded earlier in do_grant_op(), but the code reads as if it is
buggy, and GCC obviously can't spot this either.
Bloat-o-meter reports:
add/remove: 0/0 grow/shrink: 0/4 up/down: 0/-95 (-95)
Function old new delta
do_grant_table_op 7155 7140 -15
gnttab_transfer 2732 2716 -16
gnttab_unmap_grant_ref 771 739 -32
gnttab_unmap_and_replace 771 739 -32
Total: Before=2996364, After=2996269, chg -0.00%
and inspection of gnttab_unmap_grant_ref() at least reveals one fewer local
variables on the stack.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
gnttab_unmap_grant_ref()'s stack frame size is 0x740 (dropping to 0x738) which
is alarmingly close to 2k.
---
xen/common/grant_table.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index bc37acae0e..0f81875bee 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1270,7 +1270,7 @@ static long
gnttab_map_grant_ref(
XEN_GUEST_HANDLE_PARAM(gnttab_map_grant_ref_t) uop, unsigned int count)
{
- int i;
+ unsigned int i;
struct gnttab_map_grant_ref op;
for ( i = 0; i < count; i++ )
@@ -1568,13 +1568,14 @@ static long
gnttab_unmap_grant_ref(
XEN_GUEST_HANDLE_PARAM(gnttab_unmap_grant_ref_t) uop, unsigned int count)
{
- int i, c, partial_done, done = 0;
+ unsigned int i, partial_done, done = 0;
struct gnttab_unmap_grant_ref op;
struct gnttab_unmap_common common[GNTTAB_UNMAP_BATCH_SIZE];
while ( count != 0 )
{
- c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE);
+ unsigned int c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE);
+
partial_done = 0;
for ( i = 0; i < c; i++ )
@@ -1633,13 +1634,14 @@ static long
gnttab_unmap_and_replace(
XEN_GUEST_HANDLE_PARAM(gnttab_unmap_and_replace_t) uop, unsigned int count)
{
- int i, c, partial_done, done = 0;
+ unsigned int i, partial_done, done = 0;
struct gnttab_unmap_and_replace op;
struct gnttab_unmap_common common[GNTTAB_UNMAP_BATCH_SIZE];
while ( count != 0 )
{
- c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE);
+ unsigned int c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE);
+
partial_done = 0;
for ( i = 0; i < c; i++ )
@@ -2142,7 +2144,7 @@ gnttab_transfer(
struct domain *d = current->domain;
struct domain *e;
struct page_info *page;
- int i;
+ unsigned int i;
struct gnttab_transfer gop;
mfn_t mfn;
unsigned int max_bitsize;
@@ -3359,7 +3361,7 @@ static long
gnttab_swap_grant_ref(XEN_GUEST_HANDLE_PARAM(gnttab_swap_grant_ref_t) uop,
unsigned int count)
{
- int i;
+ unsigned int i;
gnttab_swap_grant_ref_t op;
for ( i = 0; i < count; i++ )
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Xen-devel] [PATCH] xen/grant: Fix signed/unsigned comparisons issues
2020-02-27 18:46 [Xen-devel] [PATCH] xen/grant: Fix signed/unsigned comparisons issues Andrew Cooper
@ 2020-02-28 8:41 ` Jan Beulich
2020-02-28 9:09 ` Julien Grall
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2020-02-28 8:41 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: George Dunlap, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk
On 27.02.2020 19:46, Andrew Cooper wrote:
> Each function takes an unsigned count, and loops based on a signed i. This
> causes problems when between 2 and 4 billion operations are requested.
I can see problems, but not (as the title says) with comparison issues
(the signed i gets converted to unsigned for the purpose of the
comparison).
> In practice, signed-ness issues aren't possible because count exceeding
> INT_MAX is excluded earlier in do_grant_op(), but the code reads as if it is
> buggy, and GCC obviously can't spot this either.
>
> Bloat-o-meter reports:
> add/remove: 0/0 grow/shrink: 0/4 up/down: 0/-95 (-95)
> Function old new delta
> do_grant_table_op 7155 7140 -15
> gnttab_transfer 2732 2716 -16
> gnttab_unmap_grant_ref 771 739 -32
> gnttab_unmap_and_replace 771 739 -32
> Total: Before=2996364, After=2996269, chg -0.00%
>
> and inspection of gnttab_unmap_grant_ref() at least reveals one fewer local
> variables on the stack.
This is a good thing for x86. However, having started to familiarize
myself a tiny bit with RISC-V, I'm no longer certain our present way
of preferring unsigned int for array indexing variable and alike is
actually a good thing without further abstraction. Nevertheless, this
isn't an immediate issue, so ...
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit preferably with the title adjusted, and with one more
suggestion:
> @@ -1568,13 +1568,14 @@ static long
> gnttab_unmap_grant_ref(
> XEN_GUEST_HANDLE_PARAM(gnttab_unmap_grant_ref_t) uop, unsigned int count)
> {
> - int i, c, partial_done, done = 0;
> + unsigned int i, partial_done, done = 0;
> struct gnttab_unmap_grant_ref op;
> struct gnttab_unmap_common common[GNTTAB_UNMAP_BATCH_SIZE];
>
> while ( count != 0 )
> {
> - c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE);
> + unsigned int c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE);
Seeing this and another instance further down, would you mind
dropping the cast on this occasion, in favor of adding a U
suffix to GNTTAB_UNMAP_BATCH_SIZE's definition?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Xen-devel] [PATCH] xen/grant: Fix signed/unsigned comparisons issues
2020-02-28 8:41 ` Jan Beulich
@ 2020-02-28 9:09 ` Julien Grall
2020-02-28 9:19 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Julien Grall @ 2020-02-28 9:09 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper, Xen-devel
Cc: George Dunlap, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk
Hi Jan,
On 28/02/2020 08:41, Jan Beulich wrote:
> On 27.02.2020 19:46, Andrew Cooper wrote:
>> Each function takes an unsigned count, and loops based on a signed i. This
>> causes problems when between 2 and 4 billion operations are requested.
>
> I can see problems, but not (as the title says) with comparison issues
> (the signed i gets converted to unsigned for the purpose of the
> comparison).
>
>> In practice, signed-ness issues aren't possible because count exceeding
>> INT_MAX is excluded earlier in do_grant_op(), but the code reads as if it is
>> buggy, and GCC obviously can't spot this either.
>>
>> Bloat-o-meter reports:
>> add/remove: 0/0 grow/shrink: 0/4 up/down: 0/-95 (-95)
>> Function old new delta
>> do_grant_table_op 7155 7140 -15
>> gnttab_transfer 2732 2716 -16
>> gnttab_unmap_grant_ref 771 739 -32
>> gnttab_unmap_and_replace 771 739 -32
>> Total: Before=2996364, After=2996269, chg -0.00%
>>
>> and inspection of gnttab_unmap_grant_ref() at least reveals one fewer local
>> variables on the stack.
>
> This is a good thing for x86. However, having started to familiarize
> myself a tiny bit with RISC-V, I'm no longer certain our present way
> of preferring unsigned int for array indexing variable and alike is
> actually a good thing without further abstraction. Nevertheless, this
> isn't an immediate issue, so ...
Would you mind expanding a bit more about this comment here? How
unsigned int is going to make things worse on RISC-V?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Xen-devel] [PATCH] xen/grant: Fix signed/unsigned comparisons issues
2020-02-28 9:09 ` Julien Grall
@ 2020-02-28 9:19 ` Jan Beulich
2020-02-28 9:37 ` Julien Grall
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2020-02-28 9:19 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Xen-devel
On 28.02.2020 10:09, Julien Grall wrote:
> Hi Jan,
>
> On 28/02/2020 08:41, Jan Beulich wrote:
>> On 27.02.2020 19:46, Andrew Cooper wrote:
>>> Each function takes an unsigned count, and loops based on a signed i. This
>>> causes problems when between 2 and 4 billion operations are requested.
>>
>> I can see problems, but not (as the title says) with comparison issues
>> (the signed i gets converted to unsigned for the purpose of the
>> comparison).
>>
>>> In practice, signed-ness issues aren't possible because count exceeding
>>> INT_MAX is excluded earlier in do_grant_op(), but the code reads as if it is
>>> buggy, and GCC obviously can't spot this either.
>>>
>>> Bloat-o-meter reports:
>>> add/remove: 0/0 grow/shrink: 0/4 up/down: 0/-95 (-95)
>>> Function old new delta
>>> do_grant_table_op 7155 7140 -15
>>> gnttab_transfer 2732 2716 -16
>>> gnttab_unmap_grant_ref 771 739 -32
>>> gnttab_unmap_and_replace 771 739 -32
>>> Total: Before=2996364, After=2996269, chg -0.00%
>>>
>>> and inspection of gnttab_unmap_grant_ref() at least reveals one fewer local
>>> variables on the stack.
>>
>> This is a good thing for x86. However, having started to familiarize
>> myself a tiny bit with RISC-V, I'm no longer certain our present way
>> of preferring unsigned int for array indexing variable and alike is
>> actually a good thing without further abstraction. Nevertheless, this
>> isn't an immediate issue, so ...
>
> Would you mind expanding a bit more about this comment here? How
> unsigned int is going to make things worse on RISC-V?
Other than x86-64 and Arm64, 64-bit (and wider) RISC-V sign-extend
the result of 32-bit operations by default. Hence for an unsigned
32-bit type to be used as array index, an additional zero-extending
insn is going to be needed (just like for our existing ports a
sign-extending insn is needed when array indexing variables are
calculated as 32-bit signed quantities, which is one of the reasons
why right now we prefer unsigned int in such cases).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Xen-devel] [PATCH] xen/grant: Fix signed/unsigned comparisons issues
2020-02-28 9:19 ` Jan Beulich
@ 2020-02-28 9:37 ` Julien Grall
0 siblings, 0 replies; 5+ messages in thread
From: Julien Grall @ 2020-02-28 9:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Xen-devel
Hi Jan,
On 28/02/2020 09:19, Jan Beulich wrote:
> On 28.02.2020 10:09, Julien Grall wrote:
>> Hi Jan,
>>
>> On 28/02/2020 08:41, Jan Beulich wrote:
>>> On 27.02.2020 19:46, Andrew Cooper wrote:
>>>> Each function takes an unsigned count, and loops based on a signed i. This
>>>> causes problems when between 2 and 4 billion operations are requested.
>>>
>>> I can see problems, but not (as the title says) with comparison issues
>>> (the signed i gets converted to unsigned for the purpose of the
>>> comparison).
>>>
>>>> In practice, signed-ness issues aren't possible because count exceeding
>>>> INT_MAX is excluded earlier in do_grant_op(), but the code reads as if it is
>>>> buggy, and GCC obviously can't spot this either.
>>>>
>>>> Bloat-o-meter reports:
>>>> add/remove: 0/0 grow/shrink: 0/4 up/down: 0/-95 (-95)
>>>> Function old new delta
>>>> do_grant_table_op 7155 7140 -15
>>>> gnttab_transfer 2732 2716 -16
>>>> gnttab_unmap_grant_ref 771 739 -32
>>>> gnttab_unmap_and_replace 771 739 -32
>>>> Total: Before=2996364, After=2996269, chg -0.00%
>>>>
>>>> and inspection of gnttab_unmap_grant_ref() at least reveals one fewer local
>>>> variables on the stack.
>>>
>>> This is a good thing for x86. However, having started to familiarize
>>> myself a tiny bit with RISC-V, I'm no longer certain our present way
>>> of preferring unsigned int for array indexing variable and alike is
>>> actually a good thing without further abstraction. Nevertheless, this
>>> isn't an immediate issue, so ...
>>
>> Would you mind expanding a bit more about this comment here? How
>> unsigned int is going to make things worse on RISC-V?
>
> Other than x86-64 and Arm64, 64-bit (and wider) RISC-V sign-extend
> the result of 32-bit operations by default. Hence for an unsigned
> 32-bit type to be used as array index, an additional zero-extending
> insn is going to be needed (just like for our existing ports a
> sign-extending insn is needed when array indexing variables are
> calculated as 32-bit signed quantities, which is one of the reasons
> why right now we prefer unsigned int in such cases).
Meh, I am not convinced this is a good enough reason to switch array
indexes to signed int for RISC-V. There are probably better place to
look at optimization in common code and would benefits all arch.
Regarding the reason for "unsigned int", I don't request it because it
produce "shorter' code but because there is no reason to have a signed
index for array.
Anyway, let's cross that bridge when we have an actual RISC-V port for
Xen merged.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-28 9:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-27 18:46 [Xen-devel] [PATCH] xen/grant: Fix signed/unsigned comparisons issues Andrew Cooper
2020-02-28 8:41 ` Jan Beulich
2020-02-28 9:09 ` Julien Grall
2020-02-28 9:19 ` Jan Beulich
2020-02-28 9:37 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.