mm: zswap: reduce request contention on loads

Stores and loads serialize on the same per-CPU acomp request and mutex. A low-priority store can be preempted as soon as the compressor drops its stream lock, while it still holds the mutex. A higher-priority load on that CPU then waits until the store runs again, which can take a long time when other tasks are runnable.

Give compression and decompression their own request, completion wait and mutex. Since commit e2c3b6b21c77f ("mm: zswap: use SG list decompression APIs from zsmalloc"), the per-CPU buffer is only used for compression, so group it with the compression request. Independent requests can share the per-CPU transform; the codecs and drivers synchronize their shared state internally. Loads can still wait for each other on the decompression mutex, and stores still serialize on the compression mutex.

acomp_request_free() ignores NULL requests, so drop the check in acomp_ctx_free().

This follows the proposal from Sergey Senozhatsky for the same split for zram [1].

[1] https://lore.kernel.org/all/[email protected]/

Acked-by Nhat Pham <[email protected]>

Signed-off-by Usama Arif <[email protected]>

---
 mm/zswap.c | 108 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 64 insertions(+), 44 deletions(-)

mm/zswap.c +44 -15

index ae19e301fced7..23c4298765795 100644
@@ -137,14 +137,25 @@ bool zswap_never_enabled(void)
 * data structures
 **********************************/
 
-struct crypto_acomp_ctx {
-       struct crypto_acomp *acomp;
+struct zswap_acomp_req {
        struct acomp_req *req;
        struct crypto_wait wait;
-       u8 *buffer;
        struct mutex mutex;
 };
 
+/* The compression mutex also protects the output buffer. */
+struct zswap_comp_ctx {
+       struct zswap_acomp_req areq;
+       u8 *buffer;
+};
+
+/* Separate requests, so that decompression does not wait for compression. */
+struct crypto_acomp_ctx {
+       struct crypto_acomp *acomp;
+       struct zswap_comp_ctx comp;
+       struct zswap_acomp_req decomp;
+};
+
 /*
  * The lock ordering is zswap_tree.lock -> zswap_pool.lru_lock.
  * The only case where lru_lock is not acquired while holding tree.lock is

@@ -270,14 +281,10 @@ static void acomp_ctx_free(struct crypto_acomp_ctx *acomp_ctx)
        if (!acomp_ctx)
                return;
 
-       /*
-        * If there was an error in allocating @acomp_ctx->req, it
-        * would be set to NULL.
-        */
-       if (acomp_ctx->req)
-               acomp_request_free(acomp_ctx->req);
-
-       acomp_ctx->req = NULL;
+       acomp_request_free(acomp_ctx->comp.areq.req);
+       acomp_ctx->comp.areq.req = NULL;
+       acomp_request_free(acomp_ctx->decomp.req);
+       acomp_ctx->decomp.req = NULL;
 
        /*
         * We have to handle both cases here: an error pointer return from

@@ -289,8 +296,8 @@ static void acomp_ctx_free(struct crypto_acomp_ctx *acomp_ctx)
 
        acomp_ctx->acomp = NULL;
 
-       kfree(acomp_ctx->buffer);
-       acomp_ctx->buffer = NULL;
+       kfree(acomp_ctx->comp.buffer);
+       acomp_ctx->comp.buffer = NULL;
 }
 
 static struct zswap_pool *zswap_pool_create(char *compressor)

@@ -796,6 +803,28 @@ static void zswap_entry_free(struct zswap_entry *entry)
 /*********************************
 * compressed storage functions
 **********************************/
+static int zswap_acomp_req_init(struct zswap_acomp_req *areq,
+                               struct crypto_acomp *acomp)
+{
+       /* acomp_request_alloc() returns NULL in case of an error. */
+       areq->req = acomp_request_alloc(acomp);
+       if (!areq->req)
+               return -ENOMEM;
+
+       crypto_init_wait(&areq->wait);
+
+       /*
+        * if the backend of acomp is async zip, crypto_req_done() will wakeup
+        * crypto_wait_req(); if the backend of acomp is scomp, the callback
+        * won't be called, crypto_wait_req() will return without blocking.
+        */
+       acomp_request_set_callback(areq->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+                                  crypto_req_done, &areq->wait);
+
+       mutex_init(&areq->mutex);
+       return 0;
+}
+
 static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
 {
        struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node);

@@ -811,8 +840,8 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
                return 0;
        }
 
-       acomp_ctx->buffer = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu));
-       if (!acomp_ctx->buffer)
+       acomp_ctx->comp.buffer = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu));
+       if (!acomp_ctx->comp.buffer)
                return ret;
 
        /*

mm/zswap.c +0 -11

                pr_err("could not alloc crypto acomp_request %s\n",
                       pool->tfm_name);
                goto fail;
        }
 
-       crypto_init_wait(&acomp_ctx->wait);
-
-       /*
-        * if the backend of acomp is async zip, crypto_req_done() will wakeup
-        * crypto_wait_req(); if the backend of acomp is scomp, the callback
-        * won't be called, crypto_wait_req() will return without blocking.
-        */
-       acomp_request_set_callback(acomp_ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
-                                  crypto_req_done, &acomp_ctx->wait);
-
-       mutex_init(&acomp_ctx->mutex);
        return 0;
 
 fail:

Quoted 7 lines · Usama Arif

Stores and loads serialize on the same per-CPU acomp request and mutex. A low-priority store can be preempted as soon as the compressor drops its stream lock, while it still holds the mutex. A higher-priority load on that CPU then waits until the store runs again, which can take a long time when other tasks are runnable.

Quoted 8 lines

Give compression and decompression their own request, completion wait and mutex. Since commit e2c3b6b21c77f ("mm: zswap: use SG list decompression APIs from zsmalloc"), the per-CPU buffer is only used for compression, so group it with the compression request. Independent requests can share the per-CPU transform; the codecs and drivers synchronize their shared state internally. Loads can still wait for each other on the decompression mutex, and stores still serialize on the compression mutex.

Quoted 22 lines · Usama Arif

Stores and loads serialize on the same per-CPU acomp request and mutex. A low-priority store can be preempted as soon as the compressor drops its stream lock, while it still holds the mutex. A higher-priority load on that CPU then waits until the store runs again, which can take a long time when other tasks are runnable.

Give compression and decompression their own request, completion wait and mutex. Since commit e2c3b6b21c77f ("mm: zswap: use SG list decompression APIs from zsmalloc"), the per-CPU buffer is only used for compression, so group it with the compression request. Independent requests can share the per-CPU transform; the codecs and drivers synchronize their shared state internally. Loads can still wait for each other on the decompression mutex, and stores still serialize on the compression mutex.

acomp_request_free() ignores NULL requests, so drop the check in acomp_ctx_free().

This follows the proposal from Sergey Senozhatsky for the same split for zram [1].

Quoted 4 lines

[1] https://lore.kernel.org/all/[email protected]/

Acked-by Nhat Pham <[email protected]>

Signed-off-by Usama Arif <[email protected]>

Why didn't you take Herbert's Ack?

LGTM with one nit below:

Acked-by Yosry Ahmed <[email protected]>

Quoted 108 lines

---
 mm/zswap.c | 108 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 64 insertions(+), 44 deletions(-)

mm/zswap.c +44 -15

index ae19e301fced7..23c4298765795 100644
@@ -137,14 +137,25 @@ bool zswap_never_enabled(void)
 * data structures
 **********************************/
 
-struct crypto_acomp_ctx {
-       struct crypto_acomp *acomp;
+struct zswap_acomp_req {
        struct acomp_req *req;
        struct crypto_wait wait;
-       u8 *buffer;
        struct mutex mutex;
 };
 
+/* The compression mutex also protects the output buffer. */
+struct zswap_comp_ctx {
+       struct zswap_acomp_req areq;
+       u8 *buffer;
+};
+
+/* Separate requests, so that decompression does not wait for compression. */
+struct crypto_acomp_ctx {
+       struct crypto_acomp *acomp;
+       struct zswap_comp_ctx comp;
+       struct zswap_acomp_req decomp;
+};
+
 /*
  * The lock ordering is zswap_tree.lock -> zswap_pool.lru_lock.
  * The only case where lru_lock is not acquired while holding tree.lock is

@@ -270,14 +281,10 @@ static void acomp_ctx_free(struct crypto_acomp_ctx *acomp_ctx)
        if (!acomp_ctx)
                return;
 
-       /*
-        * If there was an error in allocating @acomp_ctx->req, it
-        * would be set to NULL.
-        */
-       if (acomp_ctx->req)
-               acomp_request_free(acomp_ctx->req);
-
-       acomp_ctx->req = NULL;
+       acomp_request_free(acomp_ctx->comp.areq.req);
+       acomp_ctx->comp.areq.req = NULL;
+       acomp_request_free(acomp_ctx->decomp.req);
+       acomp_ctx->decomp.req = NULL;
 
        /*
         * We have to handle both cases here: an error pointer return from

@@ -289,8 +296,8 @@ static void acomp_ctx_free(struct crypto_acomp_ctx *acomp_ctx)
 
        acomp_ctx->acomp = NULL;
 
-       kfree(acomp_ctx->buffer);
-       acomp_ctx->buffer = NULL;
+       kfree(acomp_ctx->comp.buffer);
+       acomp_ctx->comp.buffer = NULL;
 }
 
 static struct zswap_pool *zswap_pool_create(char *compressor)

@@ -796,6 +803,28 @@ static void zswap_entry_free(struct zswap_entry *entry)
 /*********************************
 * compressed storage functions
 **********************************/
+static int zswap_acomp_req_init(struct zswap_acomp_req *areq,
+                               struct crypto_acomp *acomp)
+{
+       /* acomp_request_alloc() returns NULL in case of an error. */
+       areq->req = acomp_request_alloc(acomp);
+       if (!areq->req)
+               return -ENOMEM;
+
+       crypto_init_wait(&areq->wait);
+
+       /*
+        * if the backend of acomp is async zip, crypto_req_done() will wakeup
+        * crypto_wait_req(); if the backend of acomp is scomp, the callback
+        * won't be called, crypto_wait_req() will return without blocking.
+        */
+       acomp_request_set_callback(areq->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+                                  crypto_req_done, &areq->wait);
+
+       mutex_init(&areq->mutex);
+       return 0;
+}
+
 static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
 {
        struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node);

@@ -811,8 +840,8 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
                return 0;
        }
 
-       acomp_ctx->buffer = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu));
-       if (!acomp_ctx->buffer)
+       acomp_ctx->comp.buffer = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu));
+       if (!acomp_ctx->comp.buffer)
                return ret;
 
        /*

mm/zswap.c static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node) · line 860

Quoted 8 lines

mm/zswap.c +2 -3

@@ -827,25 +856,13 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
                goto fail;
        }
 
-       /* acomp_request_alloc() returns NULL in case of an error. */
-       acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
-       if (!acomp_ctx->req) {
+       if (zswap_acomp_req_init(&acomp_ctx->comp.areq, acomp_ctx->acomp) ||
+           zswap_acomp_req_init(&acomp_ctx->decomp, acomp_ctx->acomp)) {

We stop propagating the output of these functions to the caller here. It still works because ret is initialized as -ENOMEM, and that's the only possible failure of zswap_acomp_req_init(), but that's really subtle.

I'd rather either propagate the return value as-is on failure, or make zswap_acomp_req_init() a boolean so that it's obvious there's one failure mode and future changes can't just return a new error code that would be silently thrown away.

Quoted 19 lines

mm/zswap.c +0 -11

                pr_err("could not alloc crypto acomp_request %s\n",
                       pool->tfm_name);
                goto fail;
        }
 
-       crypto_init_wait(&acomp_ctx->wait);
-
-       /*
-        * if the backend of acomp is async zip, crypto_req_done() will wakeup
-        * crypto_wait_req(); if the backend of acomp is scomp, the callback
-        * won't be called, crypto_wait_req() will return without blocking.
-        */
-       acomp_request_set_callback(acomp_ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
-                                  crypto_req_done, &acomp_ctx->wait);
-
-       mutex_init(&acomp_ctx->mutex);
        return 0;
 
 fail:

With separate requests for compression and decompression, loads still serialize on the per-CPU decompression mutex. A low-priority load that is preempted after the codec drops its stream lock keeps holding the mutex and stalls every other load on that CPU, including higher-priority ones.

Synchronous algorithms can use an on-stack request, so decompress with one and take no zswap lock. All in-tree software compressors qualify. The Crypto API enforces MAX_SYNC_COMP_REQSIZE during initialization, so checking acomp_is_async() is sufficient. Asynchronous algorithms keep the per-CPU request and mutex, which is still taken before the zsmalloc read lock.

Reading the per-CPU context without the mutex is safe. Since commit ef3c0f6cb798e ("mm: zswap: tie per-CPU acomp_ctx lifetime to the pool"), it is set up before its CPU comes online and is not torn down until the pool is destroyed. The codecs keep their own stream locks, and crypto_acomp_decompress() rejects on-stack requests only for asynchronous transforms, which never take this path.

For software compressors this drops the heap request added for decompression. The on-stack request and wait take 216 bytes, which makes the load path about 270 bytes deeper on x86-64. Asynchronous algorithms pay this too.

Acked-by Nhat Pham <[email protected]>

Signed-off-by Usama Arif <[email protected]>