Setting 'limit' variable to 0 might seem like it means "no limit". But
in the memblock API, 0 actually means the 'MEMBLOCK_ALLOC_ACCESSIBLE'
enum, which limits the physical address range based on
'memblock.current_limit'. This can be confusing.
To make things clearer, I suggest renaming the variable to
'limit_or_flag'. This name shows that the variable can either be a
number for limits or an enum for a flag. This way, readers will easily
understand what kind of value is being passed to the memblock API and
how it works without needing to look into the API details.
Signed-off-by: Leesoo Ahn <[email protected]>
---
mm/sparse.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/mm/sparse.c b/mm/sparse.c
index de40b2c73406..80e50ba26f24 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -333,7 +333,7 @@ sparse_early_usemaps_alloc_pgdat_section(struct pglist_data *pgdat,
unsigned long size)
{
struct mem_section_usage *usage;
- unsigned long goal, limit;
+ unsigned long goal, limit_or_flag;
int nid;
/*
* A page may contain usemaps for other sections preventing the
@@ -346,12 +346,13 @@ sparse_early_usemaps_alloc_pgdat_section(struct pglist_data *pgdat,
* this problem.
*/
goal = pgdat_to_phys(pgdat) & (PAGE_SECTION_MASK << PAGE_SHIFT);
- limit = goal + (1UL << PA_SECTION_SHIFT);
+ limit_or_flag = goal + (1UL << PA_SECTION_SHIFT);
nid = early_pfn_to_nid(goal >> PAGE_SHIFT);
again:
- usage = memblock_alloc_try_nid(size, SMP_CACHE_BYTES, goal, limit, nid);
- if (!usage && limit) {
- limit = 0;
+ usage = memblock_alloc_try_nid(size, SMP_CACHE_BYTES, goal,
+ limit_or_flag, nid);
+ if (!usage && (limit_or_flag != MEMBLOCK_ALLOC_ACCESSIBLE)) {
+ limit_or_flag = MEMBLOCK_ALLOC_ACCESSIBLE;
goto again;
}
return usage;
--
2.34.1
On Sun, 9 Jun 2024 00:21:14 +0900 Leesoo Ahn <[email protected]> wrote:
> Setting 'limit' variable to 0 might seem like it means "no limit". But
> in the memblock API, 0 actually means the 'MEMBLOCK_ALLOC_ACCESSIBLE'
> enum, which limits the physical address range based on
> 'memblock.current_limit'. This can be confusing.
Does it? From my reading, this meaning applies to the range end
address, in memblock_find_in_range_node()? If your interpretation is
correct, this should be documented in the relevant memblock kerneldoc.
> To make things clearer, I suggest renaming the variable to
> 'limit_or_flag'. This name shows that the variable can either be a
> number for limits or an enum for a flag. This way, readers will easily
> understand what kind of value is being passed to the memblock API and
> how it works without needing to look into the API details.
>
I think I'll cc Mike and run away ;)
2024년 6월 10일 (월) 오전 6:03, Andrew Morton <[email protected]>님이 작성:
>
> On Sun, 9 Jun 2024 00:21:14 +0900 Leesoo Ahn <[email protected]> wrote:
>
> > Setting 'limit' variable to 0 might seem like it means "no limit". But
> > in the memblock API, 0 actually means the 'MEMBLOCK_ALLOC_ACCESSIBLE'
> > enum, which limits the physical address range based on
> > 'memblock.current_limit'. This can be confusing.
>
> Does it? From my reading, this meaning applies to the range end
> address, in memblock_find_in_range_node()? If your interpretation is
> correct, this should be documented in the relevant memblock kerneldoc.
IMO, regardless of memblock documentation, it better uses
MEMBLOCK_ALLOC_ACCESSIBLE enum instead of 0 as a value for the variable.
Best regards,
Leesoo
On Mon, Jun 10, 2024 at 12:39:28PM +0900, Leesoo Ahn wrote:
> 2024년 6월 10일 (월) 오전 6:03, Andrew Morton <[email protected]>님이 작성:
> >
> > On Sun, 9 Jun 2024 00:21:14 +0900 Leesoo Ahn <[email protected]> wrote:
> >
> > > Setting 'limit' variable to 0 might seem like it means "no limit". But
> > > in the memblock API, 0 actually means the 'MEMBLOCK_ALLOC_ACCESSIBLE'
> > > enum, which limits the physical address range based on
> > > 'memblock.current_limit'. This can be confusing.
> >
> > Does it? From my reading, this meaning applies to the range end
> > address, in memblock_find_in_range_node()? If your interpretation is
> > correct, this should be documented in the relevant memblock kerneldoc.
It is :-P
> IMO, regardless of memblock documentation, it better uses
> MEMBLOCK_ALLOC_ACCESSIBLE enum instead of 0 as a value for the variable.
Using MEMBLOCK_ALLOC_ACCESSIBLE is a slight improvement, but renaming the
variable is not, IMO.
> Best regards,
> Leesoo
--
Sincerely yours,
Mike.
2024년 6월 10일 (월) 오후 3:08, Mike Rapoport <[email protected]>님이 작성:
>
> On Mon, Jun 10, 2024 at 12:39:28PM +0900, Leesoo Ahn wrote:
> > 2024년 6월 10일 (월) 오전 6:03, Andrew Morton <[email protected]>님이 작성:
> > >
> > > On Sun, 9 Jun 2024 00:21:14 +0900 Leesoo Ahn <[email protected]> wrote:
> > >
> > > > Setting 'limit' variable to 0 might seem like it means "no limit". But
> > > > in the memblock API, 0 actually means the 'MEMBLOCK_ALLOC_ACCESSIBLE'
> > > > enum, which limits the physical address range based on
> > > > 'memblock.current_limit'. This can be confusing.
> > >
> > > Does it? From my reading, this meaning applies to the range end
> > > address, in memblock_find_in_range_node()? If your interpretation is
> > > correct, this should be documented in the relevant memblock kerneldoc.
>
> It is :-P
>
> > IMO, regardless of memblock documentation, it better uses
> > MEMBLOCK_ALLOC_ACCESSIBLE enum instead of 0 as a value for the variable.
>
> Using MEMBLOCK_ALLOC_ACCESSIBLE is a slight improvement, but renaming the
> variable is not, IMO.
I will post v2 as it replaces 0 with MEMBLOCK_ALLOC_ACCESSIBLE without
modifying the variable.
Thank you, Andrew and Mike for the reviews.
>
> > Best regards,
> > Leesoo
>
> --
> Sincerely yours,
> Mike.
Best regards,
Leesoo.