All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mel Gorman <mgorman@techsingularity.net>
To: Matthew Wilcox <willy@infradead.org>
Cc: linux-mm@kvack.org, Jaegeuk Kim <jaegeuk@kernel.org>,
	linux-kernel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH] f2fs: initialize page->private when using for our internal use
Date: Tue, 6 Jul 2021 10:12:11 +0100	[thread overview]
Message-ID: <20210706091211.GR3840@techsingularity.net> (raw)
In-Reply-To: <YONTRlrJugeVq6Fj@casper.infradead.org>

On Mon, Jul 05, 2021 at 07:45:26PM +0100, Matthew Wilcox wrote:
> On Mon, Jul 05, 2021 at 11:04:21AM -0700, Jaegeuk Kim wrote:
> > On 07/05, Matthew Wilcox wrote:
> > > I think freshly allocated pages have a page->private of 0.  ie this
> > > code in mm/page_alloc.c:
> > > 
> > >                 page = rmqueue(ac->preferred_zoneref->zone, zone, order,
> > >                                 gfp_mask, alloc_flags, ac->migratetype);
> > >                 if (page) {
> > >                         prep_new_page(page, order, gfp_mask, alloc_flags);
> > > 
> > > where prep_new_page() calls post_alloc_hook() which contains:
> > >         set_page_private(page, 0);
> > 
> > Hmm, I can see it in 4.14 and 5.10 kernel.
> > 
> > The trace is on:
> > 
> >  30875 [ 1065.118750] c3     87  f2fs_migrate_page+0x354/0x45c
> >  30876 [ 1065.123872] c3     87  move_to_new_page+0x70/0x30c
> >  30877 [ 1065.128813] c3     87  migrate_pages+0x3a0/0x964
> >  30878 [ 1065.133583] c3     87  compact_zone+0x608/0xb04
> >  30879 [ 1065.138257] c3     87  kcompactd+0x378/0x4ec
> >  30880 [ 1065.142664] c3     87  kthread+0x11c/0x12c
> >  30881 [ 1065.146897] c3     87  ret_from_fork+0x10/0x18
> > 
> >  It seems compaction_alloc() gets a free page which doesn't reset the fields?
> 
> I'm not really familiar with the compaction code.  Mel, I see a call
> to post_alloc_hook() in split_map_pages().  Are there other ways of
> getting the compaction code to allocate a page which don't go through
> split_map_pages()?

I don't *think* so but I didn't look too hard as I had limited time
available before a meeting. compaction_alloc calls isolate_freepages
and that calls split_map_pages whether fast or slow isolating pages. The
problem *may* be in split_page because only the head page gets order set
to 0 but it's a bad fit because tail pages should be cleared of private
state by del_page_from_free_list. It might be worth adding a debugging
patch to split_pages that prints a warning once if a tail page has private
state and dump the contents of private to see if it looks like an order.

-- 
Mel Gorman
SUSE Labs


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

WARNING: multiple messages have this Message-ID (diff)
From: Mel Gorman <mgorman@techsingularity.net>
To: Matthew Wilcox <willy@infradead.org>
Cc: Jaegeuk Kim <jaegeuk@kernel.org>, Chao Yu <chao@kernel.org>,
	linux-kernel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net, linux-mm@kvack.org
Subject: Re: [f2fs-dev] [PATCH] f2fs: initialize page->private when using for our internal use
Date: Tue, 6 Jul 2021 10:12:11 +0100	[thread overview]
Message-ID: <20210706091211.GR3840@techsingularity.net> (raw)
In-Reply-To: <YONTRlrJugeVq6Fj@casper.infradead.org>

On Mon, Jul 05, 2021 at 07:45:26PM +0100, Matthew Wilcox wrote:
> On Mon, Jul 05, 2021 at 11:04:21AM -0700, Jaegeuk Kim wrote:
> > On 07/05, Matthew Wilcox wrote:
> > > I think freshly allocated pages have a page->private of 0.  ie this
> > > code in mm/page_alloc.c:
> > > 
> > >                 page = rmqueue(ac->preferred_zoneref->zone, zone, order,
> > >                                 gfp_mask, alloc_flags, ac->migratetype);
> > >                 if (page) {
> > >                         prep_new_page(page, order, gfp_mask, alloc_flags);
> > > 
> > > where prep_new_page() calls post_alloc_hook() which contains:
> > >         set_page_private(page, 0);
> > 
> > Hmm, I can see it in 4.14 and 5.10 kernel.
> > 
> > The trace is on:
> > 
> >  30875 [ 1065.118750] c3     87  f2fs_migrate_page+0x354/0x45c
> >  30876 [ 1065.123872] c3     87  move_to_new_page+0x70/0x30c
> >  30877 [ 1065.128813] c3     87  migrate_pages+0x3a0/0x964
> >  30878 [ 1065.133583] c3     87  compact_zone+0x608/0xb04
> >  30879 [ 1065.138257] c3     87  kcompactd+0x378/0x4ec
> >  30880 [ 1065.142664] c3     87  kthread+0x11c/0x12c
> >  30881 [ 1065.146897] c3     87  ret_from_fork+0x10/0x18
> > 
> >  It seems compaction_alloc() gets a free page which doesn't reset the fields?
> 
> I'm not really familiar with the compaction code.  Mel, I see a call
> to post_alloc_hook() in split_map_pages().  Are there other ways of
> getting the compaction code to allocate a page which don't go through
> split_map_pages()?

I don't *think* so but I didn't look too hard as I had limited time
available before a meeting. compaction_alloc calls isolate_freepages
and that calls split_map_pages whether fast or slow isolating pages. The
problem *may* be in split_page because only the head page gets order set
to 0 but it's a bad fit because tail pages should be cleared of private
state by del_page_from_free_list. It might be worth adding a debugging
patch to split_pages that prints a warning once if a tail page has private
state and dump the contents of private to see if it looks like an order.

-- 
Mel Gorman
SUSE Labs


  reply	other threads:[~2021-07-06  9:28 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-05  5:22 [f2fs-dev] [PATCH] f2fs: initialize page->private when using for our internal use Jaegeuk Kim
2021-07-05  5:22 ` Jaegeuk Kim
2021-07-05  6:32 ` [f2fs-dev] " Chao Yu
2021-07-05  6:32   ` Chao Yu
2021-07-05  8:56   ` Jaegeuk Kim
2021-07-05  8:56     ` Jaegeuk Kim
2021-07-05 11:33     ` Chao Yu
2021-07-05 11:33       ` Chao Yu
2021-07-05 11:47       ` Matthew Wilcox
2021-07-05 11:47         ` Matthew Wilcox
2021-07-05 16:09         ` Chao Yu
2021-07-05 16:09           ` Chao Yu
2021-07-05 18:06           ` Jaegeuk Kim
2021-07-05 18:06             ` Jaegeuk Kim
2021-07-06  0:16             ` Chao Yu
2021-07-06  0:16               ` Chao Yu
2021-07-05 18:04         ` Jaegeuk Kim
2021-07-05 18:04           ` Jaegeuk Kim
2021-07-05 18:45           ` Matthew Wilcox
2021-07-05 18:45             ` Matthew Wilcox
2021-07-06  9:12             ` Mel Gorman [this message]
2021-07-06  9:12               ` Mel Gorman
2021-07-07  0:48               ` Chao Yu
2021-07-07  0:48                 ` Chao Yu
2021-07-07  9:57                 ` Mel Gorman
2021-07-07  9:57                   ` Mel Gorman
2021-07-10  8:11                   ` Chao Yu
2021-07-10  8:11                     ` Chao Yu
2021-07-12  6:53                     ` Michal Hocko via Linux-f2fs-devel
2021-07-12  6:53                       ` Michal Hocko
2021-07-13  0:46                       ` Chao Yu
2021-07-13  0:46                         ` Chao Yu

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=20210706091211.GR3840@techsingularity.net \
    --to=mgorman@techsingularity.net \
    --cc=jaegeuk@kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=willy@infradead.org \
    /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
Be 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.