Message ID | 20200227184602.28282-1-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | xen/grant: Fix signed/unsigned comparisons issues | expand |
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
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,
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
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,
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++ )
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(-)