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

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

[Andrew Cooper]

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

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;
+    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));
+    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);
+    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))
 #define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
 
 #define IS_ALIGNED(val, align) (!((val) & ((align) - 1)))

base-commit: b1c16800e52743d9afd9af62c810f03af16dd942
-- 
2.30.2

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

[Alejandro Vallejo]

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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> 
> 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.
>  #define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
>  
>  #define IS_ALIGNED(val, align) (!((val) & ((align) - 1)))
> 
> base-commit: b1c16800e52743d9afd9af62c810f03af16dd942
> -- 
> 2.30.2
> 
> 

[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.

Alejandro

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

[Jan Beulich]

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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>>
>> 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

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

[Alejandro Vallejo]

On Wed, Jul 19, 2023 at 02:09:55PM +0200, Jan Beulich wrote:
> > [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
Considering the last Xen Summit sessions I think it's reasonable to assume
we do want automatic style checking to become a reality. If we want an
automatic style checker to be eventually introduced I think we should be
mindful of style changes unlikely to be captured by _any_ policy we may end
up having. In particular, alignment of arguments across different
statements on different functions/macros is unsupported on (most?) major
style checkers, and that's highly unlikely to ever change.

In particular, any style checker must follow strict rules in order for it
to yield consistently deterministic results (otherwise it might suffer from
termination issues). Expecting a style checker to automatically generate
heuristics that happen to match our current code configuration is not a
realistic goal, I reckon.

In this sense, having a guideline (i.e: not a rule) about trying to avoid
hand-crafted alignment where the benefit of it is not critical would be
good. In the spirit of making a complicated problem simpler rather than...
well, complicated.

Alejandro

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

[Jan Beulich]

On 19.07.2023 14:54, Alejandro Vallejo wrote:
> On Wed, Jul 19, 2023 at 02:09:55PM +0200, Jan Beulich wrote:
>>> [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.
>>
> Considering the last Xen Summit sessions I think it's reasonable to assume
> we do want automatic style checking to become a reality. If we want an
> automatic style checker to be eventually introduced I think we should be
> mindful of style changes unlikely to be captured by _any_ policy we may end
> up having. In particular, alignment of arguments across different
> statements on different functions/macros is unsupported on (most?) major
> style checkers, and that's highly unlikely to ever change.
> 
> In particular, any style checker must follow strict rules in order for it
> to yield consistently deterministic results (otherwise it might suffer from
> termination issues). Expecting a style checker to automatically generate
> heuristics that happen to match our current code configuration is not a
> realistic goal, I reckon.

I wasn't thinking of heuristics. I also wasn't expecting a style checker
to actually request adjustments to insert padding where may (seem to be)
missing. What I would expect is that it also doesn't point out such
seemingly excessive padding, requesting it to be dropped. I'll admit that
this may lead to the checking being slightly less useful, because of
potentially not pointing out an issue where there is one, but that would
still seem better to me than involving heuristics.

Jan

> In this sense, having a guideline (i.e: not a rule) about trying to avoid
> hand-crafted alignment where the benefit of it is not critical would be
> good. In the spirit of making a complicated problem simpler rather than...
> well, complicated.
> 
> Alejandro

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

[Jan Beulich]

On 19.07.2023 14:54, Alejandro Vallejo wrote:
> On Wed, Jul 19, 2023 at 02:09:55PM +0200, Jan Beulich wrote:
>>> [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.
>>
> Considering the last Xen Summit sessions I think it's reasonable to assume
> we do want automatic style checking to become a reality.

Oh, just to avoid potential misunderstanding: I certainly agree here.
Just that, like in many other cases where computers (or automation in
more general terms) are involved I think that ...

> If we want an
> automatic style checker to be eventually introduced I think we should be
> mindful of style changes unlikely to be captured by _any_ policy we may end
> up having. In particular, alignment of arguments across different
> statements on different functions/macros is unsupported on (most?) major
> style checkers, and that's highly unlikely to ever change.

... the goal wants to be that computers (or alike) adapt to their
users, not the other way around. Sadly we're still extremely far from
that ...

Jan