-----Original Message-----
From: Vitaly Wool [mailto:vitaly.wool@konsulko.com]
Sent: Thursday, January 21, 2021 10:17 PM
To: tiantao (H) <tiantao6@hisilicon.com>
Cc: Seth Jennings <redacted>; Dan Streetman <redacted>;
Andrew Morton [off-list ref]; Song Bao Hua (Barry Song)
[off-list ref]; Linux-MM [off-list ref]
Subject: Re: [RFC mm/zswap 1/2] mm/zswap: add the flag can_sleep_mapped

On Fri, Dec 25, 2020 at 12:02 PM Tian Tao [off-list ref] wrote:

quoted

add a flag to zpool, named is "can_sleep_mapped", and have it set true
for zbud/z3fold, set false for zsmalloc. Then zswap could go the current
path if the flag is true; and if it's false, copy data from src to a
temporary buffer, then unmap the handle, take the mutex, process the
buffer instead of src to avoid sleeping function called from atomic
context.

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
 include/linux/zpool.h |  3 +++
 mm/zpool.c            | 13 +++++++++++++
 mm/zswap.c            | 50

+++++++++++++++++++++++++++++++++++++++++++++-----

quoted

 3 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/include/linux/zpool.h b/include/linux/zpool.h
index 51bf430..e899701 100644
--- a/include/linux/zpool.h
+++ b/include/linux/zpool.h

@@ -73,6 +73,7 @@ u64 zpool_get_total_size(struct zpool *pool);
  * @malloc:    allocate mem from a pool.
  * @free:      free mem from a pool.
  * @shrink:    shrink the pool.
+ * @sleep_mapped: whether zpool driver can sleep during map.
  * @map:       map a handle.
  * @unmap:     unmap a handle.
  * @total_size:        get total size of a pool.

@@ -100,6 +101,7 @@ struct zpool_driver {
        int (*shrink)(void *pool, unsigned int pages,
                                unsigned int *reclaimed);

+       bool sleep_mapped;
        void *(*map)(void *pool, unsigned long handle,
                                enum zpool_mapmode mm);
        void (*unmap)(void *pool, unsigned long handle);

@@ -112,5 +114,6 @@ void zpool_register_driver(struct zpool_driver *driver);
 int zpool_unregister_driver(struct zpool_driver *driver);

 bool zpool_evictable(struct zpool *pool);
+bool zpool_can_sleep_mapped(struct zpool *pool);

 #endif

diff --git a/mm/zpool.c b/mm/zpool.c
index 3744a2d..5ed7120 100644
--- a/mm/zpool.c
+++ b/mm/zpool.c

@@ -23,6 +23,7 @@ struct zpool {
        void *pool;
        const struct zpool_ops *ops;
        bool evictable;
+       bool can_sleep_mapped;

        struct list_head list;
 };

@@ -183,6 +184,7 @@ struct zpool *zpool_create_pool(const char *type, const

char *name, gfp_t gfp,

quoted

        zpool->pool = driver->create(name, gfp, ops, zpool);
        zpool->ops = ops;
        zpool->evictable = driver->shrink && ops && ops->evict;
+       zpool->can_sleep_mapped = driver->sleep_mapped;

        if (!zpool->pool) {
                pr_err("couldn't create %s pool\n", type);

@@ -393,6 +395,17 @@ bool zpool_evictable(struct zpool *zpool)
        return zpool->evictable;
 }

+/**
+ * zpool_can_sleep_mapped - Test if zpool can sleep when do mapped.
+ * @zpool:     The zpool to test
+ *
+ * Returns: true if zpool can sleep; false otherwise.
+ */
+bool zpool_can_sleep_mapped(struct zpool *zpool)
+{
+       return zpool->can_sleep_mapped;
+}
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Dan Streetman <ddstreet@ieee.org>");
 MODULE_DESCRIPTION("Common API for compressed memory storage");

diff --git a/mm/zswap.c b/mm/zswap.c
index 182f6ad..67d4555 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c

@@ -935,13 +935,20 @@ static int zswap_writeback_entry(struct zpool *pool,

unsigned long handle)

quoted

        struct scatterlist input, output;
        struct crypto_acomp_ctx *acomp_ctx;

-       u8 *src;
+       u8 *src, *tmp;
        unsigned int dlen;
        int ret;
        struct writeback_control wbc = {
                .sync_mode = WB_SYNC_NONE,
        };

+       if (!zpool_can_sleep_mapped(pool)) {
+
+               tmp = kmalloc(entry->length, GFP_ATOMIC);

This has escaped my attention, but this is obviously a bug. 'entry' is
not initialized at this point.
You either have to move memory allocation further down when
entry->length starts making sense, or allocate PAGE_SIZE.

Since zsmalloc has no evict entry, we have never arrived here.
That's why the testing by both Tiantao and Sebastian didn't
show the problem.

Best regards,
   Vitaly

quoted

+               if (!tmp)
+                       return -ENOMEM;
+       }
+
        /* extract swpentry from data */
        zhdr = zpool_map_handle(pool, handle, ZPOOL_MM_RO);
        swpentry = zhdr->swpentry; /* here */

@@ -979,6 +986,14 @@ static int zswap_writeback_entry(struct zpool *pool,

unsigned long handle)

quoted

                dlen = PAGE_SIZE;
                src = (u8 *)zhdr + sizeof(struct zswap_header);

+               if (!zpool_can_sleep_mapped(pool)) {
+
+                       memcpy(tmp, src, entry->length);
+                       src = tmp;
+
+                       zpool_unmap_handle(pool, handle);
+               }
+
                mutex_lock(acomp_ctx->mutex);
                sg_init_one(&input, src, entry->length);
                sg_init_table(&output, 1);

@@ -1033,7 +1048,11 @@ static int zswap_writeback_entry(struct zpool *pool,

unsigned long handle)

quoted

        spin_unlock(&tree->lock);

 end:
-       zpool_unmap_handle(pool, handle);
+       if (zpool_can_sleep_mapped(pool))
+               zpool_unmap_handle(pool, handle);
+       else
+               kfree(tmp);
+
        return ret;
 }

@@ -1235,7 +1254,7 @@ static int zswap_frontswap_load(unsigned type, pgoff_t

offset,

quoted

        struct zswap_entry *entry;
        struct scatterlist input, output;
        struct crypto_acomp_ctx *acomp_ctx;
-       u8 *src, *dst;
+       u8 *src, *dst, *tmp;
        unsigned int dlen;
        int ret;

@@ -1256,12 +1275,29 @@ static int zswap_frontswap_load(unsigned type, pgoff_t

offset,

quoted

                goto freeentry;
        }

+       if (!zpool_can_sleep_mapped(entry->pool->zpool)) {
+
+               tmp = kmalloc(entry->length, GFP_ATOMIC);
+               if (!tmp) {
+                       ret = -ENOMEM;
+                       goto freeentry;
+               }
+       }
+
        /* decompress */
        dlen = PAGE_SIZE;
        src = zpool_map_handle(entry->pool->zpool, entry->handle,

ZPOOL_MM_RO);

quoted

        if (zpool_evictable(entry->pool->zpool))
                src += sizeof(struct zswap_header);

+       if (!zpool_can_sleep_mapped(entry->pool->zpool)) {
+
+               memcpy(tmp, src, entry->length);
+               src = tmp;
+
+               zpool_unmap_handle(entry->pool->zpool, entry->handle);
+       }
+
        acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
        mutex_lock(acomp_ctx->mutex);
        sg_init_one(&input, src, entry->length);

@@ -1271,7 +1307,11 @@ static int zswap_frontswap_load(unsigned type, pgoff_t

offset,

quoted

        ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req),

&acomp_ctx->wait);

quoted

        mutex_unlock(acomp_ctx->mutex);

-       zpool_unmap_handle(entry->pool->zpool, entry->handle);
+       if (zpool_can_sleep_mapped(entry->pool->zpool))
+               zpool_unmap_handle(entry->pool->zpool, entry->handle);
+       else
+               kfree(tmp);
+
        BUG_ON(ret);

 freeentry:

@@ -1279,7 +1319,7 @@ static int zswap_frontswap_load(unsigned type, pgoff_t

offset,

quoted

        zswap_entry_put(tree, entry);
        spin_unlock(&tree->lock);

-       return 0;
+       return ret;
 }

 /* frees an entry in zswap */
--
2.7.4

Thanks
Barry