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> <CAJD7tka+_-MZDwbyt8vewvgRzRNg9jpFL7pxfu4ruceGpCkqCw@mail.gmail.com>
In-Reply-To: <CAJD7tka+_-MZDwbyt8vewvgRzRNg9jpFL7pxfu4ruceGpCkqCw@mail.gmail.com>
From:   Domenico Cerasuolo <cerasuolodomenico@gmail.com>
Date:   Tue, 6 Jun 2023 11:35:17 +0200
Message-ID: <CA+CLi1ikS52d+ztjMbT56bMD0Cqnb2H0WimXgOJw72hznymmEg@mail.gmail.com>
Subject: Re: [RFC PATCH 1/7] mm: zswap: add pool shrinking mechanism
To:     Yosry Ahmed <yosryahmed@google.com>
Cc:     vitaly.wool@konsulko.com, minchan@kernel.org,
        senozhatsky@chromium.org, linux-mm@kvack.org, ddstreet@ieee.org,
        sjenning@redhat.com, nphamcs@gmail.com, hannes@cmpxchg.org,
        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 Tue, Jun 6, 2023 at 4:19=E2=80=AFAM Yosry Ahmed <yosryahmed@google.com> =
wrote:
>
> Hi Domenico,
>
> On Mon, Jun 5, 2023 at 1:54=E2=80=AFAM Domenico Cerasuolo
> <cerasuolodomenico@gmail.com> wrote:
> >
> > Each zpool driver (zbud, z3fold and zsmalloc) implements its own shrink
> > function, which is called from zpool_shrink. However, with this commit,
> > a unified shrink function is added to zswap. The ultimate goal is to
> > eliminate the need for zpool_shrink once all zpool implementations have
> > dropped their shrink code.
> >
> > To ensure the functionality of each commit, this change focuses solely
> > on adding the mechanism itself. No modifications are made to
> > the backends, meaning that functionally, there are no immediate changes=
.
> > The zswap mechanism will only come into effect once the backends have
> > removed their shrink code. The subsequent commits will address the
> > modifications needed in the backends.
> >
> > Signed-off-by: Domenico Cerasuolo <cerasuolodomenico@gmail.com>
> > ---
> >  mm/zswap.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 78 insertions(+), 5 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index bcb82e09eb64..80d7780bf066 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -159,6 +159,8 @@ struct zswap_pool {
> >         struct work_struct shrink_work;
> >         struct hlist_node node;
> >         char tfm_name[CRYPTO_MAX_ALG_NAME];
> > +       struct list_head lru;
> > +       spinlock_t lock;
>
> If this lock is only protecting the lru then I believe it's better to
> call in lru_lock to make it explicit.

Hi Yosry,

thanks for the input, it makes sense to call it lru_lock, will update.

>
> >  };
> >
> >  /*
> > @@ -176,10 +178,12 @@ struct zswap_pool {
> >   *            be held while changing the refcount.  Since the lock mus=
t
> >   *            be held, there is no reason to also make refcount atomic=
.
> >   * length - the length in bytes of the compressed page data.  Needed d=
uring
> > - *          decompression. For a same value filled page length is 0.
> > + *          decompression. For a same value filled page length is 0, a=
nd both
> > + *          pool and lru are invalid and must be ignored.
> >   * pool - the zswap_pool the entry's data is in
> >   * handle - zpool allocation handle that stores the compressed page da=
ta
> >   * value - value of the same-value filled pages which have same conten=
t
> > + * lru - handle to the pool's lru used to evict pages.
> >   */
> >  struct zswap_entry {
> >         struct rb_node rbnode;
> > @@ -192,6 +196,7 @@ struct zswap_entry {
> >                 unsigned long value;
> >         };
> >         struct obj_cgroup *objcg;
> > +       struct list_head lru;
> >  };
> >
> >  struct zswap_header {
> > @@ -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);
>
> I think we should document the lock ordering somewhere (tree lock ->
> lru lock), otherwise we may run into an ABBA deadlock down the road.

Will update in the next iteration.

>
> >                 zpool_free(entry->pool->zpool, entry->handle);
> >                 zswap_pool_put(entry->pool);
> >         }
> > @@ -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, l=
ru);
> > +       list_del_init(&lru_entry->lru);
> > +       zhdr =3D zpool_map_handle(pool->zpool, lru_entry->handle, ZPOOL=
_MM_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);
> > +
> > +       /* hold a reference from tree so it won't be freed during write=
back */
> > +       spin_lock(&tree->lock);
> > +       tree_entry =3D zswap_entry_find_get(&tree->rbroot, swp_offset(s=
wpentry));
> > +       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);
> > +
> > +       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 mig=
rated*/
> >         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);
> > +       /* zpool_evictable will be removed once all 3 backends have mig=
rated*/
> > +       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 */
> > @@ -1384,8 +1452,7 @@ static int zswap_frontswap_load(unsigned type, pg=
off_t offset,
> >         /* decompress */
> >         dlen =3D PAGE_SIZE;
> >         src =3D zpool_map_handle(entry->pool->zpool, entry->handle, ZPO=
OL_MM_RO);
> > -       if (zpool_evictable(entry->pool->zpool))
> > -               src +=3D sizeof(struct zswap_header);
> > +       src +=3D sizeof(struct zswap_header);
> >
> >         if (!zpool_can_sleep_mapped(entry->pool->zpool)) {
> >                 memcpy(tmp, src, entry->length);
> > @@ -1415,6 +1482,12 @@ static int zswap_frontswap_load(unsigned type, p=
goff_t offset,
> >  freeentry:
> >         spin_lock(&tree->lock);
> >         zswap_entry_put(tree, entry);
> > +       /* zpool_evictable will be removed once all 3 backends have mig=
rated*/
> > +       if (entry->length && !zpool_evictable(entry->pool->zpool)) {
> > +               spin_lock(&entry->pool->lock);
> > +               list_move(&entry->lru, &entry->pool->lru);
> > +               spin_unlock(&entry->pool->lock);
> > +       }
> >         spin_unlock(&tree->lock);
> >
> >         return ret;
> > --
> > 2.34.1
> >