From: Vlastimil Babka <vbabka@suse.cz> To: Mel Gorman <mgorman@techsingularity.net>, Andrew Morton <akpm@linux-foundation.org> Cc: Linux-MM <linux-mm@kvack.org>, Linux-FSDevel <linux-fsdevel@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, Jan Kara <jack@suse.cz>, Andi Kleen <ak@linux.intel.com>, Dave Hansen <dave.hansen@intel.com>, Dave Chinner <david@fromorbit.com> Subject: Re: [PATCH 7/8] mm, Remove cold parameter from free_hot_cold_page* Date: Thu, 19 Oct 2017 15:12:33 +0200 [thread overview] Message-ID: <9e260f57-b871-81bd-66ee-b08fff949c7c@suse.cz> (raw) In-Reply-To: <20171018075952.10627-8-mgorman@techsingularity.net> On 10/18/2017 09:59 AM, Mel Gorman wrote: > Most callers users of free_hot_cold_page claim the pages being released are > cache hot. The exception is the page reclaim paths where it is likely that > enough pages will be freed in the near future that the per-cpu lists are > going to be recycled and the cache hotness information is lost. Maybe it would make sense for reclaim to skip pcplists? (out of scope of this series, of course). > As no one > really cares about the hotness of pages being released to the allocator, > just ditch the parameter. > > The APIs are renamed to indicate that it's no longer about hot/cold pages. It > should also be less confusing as there are subtle differences between them. > __free_pages drops a reference and frees a page when the refcount reaches > zero. free_hot_cold_page handled pages whose refcount was already zero > which is non-obvious from the name. free_unref_page should be more obvious. > > No performance impact is expected as the overhead is marginal. The parameter > is removed simply because it is a bit stupid to have a useless parameter > copied everywhere. > > Signed-off-by: Mel Gorman <mgorman@techsingularity.net> Acked-by: Vlastimil Babka <vbabka@suse.cz> A comment below, though. ... > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 167e163cf733..13582efc57a0 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -2590,7 +2590,7 @@ void mark_free_pages(struct zone *zone) > } > #endif /* CONFIG_PM */ > > -static bool free_hot_cold_page_prepare(struct page *page, unsigned long pfn) > +static bool free_unref_page_prepare(struct page *page, unsigned long pfn) > { > int migratetype; > > @@ -2602,8 +2602,7 @@ static bool free_hot_cold_page_prepare(struct page *page, unsigned long pfn) > return true; > } > > -static void free_hot_cold_page_commit(struct page *page, unsigned long pfn, > - bool cold) > +static void free_unref_page_commit(struct page *page, unsigned long pfn) > { > struct zone *zone = page_zone(page); > struct per_cpu_pages *pcp; > @@ -2628,10 +2627,7 @@ static void free_hot_cold_page_commit(struct page *page, unsigned long pfn, > } > > pcp = &this_cpu_ptr(zone->pageset)->pcp; > - if (!cold) > - list_add(&page->lru, &pcp->lists[migratetype]); > - else > - list_add_tail(&page->lru, &pcp->lists[migratetype]); > + list_add_tail(&page->lru, &pcp->lists[migratetype]); Did you intentionally use the cold version here? Patch 8/8 uses the hot version in __rmqueue_pcplist() and that makes more sense to me. It should be either negligible or better, not worse. > pcp->count++; > if (pcp->count >= pcp->high) { > unsigned long batch = READ_ONCE(pcp->batch);
WARNING: multiple messages have this Message-ID (diff)
From: Vlastimil Babka <vbabka@suse.cz> To: Mel Gorman <mgorman@techsingularity.net>, Andrew Morton <akpm@linux-foundation.org> Cc: Linux-MM <linux-mm@kvack.org>, Linux-FSDevel <linux-fsdevel@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, Jan Kara <jack@suse.cz>, Andi Kleen <ak@linux.intel.com>, Dave Hansen <dave.hansen@intel.com>, Dave Chinner <david@fromorbit.com> Subject: Re: [PATCH 7/8] mm, Remove cold parameter from free_hot_cold_page* Date: Thu, 19 Oct 2017 15:12:33 +0200 [thread overview] Message-ID: <9e260f57-b871-81bd-66ee-b08fff949c7c@suse.cz> (raw) In-Reply-To: <20171018075952.10627-8-mgorman@techsingularity.net> On 10/18/2017 09:59 AM, Mel Gorman wrote: > Most callers users of free_hot_cold_page claim the pages being released are > cache hot. The exception is the page reclaim paths where it is likely that > enough pages will be freed in the near future that the per-cpu lists are > going to be recycled and the cache hotness information is lost. Maybe it would make sense for reclaim to skip pcplists? (out of scope of this series, of course). > As no one > really cares about the hotness of pages being released to the allocator, > just ditch the parameter. > > The APIs are renamed to indicate that it's no longer about hot/cold pages. It > should also be less confusing as there are subtle differences between them. > __free_pages drops a reference and frees a page when the refcount reaches > zero. free_hot_cold_page handled pages whose refcount was already zero > which is non-obvious from the name. free_unref_page should be more obvious. > > No performance impact is expected as the overhead is marginal. The parameter > is removed simply because it is a bit stupid to have a useless parameter > copied everywhere. > > Signed-off-by: Mel Gorman <mgorman@techsingularity.net> Acked-by: Vlastimil Babka <vbabka@suse.cz> A comment below, though. ... > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 167e163cf733..13582efc57a0 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -2590,7 +2590,7 @@ void mark_free_pages(struct zone *zone) > } > #endif /* CONFIG_PM */ > > -static bool free_hot_cold_page_prepare(struct page *page, unsigned long pfn) > +static bool free_unref_page_prepare(struct page *page, unsigned long pfn) > { > int migratetype; > > @@ -2602,8 +2602,7 @@ static bool free_hot_cold_page_prepare(struct page *page, unsigned long pfn) > return true; > } > > -static void free_hot_cold_page_commit(struct page *page, unsigned long pfn, > - bool cold) > +static void free_unref_page_commit(struct page *page, unsigned long pfn) > { > struct zone *zone = page_zone(page); > struct per_cpu_pages *pcp; > @@ -2628,10 +2627,7 @@ static void free_hot_cold_page_commit(struct page *page, unsigned long pfn, > } > > pcp = &this_cpu_ptr(zone->pageset)->pcp; > - if (!cold) > - list_add(&page->lru, &pcp->lists[migratetype]); > - else > - list_add_tail(&page->lru, &pcp->lists[migratetype]); > + list_add_tail(&page->lru, &pcp->lists[migratetype]); Did you intentionally use the cold version here? Patch 8/8 uses the hot version in __rmqueue_pcplist() and that makes more sense to me. It should be either negligible or better, not worse. > pcp->count++; > if (pcp->count >= pcp->high) { > unsigned long batch = READ_ONCE(pcp->batch); -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2017-10-19 13:12 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-10-18 7:59 [PATCH 0/8] Follow-up for speed up page cache truncation v2 Mel Gorman 2017-10-18 7:59 ` Mel Gorman 2017-10-18 7:59 ` [PATCH 1/8] mm, page_alloc: Enable/disable IRQs once when freeing a list of pages Mel Gorman 2017-10-18 7:59 ` Mel Gorman 2017-10-18 9:02 ` Vlastimil Babka 2017-10-18 9:02 ` Vlastimil Babka 2017-10-18 10:15 ` Mel Gorman 2017-10-18 10:15 ` Mel Gorman 2017-10-18 7:59 ` [PATCH 2/8] mm, truncate: Do not check mapping for every page being truncated Mel Gorman 2017-10-18 7:59 ` Mel Gorman 2017-10-19 8:11 ` Jan Kara 2017-10-19 8:11 ` Jan Kara 2017-10-18 7:59 ` [PATCH 3/8] mm, truncate: Remove all exceptional entries from pagevec under one lock Mel Gorman 2017-10-18 7:59 ` Mel Gorman 2017-10-18 7:59 ` [PATCH 4/8] mm: Only drain per-cpu pagevecs once per pagevec usage Mel Gorman 2017-10-18 7:59 ` Mel Gorman 2017-10-19 9:12 ` Vlastimil Babka 2017-10-19 9:12 ` Vlastimil Babka 2017-10-19 9:33 ` Mel Gorman 2017-10-19 9:33 ` Mel Gorman 2017-10-19 13:21 ` Vlastimil Babka 2017-10-19 13:21 ` Vlastimil Babka 2017-10-18 7:59 ` [PATCH 5/8] mm, pagevec: Remove cold parameter for pagevecs Mel Gorman 2017-10-18 7:59 ` Mel Gorman 2017-10-19 9:24 ` Vlastimil Babka 2017-10-19 9:24 ` Vlastimil Babka 2017-10-18 7:59 ` [PATCH 6/8] mm: Remove cold parameter for release_pages Mel Gorman 2017-10-18 7:59 ` Mel Gorman 2017-10-19 9:26 ` Vlastimil Babka 2017-10-19 9:26 ` Vlastimil Babka 2017-10-18 7:59 ` [PATCH 7/8] mm, Remove cold parameter from free_hot_cold_page* Mel Gorman 2017-10-18 7:59 ` Mel Gorman 2017-10-19 13:12 ` Vlastimil Babka [this message] 2017-10-19 13:12 ` Vlastimil Babka 2017-10-19 15:43 ` Mel Gorman 2017-10-19 15:43 ` Mel Gorman 2017-10-18 7:59 ` [PATCH 8/8] mm: Remove __GFP_COLD Mel Gorman 2017-10-18 7:59 ` Mel Gorman 2017-10-19 13:18 ` Vlastimil Babka 2017-10-19 13:18 ` Vlastimil Babka 2017-10-19 13:42 ` Vlastimil Babka 2017-10-19 13:42 ` Vlastimil Babka 2017-10-19 13:42 ` Vlastimil Babka 2017-10-19 14:32 ` Mel Gorman 2017-10-19 14:32 ` Mel Gorman -- strict thread matches above, loose matches on Subject: below -- 2017-10-12 9:30 [PATCH 0/8] Follow-up for speed up page cache truncation Mel Gorman 2017-10-12 9:31 ` [PATCH 7/8] mm, Remove cold parameter from free_hot_cold_page* Mel Gorman 2017-10-12 9:31 ` Mel Gorman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=9e260f57-b871-81bd-66ee-b08fff949c7c@suse.cz \ --to=vbabka@suse.cz \ --cc=ak@linux.intel.com \ --cc=akpm@linux-foundation.org \ --cc=dave.hansen@intel.com \ --cc=david@fromorbit.com \ --cc=jack@suse.cz \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mgorman@techsingularity.net \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.