* [PATCH] mm: remove lru_add_drain_all in alloc_contig_range
@ 2021-02-09 17:50 Minchan Kim
2021-02-09 18:17 ` David Hildenbrand
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Minchan Kim @ 2021-02-09 17:50 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, LKML, Michal Hocko, David Hildenbrand, Minchan Kim
__alloc_contig_migrate_range already has lru_add_drain_all call
via migrate_prep. It's necessary to move LRU taget pages into
LRU list to be able to isolated. However, lru_add_drain_all call
after __alloc_contig_migrate_range is called is pointless.
This patch removes it.
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
mm/page_alloc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6446778cbc6b..f8fbee73dd6d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -8603,8 +8603,6 @@ int alloc_contig_range(unsigned long start, unsigned long end,
* isolated thus they won't get removed from buddy.
*/
- lru_add_drain_all();
-
order = 0;
outer_start = start;
while (!PageBuddy(pfn_to_page(outer_start))) {
--
2.30.0.478.g8a0d178c01-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: remove lru_add_drain_all in alloc_contig_range
2021-02-09 17:50 [PATCH] mm: remove lru_add_drain_all in alloc_contig_range Minchan Kim
@ 2021-02-09 18:17 ` David Hildenbrand
2021-02-09 19:03 ` Oscar Salvador
2021-02-10 12:13 ` Vlastimil Babka
2021-02-10 14:28 ` Oscar Salvador
2 siblings, 1 reply; 9+ messages in thread
From: David Hildenbrand @ 2021-02-09 18:17 UTC (permalink / raw)
To: Minchan Kim, Andrew Morton; +Cc: linux-mm, LKML, Michal Hocko, Vlastimil Babka
On 09.02.21 18:50, Minchan Kim wrote:
> __alloc_contig_migrate_range already has lru_add_drain_all call
> via migrate_prep. It's necessary to move LRU taget pages into
> LRU list to be able to isolated. However, lru_add_drain_all call
> after __alloc_contig_migrate_range is called is pointless.
>
> This patch removes it.
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
> mm/page_alloc.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6446778cbc6b..f8fbee73dd6d 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8603,8 +8603,6 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> * isolated thus they won't get removed from buddy.
> */
>
> - lru_add_drain_all();
> -
> order = 0;
> outer_start = start;
> while (!PageBuddy(pfn_to_page(outer_start))) {
>
I was expecting some magical reason why this is still required but I am
not able to find a compelling one. Maybe this is really some historical
artifact.
Let's see if other people know why this call here still exists.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: remove lru_add_drain_all in alloc_contig_range
2021-02-09 18:17 ` David Hildenbrand
@ 2021-02-09 19:03 ` Oscar Salvador
2021-02-10 12:17 ` Vlastimil Babka
0 siblings, 1 reply; 9+ messages in thread
From: Oscar Salvador @ 2021-02-09 19:03 UTC (permalink / raw)
To: David Hildenbrand
Cc: Minchan Kim, Andrew Morton, linux-mm, LKML, Michal Hocko,
Vlastimil Babka
On Tue, Feb 09, 2021 at 07:17:59PM +0100, David Hildenbrand wrote:
> On 09.02.21 18:50, Minchan Kim wrote:
> > __alloc_contig_migrate_range already has lru_add_drain_all call
> > via migrate_prep. It's necessary to move LRU taget pages into
> > LRU list to be able to isolated. However, lru_add_drain_all call
> > after __alloc_contig_migrate_range is called is pointless.
> >
> > This patch removes it.
> >
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> > mm/page_alloc.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 6446778cbc6b..f8fbee73dd6d 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -8603,8 +8603,6 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> > * isolated thus they won't get removed from buddy.
> > */
> > - lru_add_drain_all();
> > -
> > order = 0;
> > outer_start = start;
> > while (!PageBuddy(pfn_to_page(outer_start))) {
> >
>
> I was expecting some magical reason why this is still required but I am not
> able to find a compelling one. Maybe this is really some historical
> artifact.
>
> Let's see if other people know why this call here still exists.
I also stumbled upon this while working on adding hugetlb support for
alloc_acontig_range [1].
I have to confess I puzzled me a bit.
I saw it going back to when the function was first introduced by
commit 041d3a8cdc18dc375a128d90bbb753949a81b1fb
Author: Michal Nazarewicz <mina86@mina86.com>
Date: Thu Dec 29 13:09:50 2011 +0100
mm: page_alloc: introduce alloc_contig_range()
It does not make much sense to me. At this point our pages are free, so
we do not care about LRU handling here.
But I might be missing something.
[1] https://lore.kernel.org/linux-mm/20210208103935.GA32103@linux/T/#md651fc6e73c656105179382f92f8b2d6073051d1
--
Oscar Salvador
SUSE L3
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: remove lru_add_drain_all in alloc_contig_range
2021-02-09 17:50 [PATCH] mm: remove lru_add_drain_all in alloc_contig_range Minchan Kim
2021-02-09 18:17 ` David Hildenbrand
@ 2021-02-10 12:13 ` Vlastimil Babka
2021-02-10 14:28 ` Oscar Salvador
2 siblings, 0 replies; 9+ messages in thread
From: Vlastimil Babka @ 2021-02-10 12:13 UTC (permalink / raw)
To: Minchan Kim, Andrew Morton
Cc: linux-mm, LKML, Michal Hocko, David Hildenbrand
On 2/9/21 6:50 PM, Minchan Kim wrote:
> __alloc_contig_migrate_range already has lru_add_drain_all call
> via migrate_prep. It's necessary to move LRU taget pages into
> LRU list to be able to isolated. However, lru_add_drain_all call
> after __alloc_contig_migrate_range is called is pointless.
>
> This patch removes it.
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> mm/page_alloc.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6446778cbc6b..f8fbee73dd6d 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8603,8 +8603,6 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> * isolated thus they won't get removed from buddy.
> */
>
> - lru_add_drain_all();
> -
> order = 0;
> outer_start = start;
> while (!PageBuddy(pfn_to_page(outer_start))) {
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: remove lru_add_drain_all in alloc_contig_range
2021-02-09 19:03 ` Oscar Salvador
@ 2021-02-10 12:17 ` Vlastimil Babka
2021-02-10 12:23 ` Michal Hocko
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Vlastimil Babka @ 2021-02-10 12:17 UTC (permalink / raw)
To: Oscar Salvador, David Hildenbrand
Cc: Minchan Kim, Andrew Morton, linux-mm, LKML, Michal Hocko
On 2/9/21 8:03 PM, Oscar Salvador wrote:
> On Tue, Feb 09, 2021 at 07:17:59PM +0100, David Hildenbrand wrote:
>> I was expecting some magical reason why this is still required but I am not
>> able to find a compelling one. Maybe this is really some historical
>> artifact.
>>
>> Let's see if other people know why this call here still exists.
>
> I also stumbled upon this while working on adding hugetlb support for
> alloc_acontig_range [1].
> I have to confess I puzzled me a bit.
>
> I saw it going back to when the function was first introduced by
>
> commit 041d3a8cdc18dc375a128d90bbb753949a81b1fb
> Author: Michal Nazarewicz <mina86@mina86.com>
> Date: Thu Dec 29 13:09:50 2011 +0100
>
> mm: page_alloc: introduce alloc_contig_range()
>
>
> It does not make much sense to me. At this point our pages are free, so
> we do not care about LRU handling here.
> But I might be missing something.
AFAICS, at the time page migration used putback_lru_page() to release the
migration source page. This would put the page on lru pvec even if it was in
fact not mapped anywhere anymore, and only the drain would actually free it.
Seems Minchan optimized this in 2016 by c6c919eb90e0 ("mm: use put_page() to
free page instead of putback_lru_page()")
> [1] https://lore.kernel.org/linux-mm/20210208103935.GA32103@linux/T/#md651fc6e73c656105179382f92f8b2d6073051d1
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: remove lru_add_drain_all in alloc_contig_range
2021-02-10 12:17 ` Vlastimil Babka
@ 2021-02-10 12:23 ` Michal Hocko
2021-02-10 14:27 ` Oscar Salvador
2021-02-10 15:58 ` Minchan Kim
2 siblings, 0 replies; 9+ messages in thread
From: Michal Hocko @ 2021-02-10 12:23 UTC (permalink / raw)
To: Minchan Kim
Cc: Oscar Salvador, David Hildenbrand, Vlastimil Babka,
Andrew Morton, linux-mm, LKML
On Wed 10-02-21 13:17:33, Vlastimil Babka wrote:
> On 2/9/21 8:03 PM, Oscar Salvador wrote:
> > On Tue, Feb 09, 2021 at 07:17:59PM +0100, David Hildenbrand wrote:
> >> I was expecting some magical reason why this is still required but I am not
> >> able to find a compelling one. Maybe this is really some historical
> >> artifact.
> >>
> >> Let's see if other people know why this call here still exists.
> >
> > I also stumbled upon this while working on adding hugetlb support for
> > alloc_acontig_range [1].
> > I have to confess I puzzled me a bit.
> >
> > I saw it going back to when the function was first introduced by
> >
> > commit 041d3a8cdc18dc375a128d90bbb753949a81b1fb
> > Author: Michal Nazarewicz <mina86@mina86.com>
> > Date: Thu Dec 29 13:09:50 2011 +0100
> >
> > mm: page_alloc: introduce alloc_contig_range()
> >
> >
> > It does not make much sense to me. At this point our pages are free, so
> > we do not care about LRU handling here.
> > But I might be missing something.
>
> AFAICS, at the time page migration used putback_lru_page() to release the
> migration source page. This would put the page on lru pvec even if it was in
> fact not mapped anywhere anymore, and only the drain would actually free it.
> Seems Minchan optimized this in 2016 by c6c919eb90e0 ("mm: use put_page() to
> free page instead of putback_lru_page()")
This would be a great addition to the changelog. Thanks a lot Vlastimil,
you saved me from some archeology. With that mentioned feel free to add
Acked-by: Michal Hocko <mhocko@usse.com>
>
> > [1] https://lore.kernel.org/linux-mm/20210208103935.GA32103@linux/T/#md651fc6e73c656105179382f92f8b2d6073051d1
> >
> >
>
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: remove lru_add_drain_all in alloc_contig_range
2021-02-10 12:17 ` Vlastimil Babka
2021-02-10 12:23 ` Michal Hocko
@ 2021-02-10 14:27 ` Oscar Salvador
2021-02-10 15:58 ` Minchan Kim
2 siblings, 0 replies; 9+ messages in thread
From: Oscar Salvador @ 2021-02-10 14:27 UTC (permalink / raw)
To: Vlastimil Babka
Cc: David Hildenbrand, Minchan Kim, Andrew Morton, linux-mm, LKML,
Michal Hocko
On Wed, Feb 10, 2021 at 01:17:33PM +0100, Vlastimil Babka wrote:
> AFAICS, at the time page migration used putback_lru_page() to release the
> migration source page. This would put the page on lru pvec even if it was in
> fact not mapped anywhere anymore, and only the drain would actually free it.
> Seems Minchan optimized this in 2016 by c6c919eb90e0 ("mm: use put_page() to
> free page instead of putback_lru_page()")
Ok, I see. Thanks for this valuable information Vlastimil.
--
Oscar Salvador
SUSE L3
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: remove lru_add_drain_all in alloc_contig_range
2021-02-09 17:50 [PATCH] mm: remove lru_add_drain_all in alloc_contig_range Minchan Kim
2021-02-09 18:17 ` David Hildenbrand
2021-02-10 12:13 ` Vlastimil Babka
@ 2021-02-10 14:28 ` Oscar Salvador
2 siblings, 0 replies; 9+ messages in thread
From: Oscar Salvador @ 2021-02-10 14:28 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-mm, LKML, Michal Hocko, David Hildenbrand
On Tue, Feb 09, 2021 at 09:50:48AM -0800, Minchan Kim wrote:
> __alloc_contig_migrate_range already has lru_add_drain_all call
> via migrate_prep. It's necessary to move LRU taget pages into
> LRU list to be able to isolated. However, lru_add_drain_all call
> after __alloc_contig_migrate_range is called is pointless.
>
> This patch removes it.
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
I agree with Michal that the changelog could benefit from Vastimil's
clarification, so with that:
Reviewed-by: Oscar Salvador <osalvador@suse.de>
> ---
> mm/page_alloc.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6446778cbc6b..f8fbee73dd6d 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8603,8 +8603,6 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> * isolated thus they won't get removed from buddy.
> */
>
> - lru_add_drain_all();
> -
> order = 0;
> outer_start = start;
> while (!PageBuddy(pfn_to_page(outer_start))) {
> --
> 2.30.0.478.g8a0d178c01-goog
>
>
--
Oscar Salvador
SUSE L3
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mm: remove lru_add_drain_all in alloc_contig_range
2021-02-10 12:17 ` Vlastimil Babka
2021-02-10 12:23 ` Michal Hocko
2021-02-10 14:27 ` Oscar Salvador
@ 2021-02-10 15:58 ` Minchan Kim
2 siblings, 0 replies; 9+ messages in thread
From: Minchan Kim @ 2021-02-10 15:58 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Oscar Salvador, David Hildenbrand, Andrew Morton, linux-mm, LKML,
Michal Hocko
On Wed, Feb 10, 2021 at 01:17:33PM +0100, Vlastimil Babka wrote:
> On 2/9/21 8:03 PM, Oscar Salvador wrote:
> > On Tue, Feb 09, 2021 at 07:17:59PM +0100, David Hildenbrand wrote:
> >> I was expecting some magical reason why this is still required but I am not
> >> able to find a compelling one. Maybe this is really some historical
> >> artifact.
> >>
> >> Let's see if other people know why this call here still exists.
> >
> > I also stumbled upon this while working on adding hugetlb support for
> > alloc_acontig_range [1].
> > I have to confess I puzzled me a bit.
> >
> > I saw it going back to when the function was first introduced by
> >
> > commit 041d3a8cdc18dc375a128d90bbb753949a81b1fb
> > Author: Michal Nazarewicz <mina86@mina86.com>
> > Date: Thu Dec 29 13:09:50 2011 +0100
> >
> > mm: page_alloc: introduce alloc_contig_range()
> >
> >
> > It does not make much sense to me. At this point our pages are free, so
> > we do not care about LRU handling here.
> > But I might be missing something.
>
> AFAICS, at the time page migration used putback_lru_page() to release the
> migration source page. This would put the page on lru pvec even if it was in
> fact not mapped anywhere anymore, and only the drain would actually free it.
> Seems Minchan optimized this in 2016 by c6c919eb90e0 ("mm: use put_page() to
> free page instead of putback_lru_page()")
>
> > [1] https://lore.kernel.org/linux-mm/20210208103935.GA32103@linux/T/#md651fc6e73c656105179382f92f8b2d6073051d1
Thanks for digging history, Vlastimil!
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-02-10 15:59 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-09 17:50 [PATCH] mm: remove lru_add_drain_all in alloc_contig_range Minchan Kim
2021-02-09 18:17 ` David Hildenbrand
2021-02-09 19:03 ` Oscar Salvador
2021-02-10 12:17 ` Vlastimil Babka
2021-02-10 12:23 ` Michal Hocko
2021-02-10 14:27 ` Oscar Salvador
2021-02-10 15:58 ` Minchan Kim
2021-02-10 12:13 ` Vlastimil Babka
2021-02-10 14:28 ` Oscar Salvador
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).