Received-SPF: pass (google.com: domain of linux-kernel+bounces-111551-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240321-zswap-fill-v1-1-b6180dbf7c27@kernel.org>
 <CAJD7tkY8os3yvYLiotaiRuYa1jdEGiPHQsZEU6E52zRBQ34kQQ@mail.gmail.com> <20240322031907.GA237176@cmpxchg.org>
In-Reply-To: <20240322031907.GA237176@cmpxchg.org>
From: Chris Li <chrisl@kernel.org>
Date: Fri, 22 Mar 2024 06:35:43 -0700
Message-ID: <CAF8kJuNe5xXVp00Ogk2AL_zXFK6pN0u7=0avjyPPkagB3FWy8Q@mail.gmail.com>
Subject: Re: [PATCH] zswap: initialize entry->pool on same filled entry
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: Yosry Ahmed <yosryahmed@google.com>, 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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 21, 2024 at 8:19=E2=80=AFPM Johannes Weiner <hannes@cmpxchg.org=
> wrote:
>
> On Thu, Mar 21, 2024 at 04:56:05PM -0700, Yosry Ahmed wrote:
> > On Thu, Mar 21, 2024 at 4:53=E2=80=AFPM Chris Li <chrisl@kernel.org> wr=
ote:
> > >
> > > 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 =3D 0;
> > >                         entry->value =3D value;
> > > +                       entry->pool =3D 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;
> };

I think the 3 struct with embedded and container of is a bit complex,
because the state breaks into different struct members

How about:

struct zswap_entry {
        unsigned int length;
        struct obj_cgroup *objcg;
        union {
                struct /* compressed */ {
                         struct zswap_pool *pool;
                         unsigned long handle;
                         swp_entry_t swpentry;
                         struct list_head lru;
                };
               struct /* same filled */ {
                       unsigned long value;
                };
        };
};

That should have the same effect of the above three structures. Easier
to visualize the containing structure.

What do you say?

Chris

>
> 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...
>