mm/memory.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
riscv64-gcc-linux-gnu (v8.5) reports a compile time assert in:
r[2] = DEFINE_RANGE(clamp_t(s64, fault_idx - radius, pg.start, pg.end),
clamp_t(s64, fault_idx + radius, pg.start, pg.end));
where it decides that pg.start > pg.end in:
clamp_t(s64, fault_idx + radius, pg.start, pg.end));
where pg comes from:
const struct range pg = DEFINE_RANGE(0, folio_nr_pages(folio) - 1);
That does not seem like it could be true. Even for pg.start == pg.end,
we would need folio_test_large() to evaluate to false at compile time:
static inline unsigned long folio_nr_pages(const struct folio *folio)
{
if (!folio_test_large(folio))
return 1;
return folio_large_nr_pages(folio);
}
Workaround by open coding the range computation. Also, simplify the type
declarations for the relevant variables.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202601240453.QCjgGdJa-lkp@intel.com/
Fixes: 93552c9a3350 ("mm: folio_zero_user: cache neighbouring pages")
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
Hi Andrew
As David pointed out, the previous open coded version makes a few
unnecessary changes. Could you queue this one instead?
Thanks
Ankur
mm/memory.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/mm/memory.c b/mm/memory.c
index ce933ee4a3dd..f5bfc082ab61 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -7284,7 +7284,7 @@ void folio_zero_user(struct folio *folio, unsigned long addr_hint)
const unsigned long base_addr = ALIGN_DOWN(addr_hint, folio_size(folio));
const long fault_idx = (addr_hint - base_addr) / PAGE_SIZE;
const struct range pg = DEFINE_RANGE(0, folio_nr_pages(folio) - 1);
- const int radius = FOLIO_ZERO_LOCALITY_RADIUS;
+ const long radius = FOLIO_ZERO_LOCALITY_RADIUS;
struct range r[3];
int i;
@@ -7292,24 +7292,23 @@ void folio_zero_user(struct folio *folio, unsigned long addr_hint)
* Faulting page and its immediate neighbourhood. Will be cleared at the
* end to keep its cachelines hot.
*/
- r[2] = DEFINE_RANGE(clamp_t(s64, fault_idx - radius, pg.start, pg.end),
- clamp_t(s64, fault_idx + radius, pg.start, pg.end));
+ r[2] = DEFINE_RANGE(fault_idx - radius < (long)pg.start ? pg.start : fault_idx - radius,
+ fault_idx + radius > (long)pg.end ? pg.end : fault_idx + radius);
+
/* Region to the left of the fault */
- r[1] = DEFINE_RANGE(pg.start,
- clamp_t(s64, r[2].start - 1, pg.start - 1, r[2].start));
+ r[1] = DEFINE_RANGE(pg.start, r[2].start - 1);
/* Region to the right of the fault: always valid for the common fault_idx=0 case. */
- r[0] = DEFINE_RANGE(clamp_t(s64, r[2].end + 1, r[2].end, pg.end + 1),
- pg.end);
+ r[0] = DEFINE_RANGE(r[2].end + 1, pg.end);
for (i = 0; i < ARRAY_SIZE(r); i++) {
const unsigned long addr = base_addr + r[i].start * PAGE_SIZE;
- const unsigned int nr_pages = range_len(&r[i]);
+ const long nr_pages = (long)range_len(&r[i]);
struct page *page = folio_page(folio, r[i].start);
if (nr_pages > 0)
- clear_contig_highpages(page, addr, nr_pages);
+ clear_contig_highpages(page, addr, (unsigned int)nr_pages);
}
}
--
2.31.1On 1/28/26 19:59, Ankur Arora wrote:
> riscv64-gcc-linux-gnu (v8.5) reports a compile time assert in:
>
> r[2] = DEFINE_RANGE(clamp_t(s64, fault_idx - radius, pg.start, pg.end),
> clamp_t(s64, fault_idx + radius, pg.start, pg.end));
>
> where it decides that pg.start > pg.end in:
> clamp_t(s64, fault_idx + radius, pg.start, pg.end));
>
> where pg comes from:
> const struct range pg = DEFINE_RANGE(0, folio_nr_pages(folio) - 1);
>
> That does not seem like it could be true. Even for pg.start == pg.end,
> we would need folio_test_large() to evaluate to false at compile time:
>
> static inline unsigned long folio_nr_pages(const struct folio *folio)
> {
> if (!folio_test_large(folio))
> return 1;
> return folio_large_nr_pages(folio);
> }
>
> Workaround by open coding the range computation. Also, simplify the type
> declarations for the relevant variables.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202601240453.QCjgGdJa-lkp@intel.com/
> Fixes: 93552c9a3350 ("mm: folio_zero_user: cache neighbouring pages")
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
>
> Hi Andrew
>
> As David pointed out, the previous open coded version makes a few
> unnecessary changes. Could you queue this one instead?
>
I'm late, maybe this is already upstream.
> Thanks
> Ankur
>
>
> mm/memory.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index ce933ee4a3dd..f5bfc082ab61 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -7284,7 +7284,7 @@ void folio_zero_user(struct folio *folio, unsigned long addr_hint)
> const unsigned long base_addr = ALIGN_DOWN(addr_hint, folio_size(folio));
> const long fault_idx = (addr_hint - base_addr) / PAGE_SIZE;
> const struct range pg = DEFINE_RANGE(0, folio_nr_pages(folio) - 1);
> - const int radius = FOLIO_ZERO_LOCALITY_RADIUS;
> + const long radius = FOLIO_ZERO_LOCALITY_RADIUS;
> struct range r[3];
> int i;
>
> @@ -7292,24 +7292,23 @@ void folio_zero_user(struct folio *folio, unsigned long addr_hint)
> * Faulting page and its immediate neighbourhood. Will be cleared at the
> * end to keep its cachelines hot.
> */
> - r[2] = DEFINE_RANGE(clamp_t(s64, fault_idx - radius, pg.start, pg.end),
> - clamp_t(s64, fault_idx + radius, pg.start, pg.end));
> + r[2] = DEFINE_RANGE(fault_idx - radius < (long)pg.start ? pg.start : fault_idx - radius,
> + fault_idx + radius > (long)pg.end ? pg.end : fault_idx + radius);
> +
LGTM, although it could likely be made a bit more readable by using some temporary variables.
const long fault_idx_low = fault_idx - radius;
const long fault_idx_high = fault_idx + radius;
r[2] = DEFINE_RANGE(fault_idx_low < (long)pg.start ? pg.start : fault_idx_low,
fault_idx_high > (long)pg.end ? pg.end : fault_idx_high);
Well, still a bit unreadable, so ... :)
>
> /* Region to the left of the fault */
> - r[1] = DEFINE_RANGE(pg.start,
> - clamp_t(s64, r[2].start - 1, pg.start - 1, r[2].start));
> + r[1] = DEFINE_RANGE(pg.start, r[2].start - 1);
>
> /* Region to the right of the fault: always valid for the common fault_idx=0 case. */
> - r[0] = DEFINE_RANGE(clamp_t(s64, r[2].end + 1, r[2].end, pg.end + 1),
> - pg.end);
> + r[0] = DEFINE_RANGE(r[2].end + 1, pg.end);
TBH, without the clamp that looks much more readable here.
>
> for (i = 0; i < ARRAY_SIZE(r); i++) {
> const unsigned long addr = base_addr + r[i].start * PAGE_SIZE;
> - const unsigned int nr_pages = range_len(&r[i]);
> + const long nr_pages = (long)range_len(&r[i]);
> struct page *page = folio_page(folio, r[i].start);
>
> if (nr_pages > 0)
> - clear_contig_highpages(page, addr, nr_pages);
> + clear_contig_highpages(page, addr, (unsigned int)nr_pages);
Is that cast really required?
--
Cheers,
David
On Wed, 4 Feb 2026 22:01:42 +0100 "David Hildenbrand (arm)" <david@kernel.org> wrote:
> > As David pointed out, the previous open coded version makes a few
> > unnecessary changes. Could you queue this one instead?
> >
>
> I'm late, maybe this is already upstream.
It's in mm-unstable. The second round of MM upstreaming is two weeks hence.
> >
> > /* Region to the left of the fault */
> > - r[1] = DEFINE_RANGE(pg.start,
> > - clamp_t(s64, r[2].start - 1, pg.start - 1, r[2].start));
> > + r[1] = DEFINE_RANGE(pg.start, r[2].start - 1);
> >
> > /* Region to the right of the fault: always valid for the common fault_idx=0 case. */
> > - r[0] = DEFINE_RANGE(clamp_t(s64, r[2].end + 1, r[2].end, pg.end + 1),
> > - pg.end);
> > + r[0] = DEFINE_RANGE(r[2].end + 1, pg.end);
>
> TBH, without the clamp that looks much more readable here.
me too.
> >
> > for (i = 0; i < ARRAY_SIZE(r); i++) {
> > const unsigned long addr = base_addr + r[i].start * PAGE_SIZE;
> > - const unsigned int nr_pages = range_len(&r[i]);
> > + const long nr_pages = (long)range_len(&r[i]);
> > struct page *page = folio_page(folio, r[i].start);
> >
> > if (nr_pages > 0)
> > - clear_contig_highpages(page, addr, nr_pages);
> > + clear_contig_highpages(page, addr, (unsigned int)nr_pages);
>
> Is that cast really required?
Seems not. The types for nr_pages are a bit chaotic - u64->long->uint.
Andrew Morton <akpm@linux-foundation.org> writes:
> On Wed, 4 Feb 2026 22:01:42 +0100 "David Hildenbrand (arm)" <david@kernel.org> wrote:
>
>> > As David pointed out, the previous open coded version makes a few
>> > unnecessary changes. Could you queue this one instead?
>> >
>>
>> I'm late, maybe this is already upstream.
>
> It's in mm-unstable. The second round of MM upstreaming is two weeks hence.
>
>> >
>> > /* Region to the left of the fault */
>> > - r[1] = DEFINE_RANGE(pg.start,
>> > - clamp_t(s64, r[2].start - 1, pg.start - 1, r[2].start));
>> > + r[1] = DEFINE_RANGE(pg.start, r[2].start - 1);
>> >
>> > /* Region to the right of the fault: always valid for the common fault_idx=0 case. */
>> > - r[0] = DEFINE_RANGE(clamp_t(s64, r[2].end + 1, r[2].end, pg.end + 1),
>> > - pg.end);
>> > + r[0] = DEFINE_RANGE(r[2].end + 1, pg.end);
>>
>> TBH, without the clamp that looks much more readable here.
>
> me too.
>
>> >
>> > for (i = 0; i < ARRAY_SIZE(r); i++) {
>> > const unsigned long addr = base_addr + r[i].start * PAGE_SIZE;
>> > - const unsigned int nr_pages = range_len(&r[i]);
>> > + const long nr_pages = (long)range_len(&r[i]);
>> > struct page *page = folio_page(folio, r[i].start);
>> >
>> > if (nr_pages > 0)
>> > - clear_contig_highpages(page, addr, nr_pages);
>> > + clear_contig_highpages(page, addr, (unsigned int)nr_pages);
>>
>> Is that cast really required?
>
> Seems not. The types for nr_pages are a bit chaotic - u64->long->uint.
Yes agreed.
The first u64 is because currently struct range only supports that.
Then the cast to signed long is because the range can be negative
and the clear_contig_highpages() is only done if nr_pages > 0.
And, the third one is almost certainly unnecessary for any realistic
hugepage size but since nr_pages is being truncating, I wanted that
to be explicit.
--
ankur
On 2/5/26 06:48, Ankur Arora wrote: > > Andrew Morton <akpm@linux-foundation.org> writes: > >> On Wed, 4 Feb 2026 22:01:42 +0100 "David Hildenbrand (arm)" <david@kernel.org> wrote: >> >>> >>> I'm late, maybe this is already upstream. >> >> It's in mm-unstable. The second round of MM upstreaming is two weeks hence. >> >>> >>> TBH, without the clamp that looks much more readable here. >> >> me too. >> >>> >>> Is that cast really required? >> >> Seems not. The types for nr_pages are a bit chaotic - u64->long->uint. > > Yes agreed. > > The first u64 is because currently struct range only supports that. > Then the cast to signed long is because the range can be negative > and the clear_contig_highpages() is only done if nr_pages > 0. That makes sense to me. > > And, the third one is almost certainly unnecessary for any realistic > hugepage size but since nr_pages is being truncating, I wanted that > to be explicit. But the non-silent truncation is no better? IOW, it doesn't matter. You could just make clear_contig_highpages() consume an unsigned long ... -- Cheers, David
David Hildenbrand (Arm) <david@kernel.org> writes:
> On 2/5/26 06:48, Ankur Arora wrote:
>> Andrew Morton <akpm@linux-foundation.org> writes:
>>
>>> On Wed, 4 Feb 2026 22:01:42 +0100 "David Hildenbrand (arm)" <david@kernel.org> wrote:
>>>
>>>>
>>>> I'm late, maybe this is already upstream.
>>>
>>> It's in mm-unstable. The second round of MM upstreaming is two weeks hence.
>>>
>>>>
>>>> TBH, without the clamp that looks much more readable here.
>>>
>>> me too.
>>>
>>>>
>>>> Is that cast really required?
>>>
>>> Seems not. The types for nr_pages are a bit chaotic - u64->long->uint.
>> Yes agreed.
>> The first u64 is because currently struct range only supports that.
>> Then the cast to signed long is because the range can be negative
>> and the clear_contig_highpages() is only done if nr_pages > 0.
>
> That makes sense to me.
>
>> And, the third one is almost certainly unnecessary for any realistic
>> hugepage size but since nr_pages is being truncating, I wanted that
>> to be explicit.
>
> But the non-silent truncation is no better? IOW, it doesn't matter.
I never seem to get them but I thought we had some kconfig option that
makes gcc give a warning to that effect.
I can update this patch to just implicitly truncate.
> You could just make clear_contig_highpages() consume an unsigned long ...
Unfortunately that'll be an even bigger mess. The clear_contig_highpages()
version in mm-stable uses the unsigned intness of nr_pages all over:
static void clear_contig_highpages(struct page *page, unsigned long addr,
unsigned int nr_pages)
{
unsigned int i, count;
/*
* When clearing we want to operate on the largest extent possible to
* allow for architecture specific extent based optimizations.
*
* However, since clear_user_highpages() (and primitives clear_user_pages(),
* clear_pages()), do not call cond_resched(), limit the unit size when
* running under non-preemptible scheduling models.
*/
const unsigned int unit = preempt_model_preemptible() ?
nr_pages : PROCESS_PAGES_NON_PREEMPT_BATCH;
might_sleep();
for (i = 0; i < nr_pages; i += count) {
cond_resched();
count = min(unit, nr_pages - i);
clear_user_highpages(page + i, addr + i * PAGE_SIZE, count);
}
}
Thanks
--
ankur
On 2/6/26 06:42, Ankur Arora wrote: > > David Hildenbrand (Arm) <david@kernel.org> writes: > >> On 2/5/26 06:48, Ankur Arora wrote: >>> Andrew Morton <akpm@linux-foundation.org> writes: >>> >>> Yes agreed. >>> The first u64 is because currently struct range only supports that. >>> Then the cast to signed long is because the range can be negative >>> and the clear_contig_highpages() is only done if nr_pages > 0. >> >> That makes sense to me. >> >>> And, the third one is almost certainly unnecessary for any realistic >>> hugepage size but since nr_pages is being truncating, I wanted that >>> to be explicit. >> >> But the non-silent truncation is no better? IOW, it doesn't matter. > > I never seem to get them but I thought we had some kconfig option that > makes gcc give a warning to that effect. I think we do it all the time :) > > I can update this patch to just implicitly truncate. > Yeah, I think the implicit once can just be dropped. >> You could just make clear_contig_highpages() consume an unsigned long ... > > Unfortunately that'll be an even bigger mess. The clear_contig_highpages() > version in mm-stable uses the unsigned intness of nr_pages all over: Right. In any case, thanks and feel free to add Acked-by: David Hildenbrand (Arm) <david@kernel.org> -- Cheers, David
© 2016 - 2026 Red Hat, Inc.