From: Michal Hocko <mhocko@suse.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>,
Hugh Dickins <hughd@google.com>,
Zhou Guanghui <zhouguanghui1@huawei.com>,
linux-kernel@vger.kernel.org, linux-mm@kvack.org,
akpm@linux-foundation.org, kirill.shutemov@linux.intel.com,
npiggin@gmail.com, ziy@nvidia.com, wangkefeng.wang@huawei.com,
guohanjun@huawei.com, dingtianhong@huawei.com,
chenweilong@huawei.com, rui.xiang@huawei.com
Subject: Re: [PATCH v2 2/2] mm/memcg: set memcg when split page
Date: Thu, 18 Mar 2021 15:05:00 +0100 [thread overview]
Message-ID: <YFNeDDkTOtls9/XU@dhcp22.suse.cz> (raw)
In-Reply-To: <alpine.LSU.2.11.2103111229380.7859@eggly.anvils>
On Thu 11-03-21 12:37:20, Hugh Dickins wrote:
> On Thu, 11 Mar 2021, Michal Hocko wrote:
> > On Thu 11-03-21 10:21:39, Johannes Weiner wrote:
> > > On Thu, Mar 11, 2021 at 09:37:02AM +0100, Michal Hocko wrote:
> > > > Johannes, Hugh,
> > > >
> > > > what do you think about this approach? If we want to stick with
> > > > split_page approach then we need to update the missing place Matthew has
> > > > pointed out.
> > >
> > > I find the __free_pages() code quite tricky as well. But for that
> > > reason I would actually prefer to initiate the splitting in there,
> > > since that's the place where we actually split the page, rather than
> > > spread the handling of this situation further out.
> > >
> > > The race condition shouldn't be hot, so I don't think we need to be as
> > > efficient about setting page->memcg_data only on the higher-order
> > > buddies as in Willy's scratch patch. We can call split_page_memcg(),
> > > which IMO should actually help document what's happening to the page.
> > >
> > > I think that function could also benefit a bit more from step-by-step
> > > documentation about what's going on. The kerneldoc is helpful, but I
> > > don't think it does justice to how tricky this race condition is.
> > >
> > > Something like this?
> > >
> > > void __free_pages(struct page *page, unsigned int order)
> > > {
> > > /*
> > > * Drop the base reference from __alloc_pages and free. In
> > > * case there is an outstanding speculative reference, from
> > > * e.g. the page cache, it will put and free the page later.
> > > */
> > > if (likely(put_page_testzero(page))) {
> > > free_the_page(page, order);
> > > return;
> > > }
> > >
> > > /*
> > > * The speculative reference will put and free the page.
> > > *
> > > * However, if the speculation was into a higher-order page
> > > * that isn't marked compound, the other side will know
> > > * nothing about our buddy pages and only free the order-0
> > > * page at the start of our chunk! We must split off and free
> > > * the buddy pages here.
> > > *
> > > * The buddy pages aren't individually refcounted, so they
> > > * can't have any pending speculative references themselves.
> > > */
> > > if (!PageHead(page) && order > 0) {
> > > split_page_memcg(page, 1 << order);
> > > while (order-- > 0)
> > > free_the_page(page + (1 << order), order);
> > > }
> > > }
> >
> > Fine with me. Mathew was concerned about more places that do something
> > similar but I would say that if we find out more places we might
> > reconsider and currently stay with a reasonably clear model that it is
> > only head patch that carries the memcg information and split_page_memcg
> > is necessary to break such page into smaller pieces.
>
> I agree: I do like Johannes' suggestion best, now that we already
> have split_page_memcg(). Not too worried about contrived use of
> free_unref_page() here; and whether non-compound high-order pages
> should be perpetuated is a different discussion.
Matthew, are you planning to post a patch with suggested changes or
should I do it?
--
Michal Hocko
SUSE Labs
next prev parent reply other threads:[~2021-03-18 14:05 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-04 7:40 [PATCH v2 0/2] set memcg when split page Zhou Guanghui
2021-03-04 7:40 ` [PATCH v2 1/2] mm/memcg: rename mem_cgroup_split_huge_fixup to split_page_memcg Zhou Guanghui
2021-03-04 15:50 ` Johannes Weiner
2021-03-04 16:20 ` Zi Yan
2021-03-04 18:54 ` Shakeel Butt
2021-03-08 22:37 ` Singh, Balbir
2021-03-09 8:28 ` Michal Hocko
2021-03-10 21:44 ` Singh, Balbir
2021-03-10 22:00 ` Hugh Dickins
2021-03-10 23:50 ` Singh, Balbir
2021-03-04 7:40 ` [PATCH v2 2/2] mm/memcg: set memcg when split page Zhou Guanghui
2021-03-04 15:52 ` Johannes Weiner
2021-03-04 16:22 ` Zi Yan
2021-03-04 18:55 ` Shakeel Butt
[not found] ` <YEIblNv0BMITFzYO@dhcp22.suse.cz>
2021-03-05 23:58 ` Andrew Morton
2021-03-08 8:41 ` Michal Hocko
2021-03-08 20:42 ` Andrew Morton
2021-03-08 20:47 ` Matthew Wilcox
2021-03-09 0:10 ` Andrew Morton
2021-03-08 21:02 ` Matthew Wilcox
2021-03-09 9:02 ` Michal Hocko
2021-03-09 12:32 ` Matthew Wilcox
2021-03-09 13:03 ` Michal Hocko
2021-03-11 8:37 ` Michal Hocko
2021-03-11 15:21 ` Johannes Weiner
2021-03-11 16:23 ` Matthew Wilcox
2021-03-11 16:26 ` Michal Hocko
2021-03-11 20:37 ` Hugh Dickins
2021-03-18 14:05 ` Michal Hocko [this message]
2021-03-18 15:02 ` Matthew Wilcox
2021-03-18 15:07 ` Johannes Weiner
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=YFNeDDkTOtls9/XU@dhcp22.suse.cz \
--to=mhocko@suse.com \
--cc=akpm@linux-foundation.org \
--cc=chenweilong@huawei.com \
--cc=dingtianhong@huawei.com \
--cc=guohanjun@huawei.com \
--cc=hannes@cmpxchg.org \
--cc=hughd@google.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=npiggin@gmail.com \
--cc=rui.xiang@huawei.com \
--cc=wangkefeng.wang@huawei.com \
--cc=willy@infradead.org \
--cc=zhouguanghui1@huawei.com \
--cc=ziy@nvidia.com \
/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 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).