lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAJD7tkZqAfJ9uR8B0=nq_ZhTfFQWG=Tns4_tcm9YoA7PURx63Q@mail.gmail.com>
Date:   Thu, 27 Jul 2023 11:29:24 -0700
From:   Yosry Ahmed <yosryahmed@...gle.com>
To:     Johannes Weiner <hannes@...xchg.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Nhat Pham <nphamcs@...il.com>,
        Domenico Cerasuolo <cerasuolodomenico@...il.com>,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] mm: zswap: kill zswap_get_swap_cache_page()

On Thu, Jul 27, 2023 at 9:23 AM Johannes Weiner <hannes@...xchg.org> wrote:
>
> The __read_swap_cache_async() interface isn't more difficult to
> understand than what the helper abstracts. Save the indirection and a
> level of indentation for the primary work of the writeback func.
>
> Signed-off-by: Johannes Weiner <hannes@...xchg.org>

Arguably any abstraction to the swap code is better than dealing with
the swap code, but I am not against this :P

The diffstat looks nice and the code looks correct to me.

Reviewed-by: Yosry Ahmed <yosryahmed@...gle.com>

> ---
>  mm/zswap.c | 142 ++++++++++++++++++++---------------------------------
>  1 file changed, 53 insertions(+), 89 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index e34ac89e6098..bba4ead672be 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1016,43 +1016,6 @@ static int zswap_enabled_param_set(const char *val,
>  /*********************************
>  * writeback code
>  **********************************/
> -/* return enum for zswap_get_swap_cache_page */
> -enum zswap_get_swap_ret {
> -       ZSWAP_SWAPCACHE_NEW,
> -       ZSWAP_SWAPCACHE_EXIST,
> -       ZSWAP_SWAPCACHE_FAIL,
> -};
> -
> -/*
> - * zswap_get_swap_cache_page
> - *
> - * This is an adaption of read_swap_cache_async()
> - *
> - * This function tries to find a page with the given swap entry
> - * in the swapper_space address space (the swap cache).  If the page
> - * is found, it is returned in retpage.  Otherwise, a page is allocated,
> - * added to the swap cache, and returned in retpage.
> - *
> - * If success, the swap cache page is returned in retpage
> - * Returns ZSWAP_SWAPCACHE_EXIST if page was already in the swap cache
> - * Returns ZSWAP_SWAPCACHE_NEW if the new page needs to be populated,
> - *     the new page is added to swapcache and locked
> - * Returns ZSWAP_SWAPCACHE_FAIL on error
> - */
> -static int zswap_get_swap_cache_page(swp_entry_t entry,
> -                               struct page **retpage)
> -{
> -       bool page_was_allocated;
> -
> -       *retpage = __read_swap_cache_async(entry, GFP_KERNEL,
> -                       NULL, 0, &page_was_allocated);
> -       if (page_was_allocated)
> -               return ZSWAP_SWAPCACHE_NEW;
> -       if (!*retpage)
> -               return ZSWAP_SWAPCACHE_FAIL;
> -       return ZSWAP_SWAPCACHE_EXIST;
> -}
> -
>  /*
>   * Attempts to free an entry by adding a page to the swap cache,
>   * decompressing the entry data into the page, and issuing a
> @@ -1073,7 +1036,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
>         struct scatterlist input, output;
>         struct crypto_acomp_ctx *acomp_ctx;
>         struct zpool *pool = entry->pool->zpool;
> -
> +       bool page_was_allocated;
>         u8 *src, *tmp = NULL;
>         unsigned int dlen;
>         int ret;
> @@ -1088,65 +1051,66 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
>         }
>
>         /* try to allocate swap cache page */
> -       switch (zswap_get_swap_cache_page(swpentry, &page)) {
> -       case ZSWAP_SWAPCACHE_FAIL: /* no memory or invalidate happened */
> +       page = __read_swap_cache_async(swpentry, GFP_KERNEL, NULL, 0,
> +                                      &page_was_allocated);
> +       if (!page) {
>                 ret = -ENOMEM;
>                 goto fail;
> +       }
>
> -       case ZSWAP_SWAPCACHE_EXIST:
> -               /* page is already in the swap cache, ignore for now */
> +       /* Found an existing page, we raced with load/swapin */
> +       if (!page_was_allocated) {
>                 put_page(page);
>                 ret = -EEXIST;
>                 goto fail;
> +       }
>
> -       case ZSWAP_SWAPCACHE_NEW: /* page is locked */
> -               /*
> -                * Having a local reference to the zswap entry doesn't exclude
> -                * swapping from invalidating and recycling the swap slot. Once
> -                * the swapcache is secured against concurrent swapping to and
> -                * from the slot, recheck that the entry is still current before
> -                * writing.
> -                */
> -               spin_lock(&tree->lock);
> -               if (zswap_rb_search(&tree->rbroot, swp_offset(entry->swpentry)) != entry) {
> -                       spin_unlock(&tree->lock);
> -                       delete_from_swap_cache(page_folio(page));
> -                       ret = -ENOMEM;
> -                       goto fail;
> -               }
> +       /*
> +        * Page is locked, and the swapcache is now secured against
> +        * concurrent swapping to and from the slot. Verify that the
> +        * swap entry hasn't been invalidated and recycled behind our
> +        * backs (our zswap_entry reference doesn't prevent that), to
> +        * avoid overwriting a new swap page with old compressed data.
> +        */
> +       spin_lock(&tree->lock);
> +       if (zswap_rb_search(&tree->rbroot, swp_offset(entry->swpentry)) != entry) {
>                 spin_unlock(&tree->lock);
> +               delete_from_swap_cache(page_folio(page));
> +               ret = -ENOMEM;
> +               goto fail;
> +       }
> +       spin_unlock(&tree->lock);
>
> -               /* decompress */
> -               acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
> -               dlen = PAGE_SIZE;
> +       /* decompress */
> +       acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
> +       dlen = PAGE_SIZE;
>
> -               src = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO);
> -               if (!zpool_can_sleep_mapped(pool)) {
> -                       memcpy(tmp, src, entry->length);
> -                       src = tmp;
> -                       zpool_unmap_handle(pool, entry->handle);
> -               }
> +       src = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO);
> +       if (!zpool_can_sleep_mapped(pool)) {
> +               memcpy(tmp, src, entry->length);
> +               src = tmp;
> +               zpool_unmap_handle(pool, entry->handle);
> +       }
>
> -               mutex_lock(acomp_ctx->mutex);
> -               sg_init_one(&input, src, entry->length);
> -               sg_init_table(&output, 1);
> -               sg_set_page(&output, page, PAGE_SIZE, 0);
> -               acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen);
> -               ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait);
> -               dlen = acomp_ctx->req->dlen;
> -               mutex_unlock(acomp_ctx->mutex);
> -
> -               if (!zpool_can_sleep_mapped(pool))
> -                       kfree(tmp);
> -               else
> -                       zpool_unmap_handle(pool, entry->handle);
> +       mutex_lock(acomp_ctx->mutex);
> +       sg_init_one(&input, src, entry->length);
> +       sg_init_table(&output, 1);
> +       sg_set_page(&output, page, PAGE_SIZE, 0);
> +       acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen);
> +       ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait);
> +       dlen = acomp_ctx->req->dlen;
> +       mutex_unlock(acomp_ctx->mutex);
> +
> +       if (!zpool_can_sleep_mapped(pool))
> +               kfree(tmp);
> +       else
> +               zpool_unmap_handle(pool, entry->handle);
>
> -               BUG_ON(ret);
> -               BUG_ON(dlen != PAGE_SIZE);
> +       BUG_ON(ret);
> +       BUG_ON(dlen != PAGE_SIZE);
>
> -               /* page is up to date */
> -               SetPageUptodate(page);
> -       }
> +       /* page is up to date */
> +       SetPageUptodate(page);
>
>         /* move it to the tail of the inactive list after end_writeback */
>         SetPageReclaim(page);
> @@ -1157,16 +1121,16 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
>         zswap_written_back_pages++;
>
>         return ret;
> +
>  fail:
>         if (!zpool_can_sleep_mapped(pool))
>                 kfree(tmp);
>
>         /*
> -       * if we get here due to ZSWAP_SWAPCACHE_EXIST
> -       * a load may be happening concurrently.
> -       * it is safe and okay to not free the entry.
> -       * it is also okay to return !0
> -       */
> +        * If we get here because the page is already in swapcache, a
> +        * load may be happening concurrently. It is safe and okay to
> +        * not free the entry. It is also okay to return !0.
> +        */
>         return ret;
>  }
>
> --
> 2.41.0
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ