From: Michal Hocko <mhocko@suse.com>
To: Yu Zhao <yuzhao@google.com>
Cc: Dan Schatzberg <schatzberg.dan@gmail.com>,
Andrew Morton <akpm@linux-foundation.org>,
linux-kernel@vger.kernel.org, cgroups@vger.kernel.org,
linux-mm@kvack.org, Yosry Ahmed <yosryahmed@google.com>,
David Rientjes <rientjes@google.com>,
Chris Li <chrisl@kernel.org>, Tejun Heo <tj@kernel.org>,
Zefan Li <lizefan.x@bytedance.com>,
Johannes Weiner <hannes@cmpxchg.org>,
Jonathan Corbet <corbet@lwn.net>,
Roman Gushchin <roman.gushchin@linux.dev>,
Shakeel Butt <shakeelb@google.com>,
Muchun Song <muchun.song@linux.dev>,
David Hildenbrand <david@redhat.com>,
Matthew Wilcox <willy@infradead.org>,
Kefeng Wang <wangkefeng.wang@huawei.com>,
Yue Zhao <findns94@gmail.com>, Hugh Dickins <hughd@google.com>
Subject: Re: [PATCH v6 2/2] mm: add swapiness= arg to memory.reclaim
Date: Wed, 10 Jan 2024 11:32:58 +0100 [thread overview]
Message-ID: <ZZ5yWriL-T59Bcu_@tiehlicka> (raw)
In-Reply-To: <CAOUHufbEuAWwz-51tq6OB7SPJ8W3UJ9Roq2-yXesWAbmzstdKw@mail.gmail.com>
On Tue 09-01-24 16:54:15, Yu Zhao wrote:
> On Thu, Jan 4, 2024 at 1:48 AM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Wed 03-01-24 18:07:43, Yu Zhao wrote:
> > > On Wed, Jan 03, 2024 at 01:19:59PM -0500, Dan Schatzberg wrote:
> > > > On Wed, Jan 03, 2024 at 10:19:40AM -0700, Yu Zhao wrote:
> > > > [...]
> > > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > > > > index d91963e2d47f..394e0dd46b2e 100644
> > > > > > --- a/mm/vmscan.c
> > > > > > +++ b/mm/vmscan.c
> > > > > > @@ -92,6 +92,11 @@ struct scan_control {
> > > > > > unsigned long anon_cost;
> > > > > > unsigned long file_cost;
> > > > > >
> > > > > > +#ifdef CONFIG_MEMCG
> > > > > > + /* Swappiness value for proactive reclaim. Always use sc_swappiness()! */
> > > > > > + int *proactive_swappiness;
> > > > > > +#endif
> > > > >
> > > > > Why is proactive_swappiness still a pointer? The whole point of the
> > > > > previous conversation is that sc->proactive can tell whether
> > > > > sc->swappiness is valid or not, and that's less awkward than using a
> > > > > pointer.
> > > >
> > > > It's the same reason as before - zero initialization ensures that the
> > > > pointer is NULL which tells us if it's valid or not. Proactive reclaim
> > > > might not set swappiness and you need to distinguish swappiness of 0
> > > > and not-set. See this discussion with Michal:
> > > >
> > > > https://lore.kernel.org/linux-mm/ZZUizpTWOt3gNeqR@tiehlicka/
> > >
> > > static ssize_t memory_reclaim(struct kernfs_open_file *of, char *buf,
> > > size_t nbytes, loff_t off)
> > > {
> > > struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
> > > unsigned int nr_retries = MAX_RECLAIM_RETRIES;
> > > unsigned long nr_to_reclaim, nr_reclaimed = 0;
> > > + int swappiness = -1;
> > > ...
> > > reclaimed = try_to_free_mem_cgroup_pages(memcg,
> > > min(nr_to_reclaim - nr_reclaimed, SWAP_CLUSTER_MAX),
> > > - GFP_KERNEL, reclaim_options);
> > > + GFP_KERNEL, reclaim_options,
> > > + swappiness);
> > >
> > > ...
> > >
> > > +static int sc_swappiness(struct scan_control *sc, struct mem_cgroup *memcg)
> > > +{
> > > + return sc->proactive && sc->proactive_swappiness > -1 ?
> > > + sc->proactive_swappiness : mem_cgroup_swappiness(memcg);
> > > +}
> >
> > Tpo be completely honest I really fail to see why this is such a hot
> > discussion point. To be completely clear both approaches are feasible.
>
> Feasible but not equal.
>
> > The main argument for NULL check based approach is that it is less error
> > prone from an incorrect ussage because any bug becomes obvious.
>
> Any bug becomes *fatal*, and fatal isn't only obvious but also hurts
> in production systems.
>
> This was the reason for going through the trouble switching from
> VM_BUG_ON() to VM_WARN_ON() and documenting it in
> Documentation/process/coding-style.rst:
>
> 22) Do not crash the kernel
> ---------------------------
>
> In general, the decision to crash the kernel belongs to the user, rather
> than to the kernel developer.
>
> Isn't?
I do agree with this general statement but I do not think it is
applicable in this context.
This is not an explicit BUG() when kernel explicitly sets to panic the
system. We are talking about subtle misbehavior which might be
non-trivial to debug (there are other reasons to not swap at all) vs. a
potential NULL ptr which will kill the userspace in a very obvious way.
Sure there are risks with that but checks for potential NULL ptr
dereferncing is easier than forgot explicit initialization. There are
clear pros and cons for both approaches. NULL default initialized
structures members which allow for behavior override are a general
kernel pattern so I do not really see this going way off the rails.
--
Michal Hocko
SUSE Labs
next prev parent reply other threads:[~2024-01-10 10:33 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-03 16:48 [PATCH v6 0/2] Add swappiness argument to memory.reclaim Dan Schatzberg
2024-01-03 16:48 ` [PATCH v6 1/2] mm: add defines for min/max swappiness Dan Schatzberg
2024-01-03 16:48 ` [PATCH v6 2/2] mm: add swapiness= arg to memory.reclaim Dan Schatzberg
2024-01-03 17:19 ` Yu Zhao
2024-01-03 18:19 ` Dan Schatzberg
2024-01-04 1:07 ` Yu Zhao
2024-01-04 8:48 ` Michal Hocko
2024-01-09 23:54 ` Yu Zhao
2024-01-10 10:32 ` Michal Hocko [this message]
2024-01-04 1:17 ` Yu Zhao
2024-01-04 10:09 ` Michal Hocko
2024-01-09 23:57 ` Yu Zhao
2024-06-11 19:25 ` [PATCH v6 0/2] Add swappiness argument " Shakeel Butt
2024-06-11 19:31 ` Yosry Ahmed
2024-06-11 19:48 ` Andrew Morton
2024-06-11 22:50 ` Shakeel Butt
2024-06-11 23:10 ` Yu Zhao
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=ZZ5yWriL-T59Bcu_@tiehlicka \
--to=mhocko@suse.com \
--cc=akpm@linux-foundation.org \
--cc=cgroups@vger.kernel.org \
--cc=chrisl@kernel.org \
--cc=corbet@lwn.net \
--cc=david@redhat.com \
--cc=findns94@gmail.com \
--cc=hannes@cmpxchg.org \
--cc=hughd@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=lizefan.x@bytedance.com \
--cc=muchun.song@linux.dev \
--cc=rientjes@google.com \
--cc=roman.gushchin@linux.dev \
--cc=schatzberg.dan@gmail.com \
--cc=shakeelb@google.com \
--cc=tj@kernel.org \
--cc=wangkefeng.wang@huawei.com \
--cc=willy@infradead.org \
--cc=yosryahmed@google.com \
--cc=yuzhao@google.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 read-only IMAP folder(s) and NNTP newsgroup(s).