Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Mel Gorman <mgorman@techsingularity.net>
To: Vlastimil Babka <vbabka@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	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 16:43:21 +0100
Message-ID: <20171019154321.qtpzaeftoyyw4iey@techsingularity.net> (raw)
In-Reply-To: <9e260f57-b871-81bd-66ee-b08fff949c7c@suse.cz>

On Thu, Oct 19, 2017 at 03:12:33PM +0200, Vlastimil Babka wrote:
> 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).
> 

Maybe, but it's a bit risky. The PCP lists are preserved but the number of
zone->lock acquire/releases increases as now every 14 pages reclaimed will
be an acquire/release instead of every pcp->high number of pages reclaimed.
That is a definite cost versus a possibility that the next page allocated no
that CPU will still be cache hot. That in itself may not happen as
scanning lots of pages for reclaim may have filled the cache with
useless information anyway.

> > 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.
> 

This was unintentional, thanks. The fix is below

---8<---
mm, Remove cold parameter from free_hot_cold_page* -fix

As pointed out by Vlastimil Babka, the pages being freed should be added
to the head, no the tail, of the pcpu list.

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 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 13582efc57a0..06461553a115 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2627,7 +2627,7 @@ static void free_unref_page_commit(struct page *page, unsigned long pfn)
 	}
 
 	pcp = &this_cpu_ptr(zone->pageset)->pcp;
-	list_add_tail(&page->lru, &pcp->lists[migratetype]);
+	list_add(&page->lru, &pcp->lists[migratetype]);
 	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>

  reply index

Thread overview: 23+ 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 ` [PATCH 1/8] mm, page_alloc: Enable/disable IRQs once when freeing a list of pages Mel Gorman
2017-10-18  9:02   ` Vlastimil Babka
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-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 ` [PATCH 4/8] mm: Only drain per-cpu pagevecs once per pagevec usage Mel Gorman
2017-10-19  9:12   ` Vlastimil Babka
2017-10-19  9:33     ` Mel Gorman
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-19  9:24   ` Vlastimil Babka
2017-10-18  7:59 ` [PATCH 6/8] mm: Remove cold parameter for release_pages Mel Gorman
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-19 13:12   ` Vlastimil Babka
2017-10-19 15:43     ` Mel Gorman [this message]
2017-10-18  7:59 ` [PATCH 8/8] mm: Remove __GFP_COLD Mel Gorman
2017-10-19 13:18   ` Vlastimil Babka
2017-10-19 13:42   ` Vlastimil Babka
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

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=20171019154321.qtpzaeftoyyw4iey@techsingularity.net \
    --to=mgorman@techsingularity.net \
    --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=vbabka@suse.cz \
    /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: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git