Re: [PATCH] zswap: initialize entry->pool on same filled entry

[Chris Li <chrisl@kernel.org>]

On Thu, Mar 21, 2024 at 8:19 PM 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 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;
> };

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