xen/arch/x86/x86_64/mm.c | 31 +++++++++++++------------------ xen/include/xen/macros.h | 1 + 2 files changed, 14 insertions(+), 18 deletions(-)
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.