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 <lsahn@ooseel.net>
---
 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 <lsahn@ooseel.net> 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 <akpm@linux-foundation.org>님이 작성:
>
> On Sun,  9 Jun 2024 00:21:14 +0900 Leesoo Ahn <lsahn@ooseel.net> 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 <akpm@linux-foundation.org>님이 작성:
> >
> > On Sun,  9 Jun 2024 00:21:14 +0900 Leesoo Ahn <lsahn@ooseel.net> 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 <rppt@kernel.org>님이 작성:
>
> On Mon, Jun 10, 2024 at 12:39:28PM +0900, Leesoo Ahn wrote:
> > 2024년 6월 10일 (월) 오전 6:03, Andrew Morton <akpm@linux-foundation.org>님이 작성:
> > >
> > > On Sun,  9 Jun 2024 00:21:14 +0900 Leesoo Ahn <lsahn@ooseel.net> 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.