Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230605085419.44383-1-cerasuolodomenico@gmail.com>
 <20230605085419.44383-2-cerasuolodomenico@gmail.com> <20230605152952.GA221380@cmpxchg.org>
In-Reply-To: <20230605152952.GA221380@cmpxchg.org>
From:   Domenico Cerasuolo <cerasuolodomenico@gmail.com>
Date:   Tue, 6 Jun 2023 11:31:18 +0200
Message-ID: <CA+CLi1h31BgE-kVpmAbPZnP5FqEKYVcoF-WS5SrHHFkkSNWVog@mail.gmail.com>
Subject: Re: [RFC PATCH 1/7] mm: zswap: add pool shrinking mechanism
To:     Johannes Weiner <hannes@cmpxchg.org>
Cc:     vitaly.wool@konsulko.com, minchan@kernel.org,
        senozhatsky@chromium.org, yosryahmed@google.com,
        linux-mm@kvack.org, ddstreet@ieee.org, sjenning@redhat.com,
        nphamcs@gmail.com, akpm@linux-foundation.org,
        linux-kernel@vger.kernel.org, kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jun 5, 2023 at 5:29=E2=80=AFPM Johannes Weiner <hannes@cmpxchg.org>=
 wrote:
>
> Hi Domenico,
>
> On Mon, Jun 05, 2023 at 10:54:13AM +0200, Domenico Cerasuolo wrote:
> > @@ -364,6 +369,9 @@ static void zswap_free_entry(struct zswap_entry *en=
try)
> >       if (!entry->length)
> >               atomic_dec(&zswap_same_filled_pages);
> >       else {
> > +             spin_lock(&entry->pool->lock);
> > +             list_del_init(&entry->lru);
> > +             spin_unlock(&entry->pool->lock);
>
> This should be list_del(), as the entry is freed right after this and
> the list isn't reused anymore.
>
> The slab memory is recycled, but the allocation site (commented on
> below) doesn't need the list initialized.
>
> However, I think it also needs to check !zpool_evictable(). If
> alloc/store doesn't do the list_add(), this would be a double delete.
>

Thanks Johannes, I'm addressing all of your comments in the series in a V2.

> > @@ -584,14 +592,65 @@ static struct zswap_pool *zswap_pool_find_get(cha=
r *type, char *compressor)
> >       return NULL;
> >  }
> >
> > +static int zswap_shrink(struct zswap_pool *pool)
> > +{
> > +     struct zswap_entry *lru_entry, *tree_entry =3D NULL;
> > +     struct zswap_header *zhdr;
> > +     struct zswap_tree *tree;
> > +     swp_entry_t swpentry;
> > +     int ret;
> > +
> > +     /* get a reclaimable entry from LRU */
> > +     spin_lock(&pool->lock);
> > +     if (list_empty(&pool->lru)) {
> > +             spin_unlock(&pool->lock);
> > +             return -EINVAL;
> > +     }
> > +     lru_entry =3D list_last_entry(&pool->lru, struct zswap_entry, lru=
);
> > +     list_del_init(&lru_entry->lru);
> > +     zhdr =3D zpool_map_handle(pool->zpool, lru_entry->handle, ZPOOL_M=
M_RO);
> > +     tree =3D zswap_trees[swp_type(zhdr->swpentry)];
> > +     zpool_unmap_handle(pool->zpool, lru_entry->handle);
> > +     swpentry =3D zhdr->swpentry;
> > +     spin_unlock(&pool->lock);
>
> Once the pool lock is dropped, the lru_entry might get freed. But the
> swpentry 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.
>
> This could use a comment.
>
> > +     /* hold a reference from tree so it won't be freed during writeba=
ck */
> > +     spin_lock(&tree->lock);
> > +     tree_entry =3D zswap_entry_find_get(&tree->rbroot, swp_offset(swp=
entry));
> > +     if (tree_entry !=3D lru_entry) {
> > +             if (tree_entry)
> > +                     zswap_entry_put(tree, tree_entry);
> > +             spin_unlock(&tree->lock);
> > +             return -EAGAIN;
> > +     }
> > +     spin_unlock(&tree->lock);
>
> It's pretty outrageous how much simpler this is compared to the
> <backend>_reclaim_page() functions! The backends have to jump through
> a lot of hoops to serialize against freeing, whereas zswap can simply
> hold a reference. This is clearly a much better design.
>
> > +     ret =3D zswap_writeback_entry(pool->zpool, lru_entry->handle);
> > +
> > +     spin_lock(&tree->lock);
> > +     if (ret) {
> > +             spin_lock(&pool->lock);
> > +             list_move(&lru_entry->lru, &pool->lru);
> > +             spin_unlock(&pool->lock);
> > +     }
> > +     zswap_entry_put(tree, tree_entry);
> > +     spin_unlock(&tree->lock);
> > +
> > +     return ret ? -EAGAIN : 0;
> > +}
> > +
> >  static void shrink_worker(struct work_struct *w)
> >  {
> >       struct zswap_pool *pool =3D container_of(w, typeof(*pool),
> >                                               shrink_work);
> >       int ret, failures =3D 0;
> >
> > +     /* zpool_evictable will be removed once all 3 backends have migra=
ted*/
>
> Missing space between text and */
>
> >       do {
> > -             ret =3D zpool_shrink(pool->zpool, 1, NULL);
> > +             if (zpool_evictable(pool->zpool))
> > +                     ret =3D zpool_shrink(pool->zpool, 1, NULL);
> > +             else
> > +                     ret =3D zswap_shrink(pool);
> >               if (ret) {
> >                       zswap_reject_reclaim_fail++;
> >                       if (ret !=3D -EAGAIN)
> > @@ -655,6 +714,8 @@ static struct zswap_pool *zswap_pool_create(char *t=
ype, char *compressor)
> >        */
> >       kref_init(&pool->kref);
> >       INIT_LIST_HEAD(&pool->list);
> > +     INIT_LIST_HEAD(&pool->lru);
> > +     spin_lock_init(&pool->lock);
> >       INIT_WORK(&pool->shrink_work, shrink_worker);
> >
> >       zswap_pool_debug("created", pool);
> > @@ -1270,7 +1331,7 @@ static int zswap_frontswap_store(unsigned type, p=
goff_t offset,
> >       }
> >
> >       /* store */
> > -     hlen =3D zpool_evictable(entry->pool->zpool) ? sizeof(zhdr) : 0;
> > +     hlen =3D sizeof(zhdr);
> >       gfp =3D __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
> >       if (zpool_malloc_support_movable(entry->pool->zpool))
> >               gfp |=3D __GFP_HIGHMEM | __GFP_MOVABLE;
> > @@ -1313,6 +1374,13 @@ static int zswap_frontswap_store(unsigned type, =
pgoff_t offset,
> >                       zswap_entry_put(tree, dupentry);
> >               }
> >       } while (ret =3D=3D -EEXIST);
> > +     INIT_LIST_HEAD(&entry->lru);
>
> The list_add() below initializes the entry, so this shouldn't be
> needed.
>
> > +     /* zpool_evictable will be removed once all 3 backends have migra=
ted*/
> > +     if (entry->length && !zpool_evictable(entry->pool->zpool)) {
> > +             spin_lock(&entry->pool->lock);
> > +             list_add(&entry->lru, &entry->pool->lru);
> > +             spin_unlock(&entry->pool->lock);
> > +     }
> >       spin_unlock(&tree->lock);
> >
> >       /* update stats */