Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1614884 > unrolled thread

[PATCH 3/5] zram: use zram_slot_lock instead of raw bit_spin_lock op

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/5] zram: use zram_slot_lock instead of raw bit_spin_lock op Minchan Kim <minchan@kernel.org> - 2017-04-03 07:20 +0200
    Re: [PATCH 3/5] zram: use zram_slot_lock instead of raw  bit_spin_lock op Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-04-03 08:10 +0200
      Re: [PATCH 3/5] zram: use zram_slot_lock instead of raw  bit_spin_lock op Minchan Kim <minchan@kernel.org> - 2017-04-03 08:40 +0200
        Re: [PATCH 3/5] zram: use zram_slot_lock instead of raw  bit_spin_lock op Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-04-03 10:10 +0200
    Re: [PATCH 3/5] zram: use zram_slot_lock instead of raw  bit_spin_lock op Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-04-04 04:20 +0200
      Re: [PATCH 3/5] zram: use zram_slot_lock instead of raw  bit_spin_lock op Minchan Kim <minchan@kernel.org> - 2017-04-04 07:00 +0200

#1614884 — [PATCH 3/5] zram: use zram_slot_lock instead of raw bit_spin_lock op

With this clean-up phase, I want to use zram's wrapper function
to lock table access which is more consistent with other zram's
functions.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 drivers/block/zram/zram_drv.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 7938f4b98b01..71b0a584bc85 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -415,24 +415,39 @@ static DEVICE_ATTR_RO(io_stat);
 static DEVICE_ATTR_RO(mm_stat);
 static DEVICE_ATTR_RO(debug_stat);
 
+
+static void zram_slot_lock(struct zram *zram, u32 index)
+{
+	struct zram_meta *meta = zram->meta;
+
+	bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
+}
+
+static void zram_slot_unlock(struct zram *zram, u32 index)
+{
+	struct zram_meta *meta = zram->meta;
+
+	bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
+}
+
 static bool zram_special_page_read(struct zram *zram, u32 index,
 				struct page *page,
 				unsigned int offset, unsigned int len)
 {
 	struct zram_meta *meta = zram->meta;
 
-	bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
+	zram_slot_lock(zram, index);
 	if (unlikely(!meta->table[index].handle) ||
 			zram_test_flag(meta, index, ZRAM_SAME)) {
 		void *mem;
 
-		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
+		zram_slot_unlock(zram, index);
 		mem = kmap_atomic(page);
 		zram_fill_page(mem + offset, len, meta->table[index].element);
 		kunmap_atomic(mem);
 		return true;
 	}
-	bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
+	zram_slot_unlock(zram, index);
 
 	return false;
 }
@@ -448,11 +463,11 @@ static bool zram_special_page_write(struct zram *zram, u32 index,
 
 		kunmap_atomic(mem);
 		/* Free memory associated with this sector now. */
-		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
+		zram_slot_lock(zram, index);
 		zram_free_page(zram, index);
 		zram_set_flag(meta, index, ZRAM_SAME);
 		zram_set_element(meta, index, element);
-		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
+		zram_slot_unlock(zram, index);
 
 		atomic64_inc(&zram->stats.same_pages);
 		return true;
@@ -559,7 +574,7 @@ static int zram_decompress_page(struct zram *zram, struct page *page, u32 index)
 	if (zram_special_page_read(zram, index, page, 0, PAGE_SIZE))
 		return 0;
 
-	bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
+	zram_slot_lock(zram, index);
 	handle = meta->table[index].handle;
 	size = zram_get_obj_size(meta, index);
 
@@ -578,7 +593,7 @@ static int zram_decompress_page(struct zram *zram, struct page *page, u32 index)
 		zcomp_stream_put(zram->comp);
 	}
 	zs_unmap_object(meta->mem_pool, handle);
-	bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
+	zram_slot_unlock(zram, index);
 
 	/* Should NEVER happen. Return bio error if it does. */
 	if (unlikely(ret))
@@ -731,11 +746,11 @@ static int __zram_bvec_write(struct zram *zram, struct bio_vec *bvec,
 	 * Free memory associated with this sector
 	 * before overwriting unused sectors.
 	 */
-	bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
+	zram_slot_lock(zram, index);
 	zram_free_page(zram, index);
 	meta->table[index].handle = handle;
 	zram_set_obj_size(meta, index, comp_len);
-	bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
+	zram_slot_unlock(zram, index);
 
 	/* Update stats */
 	atomic64_add(comp_len, &zram->stats.compr_data_size);
@@ -793,7 +808,6 @@ static void zram_bio_discard(struct zram *zram, u32 index,
 			     int offset, struct bio *bio)
 {
 	size_t n = bio->bi_iter.bi_size;
-	struct zram_meta *meta = zram->meta;
 
 	/*
 	 * zram manages data in physical block size units. Because logical block
@@ -814,9 +828,9 @@ static void zram_bio_discard(struct zram *zram, u32 index,
 	}
 
 	while (n >= PAGE_SIZE) {
-		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
+		zram_slot_lock(zram, index);
 		zram_free_page(zram, index);
-		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
+		zram_slot_unlock(zram, index);
 		atomic64_inc(&zram->stats.notify_free);
 		index++;
 		n -= PAGE_SIZE;
@@ -925,9 +939,9 @@ static void zram_slot_free_notify(struct block_device *bdev,
 	zram = bdev->bd_disk->private_data;
 	meta = zram->meta;
 
-	bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
+	zram_slot_lock(zram, index);
 	zram_free_page(zram, index);
-	bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
+	zram_slot_unlock(zram, index);
 	atomic64_inc(&zram->stats.notify_free);
 }
 
-- 
2.7.4

[toc] | [next] | [standalone]

#1614899 — Re: [PATCH 3/5] zram: use zram_slot_lock instead of raw bit_spin_lock op

Hello Minchan,

On (04/03/17 14:17), Minchan Kim wrote:
> With this clean-up phase, I want to use zram's wrapper function
> to lock table access which is more consistent with other zram's
> functions.

which reminds me of...

there was a discussion a long time ago, -rt people absolutely
hate bit spin_locks and they suggested us to replace it with
normal spin_locks (and I promised to take a look at it, but
got interrupted and never really returned back to it).

for !lockdep builds the impact is somewhat small; for lockdep
builds we increase the memory usage, but

a) lockdep builds are debug builds by definition, no one runs lockdep
   enabled kernels in production

b) we have lockdep in zram now, which is nice

c) spin_locks probably have better fairness guarantees


what do you think? can we, in this patch set, also replce bit
spin_locks with a normal spin_lock?

	-ss

[toc] | [prev] | [next] | [standalone]

#1614909 — Re: [PATCH 3/5] zram: use zram_slot_lock instead of raw bit_spin_lock op

Hi Sergey,

On Mon, Apr 03, 2017 at 03:08:58PM +0900, Sergey Senozhatsky wrote:
> Hello Minchan,
> 
> On (04/03/17 14:17), Minchan Kim wrote:
> > With this clean-up phase, I want to use zram's wrapper function
> > to lock table access which is more consistent with other zram's
> > functions.
> 
> which reminds me of...
> 
> there was a discussion a long time ago, -rt people absolutely
> hate bit spin_locks and they suggested us to replace it with
> normal spin_locks (and I promised to take a look at it, but
> got interrupted and never really returned back to it).
> 
> for !lockdep builds the impact is somewhat small; for lockdep
> builds we increase the memory usage, but
> 
> a) lockdep builds are debug builds by definition, no one runs lockdep
>    enabled kernels in production
> 
> b) we have lockdep in zram now, which is nice

It's really one I want to have.

> 
> c) spin_locks probably have better fairness guarantees

In fact, it wouldn't be an imporant because zram's slot lock contention
is not heavy.
> 
> 
> what do you think? can we, in this patch set, also replce bit
> spin_locks with a normal spin_lock?

With changing only zram side from bit_spin_lock to spin_lock,
it would be crippled. I mean zsmalloc should be changed, too
and it's really hard. :(

[toc] | [prev] | [next] | [standalone]

#1614961 — Re: [PATCH 3/5] zram: use zram_slot_lock instead of raw bit_spin_lock op

On (04/03/17 15:34), Minchan Kim wrote:
[..]
> > c) spin_locks probably have better fairness guarantees
> 
> In fact, it wouldn't be an imporant because zram's slot lock contention
> is not heavy.

mostly agree. I think (and I may be mistaken) direct IO
causes contention; but direct IO is probably not a usual
zram workload.

> > what do you think? can we, in this patch set, also replce bit
> > spin_locks with a normal spin_lock?
> 
> With changing only zram side from bit_spin_lock to spin_lock,
> it would be crippled. I mean zsmalloc should be changed, too
> and it's really hard. :(

hm, good point.

	-ss

[toc] | [prev] | [next] | [standalone]

#1615655 — Re: [PATCH 3/5] zram: use zram_slot_lock instead of raw bit_spin_lock op

On (04/03/17 14:17), Minchan Kim wrote:
> With this clean-up phase, I want to use zram's wrapper function
> to lock table access which is more consistent with other zram's
> functions.
> 
> Signed-off-by: Minchan Kim <minchan@kernel.org>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

[toc] | [prev] | [next] | [standalone]

#1615706 — Re: [PATCH 3/5] zram: use zram_slot_lock instead of raw bit_spin_lock op

On Tue, Apr 04, 2017 at 11:18:51AM +0900, Sergey Senozhatsky wrote:
> On (04/03/17 14:17), Minchan Kim wrote:
> > With this clean-up phase, I want to use zram's wrapper function
> > to lock table access which is more consistent with other zram's
> > functions.
> > 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> 
> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

Thanks!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web