Re: [PATCH v3 10/11] zram: use rw_semaphore for decompression streams

[Sergey Senozhatsky <senozhatsky@chromium.org>]

On (26/10/16 11:50), Sergey Senozhatsky wrote:
> > zstd backend, same A/B workload, on top of 9+10+kmalloc:
> > 
> >                        9+10+kmalloc   + preempt-disable
> >   cwrite_contended     14,880         48          (-99.7%)
> >   cwrite_wait_ns_total 137 s          0.32 s      (-99.8%)
> >   cwrite_wait_ns_max   243 ms         43 ms       (-82%)
> >   dexcl_contended      15,919         54          (-99.7%)
> >   dexcl_wait_ns_total  109.6 s        0.68 s      (-99.4%)
> >   dexcl_wait_ns_max    145 ms         34.6 ms     (-76%)
> > 
> > zstd stays 100% exclusive either way; the win comes purely from the
> > holder no longer being preempted/migrated, so the hold time stays short.
> > It's the same mechanism that took lzo-rle read contention from 24,749 to
> > 406 earlier. So the two are complementary: shared reads remove
> > reader-vs-reader queuing for stateless backends, and preempt-disable
> > covers the exclusive/write serialization that stateful backends can't
> > avoid.
> > 
> > Quick and dirty, for discussion only - Patch below.
> 
> Yeah... I need to think about it, before I can join discussions
> in a meaningful way.

A quick and dirty patch, as usual, just to play around with.  This
attempts to improve the reader side, because the reader is side
much more sensitive.  Tested only with zstd.  We probably can do
something for the writer side as well, but I'd prefer to postpone
that, until we actually have to do something.

A quick thinking-process dump:
Our shared dstrm-s are almost non-pCPU at this point (we have a
fall back bounce buffer in pCPU), so we can have any in-flight
number of those.   Doing something similar for exclusive dstrm-s
is more complicated, because those dctxes are beefy - zstd dctx
sits at ~100KB.  And this rules out the "pool of idle dctxes"
approach.  I decided to keep them pCPU, and implement exclusive
dstrm borrowing (from remote CPUs) instead.

A quick (and a bit extreme) synthetic fio test for Reader vs Reader
inversion on 24 CPUs (preempt=full, zstd, 48 low priority (nice 19)
bg readers, 24 normal priority (nice 0) CPU hogs and 24 high priority
(nice -19) fg readers).

fg reader results (average of 3 runs):

    Metric           Base               Patched
    ----------------------------------------------------------
    Read IOPS        396k               1,064k        (+2.7x)
    Read clat avg    48.32 us           9.66 us       (-5.0x)
    Read clat stdev  7,571.00 us        31.92 us      (-237x)
    Read clat p99.99 5,707.10 us        2,233.68 us   (-2.6x)
    Read clat max    3,555,300 us       6,287.57 us   (-565x)

A quick note (the last one) - happy Friday!  Time to logoff for me.

---
 drivers/block/zram/zcomp.c    | 75 +++++++++++++++++++++++++----------
 drivers/block/zram/zcomp.h    |  5 ++-
 drivers/block/zram/zram_drv.c |  4 +-
 3 files changed, 59 insertions(+), 25 deletions(-)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 81f21bad787e..af1aaa1f9e9b 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -190,38 +190,66 @@ bool zcomp_shared_decomp(struct zcomp *comp)
 	return !comp->ops->create_dctx;
 }
 
+static bool zcomp_dstrm_excl_trylock(struct zcomp_dstrm *zstrm)
+{
+	/* Cheap pre-check: don't touch a busy stream's cachelines */
+	if (mutex_is_locked(&zstrm->excl_lock))
+		return false;
+	/*
+	 * Pin the stream.  The write side of ->lock is only taken by
+	 * zcomp_cpu_dead(), which also sets ->local_copy to NULL.
+	 */
+	if (!down_read_trylock(&zstrm->lock))
+		return false;
+	if (zstrm->local_copy && mutex_trylock(&zstrm->excl_lock))
+		return true;
+	up_read(&zstrm->lock);
+	return false;
+}
+
+static struct zcomp_dstrm *zcomp_dstrm_excl_tryget(struct zcomp *comp)
+{
+	struct zcomp_dstrm *zstrm;
+	unsigned int cpu;
+
+	for_each_online_cpu_wrap(cpu, raw_smp_processor_id()) {
+		zstrm = per_cpu_ptr(comp->dstream, cpu);
+		if (zcomp_dstrm_excl_trylock(zstrm))
+			return zstrm;
+	}
+	return NULL;
+}
+
 struct zcomp_dstrm *zcomp_dstrm_get(struct zcomp *comp,
 				    enum zcomp_dstrm_mode mode)
 {
-	for (;;) {
-		struct zcomp_dstrm *zstrm = raw_cpu_ptr(comp->dstream);
-
-		/*
-		 * Pin the stream.  The write side of ->lock is only taken
-		 * by zcomp_cpu_dead(), so this never contends with other
-		 * decompression requests, regardless of their mode.
-		 */
-		down_read(&zstrm->lock);
-		if (!zstrm->local_copy) {
-			/* The stream has been destroyed, re-try */
+	struct zcomp_dstrm *zstrm;
+
+	if (mode == ZCOMP_DSTRM_SHARED) {
+		for (;;) {
+			zstrm = raw_cpu_ptr(comp->dstream);
+			down_read(&zstrm->lock);
+			if (zstrm->local_copy)
+				return zstrm;
 			up_read(&zstrm->lock);
-			continue;
 		}
-
-		/*
-		 * Exclusive requests additionally own ->local_copy and
-		 * ->ctx and hence can only wait for each other.
-		 */
-		if (mode == ZCOMP_DSTRM_EXCLUSIVE)
-			mutex_lock(&zstrm->excl_lock);
-		return zstrm;
 	}
+
+	/* wait_event() checks the condition first before touching the waitq */
+	wait_event(comp->dstrm_waitq, (zstrm = zcomp_dstrm_excl_tryget(comp)));
+	return zstrm;
 }
 
-void zcomp_dstrm_put(struct zcomp_dstrm *zstrm, enum zcomp_dstrm_mode mode)
+void zcomp_dstrm_put(struct zcomp *comp, struct zcomp_dstrm *zstrm,
+		     enum zcomp_dstrm_mode mode)
 {
-	if (mode == ZCOMP_DSTRM_EXCLUSIVE)
+	if (mode == ZCOMP_DSTRM_EXCLUSIVE) {
 		mutex_unlock(&zstrm->excl_lock);
+		up_read(&zstrm->lock);
+		if (wq_has_sleeper(&comp->dstrm_waitq))
+			wake_up(&comp->dstrm_waitq);
+		return;
+	}
 	up_read(&zstrm->lock);
 }
 
@@ -294,6 +322,8 @@ int zcomp_decompress(struct zcomp *comp, struct zcomp_dstrm *zstrm,
 	mutex_lock(&zstrm->excl_lock);
 	ret = zcomp_decompress_buf(comp, zstrm, sg, zstrm->local_copy, &req);
 	mutex_unlock(&zstrm->excl_lock);
+	if (wq_has_sleeper(&comp->dstrm_waitq))
+		wake_up(&comp->dstrm_waitq);
 	return ret;
 }
 
@@ -385,6 +415,7 @@ static int zcomp_init(struct zcomp *comp, struct zcomp_params *params,
 		ret = -ENOMEM;
 		goto cleanup;
 	}
+	init_waitqueue_head(&comp->dstrm_waitq);
 
 	for_each_possible_cpu(cpu) {
 		struct zcomp_dstrm *zstrm = per_cpu_ptr(comp->dstream, cpu);
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 6764818d2a9f..39efefc43a13 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -5,6 +5,7 @@
 
 #include <linux/mutex.h>
 #include <linux/rwsem.h>
+#include <linux/wait.h>
 
 struct scatterlist;
 
@@ -96,6 +97,7 @@ struct zcomp_ops {
 /* dynamic per-device compression frontend */
 struct zcomp {
 	struct zcomp_dstrm __percpu *dstream;
+	wait_queue_head_t dstrm_waitq;
 	struct zcomp_cstrm __percpu *cstream;
 	struct zcomp_cstrm *recomp_cstream;
 	const struct zcomp_ops *ops;
@@ -118,7 +120,8 @@ void zcomp_cstrm_put(struct zcomp_cstrm *zstrm);
 bool zcomp_shared_decomp(struct zcomp *comp);
 struct zcomp_dstrm *zcomp_dstrm_get(struct zcomp *comp,
 				    enum zcomp_dstrm_mode mode);
-void zcomp_dstrm_put(struct zcomp_dstrm *zstrm, enum zcomp_dstrm_mode mode);
+void zcomp_dstrm_put(struct zcomp *comp, struct zcomp_dstrm *zstrm,
+		     enum zcomp_dstrm_mode mode);
 
 int zcomp_compress(struct zcomp *comp, struct zcomp_cstrm *zstrm,
 		   const void *src, unsigned int *dst_len);
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index e608769bfad5..079f3723db31 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1382,7 +1382,7 @@ static int decompress_bdev_page(struct zram *zram, struct page *page,
 		copy_page(dst, zstrm->local_copy);
 		kunmap_local(dst);
 	}
-	zcomp_dstrm_put(zstrm, ZCOMP_DSTRM_EXCLUSIVE);
+	zcomp_dstrm_put(zram->comps[prio], zstrm, ZCOMP_DSTRM_EXCLUSIVE);
 	slot_unlock(zram, index);
 
 	return ret;
@@ -2135,7 +2135,7 @@ static int read_compressed_page(struct zram *zram, struct page *page,
 	dst = kmap_local_page(page);
 	ret = zcomp_decompress(zram->comps[prio], zstrm, sg, size, dst);
 	kunmap_local(dst);
-	zcomp_dstrm_put(zstrm, mode);
+	zcomp_dstrm_put(zram->comps[prio], zstrm, mode);
 	zs_obj_read_sg_end(zram->mem_pool, handle);
 
 	return ret;
-- 
2.56.0.176.g3575e65682-goog