Re: [RFC PATCH v2 7/7] mm: zswap: remove zswap_header

[Yosry Ahmed]

On Fri, Jun 9, 2023 at 9:10 AM Domenico Cerasuolo
<cerasuolodomenico@xxxxxxxxx> wrote:
>
> On Wed, Jun 7, 2023 at 11:30 AM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
> >
> > On Tue, Jun 6, 2023 at 7:56 AM Domenico Cerasuolo
> > <cerasuolodomenico@xxxxxxxxx> wrote:
> > >
> > > Previously, zswap_header served the purpose of storing the swpentry
> > > within zpool pages. This allowed zpool implementations to pass relevant
> > > information to the writeback function. However, with the current
> > > implementation, writeback is directly handled within zswap.
> > > Consequently, there is no longer a necessity for zswap_header, as the
> > > swp_entry_t can be stored directly in zswap_entry.
> > >
> > > Suggested-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
> > > Signed-off-by: Domenico Cerasuolo <cerasuolodomenico@xxxxxxxxx>
> >
> > Thanks for this cleanup. It gives us back some of the memory used by
> > list_head in zswap entry, and remove an unnecessary zpool map
> > operation.
> >
> > > ---
> > >  mm/zswap.c | 52 ++++++++++++++++++++++------------------------------
> > >  1 file changed, 22 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index ef8604812352..f689444dd5a7 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -193,7 +193,7 @@ struct zswap_pool {
> > >   */
> > >  struct zswap_entry {
> > >         struct rb_node rbnode;
> > > -       pgoff_t offset;
> > > +       swp_entry_t swpentry;
> > >         int refcount;
> > >         unsigned int length;
> > >         struct zswap_pool *pool;
> > > @@ -205,10 +205,6 @@ struct zswap_entry {
> > >         struct list_head lru;
> > >  };
> > >
> > > -struct zswap_header {
> > > -       swp_entry_t swpentry;
> > > -};
> > > -
> > >  /*
> > >   * The tree lock in the zswap_tree struct protects a few things:
> > >   * - the rbtree
> > > @@ -250,7 +246,7 @@ static bool zswap_has_pool;
> > >         pr_debug("%s pool %s/%s\n", msg, (p)->tfm_name,         \
> > >                  zpool_get_type((p)->zpool))
> > >
> > > -static int zswap_writeback_entry(struct zswap_entry *entry, struct zswap_header *zhdr,
> > > +static int zswap_writeback_entry(struct zswap_entry *entry,
> > >                                  struct zswap_tree *tree);
> > >  static int zswap_pool_get(struct zswap_pool *pool);
> > >  static void zswap_pool_put(struct zswap_pool *pool);
> > > @@ -311,12 +307,14 @@ static struct zswap_entry *zswap_rb_search(struct rb_root *root, pgoff_t offset)
> > >  {
> > >         struct rb_node *node = root->rb_node;
> > >         struct zswap_entry *entry;
> > > +       pgoff_t entry_offset;
> > >
> > >         while (node) {
> > >                 entry = rb_entry(node, struct zswap_entry, rbnode);
> > > -               if (entry->offset > offset)
> > > +               entry_offset = swp_offset(entry->swpentry);
> > > +               if (entry_offset > offset)
> > >                         node = node->rb_left;
> > > -               else if (entry->offset < offset)
> > > +               else if (entry_offset < offset)
> > >                         node = node->rb_right;
> > >                 else
> > >                         return entry;
> > > @@ -333,13 +331,15 @@ static int zswap_rb_insert(struct rb_root *root, struct zswap_entry *entry,
> > >  {
> > >         struct rb_node **link = &root->rb_node, *parent = NULL;
> > >         struct zswap_entry *myentry;
> > > +       pgoff_t myentry_offset, entry_offset = swp_offset(entry->swpentry);
> > >
> > >         while (*link) {
> > >                 parent = *link;
> > >                 myentry = rb_entry(parent, struct zswap_entry, rbnode);
> > > -               if (myentry->offset > entry->offset)
> > > +               myentry_offset = swp_offset(myentry->swpentry);
> > > +               if (myentry_offset > entry_offset)
> > >                         link = &(*link)->rb_left;
> > > -               else if (myentry->offset < entry->offset)
> > > +               else if (myentry_offset < entry_offset)
> >
> > This whole myentry thing is very confusing to me. I initially thought
> > myentry would be the entry passed in as an argument. Can we change the
> > naming here to make it more consistent with zswap_rb_search() naming?
> >
>
> I'm not sure I understood the suggestion, is it related to the addition of
> myentry_offset variable or is myentry itself that you would like to change?

Just the variable naming myentry (which existed before your patch) and
myentry_offset is confusing, because (at least to me) they seem like
they refer to the argument, but they actually refer to the search
iterator. It's also inconsistent with the naming convention used in
zswap_rb_search().

It's just a nit though, don't overthink it :)

>
> > >                         link = &(*link)->rb_right;
> > >                 else {
> > >                         *dupentry = myentry;
> > > @@ -598,7 +598,6 @@ static struct zswap_pool *zswap_pool_find_get(char *type, char *compressor)
> > >  static int zswap_shrink(struct zswap_pool *pool)
> > >  {
> > >         struct zswap_entry *lru_entry, *tree_entry = NULL;
> > > -       struct zswap_header *zhdr;
> > >         struct zswap_tree *tree;
> > >         int swpoffset;
> > >         int ret;
> > > @@ -611,15 +610,13 @@ static int zswap_shrink(struct zswap_pool *pool)
> > >         }
> > >         lru_entry = list_last_entry(&pool->lru, struct zswap_entry, lru);
> > >         list_del_init(&lru_entry->lru);
> > > -       zhdr = zpool_map_handle(pool->zpool, lru_entry->handle, ZPOOL_MM_RO);
> > > -       tree = zswap_trees[swp_type(zhdr->swpentry)];
> > > -       zpool_unmap_handle(pool->zpool, lru_entry->handle);
> > >         /*
> > >          * Once the pool lock is dropped, the lru_entry might get freed. The
> > >          * swpoffset is copied to the stack, and lru_entry isn't deref'd again
> > >          * until the entry is verified to still be alive in the tree.
> > >          */
> > > -       swpoffset = swp_offset(zhdr->swpentry);
> > > +       swpoffset = swp_offset(lru_entry->swpentry);
> > > +       tree = zswap_trees[swp_type(lru_entry->swpentry)];
> > >         spin_unlock(&pool->lru_lock);
> > >
> > >         /* hold a reference from tree so it won't be freed during writeback */
> > > @@ -633,7 +630,7 @@ static int zswap_shrink(struct zswap_pool *pool)
> > >         }
> > >         spin_unlock(&tree->lock);
> > >
> > > -       ret = zswap_writeback_entry(lru_entry, zhdr, tree);
> > > +       ret = zswap_writeback_entry(lru_entry, tree);
> > >
> > >         spin_lock(&tree->lock);
> > >         if (ret) {
> > > @@ -1046,10 +1043,10 @@ static int zswap_get_swap_cache_page(swp_entry_t entry,
> > >   * the swap cache, the compressed version stored by zswap can be
> > >   * freed.
> > >   */
> > > -static int zswap_writeback_entry(struct zswap_entry *entry, struct zswap_header *zhdr,
> > > +static int zswap_writeback_entry(struct zswap_entry *entry,
> > >                                  struct zswap_tree *tree)
> > >  {
> > > -       swp_entry_t swpentry = zhdr->swpentry;
> > > +       swp_entry_t swpentry = entry->swpentry;
> > >         struct page *page;
> > >         struct scatterlist input, output;
> > >         struct crypto_acomp_ctx *acomp_ctx;
> > > @@ -1089,7 +1086,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry, struct zswap_header
> > >                  * writing.
> > >                  */
> > >                 spin_lock(&tree->lock);
> > > -               if (zswap_rb_search(&tree->rbroot, entry->offset) != entry) {
> > > +               if (zswap_rb_search(&tree->rbroot, swp_offset(entry->swpentry)) != entry) {
> > >                         spin_unlock(&tree->lock);
> > >                         delete_from_swap_cache(page_folio(page));
> > >                         ret = -ENOMEM;
> > > @@ -1101,8 +1098,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry, struct zswap_header
> > >                 acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
> > >                 dlen = PAGE_SIZE;
> > >
> > > -               zhdr = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO);
> > > -               src = (u8 *)zhdr + sizeof(struct zswap_header);
> > > +               src = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO);
> > >                 if (!zpool_can_sleep_mapped(pool)) {
> > >                         memcpy(tmp, src, entry->length);
> > >                         src = tmp;
> > > @@ -1196,11 +1192,10 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> > >         struct obj_cgroup *objcg = NULL;
> > >         struct zswap_pool *pool;
> > >         int ret;
> > > -       unsigned int hlen, dlen = PAGE_SIZE;
> > > +       unsigned int dlen = PAGE_SIZE;
> > >         unsigned long handle, value;
> > >         char *buf;
> > >         u8 *src, *dst;
> > > -       struct zswap_header zhdr = { .swpentry = swp_entry(type, offset) };
> > >         gfp_t gfp;
> > >
> > >         /* THP isn't supported */
> > > @@ -1245,7 +1240,7 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> > >                 src = kmap_atomic(page);
> > >                 if (zswap_is_page_same_filled(src, &value)) {
> > >                         kunmap_atomic(src);
> > > -                       entry->offset = offset;
> > > +                       entry->swpentry = swp_entry(type, offset);
> > >                         entry->length = 0;
> > >                         entry->value = value;
> > >                         atomic_inc(&zswap_same_filled_pages);
> > > @@ -1299,11 +1294,10 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> > >         }
> > >
> > >         /* store */
> > > -       hlen = sizeof(zhdr);
> > >         gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
> > >         if (zpool_malloc_support_movable(entry->pool->zpool))
> > >                 gfp |= __GFP_HIGHMEM | __GFP_MOVABLE;
> > > -       ret = zpool_malloc(entry->pool->zpool, hlen + dlen, gfp, &handle);
> > > +       ret = zpool_malloc(entry->pool->zpool, dlen, gfp, &handle);
> > >         if (ret == -ENOSPC) {
> > >                 zswap_reject_compress_poor++;
> > >                 goto put_dstmem;
> > > @@ -1313,13 +1307,12 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> > >                 goto put_dstmem;
> > >         }
> > >         buf = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_WO);
> > > -       memcpy(buf, &zhdr, hlen);
> > > -       memcpy(buf + hlen, dst, dlen);
> > > +       memcpy(buf, dst, dlen);
> > >         zpool_unmap_handle(entry->pool->zpool, handle);
> > >         mutex_unlock(acomp_ctx->mutex);
> > >
> > >         /* populate entry */
> > > -       entry->offset = offset;
> > > +       entry->swpentry = swp_entry(type, offset);
> > >         entry->handle = handle;
> > >         entry->length = dlen;
> > >
> > > @@ -1418,7 +1411,6 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
> > >         /* decompress */
> > >         dlen = PAGE_SIZE;
> > >         src = zpool_map_handle(entry->pool->zpool, entry->handle, ZPOOL_MM_RO);
> > > -       src += sizeof(struct zswap_header);
> > >
> > >         if (!zpool_can_sleep_mapped(entry->pool->zpool)) {
> > >                 memcpy(tmp, src, entry->length);
> > > --
> > > 2.34.1
> > >