linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] Cleanups and fixup for page_alloc
@ 2021-08-30 14:10 Miaohe Lin
  2021-08-30 14:10 ` [PATCH 1/6] mm/page_alloc.c: remove meaningless VM_BUG_ON() in pindex_to_order() Miaohe Lin
                   ` (5 more replies)
  0 siblings, 6 replies; 36+ messages in thread
From: Miaohe Lin @ 2021-08-30 14:10 UTC (permalink / raw)
  To: akpm; +Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, linmiaohe

Hi all,
This series contains cleanups to remove meaningless VM_BUG_ON(), use
helpers to simplify the code and remove obsolete comment. Also we fix
the potential accessing to uninitialized pcp page migratetype and so
on. More details can be found in the respective changelogs. Thanks!

Miaohe Lin (6):
  mm/page_alloc.c: remove meaningless VM_BUG_ON() in pindex_to_order()
  mm/page_alloc.c: simplify the code by using macro K()
  mm/page_alloc.c: remove obsolete comment in free_pcppages_bulk()
  mm/page_alloc.c: use helper function zone_spans_pfn()
  mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype
  mm/page_alloc.c: avoid allocating highmem pages via
    alloc_pages_exact_nid()

 mm/page_alloc.c | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/6] mm/page_alloc.c: remove meaningless VM_BUG_ON() in pindex_to_order()
  2021-08-30 14:10 [PATCH 0/6] Cleanups and fixup for page_alloc Miaohe Lin
@ 2021-08-30 14:10 ` Miaohe Lin
  2021-08-31 13:34   ` Mel Gorman
  2021-08-31 14:05   ` David Hildenbrand
  2021-08-30 14:10 ` [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K() Miaohe Lin
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 36+ messages in thread
From: Miaohe Lin @ 2021-08-30 14:10 UTC (permalink / raw)
  To: akpm; +Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, linmiaohe

It's meaningless to VM_BUG_ON() order != pageblock_order just after
setting order to pageblock_order. Remove it.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/page_alloc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 91edb930b8ab..dbb3338d9287 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -677,10 +677,8 @@ static inline int pindex_to_order(unsigned int pindex)
 	int order = pindex / MIGRATE_PCPTYPES;
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-	if (order > PAGE_ALLOC_COSTLY_ORDER) {
+	if (order > PAGE_ALLOC_COSTLY_ORDER)
 		order = pageblock_order;
-		VM_BUG_ON(order != pageblock_order);
-	}
 #else
 	VM_BUG_ON(order > PAGE_ALLOC_COSTLY_ORDER);
 #endif
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K()
  2021-08-30 14:10 [PATCH 0/6] Cleanups and fixup for page_alloc Miaohe Lin
  2021-08-30 14:10 ` [PATCH 1/6] mm/page_alloc.c: remove meaningless VM_BUG_ON() in pindex_to_order() Miaohe Lin
@ 2021-08-30 14:10 ` Miaohe Lin
  2021-08-31  8:54   ` Oleksandr Natalenko
                     ` (2 more replies)
  2021-08-30 14:10 ` [PATCH 3/6] mm/page_alloc.c: remove obsolete comment in free_pcppages_bulk() Miaohe Lin
                   ` (3 subsequent siblings)
  5 siblings, 3 replies; 36+ messages in thread
From: Miaohe Lin @ 2021-08-30 14:10 UTC (permalink / raw)
  To: akpm; +Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, linmiaohe

Use helper macro K() to convert the pages to the corresponding size.
Minor readability improvement.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/page_alloc.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index dbb3338d9287..d3983244f56f 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -8134,8 +8134,7 @@ unsigned long free_reserved_area(void *start, void *end, int poison, const char
 	}
 
 	if (pages && s)
-		pr_info("Freeing %s memory: %ldK\n",
-			s, pages << (PAGE_SHIFT - 10));
+		pr_info("Freeing %s memory: %ldK\n", s, K(pages));
 
 	return pages;
 }
@@ -8180,14 +8179,13 @@ void __init mem_init_print_info(void)
 		", %luK highmem"
 #endif
 		")\n",
-		nr_free_pages() << (PAGE_SHIFT - 10),
-		physpages << (PAGE_SHIFT - 10),
+		K(nr_free_pages()), K(physpages),
 		codesize >> 10, datasize >> 10, rosize >> 10,
 		(init_data_size + init_code_size) >> 10, bss_size >> 10,
-		(physpages - totalram_pages() - totalcma_pages) << (PAGE_SHIFT - 10),
-		totalcma_pages << (PAGE_SHIFT - 10)
+		K(physpages - totalram_pages() - totalcma_pages),
+		K(totalcma_pages)
 #ifdef	CONFIG_HIGHMEM
-		, totalhigh_pages() << (PAGE_SHIFT - 10)
+		, K(totalhigh_pages())
 #endif
 		);
 }
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 3/6] mm/page_alloc.c: remove obsolete comment in free_pcppages_bulk()
  2021-08-30 14:10 [PATCH 0/6] Cleanups and fixup for page_alloc Miaohe Lin
  2021-08-30 14:10 ` [PATCH 1/6] mm/page_alloc.c: remove meaningless VM_BUG_ON() in pindex_to_order() Miaohe Lin
  2021-08-30 14:10 ` [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K() Miaohe Lin
@ 2021-08-30 14:10 ` Miaohe Lin
  2021-08-31 13:38   ` Mel Gorman
  2021-08-30 14:10 ` [PATCH 4/6] mm/page_alloc.c: use helper function zone_spans_pfn() Miaohe Lin
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 36+ messages in thread
From: Miaohe Lin @ 2021-08-30 14:10 UTC (permalink / raw)
  To: akpm; +Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, linmiaohe

It's also confusing now. Remove it.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/page_alloc.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d3983244f56f..b5edcfe112aa 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1424,17 +1424,6 @@ static inline void prefetch_buddy(struct page *page)
 	prefetch(buddy);
 }
 
-/*
- * Frees a number of pages from the PCP lists
- * Assumes all pages on list are in same zone, and of same order.
- * count is the number of pages to free.
- *
- * If the zone was previously in an "all pages pinned" state then look to
- * see if this freeing clears that state.
- *
- * And clear the zone's pages_scanned counter, to hold off the "all pages are
- * pinned" detection logic.
- */
 static void free_pcppages_bulk(struct zone *zone, int count,
 					struct per_cpu_pages *pcp)
 {
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 4/6] mm/page_alloc.c: use helper function zone_spans_pfn()
  2021-08-30 14:10 [PATCH 0/6] Cleanups and fixup for page_alloc Miaohe Lin
                   ` (2 preceding siblings ...)
  2021-08-30 14:10 ` [PATCH 3/6] mm/page_alloc.c: remove obsolete comment in free_pcppages_bulk() Miaohe Lin
@ 2021-08-30 14:10 ` Miaohe Lin
  2021-08-31 13:39   ` Mel Gorman
  2021-08-31 14:08   ` David Hildenbrand
  2021-08-30 14:10 ` [PATCH 5/6] mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype Miaohe Lin
  2021-08-30 14:10 ` [PATCH 6/6] mm/page_alloc.c: avoid allocating highmem pages via alloc_pages_exact_nid() Miaohe Lin
  5 siblings, 2 replies; 36+ messages in thread
From: Miaohe Lin @ 2021-08-30 14:10 UTC (permalink / raw)
  To: akpm; +Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, linmiaohe

Use helper function zone_spans_pfn() to check whether pfn is within a
zone to simplify the code slightly.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b5edcfe112aa..7bb79e959ab4 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1576,7 +1576,7 @@ static void __meminit init_reserved_page(unsigned long pfn)
 	for (zid = 0; zid < MAX_NR_ZONES; zid++) {
 		struct zone *zone = &pgdat->node_zones[zid];
 
-		if (pfn >= zone->zone_start_pfn && pfn < zone_end_pfn(zone))
+		if (zone_spans_pfn(zone, pfn))
 			break;
 	}
 	__init_single_page(pfn_to_page(pfn), pfn, zid, nid);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 5/6] mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype
  2021-08-30 14:10 [PATCH 0/6] Cleanups and fixup for page_alloc Miaohe Lin
                   ` (3 preceding siblings ...)
  2021-08-30 14:10 ` [PATCH 4/6] mm/page_alloc.c: use helper function zone_spans_pfn() Miaohe Lin
@ 2021-08-30 14:10 ` Miaohe Lin
  2021-08-31 13:43   ` Mel Gorman
  2021-08-30 14:10 ` [PATCH 6/6] mm/page_alloc.c: avoid allocating highmem pages via alloc_pages_exact_nid() Miaohe Lin
  5 siblings, 1 reply; 36+ messages in thread
From: Miaohe Lin @ 2021-08-30 14:10 UTC (permalink / raw)
  To: akpm; +Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, linmiaohe

If it's not prepared to free unref page, the pcp page migratetype is
unset. Thus We will get rubbish from get_pcppage_migratetype() and
might list_del &page->lru again after it's already deleted from the
list leading to grumble about data corruption.

Fixes: 3dcbe270d8ec ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/page_alloc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 7bb79e959ab4..d87b7e6e9e6b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3415,8 +3415,10 @@ void free_unref_page_list(struct list_head *list)
 	/* Prepare pages for freeing */
 	list_for_each_entry_safe(page, next, list, lru) {
 		pfn = page_to_pfn(page);
-		if (!free_unref_page_prepare(page, pfn, 0))
+		if (!free_unref_page_prepare(page, pfn, 0)) {
 			list_del(&page->lru);
+			continue;
+		}
 
 		/*
 		 * Free isolated pages directly to the allocator, see
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 6/6] mm/page_alloc.c: avoid allocating highmem pages via alloc_pages_exact_nid()
  2021-08-30 14:10 [PATCH 0/6] Cleanups and fixup for page_alloc Miaohe Lin
                   ` (4 preceding siblings ...)
  2021-08-30 14:10 ` [PATCH 5/6] mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype Miaohe Lin
@ 2021-08-30 14:10 ` Miaohe Lin
  2021-08-30 14:24   ` Matthew Wilcox
  5 siblings, 1 reply; 36+ messages in thread
From: Miaohe Lin @ 2021-08-30 14:10 UTC (permalink / raw)
  To: akpm; +Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, linmiaohe

Don't use with __GFP_HIGHMEM because page_address() cannot represent
highmem pages without kmap(). Newly allocated pages would leak as
page_address() will return NULL for highmem pages here. But It works
now because the only caller does not specify __GFP_HIGHMEM now.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d87b7e6e9e6b..858fd45ecaea 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5639,7 +5639,7 @@ void * __meminit alloc_pages_exact_nid(int nid, size_t size, gfp_t gfp_mask)
 	if (WARN_ON_ONCE(gfp_mask & __GFP_COMP))
 		gfp_mask &= ~__GFP_COMP;
 
-	p = alloc_pages_node(nid, gfp_mask, order);
+	p = alloc_pages_node(nid, gfp_mask & ~__GFP_HIGHMEM, order);
 	if (!p)
 		return NULL;
 	return make_alloc_exact((unsigned long)page_address(p), order, size);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH 6/6] mm/page_alloc.c: avoid allocating highmem pages via alloc_pages_exact_nid()
  2021-08-30 14:10 ` [PATCH 6/6] mm/page_alloc.c: avoid allocating highmem pages via alloc_pages_exact_nid() Miaohe Lin
@ 2021-08-30 14:24   ` Matthew Wilcox
  2021-08-31  1:56     ` Miaohe Lin
  0 siblings, 1 reply; 36+ messages in thread
From: Matthew Wilcox @ 2021-08-30 14:24 UTC (permalink / raw)
  To: Miaohe Lin; +Cc: akpm, vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel

On Mon, Aug 30, 2021 at 10:10:51PM +0800, Miaohe Lin wrote:
> Don't use with __GFP_HIGHMEM because page_address() cannot represent
> highmem pages without kmap(). Newly allocated pages would leak as
> page_address() will return NULL for highmem pages here. But It works
> now because the only caller does not specify __GFP_HIGHMEM now.

This is a misunderstanding of how alloc_pages_exact() /
alloc_pages_exact_nid() work.  You simply can't call them with
GFP_HIGHMEM.

If you really must change anything here,
s/__GFP_COMP/(__GFP_COMP|__GFP_HIGHMEM)/g throughout both
alloc_pages_exact() and alloc_pages_exact_nid().

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 6/6] mm/page_alloc.c: avoid allocating highmem pages via alloc_pages_exact_nid()
  2021-08-30 14:24   ` Matthew Wilcox
@ 2021-08-31  1:56     ` Miaohe Lin
  2021-08-31 16:37       ` Vlastimil Babka
  0 siblings, 1 reply; 36+ messages in thread
From: Miaohe Lin @ 2021-08-31  1:56 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: akpm, vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel

On 2021/8/30 22:24, Matthew Wilcox wrote:
> On Mon, Aug 30, 2021 at 10:10:51PM +0800, Miaohe Lin wrote:
>> Don't use with __GFP_HIGHMEM because page_address() cannot represent
>> highmem pages without kmap(). Newly allocated pages would leak as
>> page_address() will return NULL for highmem pages here. But It works
>> now because the only caller does not specify __GFP_HIGHMEM now.
> 
> This is a misunderstanding of how alloc_pages_exact() /
> alloc_pages_exact_nid() work.  You simply can't call them with
> GFP_HIGHMEM.
> 

Yep, they can't work with GFP_HIGHMEM. So IMO it might be better to
get rid of GFP_HIGHMEM explicitly or add a comment to clarify this
situation to avoid future misbehavior. But this may be a unnecessary
worry... Do you prefer to not change anything here?

Many thanks.

> If you really must change anything here,
> s/__GFP_COMP/(__GFP_COMP|__GFP_HIGHMEM)/g throughout both
> alloc_pages_exact() and alloc_pages_exact_nid().
> .
>

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K()
  2021-08-30 14:10 ` [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K() Miaohe Lin
@ 2021-08-31  8:54   ` Oleksandr Natalenko
  2021-08-31 11:08     ` Miaohe Lin
  2021-08-31 13:35   ` Mel Gorman
  2021-08-31 14:07   ` David Hildenbrand
  2 siblings, 1 reply; 36+ messages in thread
From: Oleksandr Natalenko @ 2021-08-31  8:54 UTC (permalink / raw)
  To: akpm, Miaohe Lin
  Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, linmiaohe

Hello.

On pondělí 30. srpna 2021 16:10:47 CEST Miaohe Lin wrote:
> Use helper macro K() to convert the pages to the corresponding size.
> Minor readability improvement.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/page_alloc.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index dbb3338d9287..d3983244f56f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8134,8 +8134,7 @@ unsigned long free_reserved_area(void *start, void
> *end, int poison, const char }
> 
>  	if (pages && s)
> -		pr_info("Freeing %s memory: %ldK\n",
> -			s, pages << (PAGE_SHIFT - 10));
> +		pr_info("Freeing %s memory: %ldK\n", s, K(pages));
> 
>  	return pages;
>  }
> @@ -8180,14 +8179,13 @@ void __init mem_init_print_info(void)
>  		", %luK highmem"
>  #endif
>  		")\n",
> -		nr_free_pages() << (PAGE_SHIFT - 10),
> -		physpages << (PAGE_SHIFT - 10),
> +		K(nr_free_pages()), K(physpages),
>  		codesize >> 10, datasize >> 10, rosize >> 10,
>  		(init_data_size + init_code_size) >> 10, bss_size >> 10,
> -		(physpages - totalram_pages() - totalcma_pages) << (PAGE_SHIFT 
- 10),
> -		totalcma_pages << (PAGE_SHIFT - 10)
> +		K(physpages - totalram_pages() - totalcma_pages),
> +		K(totalcma_pages)
>  #ifdef	CONFIG_HIGHMEM
> -		, totalhigh_pages() << (PAGE_SHIFT - 10)
> +		, K(totalhigh_pages())
>  #endif
>  		);
>  }

(my concern is not quite within the scope of this submission, but I'll ask 
anyway)

Given we have this:

```
git grep '#define K(x)' v5.14
v5.14:drivers/base/node.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
v5.14:drivers/net/hamradio/scc.c:#define K(x) kiss->x
v5.14:kernel/debug/kdb/kdb_main.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
v5.14:mm/backing-dev.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
v5.14:mm/memcontrol.c:#define K(x) ((x) << (PAGE_SHIFT-10))
v5.14:mm/oom_kill.c:#define K(x) ((x) << (PAGE_SHIFT-10))
v5.14:mm/page_alloc.c:#define K(x) ((x) << (PAGE_SHIFT-10))
```

Shouldn't this macro go to some header file instead to get rid of define 
repetitions?

Thanks.

-- 
Oleksandr Natalenko (post-factum)



^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K()
  2021-08-31  8:54   ` Oleksandr Natalenko
@ 2021-08-31 11:08     ` Miaohe Lin
  2021-08-31 14:19       ` Oleksandr Natalenko
  0 siblings, 1 reply; 36+ messages in thread
From: Miaohe Lin @ 2021-08-31 11:08 UTC (permalink / raw)
  To: Oleksandr Natalenko
  Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, Andrew Morton

On 2021/8/31 16:54, Oleksandr Natalenko wrote:
> Hello.
> 
> On pondělí 30. srpna 2021 16:10:47 CEST Miaohe Lin wrote:
>> Use helper macro K() to convert the pages to the corresponding size.
>> Minor readability improvement.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/page_alloc.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index dbb3338d9287..d3983244f56f 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -8134,8 +8134,7 @@ unsigned long free_reserved_area(void *start, void
>> *end, int poison, const char }
>>
>>  	if (pages && s)
>> -		pr_info("Freeing %s memory: %ldK\n",
>> -			s, pages << (PAGE_SHIFT - 10));
>> +		pr_info("Freeing %s memory: %ldK\n", s, K(pages));
>>
>>  	return pages;
>>  }
>> @@ -8180,14 +8179,13 @@ void __init mem_init_print_info(void)
>>  		", %luK highmem"
>>  #endif
>>  		")\n",
>> -		nr_free_pages() << (PAGE_SHIFT - 10),
>> -		physpages << (PAGE_SHIFT - 10),
>> +		K(nr_free_pages()), K(physpages),
>>  		codesize >> 10, datasize >> 10, rosize >> 10,
>>  		(init_data_size + init_code_size) >> 10, bss_size >> 10,
>> -		(physpages - totalram_pages() - totalcma_pages) << (PAGE_SHIFT 
> - 10),
>> -		totalcma_pages << (PAGE_SHIFT - 10)
>> +		K(physpages - totalram_pages() - totalcma_pages),
>> +		K(totalcma_pages)
>>  #ifdef	CONFIG_HIGHMEM
>> -		, totalhigh_pages() << (PAGE_SHIFT - 10)
>> +		, K(totalhigh_pages())
>>  #endif
>>  		);
>>  }
> 
> (my concern is not quite within the scope of this submission, but I'll ask 
> anyway)
> 
> Given we have this:
> 
> ```
> git grep '#define K(x)' v5.14
> v5.14:drivers/base/node.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
> v5.14:drivers/net/hamradio/scc.c:#define K(x) kiss->x
> v5.14:kernel/debug/kdb/kdb_main.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
> v5.14:mm/backing-dev.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
> v5.14:mm/memcontrol.c:#define K(x) ((x) << (PAGE_SHIFT-10))
> v5.14:mm/oom_kill.c:#define K(x) ((x) << (PAGE_SHIFT-10))
> v5.14:mm/page_alloc.c:#define K(x) ((x) << (PAGE_SHIFT-10))
> ```
> 
> Shouldn't this macro go to some header file instead to get rid of define 
> repetitions?

Many thanks for figuring this out. I think we should get rid of these repetitions too.
But I'am not sure where this definition should be. Any suggestion? Thanks.

> 
> Thanks.
> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/6] mm/page_alloc.c: remove meaningless VM_BUG_ON() in pindex_to_order()
  2021-08-30 14:10 ` [PATCH 1/6] mm/page_alloc.c: remove meaningless VM_BUG_ON() in pindex_to_order() Miaohe Lin
@ 2021-08-31 13:34   ` Mel Gorman
  2021-08-31 14:05   ` David Hildenbrand
  1 sibling, 0 replies; 36+ messages in thread
From: Mel Gorman @ 2021-08-31 13:34 UTC (permalink / raw)
  To: Miaohe Lin; +Cc: akpm, vbabka, sfr, peterz, linux-mm, linux-kernel

On Mon, Aug 30, 2021 at 10:10:46PM +0800, Miaohe Lin wrote:
> It's meaningless to VM_BUG_ON() order != pageblock_order just after
> setting order to pageblock_order. Remove it.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K()
  2021-08-30 14:10 ` [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K() Miaohe Lin
  2021-08-31  8:54   ` Oleksandr Natalenko
@ 2021-08-31 13:35   ` Mel Gorman
  2021-08-31 14:07   ` David Hildenbrand
  2 siblings, 0 replies; 36+ messages in thread
From: Mel Gorman @ 2021-08-31 13:35 UTC (permalink / raw)
  To: Miaohe Lin; +Cc: akpm, vbabka, sfr, peterz, linux-mm, linux-kernel

On Mon, Aug 30, 2021 at 10:10:47PM +0800, Miaohe Lin wrote:
> Use helper macro K() to convert the pages to the corresponding size.
> Minor readability improvement.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/6] mm/page_alloc.c: remove obsolete comment in free_pcppages_bulk()
  2021-08-30 14:10 ` [PATCH 3/6] mm/page_alloc.c: remove obsolete comment in free_pcppages_bulk() Miaohe Lin
@ 2021-08-31 13:38   ` Mel Gorman
  2021-09-01  7:49     ` Miaohe Lin
  0 siblings, 1 reply; 36+ messages in thread
From: Mel Gorman @ 2021-08-31 13:38 UTC (permalink / raw)
  To: Miaohe Lin; +Cc: akpm, vbabka, sfr, peterz, linux-mm, linux-kernel

On Mon, Aug 30, 2021 at 10:10:48PM +0800, Miaohe Lin wrote:
> It's also confusing now. Remove it.
> 

Why is the whole comment obsolete?

The second two paragraphs about "all pages pinned" and pages_scanned is
obsolete and can go but the first paragraph is valid.

-- 
Mel Gorman
SUSE Labs

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 4/6] mm/page_alloc.c: use helper function zone_spans_pfn()
  2021-08-30 14:10 ` [PATCH 4/6] mm/page_alloc.c: use helper function zone_spans_pfn() Miaohe Lin
@ 2021-08-31 13:39   ` Mel Gorman
  2021-08-31 14:08   ` David Hildenbrand
  1 sibling, 0 replies; 36+ messages in thread
From: Mel Gorman @ 2021-08-31 13:39 UTC (permalink / raw)
  To: Miaohe Lin; +Cc: akpm, vbabka, sfr, peterz, linux-mm, linux-kernel

On Mon, Aug 30, 2021 at 10:10:49PM +0800, Miaohe Lin wrote:
> Use helper function zone_spans_pfn() to check whether pfn is within a
> zone to simplify the code slightly.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 5/6] mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype
  2021-08-30 14:10 ` [PATCH 5/6] mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype Miaohe Lin
@ 2021-08-31 13:43   ` Mel Gorman
  2021-08-31 14:23     ` David Hildenbrand
  2021-08-31 16:34     ` Vlastimil Babka
  0 siblings, 2 replies; 36+ messages in thread
From: Mel Gorman @ 2021-08-31 13:43 UTC (permalink / raw)
  To: Miaohe Lin; +Cc: akpm, vbabka, sfr, peterz, linux-mm, linux-kernel

On Mon, Aug 30, 2021 at 10:10:50PM +0800, Miaohe Lin wrote:
> If it's not prepared to free unref page, the pcp page migratetype is
> unset. Thus We will get rubbish from get_pcppage_migratetype() and
> might list_del &page->lru again after it's already deleted from the
> list leading to grumble about data corruption.
> 
> Fixes: 3dcbe270d8ec ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

This fix is fairly important. Take this patch out and send it on its own
so it gets picked up relatively quickly. It does not belong in a series
that is mostly cosmetic cleanups.


-- 
Mel Gorman
SUSE Labs

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/6] mm/page_alloc.c: remove meaningless VM_BUG_ON() in pindex_to_order()
  2021-08-30 14:10 ` [PATCH 1/6] mm/page_alloc.c: remove meaningless VM_BUG_ON() in pindex_to_order() Miaohe Lin
  2021-08-31 13:34   ` Mel Gorman
@ 2021-08-31 14:05   ` David Hildenbrand
  1 sibling, 0 replies; 36+ messages in thread
From: David Hildenbrand @ 2021-08-31 14:05 UTC (permalink / raw)
  To: Miaohe Lin, akpm; +Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel

On 30.08.21 16:10, Miaohe Lin wrote:
> It's meaningless to VM_BUG_ON() order != pageblock_order just after
> setting order to pageblock_order. Remove it.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>   mm/page_alloc.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 91edb930b8ab..dbb3338d9287 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -677,10 +677,8 @@ static inline int pindex_to_order(unsigned int pindex)
>   	int order = pindex / MIGRATE_PCPTYPES;
>   
>   #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> -	if (order > PAGE_ALLOC_COSTLY_ORDER) {
> +	if (order > PAGE_ALLOC_COSTLY_ORDER)
>   		order = pageblock_order;
> -		VM_BUG_ON(order != pageblock_order);
> -	}
>   #else
>   	VM_BUG_ON(order > PAGE_ALLOC_COSTLY_ORDER);
>   #endif
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K()
  2021-08-30 14:10 ` [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K() Miaohe Lin
  2021-08-31  8:54   ` Oleksandr Natalenko
  2021-08-31 13:35   ` Mel Gorman
@ 2021-08-31 14:07   ` David Hildenbrand
  2 siblings, 0 replies; 36+ messages in thread
From: David Hildenbrand @ 2021-08-31 14:07 UTC (permalink / raw)
  To: Miaohe Lin, akpm; +Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel

On 30.08.21 16:10, Miaohe Lin wrote:
> Use helper macro K() to convert the pages to the corresponding size.
> Minor readability improvement.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>   mm/page_alloc.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index dbb3338d9287..d3983244f56f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8134,8 +8134,7 @@ unsigned long free_reserved_area(void *start, void *end, int poison, const char
>   	}
>   
>   	if (pages && s)
> -		pr_info("Freeing %s memory: %ldK\n",
> -			s, pages << (PAGE_SHIFT - 10));
> +		pr_info("Freeing %s memory: %ldK\n", s, K(pages));
>   
>   	return pages;
>   }
> @@ -8180,14 +8179,13 @@ void __init mem_init_print_info(void)
>   		", %luK highmem"
>   #endif
>   		")\n",
> -		nr_free_pages() << (PAGE_SHIFT - 10),
> -		physpages << (PAGE_SHIFT - 10),
> +		K(nr_free_pages()), K(physpages),
>   		codesize >> 10, datasize >> 10, rosize >> 10,
>   		(init_data_size + init_code_size) >> 10, bss_size >> 10,
> -		(physpages - totalram_pages() - totalcma_pages) << (PAGE_SHIFT - 10),
> -		totalcma_pages << (PAGE_SHIFT - 10)
> +		K(physpages - totalram_pages() - totalcma_pages),
> +		K(totalcma_pages)
>   #ifdef	CONFIG_HIGHMEM
> -		, totalhigh_pages() << (PAGE_SHIFT - 10)
> +		, K(totalhigh_pages())
>   #endif
>   		);
>   }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 4/6] mm/page_alloc.c: use helper function zone_spans_pfn()
  2021-08-30 14:10 ` [PATCH 4/6] mm/page_alloc.c: use helper function zone_spans_pfn() Miaohe Lin
  2021-08-31 13:39   ` Mel Gorman
@ 2021-08-31 14:08   ` David Hildenbrand
  1 sibling, 0 replies; 36+ messages in thread
From: David Hildenbrand @ 2021-08-31 14:08 UTC (permalink / raw)
  To: Miaohe Lin, akpm; +Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel

On 30.08.21 16:10, Miaohe Lin wrote:
> Use helper function zone_spans_pfn() to check whether pfn is within a
> zone to simplify the code slightly.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>   mm/page_alloc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index b5edcfe112aa..7bb79e959ab4 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1576,7 +1576,7 @@ static void __meminit init_reserved_page(unsigned long pfn)
>   	for (zid = 0; zid < MAX_NR_ZONES; zid++) {
>   		struct zone *zone = &pgdat->node_zones[zid];
>   
> -		if (pfn >= zone->zone_start_pfn && pfn < zone_end_pfn(zone))
> +		if (zone_spans_pfn(zone, pfn))
>   			break;
>   	}
>   	__init_single_page(pfn_to_page(pfn), pfn, zid, nid);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K()
  2021-08-31 11:08     ` Miaohe Lin
@ 2021-08-31 14:19       ` Oleksandr Natalenko
  2021-09-01  7:37         ` Miaohe Lin
  0 siblings, 1 reply; 36+ messages in thread
From: Oleksandr Natalenko @ 2021-08-31 14:19 UTC (permalink / raw)
  To: Miaohe Lin
  Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, Andrew Morton

Hello.

On úterý 31. srpna 2021 13:08:42 CEST Miaohe Lin wrote:
> On 2021/8/31 16:54, Oleksandr Natalenko wrote:
> > Hello.
> > 
> > On pondělí 30. srpna 2021 16:10:47 CEST Miaohe Lin wrote:
> >> Use helper macro K() to convert the pages to the corresponding size.
> >> Minor readability improvement.
> >> 
> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> >> ---
> >> 
> >>  mm/page_alloc.c | 12 +++++-------
> >>  1 file changed, 5 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> >> index dbb3338d9287..d3983244f56f 100644
> >> --- a/mm/page_alloc.c
> >> +++ b/mm/page_alloc.c
> >> @@ -8134,8 +8134,7 @@ unsigned long free_reserved_area(void *start, void
> >> *end, int poison, const char }
> >> 
> >>  	if (pages && s)
> >> 
> >> -		pr_info("Freeing %s memory: %ldK\n",
> >> -			s, pages << (PAGE_SHIFT - 10));
> >> +		pr_info("Freeing %s memory: %ldK\n", s, K(pages));
> >> 
> >>  	return pages;
> >>  
> >>  }
> >> 
> >> @@ -8180,14 +8179,13 @@ void __init mem_init_print_info(void)
> >> 
> >>  		", %luK highmem"
> >>  
> >>  #endif
> >>  
> >>  		")\n",
> >> 
> >> -		nr_free_pages() << (PAGE_SHIFT - 10),
> >> -		physpages << (PAGE_SHIFT - 10),
> >> +		K(nr_free_pages()), K(physpages),
> >> 
> >>  		codesize >> 10, datasize >> 10, rosize >> 10,
> >>  		(init_data_size + init_code_size) >> 10, bss_size >> 10,
> >> 
> >> -		(physpages - totalram_pages() - totalcma_pages) << (PAGE_SHIFT
> > 
> > - 10),
> > 
> >> -		totalcma_pages << (PAGE_SHIFT - 10)
> >> +		K(physpages - totalram_pages() - totalcma_pages),
> >> +		K(totalcma_pages)
> >> 
> >>  #ifdef	CONFIG_HIGHMEM
> >> 
> >> -		, totalhigh_pages() << (PAGE_SHIFT - 10)
> >> +		, K(totalhigh_pages())
> >> 
> >>  #endif
> >>  
> >>  		);
> >>  
> >>  }
> > 
> > (my concern is not quite within the scope of this submission, but I'll ask
> > anyway)
> > 
> > Given we have this:
> > 
> > ```
> > git grep '#define K(x)' v5.14
> > v5.14:drivers/base/node.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
> > v5.14:drivers/net/hamradio/scc.c:#define K(x) kiss->x
> > v5.14:kernel/debug/kdb/kdb_main.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
> > v5.14:mm/backing-dev.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
> > v5.14:mm/memcontrol.c:#define K(x) ((x) << (PAGE_SHIFT-10))
> > v5.14:mm/oom_kill.c:#define K(x) ((x) << (PAGE_SHIFT-10))
> > v5.14:mm/page_alloc.c:#define K(x) ((x) << (PAGE_SHIFT-10))
> > ```
> > 
> > Shouldn't this macro go to some header file instead to get rid of define
> > repetitions?
> 
> Many thanks for figuring this out. I think we should get rid of these
> repetitions too. But I'am not sure where this definition should be. Any
> suggestion? Thanks.

I'm not sure what place suits best. At first I thought maybe linux/mm.h or 
linux/mm_inline.h, but since PAGE_SHIFT is declared in asm-generic/page.h, 
probably K(x) can also go there as well?

-- 
Oleksandr Natalenko (post-factum)



^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 5/6] mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype
  2021-08-31 13:43   ` Mel Gorman
@ 2021-08-31 14:23     ` David Hildenbrand
  2021-09-01  8:02       ` Miaohe Lin
  2021-08-31 16:34     ` Vlastimil Babka
  1 sibling, 1 reply; 36+ messages in thread
From: David Hildenbrand @ 2021-08-31 14:23 UTC (permalink / raw)
  To: Mel Gorman, Miaohe Lin; +Cc: akpm, vbabka, sfr, peterz, linux-mm, linux-kernel

On 31.08.21 15:43, Mel Gorman wrote:
> On Mon, Aug 30, 2021 at 10:10:50PM +0800, Miaohe Lin wrote:
>> If it's not prepared to free unref page, the pcp page migratetype is
>> unset. Thus We will get rubbish from get_pcppage_migratetype() and
>> might list_del &page->lru again after it's already deleted from the
>> list leading to grumble about data corruption.
>>
>> Fixes: 3dcbe270d8ec ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> 
> Acked-by: Mel Gorman <mgorman@techsingularity.net>
> 
> This fix is fairly important. Take this patch out and send it on its own
> so it gets picked up relatively quickly. It does not belong in a series
> that is mostly cosmetic cleanups.

I think the commit id is wrong. Shouldn't that be

df1acc856923 ("mm/page_alloc: avoid conflating IRQs disabled with 
zone->lock")

?

-- 
Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 5/6] mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype
  2021-08-31 13:43   ` Mel Gorman
  2021-08-31 14:23     ` David Hildenbrand
@ 2021-08-31 16:34     ` Vlastimil Babka
  2021-09-01  8:04       ` Miaohe Lin
  1 sibling, 1 reply; 36+ messages in thread
From: Vlastimil Babka @ 2021-08-31 16:34 UTC (permalink / raw)
  To: Mel Gorman, Miaohe Lin; +Cc: akpm, sfr, peterz, linux-mm, linux-kernel

On 8/31/21 15:43, Mel Gorman wrote:
> On Mon, Aug 30, 2021 at 10:10:50PM +0800, Miaohe Lin wrote:
>> If it's not prepared to free unref page, the pcp page migratetype is
>> unset. Thus We will get rubbish from get_pcppage_migratetype() and
>> might list_del &page->lru again after it's already deleted from the
>> list leading to grumble about data corruption.
>> 
>> Fixes: 3dcbe270d8ec ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> 
> Acked-by: Mel Gorman <mgorman@techsingularity.net>
> 
> This fix is fairly important. Take this patch out and send it on its own
> so it gets picked up relatively quickly. It does not belong in a series
> that is mostly cosmetic cleanups.

Also Cc: stable, please.




^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 6/6] mm/page_alloc.c: avoid allocating highmem pages via alloc_pages_exact_nid()
  2021-08-31  1:56     ` Miaohe Lin
@ 2021-08-31 16:37       ` Vlastimil Babka
  2021-09-01  8:05         ` Miaohe Lin
  0 siblings, 1 reply; 36+ messages in thread
From: Vlastimil Babka @ 2021-08-31 16:37 UTC (permalink / raw)
  To: Miaohe Lin, Matthew Wilcox
  Cc: akpm, sfr, peterz, mgorman, linux-mm, linux-kernel

On 8/31/21 03:56, Miaohe Lin wrote:
> On 2021/8/30 22:24, Matthew Wilcox wrote:
>> On Mon, Aug 30, 2021 at 10:10:51PM +0800, Miaohe Lin wrote:
>>> Don't use with __GFP_HIGHMEM because page_address() cannot represent
>>> highmem pages without kmap(). Newly allocated pages would leak as
>>> page_address() will return NULL for highmem pages here. But It works
>>> now because the only caller does not specify __GFP_HIGHMEM now.
>> 
>> This is a misunderstanding of how alloc_pages_exact() /
>> alloc_pages_exact_nid() work.  You simply can't call them with
>> GFP_HIGHMEM.
>> 
> 
> Yep, they can't work with GFP_HIGHMEM. So IMO it might be better to
> get rid of GFP_HIGHMEM explicitly or add a comment to clarify this
> situation to avoid future misbehavior. But this may be a unnecessary
> worry... Do you prefer to not change anything here?

I agree with the suggestion below...

> Many thanks.
> 
>> If you really must change anything here,
>> s/__GFP_COMP/(__GFP_COMP|__GFP_HIGHMEM)/g throughout both
>> alloc_pages_exact() and alloc_pages_exact_nid().

... which means __GFP_HIGHMEM would be stripped and additionally there would
be a warning.


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K()
  2021-08-31 14:19       ` Oleksandr Natalenko
@ 2021-09-01  7:37         ` Miaohe Lin
  2021-09-01  7:46           ` Oleksandr Natalenko
  2021-09-01 21:25           ` David Laight
  0 siblings, 2 replies; 36+ messages in thread
From: Miaohe Lin @ 2021-09-01  7:37 UTC (permalink / raw)
  To: Oleksandr Natalenko
  Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, Andrew Morton

On 2021/8/31 22:19, Oleksandr Natalenko wrote:
> Hello.
> 
> On úterý 31. srpna 2021 13:08:42 CEST Miaohe Lin wrote:
>> On 2021/8/31 16:54, Oleksandr Natalenko wrote:
>>> Hello.
>>>
>>> On pondělí 30. srpna 2021 16:10:47 CEST Miaohe Lin wrote:
>>>> Use helper macro K() to convert the pages to the corresponding size.
>>>> Minor readability improvement.
>>>>
>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>>> ---
>>>>
>>>>  mm/page_alloc.c | 12 +++++-------
>>>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>> index dbb3338d9287..d3983244f56f 100644
>>>> --- a/mm/page_alloc.c
>>>> +++ b/mm/page_alloc.c
>>>> @@ -8134,8 +8134,7 @@ unsigned long free_reserved_area(void *start, void
>>>> *end, int poison, const char }
>>>>
>>>>  	if (pages && s)
>>>>
>>>> -		pr_info("Freeing %s memory: %ldK\n",
>>>> -			s, pages << (PAGE_SHIFT - 10));
>>>> +		pr_info("Freeing %s memory: %ldK\n", s, K(pages));
>>>>
>>>>  	return pages;
>>>>  
>>>>  }
>>>>
>>>> @@ -8180,14 +8179,13 @@ void __init mem_init_print_info(void)
>>>>
>>>>  		", %luK highmem"
>>>>  
>>>>  #endif
>>>>  
>>>>  		")\n",
>>>>
>>>> -		nr_free_pages() << (PAGE_SHIFT - 10),
>>>> -		physpages << (PAGE_SHIFT - 10),
>>>> +		K(nr_free_pages()), K(physpages),
>>>>
>>>>  		codesize >> 10, datasize >> 10, rosize >> 10,
>>>>  		(init_data_size + init_code_size) >> 10, bss_size >> 10,
>>>>
>>>> -		(physpages - totalram_pages() - totalcma_pages) << (PAGE_SHIFT
>>>
>>> - 10),
>>>
>>>> -		totalcma_pages << (PAGE_SHIFT - 10)
>>>> +		K(physpages - totalram_pages() - totalcma_pages),
>>>> +		K(totalcma_pages)
>>>>
>>>>  #ifdef	CONFIG_HIGHMEM
>>>>
>>>> -		, totalhigh_pages() << (PAGE_SHIFT - 10)
>>>> +		, K(totalhigh_pages())
>>>>
>>>>  #endif
>>>>  
>>>>  		);
>>>>  
>>>>  }
>>>
>>> (my concern is not quite within the scope of this submission, but I'll ask
>>> anyway)
>>>
>>> Given we have this:
>>>
>>> ```
>>> git grep '#define K(x)' v5.14
>>> v5.14:drivers/base/node.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
>>> v5.14:drivers/net/hamradio/scc.c:#define K(x) kiss->x
>>> v5.14:kernel/debug/kdb/kdb_main.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
>>> v5.14:mm/backing-dev.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
>>> v5.14:mm/memcontrol.c:#define K(x) ((x) << (PAGE_SHIFT-10))
>>> v5.14:mm/oom_kill.c:#define K(x) ((x) << (PAGE_SHIFT-10))
>>> v5.14:mm/page_alloc.c:#define K(x) ((x) << (PAGE_SHIFT-10))
>>> ```
>>>
>>> Shouldn't this macro go to some header file instead to get rid of define
>>> repetitions?
>>
>> Many thanks for figuring this out. I think we should get rid of these
>> repetitions too. But I'am not sure where this definition should be. Any
>> suggestion? Thanks.
> 
> I'm not sure what place suits best. At first I thought maybe linux/mm.h or 
> linux/mm_inline.h, but since PAGE_SHIFT is declared in asm-generic/page.h, 
> probably K(x) can also go there as well?

K(x) is relevant with PAGE_SHIFT. So I think K(x) can also go asm-generic/page.h too.
Am I supposed to do this when free or will you kindly do this?

Many thanks.

> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K()
  2021-09-01  7:37         ` Miaohe Lin
@ 2021-09-01  7:46           ` Oleksandr Natalenko
  2021-09-01  8:17             ` Miaohe Lin
  2021-09-01 21:25           ` David Laight
  1 sibling, 1 reply; 36+ messages in thread
From: Oleksandr Natalenko @ 2021-09-01  7:46 UTC (permalink / raw)
  To: Miaohe Lin
  Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, Andrew Morton

Hello.

On středa 1. září 2021 9:37:49 CEST Miaohe Lin wrote:
> On 2021/8/31 22:19, Oleksandr Natalenko wrote:
> > On úterý 31. srpna 2021 13:08:42 CEST Miaohe Lin wrote:
> >> On 2021/8/31 16:54, Oleksandr Natalenko wrote:
> >>> On pondělí 30. srpna 2021 16:10:47 CEST Miaohe Lin wrote:
> >>>> Use helper macro K() to convert the pages to the corresponding size.
> >>>> Minor readability improvement.
> >>>> 
> >>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> >>>> ---
> >>>> 
> >>>>  mm/page_alloc.c | 12 +++++-------
> >>>>  1 file changed, 5 insertions(+), 7 deletions(-)
> >>>> 
> >>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> >>>> index dbb3338d9287..d3983244f56f 100644
> >>>> --- a/mm/page_alloc.c
> >>>> +++ b/mm/page_alloc.c
> >>>> @@ -8134,8 +8134,7 @@ unsigned long free_reserved_area(void *start,
> >>>> void
> >>>> *end, int poison, const char }
> >>>> 
> >>>>  	if (pages && s)
> >>>> 
> >>>> -		pr_info("Freeing %s memory: %ldK\n",
> >>>> -			s, pages << (PAGE_SHIFT - 10));
> >>>> +		pr_info("Freeing %s memory: %ldK\n", s, K(pages));
> >>>> 
> >>>>  	return pages;
> >>>>  
> >>>>  }
> >>>> 
> >>>> @@ -8180,14 +8179,13 @@ void __init mem_init_print_info(void)
> >>>> 
> >>>>  		", %luK highmem"
> >>>>  
> >>>>  #endif
> >>>>  
> >>>>  		")\n",
> >>>> 
> >>>> -		nr_free_pages() << (PAGE_SHIFT - 10),
> >>>> -		physpages << (PAGE_SHIFT - 10),
> >>>> +		K(nr_free_pages()), K(physpages),
> >>>> 
> >>>>  		codesize >> 10, datasize >> 10, rosize >> 10,
> >>>>  		(init_data_size + init_code_size) >> 10, bss_size >> 10,
> >>>> 
> >>>> -		(physpages - totalram_pages() - totalcma_pages) << 
(PAGE_SHIFT
> >>> 
> >>> - 10),
> >>> 
> >>>> -		totalcma_pages << (PAGE_SHIFT - 10)
> >>>> +		K(physpages - totalram_pages() - totalcma_pages),
> >>>> +		K(totalcma_pages)
> >>>> 
> >>>>  #ifdef	CONFIG_HIGHMEM
> >>>> 
> >>>> -		, totalhigh_pages() << (PAGE_SHIFT - 10)
> >>>> +		, K(totalhigh_pages())
> >>>> 
> >>>>  #endif
> >>>>  
> >>>>  		);
> >>>>  
> >>>>  }
> >>> 
> >>> (my concern is not quite within the scope of this submission, but I'll
> >>> ask
> >>> anyway)
> >>> 
> >>> Given we have this:
> >>> 
> >>> ```
> >>> git grep '#define K(x)' v5.14
> >>> v5.14:drivers/base/node.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
> >>> v5.14:drivers/net/hamradio/scc.c:#define K(x) kiss->x
> >>> v5.14:kernel/debug/kdb/kdb_main.c:#define K(x) ((x) << (PAGE_SHIFT -
> >>> 10))
> >>> v5.14:mm/backing-dev.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
> >>> v5.14:mm/memcontrol.c:#define K(x) ((x) << (PAGE_SHIFT-10))
> >>> v5.14:mm/oom_kill.c:#define K(x) ((x) << (PAGE_SHIFT-10))
> >>> v5.14:mm/page_alloc.c:#define K(x) ((x) << (PAGE_SHIFT-10))
> >>> ```
> >>> 
> >>> Shouldn't this macro go to some header file instead to get rid of define
> >>> repetitions?
> >> 
> >> Many thanks for figuring this out. I think we should get rid of these
> >> repetitions too. But I'am not sure where this definition should be. Any
> >> suggestion? Thanks.
> > 
> > I'm not sure what place suits best. At first I thought maybe linux/mm.h or
> > linux/mm_inline.h, but since PAGE_SHIFT is declared in asm-generic/page.h,
> > probably K(x) can also go there as well?
> 
> K(x) is relevant with PAGE_SHIFT. So I think K(x) can also go
> asm-generic/page.h too.

Actually, the comment in this file says:

```
4 /*
5  * Generic page.h implementation, for NOMMU architectures.
6  * This provides the dummy definitions for the memory management.
7  */
```

so it seems I was wrong about this being an appropriate place.

> Am I supposed to do this when free or will you
> kindly do this?

Let me just try to cram this into mm.h and send it out, and then we'll see 
what comments people suggest.

Thanks.

-- 
Oleksandr Natalenko (post-factum)



^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/6] mm/page_alloc.c: remove obsolete comment in free_pcppages_bulk()
  2021-08-31 13:38   ` Mel Gorman
@ 2021-09-01  7:49     ` Miaohe Lin
  2021-09-01 15:14       ` Mel Gorman
  0 siblings, 1 reply; 36+ messages in thread
From: Miaohe Lin @ 2021-09-01  7:49 UTC (permalink / raw)
  To: Mel Gorman; +Cc: akpm, vbabka, sfr, peterz, linux-mm, linux-kernel

On 2021/8/31 21:38, Mel Gorman wrote:
> On Mon, Aug 30, 2021 at 10:10:48PM +0800, Miaohe Lin wrote:
>> It's also confusing now. Remove it.
>>
> 
> Why is the whole comment obsolete?
> 
> The second two paragraphs about "all pages pinned" and pages_scanned is
> obsolete and can go but the first paragraph is valid.
> 

I think the first paragraph is invalid due to the below statement:
"Assumes all pages on list are in same zone, and of same order."
There are NR_PCP_LISTS lists and PAGE_ALLOC_COSTLY_ORDER + 1 + NR_PCP_THP
orders in pcp. So I think it's obsolete.

Should I delete this statement in the first paragraph only?

Many Thanks.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 5/6] mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype
  2021-08-31 14:23     ` David Hildenbrand
@ 2021-09-01  8:02       ` Miaohe Lin
  2021-09-01  8:10         ` David Hildenbrand
  0 siblings, 1 reply; 36+ messages in thread
From: Miaohe Lin @ 2021-09-01  8:02 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: akpm, vbabka, sfr, peterz, linux-mm, linux-kernel, Mel Gorman

On 2021/8/31 22:23, David Hildenbrand wrote:
> On 31.08.21 15:43, Mel Gorman wrote:
>> On Mon, Aug 30, 2021 at 10:10:50PM +0800, Miaohe Lin wrote:
>>> If it's not prepared to free unref page, the pcp page migratetype is
>>> unset. Thus We will get rubbish from get_pcppage_migratetype() and
>>> might list_del &page->lru again after it's already deleted from the
>>> list leading to grumble about data corruption.
>>>
>>> Fixes: 3dcbe270d8ec ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>
>> Acked-by: Mel Gorman <mgorman@techsingularity.net>
>>
>> This fix is fairly important. Take this patch out and send it on its own
>> so it gets picked up relatively quickly. It does not belong in a series
>> that is mostly cosmetic cleanups.
> 
> I think the commit id is wrong. Shouldn't that be
> 
> df1acc856923 ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
> 
> ?
> 

Many thanks for pointing this out.

I used to save the git log in a file to make life easier. But it seems this leads
to the old commit id above.

commit 3dcbe270d8ec57e534f5c605279cdc3ceb1f044a
Author: Mel Gorman <mgorman@techsingularity.net>
Date:   Fri Jun 4 14:20:03 2021 +1000

    mm/page_alloc: avoid conflating IRQs disabled with zone->lock

git name-rev 3dcbe270d8ec
3dcbe270d8ec tags/next-20210604~2^2~196

vs

commit df1acc856923c0a65c28b588585449106c316b71
Author: Mel Gorman <mgorman@techsingularity.net>
Date:   Mon Jun 28 19:42:00 2021 -0700

    mm/page_alloc: avoid conflating IRQs disabled with zone->lock

git name-rev df1acc856923
df1acc856923 tags/next-20210630~2^2~278

Their contents are same but with different commit id. The previous one
could have been git-rebased.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 5/6] mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype
  2021-08-31 16:34     ` Vlastimil Babka
@ 2021-09-01  8:04       ` Miaohe Lin
  0 siblings, 0 replies; 36+ messages in thread
From: Miaohe Lin @ 2021-09-01  8:04 UTC (permalink / raw)
  To: Vlastimil Babka, Mel Gorman; +Cc: akpm, sfr, peterz, linux-mm, linux-kernel

On 2021/9/1 0:34, Vlastimil Babka wrote:
> On 8/31/21 15:43, Mel Gorman wrote:
>> On Mon, Aug 30, 2021 at 10:10:50PM +0800, Miaohe Lin wrote:
>>> If it's not prepared to free unref page, the pcp page migratetype is
>>> unset. Thus We will get rubbish from get_pcppage_migratetype() and
>>> might list_del &page->lru again after it's already deleted from the
>>> list leading to grumble about data corruption.
>>>
>>> Fixes: 3dcbe270d8ec ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>
>> Acked-by: Mel Gorman <mgorman@techsingularity.net>
>>
>> This fix is fairly important. Take this patch out and send it on its own
>> so it gets picked up relatively quickly. It does not belong in a series
>> that is mostly cosmetic cleanups.
> 
> Also Cc: stable, please.
> 
> 

Will do. Many thanks for both of your suggestions!

> 
> .
> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 6/6] mm/page_alloc.c: avoid allocating highmem pages via alloc_pages_exact_nid()
  2021-08-31 16:37       ` Vlastimil Babka
@ 2021-09-01  8:05         ` Miaohe Lin
  0 siblings, 0 replies; 36+ messages in thread
From: Miaohe Lin @ 2021-09-01  8:05 UTC (permalink / raw)
  To: Vlastimil Babka, Matthew Wilcox
  Cc: akpm, sfr, peterz, mgorman, linux-mm, linux-kernel

On 2021/9/1 0:37, Vlastimil Babka wrote:
> On 8/31/21 03:56, Miaohe Lin wrote:
>> On 2021/8/30 22:24, Matthew Wilcox wrote:
>>> On Mon, Aug 30, 2021 at 10:10:51PM +0800, Miaohe Lin wrote:
>>>> Don't use with __GFP_HIGHMEM because page_address() cannot represent
>>>> highmem pages without kmap(). Newly allocated pages would leak as
>>>> page_address() will return NULL for highmem pages here. But It works
>>>> now because the only caller does not specify __GFP_HIGHMEM now.
>>>
>>> This is a misunderstanding of how alloc_pages_exact() /
>>> alloc_pages_exact_nid() work.  You simply can't call them with
>>> GFP_HIGHMEM.
>>>
>>
>> Yep, they can't work with GFP_HIGHMEM. So IMO it might be better to
>> get rid of GFP_HIGHMEM explicitly or add a comment to clarify this
>> situation to avoid future misbehavior. But this may be a unnecessary
>> worry... Do you prefer to not change anything here?
> 
> I agree with the suggestion below...
> 
>> Many thanks.
>>
>>> If you really must change anything here,
>>> s/__GFP_COMP/(__GFP_COMP|__GFP_HIGHMEM)/g throughout both
>>> alloc_pages_exact() and alloc_pages_exact_nid().
> 
> ... which means __GFP_HIGHMEM would be stripped and additionally there would
> be a warning.
> 

Looks good for me. Will do. Many thanks!

> .
> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 5/6] mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype
  2021-09-01  8:02       ` Miaohe Lin
@ 2021-09-01  8:10         ` David Hildenbrand
  2021-09-01  8:18           ` Miaohe Lin
  0 siblings, 1 reply; 36+ messages in thread
From: David Hildenbrand @ 2021-09-01  8:10 UTC (permalink / raw)
  To: Miaohe Lin; +Cc: akpm, vbabka, sfr, peterz, linux-mm, linux-kernel, Mel Gorman

On 01.09.21 10:02, Miaohe Lin wrote:
> On 2021/8/31 22:23, David Hildenbrand wrote:
>> On 31.08.21 15:43, Mel Gorman wrote:
>>> On Mon, Aug 30, 2021 at 10:10:50PM +0800, Miaohe Lin wrote:
>>>> If it's not prepared to free unref page, the pcp page migratetype is
>>>> unset. Thus We will get rubbish from get_pcppage_migratetype() and
>>>> might list_del &page->lru again after it's already deleted from the
>>>> list leading to grumble about data corruption.
>>>>
>>>> Fixes: 3dcbe270d8ec ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>>
>>> Acked-by: Mel Gorman <mgorman@techsingularity.net>
>>>
>>> This fix is fairly important. Take this patch out and send it on its own
>>> so it gets picked up relatively quickly. It does not belong in a series
>>> that is mostly cosmetic cleanups.
>>
>> I think the commit id is wrong. Shouldn't that be
>>
>> df1acc856923 ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
>>
>> ?
>>
> 
> Many thanks for pointing this out.
> 
> I used to save the git log in a file to make life easier. But it seems this leads
> to the old commit id above.
> 
> commit 3dcbe270d8ec57e534f5c605279cdc3ceb1f044a
> Author: Mel Gorman <mgorman@techsingularity.net>
> Date:   Fri Jun 4 14:20:03 2021 +1000
> 
>      mm/page_alloc: avoid conflating IRQs disabled with zone->lock
> 
> git name-rev 3dcbe270d8ec
> 3dcbe270d8ec tags/next-20210604~2^2~196
> 
> vs
> 
> commit df1acc856923c0a65c28b588585449106c316b71
> Author: Mel Gorman <mgorman@techsingularity.net>
> Date:   Mon Jun 28 19:42:00 2021 -0700
> 
>      mm/page_alloc: avoid conflating IRQs disabled with zone->lock
> 
> git name-rev df1acc856923
> df1acc856923 tags/next-20210630~2^2~278
> 
> Their contents are same but with different commit id. The previous one
> could have been git-rebased.
> 

-mm tree commit ids keep changing until patches are upstream. Therefore, 
you can observe changing commit ids in -next. Always use the ones from 
Linus' tree, they are stable.

-- 
Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K()
  2021-09-01  7:46           ` Oleksandr Natalenko
@ 2021-09-01  8:17             ` Miaohe Lin
  0 siblings, 0 replies; 36+ messages in thread
From: Miaohe Lin @ 2021-09-01  8:17 UTC (permalink / raw)
  To: Oleksandr Natalenko
  Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, Andrew Morton

On 2021/9/1 15:46, Oleksandr Natalenko wrote:
> Hello.
> 
> On středa 1. září 2021 9:37:49 CEST Miaohe Lin wrote:
>> On 2021/8/31 22:19, Oleksandr Natalenko wrote:
>>> On úterý 31. srpna 2021 13:08:42 CEST Miaohe Lin wrote:
>>>> On 2021/8/31 16:54, Oleksandr Natalenko wrote:
>>>>> On pondělí 30. srpna 2021 16:10:47 CEST Miaohe Lin wrote:
>>>>>> Use helper macro K() to convert the pages to the corresponding size.
>>>>>> Minor readability improvement.
>>>>>>
>>>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>>>>> ---
>>>>>>
>>>>>>  mm/page_alloc.c | 12 +++++-------
>>>>>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>>>> index dbb3338d9287..d3983244f56f 100644
>>>>>> --- a/mm/page_alloc.c
>>>>>> +++ b/mm/page_alloc.c
>>>>>> @@ -8134,8 +8134,7 @@ unsigned long free_reserved_area(void *start,
>>>>>> void
>>>>>> *end, int poison, const char }
>>>>>>
>>>>>>  	if (pages && s)
>>>>>>
>>>>>> -		pr_info("Freeing %s memory: %ldK\n",
>>>>>> -			s, pages << (PAGE_SHIFT - 10));
>>>>>> +		pr_info("Freeing %s memory: %ldK\n", s, K(pages));
>>>>>>
>>>>>>  	return pages;
>>>>>>  
>>>>>>  }
>>>>>>
>>>>>> @@ -8180,14 +8179,13 @@ void __init mem_init_print_info(void)
>>>>>>
>>>>>>  		", %luK highmem"
>>>>>>  
>>>>>>  #endif
>>>>>>  
>>>>>>  		")\n",
>>>>>>
>>>>>> -		nr_free_pages() << (PAGE_SHIFT - 10),
>>>>>> -		physpages << (PAGE_SHIFT - 10),
>>>>>> +		K(nr_free_pages()), K(physpages),
>>>>>>
>>>>>>  		codesize >> 10, datasize >> 10, rosize >> 10,
>>>>>>  		(init_data_size + init_code_size) >> 10, bss_size >> 10,
>>>>>>
>>>>>> -		(physpages - totalram_pages() - totalcma_pages) << 
> (PAGE_SHIFT
>>>>>
>>>>> - 10),
>>>>>
>>>>>> -		totalcma_pages << (PAGE_SHIFT - 10)
>>>>>> +		K(physpages - totalram_pages() - totalcma_pages),
>>>>>> +		K(totalcma_pages)
>>>>>>
>>>>>>  #ifdef	CONFIG_HIGHMEM
>>>>>>
>>>>>> -		, totalhigh_pages() << (PAGE_SHIFT - 10)
>>>>>> +		, K(totalhigh_pages())
>>>>>>
>>>>>>  #endif
>>>>>>  
>>>>>>  		);
>>>>>>  
>>>>>>  }
>>>>>
>>>>> (my concern is not quite within the scope of this submission, but I'll
>>>>> ask
>>>>> anyway)
>>>>>
>>>>> Given we have this:
>>>>>
>>>>> ```
>>>>> git grep '#define K(x)' v5.14
>>>>> v5.14:drivers/base/node.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
>>>>> v5.14:drivers/net/hamradio/scc.c:#define K(x) kiss->x
>>>>> v5.14:kernel/debug/kdb/kdb_main.c:#define K(x) ((x) << (PAGE_SHIFT -
>>>>> 10))
>>>>> v5.14:mm/backing-dev.c:#define K(x) ((x) << (PAGE_SHIFT - 10))
>>>>> v5.14:mm/memcontrol.c:#define K(x) ((x) << (PAGE_SHIFT-10))
>>>>> v5.14:mm/oom_kill.c:#define K(x) ((x) << (PAGE_SHIFT-10))
>>>>> v5.14:mm/page_alloc.c:#define K(x) ((x) << (PAGE_SHIFT-10))
>>>>> ```
>>>>>
>>>>> Shouldn't this macro go to some header file instead to get rid of define
>>>>> repetitions?
>>>>
>>>> Many thanks for figuring this out. I think we should get rid of these
>>>> repetitions too. But I'am not sure where this definition should be. Any
>>>> suggestion? Thanks.
>>>
>>> I'm not sure what place suits best. At first I thought maybe linux/mm.h or
>>> linux/mm_inline.h, but since PAGE_SHIFT is declared in asm-generic/page.h,
>>> probably K(x) can also go there as well?
>>
>> K(x) is relevant with PAGE_SHIFT. So I think K(x) can also go
>> asm-generic/page.h too.
> 
> Actually, the comment in this file says:
> 
> ```
> 4 /*
> 5  * Generic page.h implementation, for NOMMU architectures.
> 6  * This provides the dummy definitions for the memory management.
> 7  */
> ```
> 
> so it seems I was wrong about this being an appropriate place.

It's hard to find the appropriate place.

> 
>> Am I supposed to do this when free or will you
>> kindly do this?
> 
> Let me just try to cram this into mm.h and send it out, and then we'll see 
> what comments people suggest.
> 

Many thanks for doing this. :)

> Thanks.
> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 5/6] mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype
  2021-09-01  8:10         ` David Hildenbrand
@ 2021-09-01  8:18           ` Miaohe Lin
  0 siblings, 0 replies; 36+ messages in thread
From: Miaohe Lin @ 2021-09-01  8:18 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: akpm, vbabka, sfr, peterz, linux-mm, linux-kernel, Mel Gorman

On 2021/9/1 16:10, David Hildenbrand wrote:
> On 01.09.21 10:02, Miaohe Lin wrote:
>> On 2021/8/31 22:23, David Hildenbrand wrote:
>>> On 31.08.21 15:43, Mel Gorman wrote:
>>>> On Mon, Aug 30, 2021 at 10:10:50PM +0800, Miaohe Lin wrote:
>>>>> If it's not prepared to free unref page, the pcp page migratetype is
>>>>> unset. Thus We will get rubbish from get_pcppage_migratetype() and
>>>>> might list_del &page->lru again after it's already deleted from the
>>>>> list leading to grumble about data corruption.
>>>>>
>>>>> Fixes: 3dcbe270d8ec ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
>>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>>>
>>>> Acked-by: Mel Gorman <mgorman@techsingularity.net>
>>>>
>>>> This fix is fairly important. Take this patch out and send it on its own
>>>> so it gets picked up relatively quickly. It does not belong in a series
>>>> that is mostly cosmetic cleanups.
>>>
>>> I think the commit id is wrong. Shouldn't that be
>>>
>>> df1acc856923 ("mm/page_alloc: avoid conflating IRQs disabled with zone->lock")
>>>
>>> ?
>>>
>>
>> Many thanks for pointing this out.
>>
>> I used to save the git log in a file to make life easier. But it seems this leads
>> to the old commit id above.
>>
>> commit 3dcbe270d8ec57e534f5c605279cdc3ceb1f044a
>> Author: Mel Gorman <mgorman@techsingularity.net>
>> Date:   Fri Jun 4 14:20:03 2021 +1000
>>
>>      mm/page_alloc: avoid conflating IRQs disabled with zone->lock
>>
>> git name-rev 3dcbe270d8ec
>> 3dcbe270d8ec tags/next-20210604~2^2~196
>>
>> vs
>>
>> commit df1acc856923c0a65c28b588585449106c316b71
>> Author: Mel Gorman <mgorman@techsingularity.net>
>> Date:   Mon Jun 28 19:42:00 2021 -0700
>>
>>      mm/page_alloc: avoid conflating IRQs disabled with zone->lock
>>
>> git name-rev df1acc856923
>> df1acc856923 tags/next-20210630~2^2~278
>>
>> Their contents are same but with different commit id. The previous one
>> could have been git-rebased.
>>
> 
> -mm tree commit ids keep changing until patches are upstream. Therefore, you can observe changing commit ids in -next. Always use the ones from Linus' tree, they are stable.
> 
Many thanks for your advice, David. :)

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/6] mm/page_alloc.c: remove obsolete comment in free_pcppages_bulk()
  2021-09-01  7:49     ` Miaohe Lin
@ 2021-09-01 15:14       ` Mel Gorman
  2021-09-02  6:25         ` Miaohe Lin
  0 siblings, 1 reply; 36+ messages in thread
From: Mel Gorman @ 2021-09-01 15:14 UTC (permalink / raw)
  To: Miaohe Lin; +Cc: akpm, vbabka, sfr, peterz, linux-mm, linux-kernel

On Wed, Sep 01, 2021 at 03:49:03PM +0800, Miaohe Lin wrote:
> On 2021/8/31 21:38, Mel Gorman wrote:
> > On Mon, Aug 30, 2021 at 10:10:48PM +0800, Miaohe Lin wrote:
> >> It's also confusing now. Remove it.
> >>
> > 
> > Why is the whole comment obsolete?
> > 
> > The second two paragraphs about "all pages pinned" and pages_scanned is
> > obsolete and can go but the first paragraph is valid.
> > 
> 
> I think the first paragraph is invalid due to the below statement:
> "Assumes all pages on list are in same zone, and of same order."
> There are NR_PCP_LISTS lists and PAGE_ALLOC_COSTLY_ORDER + 1 + NR_PCP_THP
> orders in pcp. So I think it's obsolete.
> 

Ah.

> Should I delete this statement in the first paragraph only?
> 

Remove ", and of same order"

-- 
Mel Gorman
SUSE Labs

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K()
  2021-09-01  7:37         ` Miaohe Lin
  2021-09-01  7:46           ` Oleksandr Natalenko
@ 2021-09-01 21:25           ` David Laight
  2021-09-02  6:32             ` Miaohe Lin
  1 sibling, 1 reply; 36+ messages in thread
From: David Laight @ 2021-09-01 21:25 UTC (permalink / raw)
  To: 'Miaohe Lin', Oleksandr Natalenko
  Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, Andrew Morton

From: Miaohe Lin
> Sent: 01 September 2021 08:38
...
> >>> Shouldn't this macro go to some header file instead to get rid of define
> >>> repetitions?
> >>
> >> Many thanks for figuring this out. I think we should get rid of these
> >> repetitions too. But I'am not sure where this definition should be. Any
> >> suggestion? Thanks.
> >
> > I'm not sure what place suits best. At first I thought maybe linux/mm.h or
> > linux/mm_inline.h, but since PAGE_SHIFT is declared in asm-generic/page.h,
> > probably K(x) can also go there as well?
> 
> K(x) is relevant with PAGE_SHIFT. So I think K(x) can also go asm-generic/page.h too.
> Am I supposed to do this when free or will you kindly do this?

It is a bit of a short name to go in a public header file.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/6] mm/page_alloc.c: remove obsolete comment in free_pcppages_bulk()
  2021-09-01 15:14       ` Mel Gorman
@ 2021-09-02  6:25         ` Miaohe Lin
  0 siblings, 0 replies; 36+ messages in thread
From: Miaohe Lin @ 2021-09-02  6:25 UTC (permalink / raw)
  To: Mel Gorman; +Cc: akpm, vbabka, sfr, peterz, linux-mm, linux-kernel

On 2021/9/1 23:14, Mel Gorman wrote:
> On Wed, Sep 01, 2021 at 03:49:03PM +0800, Miaohe Lin wrote:
>> On 2021/8/31 21:38, Mel Gorman wrote:
>>> On Mon, Aug 30, 2021 at 10:10:48PM +0800, Miaohe Lin wrote:
>>>> It's also confusing now. Remove it.
>>>>
>>>
>>> Why is the whole comment obsolete?
>>>
>>> The second two paragraphs about "all pages pinned" and pages_scanned is
>>> obsolete and can go but the first paragraph is valid.
>>>
>>
>> I think the first paragraph is invalid due to the below statement:
>> "Assumes all pages on list are in same zone, and of same order."
>> There are NR_PCP_LISTS lists and PAGE_ALLOC_COSTLY_ORDER + 1 + NR_PCP_THP
>> orders in pcp. So I think it's obsolete.
>>
> 
> Ah.
> 
>> Should I delete this statement in the first paragraph only?
>>
> 
> Remove ", and of same order"

Will do this in v2. Thanks.

> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K()
  2021-09-01 21:25           ` David Laight
@ 2021-09-02  6:32             ` Miaohe Lin
  0 siblings, 0 replies; 36+ messages in thread
From: Miaohe Lin @ 2021-09-02  6:32 UTC (permalink / raw)
  To: David Laight, Oleksandr Natalenko
  Cc: vbabka, sfr, peterz, mgorman, linux-mm, linux-kernel, Andrew Morton

On 2021/9/2 5:25, David Laight wrote:
> From: Miaohe Lin
>> Sent: 01 September 2021 08:38
> ...
>>>>> Shouldn't this macro go to some header file instead to get rid of define
>>>>> repetitions?
>>>>
>>>> Many thanks for figuring this out. I think we should get rid of these
>>>> repetitions too. But I'am not sure where this definition should be. Any
>>>> suggestion? Thanks.
>>>
>>> I'm not sure what place suits best. At first I thought maybe linux/mm.h or
>>> linux/mm_inline.h, but since PAGE_SHIFT is declared in asm-generic/page.h,
>>> probably K(x) can also go there as well?
>>
>> K(x) is relevant with PAGE_SHIFT. So I think K(x) can also go asm-generic/page.h too.
>> Am I supposed to do this when free or will you kindly do this?
> 
> It is a bit of a short name to go in a public header file.
> 

It seems K(x) is a bit of short and it needs a more self-annotated name.
We can discuss this there:
https://lkml.org/lkml/2021/9/1/334

Thanks.

> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2021-09-02  6:32 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-30 14:10 [PATCH 0/6] Cleanups and fixup for page_alloc Miaohe Lin
2021-08-30 14:10 ` [PATCH 1/6] mm/page_alloc.c: remove meaningless VM_BUG_ON() in pindex_to_order() Miaohe Lin
2021-08-31 13:34   ` Mel Gorman
2021-08-31 14:05   ` David Hildenbrand
2021-08-30 14:10 ` [PATCH 2/6] mm/page_alloc.c: simplify the code by using macro K() Miaohe Lin
2021-08-31  8:54   ` Oleksandr Natalenko
2021-08-31 11:08     ` Miaohe Lin
2021-08-31 14:19       ` Oleksandr Natalenko
2021-09-01  7:37         ` Miaohe Lin
2021-09-01  7:46           ` Oleksandr Natalenko
2021-09-01  8:17             ` Miaohe Lin
2021-09-01 21:25           ` David Laight
2021-09-02  6:32             ` Miaohe Lin
2021-08-31 13:35   ` Mel Gorman
2021-08-31 14:07   ` David Hildenbrand
2021-08-30 14:10 ` [PATCH 3/6] mm/page_alloc.c: remove obsolete comment in free_pcppages_bulk() Miaohe Lin
2021-08-31 13:38   ` Mel Gorman
2021-09-01  7:49     ` Miaohe Lin
2021-09-01 15:14       ` Mel Gorman
2021-09-02  6:25         ` Miaohe Lin
2021-08-30 14:10 ` [PATCH 4/6] mm/page_alloc.c: use helper function zone_spans_pfn() Miaohe Lin
2021-08-31 13:39   ` Mel Gorman
2021-08-31 14:08   ` David Hildenbrand
2021-08-30 14:10 ` [PATCH 5/6] mm/page_alloc.c: avoid accessing uninitialized pcp page migratetype Miaohe Lin
2021-08-31 13:43   ` Mel Gorman
2021-08-31 14:23     ` David Hildenbrand
2021-09-01  8:02       ` Miaohe Lin
2021-09-01  8:10         ` David Hildenbrand
2021-09-01  8:18           ` Miaohe Lin
2021-08-31 16:34     ` Vlastimil Babka
2021-09-01  8:04       ` Miaohe Lin
2021-08-30 14:10 ` [PATCH 6/6] mm/page_alloc.c: avoid allocating highmem pages via alloc_pages_exact_nid() Miaohe Lin
2021-08-30 14:24   ` Matthew Wilcox
2021-08-31  1:56     ` Miaohe Lin
2021-08-31 16:37       ` Vlastimil Babka
2021-09-01  8:05         ` Miaohe Lin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).