From: Johannes Weiner <hannes@cmpxchg.org>
To: Roman Gushchin <guro@fb.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Michal Hocko <mhocko@suse.com>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"cgroups@vger.kernel.org" <cgroups@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH 2/8] mm: clean up and clarify lruvec lookup procedure
Date: Tue, 22 Oct 2019 17:31:30 -0400 [thread overview]
Message-ID: <20191022213130.GA361040@cmpxchg.org> (raw)
In-Reply-To: <20191022192456.GB11461@tower.DHCP.thefacebook.com>
On Tue, Oct 22, 2019 at 07:25:01PM +0000, Roman Gushchin wrote:
> On Tue, Oct 22, 2019 at 10:47:57AM -0400, Johannes Weiner wrote:
> > There is a per-memcg lruvec and a NUMA node lruvec. Which one is being
> > used is somewhat confusing right now, and it's easy to make mistakes -
> > especially when it comes to global reclaim.
> >
> > How it works: when memory cgroups are enabled, we always use the
> > root_mem_cgroup's per-node lruvecs. When memory cgroups are not
> > compiled in or disabled at runtime, we use pgdat->lruvec.
> >
> > Document that in a comment.
> >
> > Due to the way the reclaim code is generalized, all lookups use the
> > mem_cgroup_lruvec() helper function, and nobody should have to find
> > the right lruvec manually right now. But to avoid future mistakes,
> > rename the pgdat->lruvec member to pgdat->__lruvec and delete the
> > convenience wrapper that suggests it's a commonly accessed member.
>
> This part looks great!
Thanks!
> > While in this area, swap the mem_cgroup_lruvec() argument order. The
> > name suggests a memcg operation, yet it takes a pgdat first and a
> > memcg second. I have to double take every time I call this. Fix that.
>
> Idk, I agree that the new order makes more sense (slightly), but
> such changes make any backports / git blame searches more complex.
> So, I'm not entirely convinced that it worth it. The compiler will
> prevent passing bad arguments by mistake.
Lol, this has cost me a lot of time since we've had it. It takes you
out of the flow while writing code and make you think about something
stupid and trivial.
The backport period is limited, but the mental overhead of a stupid
interface is forever!
next prev parent reply other threads:[~2019-10-22 21:31 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-22 14:47 [PATCH 0/8]: mm: vmscan: cgroup-related cleanups Johannes Weiner
2019-10-22 14:47 ` [PATCH 1/8] mm: vmscan: simplify lruvec_lru_size() Johannes Weiner
2019-10-22 19:18 ` Roman Gushchin
2019-10-23 13:48 ` Michal Hocko
2019-10-22 14:47 ` [PATCH 2/8] mm: clean up and clarify lruvec lookup procedure Johannes Weiner
2019-10-22 19:25 ` Roman Gushchin
2019-10-22 21:31 ` Johannes Weiner [this message]
2019-10-23 14:00 ` Michal Hocko
2019-10-22 14:47 ` [PATCH 3/8] mm: vmscan: move inactive_list_is_low() swap check to the caller Johannes Weiner
2019-10-22 19:28 ` Roman Gushchin
2019-10-23 14:06 ` Michal Hocko
2019-10-22 14:47 ` [PATCH 4/8] mm: vmscan: naming fixes: global_reclaim() and sane_reclaim() Johannes Weiner
2019-10-22 19:40 ` Roman Gushchin
2019-10-23 16:02 ` Johannes Weiner
2019-10-23 14:14 ` Michal Hocko
2019-10-23 15:56 ` Johannes Weiner
2019-10-22 14:48 ` [PATCH 5/8] mm: vmscan: replace shrink_node() loop with a retry jump Johannes Weiner
2019-10-22 19:56 ` Roman Gushchin
2019-10-22 21:42 ` Johannes Weiner
2019-10-22 22:46 ` Roman Gushchin
2019-10-23 14:18 ` Michal Hocko
2019-10-25 13:44 ` Johannes Weiner
2019-10-22 14:48 ` [PATCH 6/8] mm: vmscan: turn shrink_node_memcg() into shrink_lruvec() Johannes Weiner
2019-10-22 20:04 ` Roman Gushchin
2019-10-23 14:21 ` Michal Hocko
2019-10-22 14:48 ` [PATCH 7/8] mm: vmscan: split shrink_node() into node part and memcgs part Johannes Weiner
2019-10-22 20:08 ` Roman Gushchin
2019-10-25 14:36 ` Johannes Weiner
2019-10-23 14:24 ` Michal Hocko
2019-10-22 14:48 ` [PATCH 8/8] mm: vmscan: harmonize writeback congestion tracking for nodes & memcgs Johannes Weiner
2019-10-22 21:03 ` Roman Gushchin
2019-10-25 14:41 ` 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=20191022213130.GA361040@cmpxchg.org \
--to=hannes@cmpxchg.org \
--cc=Kernel-team@fb.com \
--cc=akpm@linux-foundation.org \
--cc=cgroups@vger.kernel.org \
--cc=guro@fb.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@suse.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).