Received-SPF: pass (google.com: domain of linux-kernel+bounces-110897-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 21 Mar 2024 23:19:07 -0400
From: Johannes Weiner <hannes@cmpxchg.org>
To: Yosry Ahmed <yosryahmed@google.com>
Cc: Chris Li <chrisl@kernel.org>, Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Nhat Pham <nphamcs@gmail.com>,
	Chengming Zhou <zhouchengming@bytedance.com>
Subject: Re: [PATCH] zswap: initialize entry->pool on same filled entry
Message-ID: <20240322031907.GA237176@cmpxchg.org>
References: <20240321-zswap-fill-v1-1-b6180dbf7c27@kernel.org>
 <CAJD7tkY8os3yvYLiotaiRuYa1jdEGiPHQsZEU6E52zRBQ34kQQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAJD7tkY8os3yvYLiotaiRuYa1jdEGiPHQsZEU6E52zRBQ34kQQ@mail.gmail.com>

On Thu, Mar 21, 2024 at 04:56:05PM -0700, Yosry Ahmed wrote:
> On Thu, Mar 21, 2024 at 4:53 PM Chris Li <chrisl@kernel.org> wrote:
> >
> > Current zswap will leave the entry->pool uninitialized if
> > the page is same  filled. The entry->pool pointer can
> > contain data written by previous usage.
> >
> > Initialize entry->pool to zero for the same filled zswap entry.
> >
> > Signed-off-by: Chris Li <chrisl@kernel.org>
> > ---
> > Per Yosry's suggestion to split out this clean up
> > from the zxwap rb tree to xarray patch.
> >
> > https://lore.kernel.org/all/ZemDuW25YxjqAjm-@google.com/
> > ---
> >  mm/zswap.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index b31c977f53e9..f04a75a36236 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -1527,6 +1527,7 @@ bool zswap_store(struct folio *folio)
> >                         kunmap_local(src);
> >                         entry->length = 0;
> >                         entry->value = value;
> > +                       entry->pool = 0;
> 
> This should be NULL.
> 
> That being said, I am working on a series that should make non-filled
> entries not use a zswap_entry at all. So I think this cleanup is
> unnecessary, especially that it is documented in the definition of
> struct zswap_entry that entry->pool is invalid for same-filled
> entries.

Yeah I don't think it's necessary to initialize. The field isn't valid
when it's a same-filled entry, just like `handle` would contain
nonsense as it's unionized with value.

What would actually be safer is to make the two subtypes explicit, and
not have unused/ambiguous/overloaded members at all:

struct zswap_entry {
	unsigned int length;
	struct obj_cgroup *objcg;
};

struct zswap_compressed_entry {
	struct zswap_entry entry;
	struct zswap_pool *pool;
	unsigned long handle;
	struct list_head lru;
	swp_entry_t swpentry;
};

struct zswap_samefilled_entry {
	struct zswap_entry entry;
	unsigned long value;
};

Then put zswap_entry pointers in the tree and use the appropriate
container_of() calls in just a handful of central places. This would
limit the the points where mistakes can be made, and suggests how the
code paths to handle them should split naturally.

Might be useful even with your series, since it disambiguates things
first, and separates the cleanup bits from any functional changes,
instead of having to do kind of everything at once...