* [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* 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 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
* [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* 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 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 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 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 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 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
* 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 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
* [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* 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 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 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 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
* [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* 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 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
* [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* 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 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 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-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 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 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 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
* [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 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 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