Re: [PATCHv5 0/4] add compressing abstraction and multi stream support

[Minchan Kim <minchan@kernel.org>]

Hello Sergey,

On Thu, Feb 13, 2014 at 08:43:18PM +0300, Sergey Senozhatsky wrote:
> This patchset introduces zcomp compression backend abstraction
> adding ability to support compression algorithms other than LZO;
> support for multi compression streams, making parallel compressions
> possible.
> 
> Diff from v4 (reviewed by Minchan Kim):
> -- renamed zcomp buffer_lock; removed src len and dst len from
>    compress() and decompress(); not using term `buffer' and
>    `workmem' in code and documentation; define compress() and
>    decompress() functions for LZO backend; not using goto's;
>    do not put idle zcomp_strm to idle list tail.
> 
> Diff from v3:
> -- renamed compression backend and working memory structs as requested
>    by Minchan Kim; fixed several issues noted by Minchan Kim.
> 
> Sergey Senozhatsky (4):
>   zram: introduce compressing backend abstraction
>   zram: use zcomp compressing backends
>   zram: zcomp support multi compressing streams
>   zram: document max_comp_streams
> 
>  Documentation/ABI/testing/sysfs-block-zram |   9 +-
>  Documentation/blockdev/zram.txt            |  20 +++-
>  drivers/block/zram/Makefile                |   2 +-
>  drivers/block/zram/zcomp.c                 | 155 +++++++++++++++++++++++++++++
>  drivers/block/zram/zcomp.h                 |  56 +++++++++++
>  drivers/block/zram/zcomp_lzo.c             |  48 +++++++++
>  drivers/block/zram/zram_drv.c              | 102 ++++++++++++-------
>  drivers/block/zram/zram_drv.h              |  10 +-
>  8 files changed, 355 insertions(+), 47 deletions(-)
>  create mode 100644 drivers/block/zram/zcomp.c
>  create mode 100644 drivers/block/zram/zcomp.h
>  create mode 100644 drivers/block/zram/zcomp_lzo.c

I reviewed patchset and implement looked good to me but
I found severe regression in my test which was one stream.

Base is current upstream and zcomp-multi is yours.
At last, zcom-single is where I tweaked to see schedule overhead
cause by event.
   
   Base                      zcomp-multi                      zcomp-single

==Initial  write             ==Initial  write              ==Initial  write
records:   5                 records:   5                  records:   5
avg:       1642424.35        avg:       699610.40          avg:       1655583.71
std:       39890.95(2.43%)   std:       232014.19(33.16%)  std:       52293.96(3.16%)
max:       1690170.94        max:       1163473.45         max:       1697164.75
min:       1568669.52        min:       573429.88          min:       1553410.23
==Rewrite  ==Rewrite         ==Rewrite
records:   5                 records:   5                  records:   5
avg:       1611775.39        avg:       501406.64          avg:       1684419.11
std:       17144.58(1.06%)   std:       15354.41(3.06%)    std:       18367.42(1.09%)
max:       1641800.95        max:       531356.78          max:       1706445.84
min:       1593515.27        min:       488817.78          min:       1655335.73
==Read     ==Read            ==Read
records:   5                 records:   5                  records:   5
avg:       2418916.31        avg:       2385573.68         avg:       2316542.26
std:       55324.98(2.29%)   std:       64475.37(2.70%)    std:       50621.10(2.19%)
max:       2517417.72        max:       2444138.89         max:       2383321.09
min:       2364690.92        min:       2263763.77         min:       2233198.12
==Re-read  ==Re-read         ==Re-read
records:   5                 records:   5                  records:   5
avg:       2351292.92        avg:       2333131.90         avg:       2336306.89
std:       73358.82(3.12%)   std:       34726.09(1.49%)    std:       74001.47(3.17%)
max:       2444053.92        max:       2396357.19         max:       2432697.06
min:       2255477.41        min:       2299239.74         min:       2231400.94
==Reverse  Read              ==Reverse  Read               ==Reverse  Read
records:   5                 records:   5                  records:   5
avg:       2383917.40        avg:       2267700.38         avg:       2328689.00
std:       51389.78(2.16%)   std:       57018.67(2.51%)    std:       44955.21(1.93%)
max:       2435560.11        max:       2346465.31         max:       2375047.77
min:       2290726.91        min:       2188208.84         min:       2251465.20
==Stride   read              ==Stride   read               ==Stride   read
records:   5                 records:   5                  records:   5
avg:       2361656.52        avg:       2292182.65         avg:       2302384.26
std:       64730.61(2.74%)   std:       34649.38(1.51%)    std:       51145.23(2.22%)
max:       2471425.77        max:       2341282.19         max:       2375936.66
min:       2279913.31        min:       2242688.59         min:       2224134.48
==Random   read              ==Random   read               ==Random   read
records:   5                 records:   5                  records:   5
avg:       2369086.95        avg:       2315431.27         avg:       2352314.50
std:       19870.36(0.84%)   std:       43020.16(1.86%)    std:       51677.02(2.20%)
max:       2391210.41        max:       2390286.89         max:       2415472.89
min:       2337092.91        min:       2266433.95         min:       2264088.05
==Mixed    workload          ==Mixed    workload           ==Mixed    workload
records:   5                 records:   5                  records:   5
avg:       1822253.03        avg:       2006455.50         avg:       1918270.29
std:       95037.10(5.22%)   std:       67792.78(3.38%)    std:       45933.99(2.39%)
max:       1934495.87        max:       2079128.36         max:       1995693.03
min:       1694223.48        min:       1914532.49         min:       1856410.41
==Random   write             ==Random   write              ==Random   write
records:   5                 records:   5                  records:   5
avg:       1626318.29        avg:       497250.78          avg:       1695582.06
std:       38550.23(2.37%)   std:       1405.42(0.28%)     std:       9211.98(0.54%)
max:       1665839.62        max:       498585.88          max:       1703808.22
min:       1562141.21        min:       494526.45          min:       1677664.94
==Pwrite   ==Pwrite          ==Pwrite
records:   5                 records:   5                  records:   5
avg:       1654641.25        avg:       581709.22          avg:       1641452.34
std:       47202.59(2.85%)   std:       9670.46(1.66%)     std:       38963.62(2.37%)
max:       1740682.36        max:       591300.09          max:       1687387.69
min:       1611436.34        min:       564324.38          min:       1570496.11
==Pread    ==Pread           ==Pread
records:   5                 records:   5                  records:   5
avg:       2308891.23        avg:       2381945.97         avg:       2347688.68
std:       192436.37(8.33%)  std:       22957.85(0.96%)    std:       31682.43(1.35%)
max:       2548482.56        max:       2415788.27         max:       2381937.58
min:       1960229.11        min:       2349188.16         min:       2292507.52


zcomp-single patch

>From 99abaf9c185953c35b0e3cd12ebb90b57f3bbd50 Mon Sep 17 00:00:00 2001
From: Minchan Kim <minchan@kernel.org>
Date: Tue, 18 Feb 2014 11:41:11 +0900
Subject: [PATCH] zram: add single lock

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 drivers/block/zram/zcomp.c | 24 +++++-------------------
 drivers/block/zram/zcomp.h |  2 +-
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index a032f49a52e2..03f4241083ac 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -56,32 +56,18 @@ struct zcomp_strm *zcomp_strm_get(struct zcomp *comp)
 {
 	struct zcomp_strm *zstrm;
 
-	while (1) {
-		spin_lock(&comp->strm_lock);
-		if (list_empty(&comp->idle_strm)) {
-			spin_unlock(&comp->strm_lock);
-			wait_event(comp->strm_wait,
-					!list_empty(&comp->idle_strm));
-			continue;
-		}
-
-		zstrm = list_entry(comp->idle_strm.next,
+	mutex_lock(&comp->strm_lock);
+	zstrm = list_entry(comp->idle_strm.next,
 				struct zcomp_strm, list);
-		list_del(&zstrm->list);
-		spin_unlock(&comp->strm_lock);
-		break;
-	}
+	list_del(&zstrm->list);
 	return zstrm;
 }
 
 /* add zcomp_strm back to idle list and wake up waiter (if any) */
 void zcomp_strm_put(struct zcomp *comp, struct zcomp_strm *zstrm)
 {
-	spin_lock(&comp->strm_lock);
 	list_add(&zstrm->list, &comp->idle_strm);
-	spin_unlock(&comp->strm_lock);
-
-	wake_up(&comp->strm_wait);
+	mutex_unlock(&comp->strm_lock);
 }
 
 int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
@@ -138,7 +124,7 @@ struct zcomp *zcomp_create(const char *compress, int num_strm)
 		return NULL;
 
 	comp->backend = backend;
-	spin_lock_init(&comp->strm_lock);
+	mutex_init(&comp->strm_lock);
 	INIT_LIST_HEAD(&comp->idle_strm);
 	init_waitqueue_head(&comp->strm_wait);
 
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 52899de40ca5..bfcec1cf7d2e 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -35,7 +35,7 @@ struct zcomp_backend {
 /* dynamic per-device compression frontend */
 struct zcomp {
 	/* protect strm list */
-	spinlock_t strm_lock;
+	struct mutex strm_lock;
 	/* list of available strms */
 	struct list_head idle_strm;
 	wait_queue_head_t strm_wait;
-- 
1.8.5.3

As you can see, your patch made zram too much regressed when it
used one stream buffer. The reason I guessed is overhead of
scheduling by sleep/wakeup when others couldn't find a idle stream
so I had an experiment with below simple patch to restore old behavior
so it works well again. The reason old behaivor was really good is
it uses mutex with spin_on_owner so that it could avoid frequent
sleep/wakeup.

A solution I could think is that we could grow up the number of
buffer dynamically up to max_buffers(ex, 2 * number of CPU) so we
could grab a idle stream easily for each CPU while we could release
buffers by shrinker when the memory pressure is severe.
Of course, we should keep one buffer to work.

Another solution I can imagaine is to define CONFIG_ZRAM_MULTI_STREAM
and CONFIG_ZRAM_SINGLE_STREAM(ie, default) so we couldn't break good
performance for old cases and new users who want write parallel
could use ZRAM_MULTI which should be several streams at a default
rather than a stream. And we could ehhacne it further by dynamic control
as I mentioned.

Thoughts?


> 
> -- 
> 1.9.0.rc3.260.g4cf525c
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Kind regards,
Minchan Kim