From: Michal Hocko <mhocko@suse.com>
To: Muchun Song <songmuchun@bytedance.com>
Cc: Roman Gushchin <guro@fb.com>,
Johannes Weiner <hannes@cmpxchg.org>,
Andrew Morton <akpm@linux-foundation.org>,
Shakeel Butt <shakeelb@google.com>,
LKML <linux-kernel@vger.kernel.org>,
Linux Memory Management List <linux-mm@kvack.org>
Subject: Re: [External] Re: [PATCH] mm: memcontrol: fix kernel stack account
Date: Tue, 2 Mar 2021 10:34:09 +0100 [thread overview]
Message-ID: <YD4GkRnTN6RK5CyG@dhcp22.suse.cz> (raw)
In-Reply-To: <CAMZfGtX=EmE8iOLfO3duCyMWOmu-OYra9Rk4mKsknds+5MueMg@mail.gmail.com>
On Tue 02-03-21 17:23:42, Muchun Song wrote:
> On Tue, Mar 2, 2021 at 4:44 PM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Tue 02-03-21 15:37:33, Muchun Song wrote:
> > > The alloc_thread_stack_node() cannot guarantee that allocated stack pages
> > > are in the same node when CONFIG_VMAP_STACK. Because we do not specify
> > > __GFP_THISNODE to __vmalloc_node_range(). Fix it by caling
> > > mod_lruvec_page_state() for each page one by one.
> >
> > What is the actual problem you are trying to address by this patch?
> > 991e7673859e has deliberately dropped the per page accounting. Can you
> > explain why that was incorrect? There surely is some imprecision
> > involved but does it matter and is it even observable?
>
> When I read the code of account_kernel_stack(), I see a comment that
> says "All stack pages are in the same node". I am confused about this.
> IIUC, there is no guarantee about this. Right?
Yes there is no guarantee indeed. Please always make sure to describe
the underlying reasoning for the patch. Subject of this patch refers to
a fix without explaining the actual problem. If a change is motivated by
code reading then make it explicit. Also if you are refering to a
different commit by Fixes: tag then it would be really helpful to
explicitly mention why that commit is incorrect or cause a visible
problems.
> Yeah, imprecision may
> not be a problem. But if this is what we did deliberately, I think that
> it is better to add a comment there. Thanks.
Yes the comment is quite confusing. I suspect it meant to say
/* All stack pages are accounted to the same node */
--
Michal Hocko
SUSE Labs
next prev parent reply other threads:[~2021-03-02 9:34 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-02 7:37 Muchun Song
2021-03-02 8:44 ` Michal Hocko
2021-03-02 9:23 ` [External] " Muchun Song
2021-03-02 9:34 ` Michal Hocko [this message]
2021-03-02 9:49 ` Muchun Song
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=YD4GkRnTN6RK5CyG@dhcp22.suse.cz \
--to=mhocko@suse.com \
--cc=akpm@linux-foundation.org \
--cc=guro@fb.com \
--cc=hannes@cmpxchg.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=shakeelb@google.com \
--cc=songmuchun@bytedance.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