| Message ID | 20240608152114.867961-1-lsahn@wewakecorp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: sparse: clarify a variable name and its value | expand |
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
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.
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;
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(-)