Re: [Xen-devel] [PATCH] xen/grant: Fix signed/unsigned comparisons issues

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel