Received-SPF: pass (google.com: domain of linux-kernel+bounces-16397-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 4 Jan 2024 09:48:36 +0100
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
Message-ID: <ZZZw5NSEFNYwbjZM@tiehlicka>
References: <20240103164841.2800183-1-schatzberg.dan@gmail.com>
 <20240103164841.2800183-3-schatzberg.dan@gmail.com>
 <CAOUHufZ-hTwdiy7eYgJWo=CHyPbdxTX60hxjPmwa9Ox6FXMYQQ@mail.gmail.com>
 <ZZWlT5wmDaMceSlQ@dschatzberg-fedora-PC0Y6AEN>
 <ZZYE36e0BFFzi0X3@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZZYE36e0BFFzi0X3@google.com>

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.

The main argument for NULL check based approach is that it is less error
prone from an incorrect ussage because any bug becomes obvious. If we
use any other special constant a missing initialization would be much
harder to spot because they would be subtle behavior change.

Are there really any strong arguments to go against this "default
initialization is safe" policy?
-- 
Michal Hocko
SUSE Labs