On 29.01.2021 00:32, Andrew Cooper wrote:
> On 15/01/2021 15:37, Jan Beulich wrote:
>> On 12.01.2021 20:48, Andrew Cooper wrote:
>>> @@ -446,6 +430,31 @@ int compat_memory_op(unsigned int cmd,
>>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>>
>>> #undef XLAT_mem_acquire_resource_HNDL_frame_list
>>>
>>> + if ( xen_frame_list && cmp.mar.nr_frames )
>>> + {
>>> + /*
>>> + * frame_list is an input for translated guests, and an
>>> output
>>> + * for untranslated guests. Only copy in for translated
>>> guests.
>>> + */
>>> + if ( paging_mode_translate(currd) )
>>> + {
>>> + compat_pfn_t *compat_frame_list = (void
>>> *)xen_frame_list;
>>> +
>>> + if ( !compat_handle_okay(cmp.mar.frame_list,
>>> + cmp.mar.nr_frames) ||
>>> + __copy_from_compat_offset(
>>> + compat_frame_list, cmp.mar.frame_list,
>>> + 0, cmp.mar.nr_frames) )
>>> + return -EFAULT;
>>> +
>>> + /*
>>> + * Iterate backwards over compat_frame_list[] expanding
>>> + * compat_pfn_t to xen_pfn_t in place.
>>> + */
>>> + for ( int x = cmp.mar.nr_frames - 1; x >= 0; --x )
>>> + xen_frame_list[x] = compat_frame_list[x];
>> Just as a nit, without requiring you to adjust (but with the
>> request to consider adjusting) - x getting used as array index
>> would generally suggest it wants to be an unsigned type (despite
>> me guessing the compiler ought to be able to avoid an explicit
>> sign-extension for the actual memory accesses):
>>
>> for ( unsigned int x = cmp.mar.nr_frames; x--; )
>> xen_frame_list[x] = compat_frame_list[x];
>
> Signed numbers are not inherently evil. The range of x is between 0 and
> 1020 so there is no issue with failing to enter the loop.
>
> It is the compilers job to make this optimisation. It is a very poor
> use of a developers time to write logic which takes extra effort to
> figure out whether it is correct or not.
I don't see why my suggested alternative is any more difficult to
understand. It's one less expression, so perhaps even less cognitive
load. But yes, this is easily getting subjective.
> You know what my attitude will be towards a compiler which is incapable
> of making the optimisation, and you've got to go back a decade to find a
> processor old enough to not have identical performance between the
> unoptimised signed and unsigned forms.
I'm not sure I see how the compiler could transform this to using
unsigned int. By observation, gcc10 doesn't, despite -O2 (release
build). It still emits an otherwise unnecessary MOVSXD, and the
loop body is one insn shorter with an unsigned induction variable
(albeit that's likely just a side effect in this specific example).
> Both signs of numbers have their uses, and a rigid policy of using
> unsigned numbers does more harm than good (in this case, concerning the
> simplicity of the code).
Of course. But array accesses are where we'd better limit ourselves
to unsigned indexing variables, imo.
Jan
