On Thu, Mar 21, 2024 at 8:19 PM Johannes Weiner <hannes@xxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> > > > --- > > > Per Yosry's suggestion to split out this clean up > > > from the zxwap rb tree to xarray patch. > > > > > > https://lore.kernel.org/all/ZemDuW25YxjqAjm-@xxxxxxxxxx/ > > > --- > > > 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... >
