Re: [PATCH] x86/mem: Make mem_hotadd_check() more legible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.07.2023 13:35, Alejandro Vallejo wrote:
> On Wed, Jul 19, 2023 at 11:08:08AM +0100, Andrew Cooper wrote:
>> Introduce a ROUND() macro to mirror ROUNDUP().  Use both to remove all the
>> opencoded rounding in mem_hotadd_check().  Fix other minor style issues.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>>
>> The compiled binary is identical.
>> ---
>>  xen/arch/x86/x86_64/mm.c | 31 +++++++++++++------------------
>>  xen/include/xen/macros.h |  1 +
>>  2 files changed, 14 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
>> index 60db439af3ec..38f978cab269 100644
>> --- a/xen/arch/x86/x86_64/mm.c
>> +++ b/xen/arch/x86/x86_64/mm.c
>> @@ -1159,10 +1159,10 @@ static int mem_hotadd_check(unsigned long spfn, 
>> unsigned long epfn)
>>  {
>>      unsigned long s, e, length, sidx, eidx;
>>  
>> -    if ( (spfn >= epfn) )
>> +    if ( spfn >= epfn )
>>          return 0;
>>  
>> -    if (pfn_to_pdx(epfn) > FRAMETABLE_NR)
>> +    if ( pfn_to_pdx(epfn) > FRAMETABLE_NR )
>>          return 0;
>>  
>>      if ( (spfn | epfn) & ((1UL << PAGETABLE_ORDER) - 1) )
>> @@ -1172,10 +1172,9 @@ static int mem_hotadd_check(unsigned long spfn, 
>> unsigned long epfn)
>>          return 0;
>>  
>>      /* Make sure the new range is not present now */
>> -    sidx = ((pfn_to_pdx(spfn) + PDX_GROUP_COUNT - 1)  & ~(PDX_GROUP_COUNT - 
>> 1))
>> -            / PDX_GROUP_COUNT;
>> -    eidx = (pfn_to_pdx(epfn - 1) & ~(PDX_GROUP_COUNT - 1)) / 
>> PDX_GROUP_COUNT;
>> -    if (sidx >= eidx)
>> +    sidx = ROUNDUP(pfn_to_pdx(spfn),     PDX_GROUP_COUNT) / PDX_GROUP_COUNT;
>> +    eidx = ROUND  (pfn_to_pdx(epfn - 1), PDX_GROUP_COUNT) / PDX_GROUP_COUNT;
> See [1] for both the sidx and eidx lines.
>> +    if ( sidx >= eidx )
>>          return 0;
>>  
>>      s = find_next_zero_bit(pdx_group_valid, eidx, sidx);
>> @@ -1186,28 +1185,24 @@ static int mem_hotadd_check(unsigned long spfn, 
>> unsigned long epfn)
>>          return 0;
>>  
>>      /* Caculate at most required m2p/compat m2p/frametable pages */
>> -    s = (spfn & ~((1UL << (L2_PAGETABLE_SHIFT - 3)) - 1));
>> -    e = (epfn + (1UL << (L2_PAGETABLE_SHIFT - 3)) - 1) &
>> -            ~((1UL << (L2_PAGETABLE_SHIFT - 3)) - 1);
>> +    s = ROUND  (spfn, 1UL << (L2_PAGETABLE_SHIFT - 3));
>> +    e = ROUNDUP(epfn, 1UL << (L2_PAGETABLE_SHIFT - 3));
>>  
>>      length = (e - s) * sizeof(unsigned long);
>>  
>> -    s = (spfn & ~((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1));
>> -    e = (epfn + (1UL << (L2_PAGETABLE_SHIFT - 2)) - 1) &
>> -            ~((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1);
>> -
>> -    e = min_t(unsigned long, e,
>> +    s =     ROUND  (spfn, 1ULL << (L2_PAGETABLE_SHIFT - 2));
> See [1] for s.
>> +    e = min(ROUNDUP(epfn, 1ULL << (L2_PAGETABLE_SHIFT - 2)),
>>              (RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START) >> 2);
>>  
>>      if ( e > s )
>> -        length += (e -s) * sizeof(unsigned int);
>> +        length += (e - s) * sizeof(unsigned int);
>>  
>> -    s = pfn_to_pdx(spfn) & ~(PDX_GROUP_COUNT - 1);
>> -    e = ( pfn_to_pdx(epfn) + (PDX_GROUP_COUNT - 1) ) & ~(PDX_GROUP_COUNT - 
>> 1);
>> +    s = ROUND  (pfn_to_pdx(spfn), PDX_GROUP_COUNT);
> See [1] for s.
>> +    e = ROUNDUP(pfn_to_pdx(epfn), PDX_GROUP_COUNT);
>>  
>>      length += (e - s) * sizeof(struct page_info);
>>  
>> -    if ((length >> PAGE_SHIFT) > (epfn - spfn))
>> +    if ( (length >> PAGE_SHIFT) > (epfn - spfn) )
>>          return 0;
>>  
>>      return 1;
>> diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
>> index 7b92d345044d..ceeffcaa95ff 100644
>> --- a/xen/include/xen/macros.h
>> +++ b/xen/include/xen/macros.h
>> @@ -1,6 +1,7 @@
>>  #ifndef __MACROS_H__
>>  #define __MACROS_H__
>>  
>> +#define ROUND(x, a)   ((x) & ~((a) - 1))
> Why not ROUNDDOWN() or ROUND_DOWN()? ROUND() doesn't imply a specific
> direction and can be confusing if ROUNDUP is not seen next to it.

ROUNDDN() would address some of your blank padding concerns, for ...

>>  #define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))

... being the same length as this then. That said, ...

> [1] The hand-crafted alignment there is going to collide with the efforts
> to integrate automatic style checkers. It's also not conveying critical
> information, so I'd argue for its removal in the spirit of making future
> diffs less intrusive.

... I don't agree here. First of all I don't see why this should
make style checking harder. There's nothing written anywhere that
such alignment padding isn't allowed in our code, so any checker
we want to use would need to tolerate it. Plus while such padding
doesn't convey critical information, it still helps readability.

Jan