xen/grant: Fix signed/unsigned comparisons issues
diff mbox series

Message ID 20200227184602.28282-1-andrew.cooper3@citrix.com
State New
Headers show
Series
  • xen/grant: Fix signed/unsigned comparisons issues
Related show

Commit Message

Andrew Cooper Feb. 27, 2020, 6:46 p.m. UTC
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(-)

Comments

Jan Beulich Feb. 28, 2020, 8:41 a.m. UTC | #1
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
Julien Grall Feb. 28, 2020, 9:09 a.m. UTC | #2
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,
Jan Beulich Feb. 28, 2020, 9:19 a.m. UTC | #3
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
Julien Grall Feb. 28, 2020, 9:37 a.m. UTC | #4
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,

Patch
diff mbox series

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++ )