On Tue, Jun 3, 2014 at 4:22 PM, Minchan Kim <minchan@xxxxxxxxxx> wrote: > Hello, > > On Tue, Jun 03, 2014 at 03:59:06PM +0800, Weijie Yang wrote: >> On Mon, Jun 2, 2014 at 8:43 AM, Minchan Kim <minchan@xxxxxxxxxx> wrote: >> > Hello Weijie, >> > >> > Thanks for resending. >> > Below are mostly nitpicks. >> > >> > On Fri, May 30, 2014 at 04:34:44PM +0800, Weijie Yang wrote: >> >> Currently, we use a rwlock tb_lock to protect concurrent access to >> >> the whole zram meta table. However, according to the actual access model, >> >> there is only a small chance for upper user to access the same table[index], >> >> so the current lock granularity is too big. >> >> >> >> The idea of optimization is to change the lock granularity from whole >> >> meta table to per table entry (table -> table[index]), so that we can >> >> protect concurrent access to the same table[index], meanwhile allow >> >> the maximum concurrency. >> >> With this in mind, several kinds of locks which could be used as a >> >> per-entry lock were tested and compared: >> >> >> >> Test environment: >> >> x86-64 Intel Core2 Q8400, system memory 4GB, Ubuntu 12.04, >> >> kernel v3.15.0-rc3 as base, zram with 4 max_comp_streams LZO. >> >> >> >> iozone test: >> >> iozone -t 4 -R -r 16K -s 200M -I +Z >> >> (1GB zram with ext4 filesystem, take the average of 10 tests, KB/s) >> >> >> >> Test base CAS spinlock rwlock bit_spinlock >> >> ------------------------------------------------------------------- >> >> Initial write 1381094 1425435 1422860 1423075 1421521 >> >> Rewrite 1529479 1641199 1668762 1672855 1654910 >> >> Read 8468009 11324979 11305569 11117273 10997202 >> >> Re-read 8467476 11260914 11248059 11145336 10906486 >> >> Reverse Read 6821393 8106334 8282174 8279195 8109186 >> >> Stride read 7191093 8994306 9153982 8961224 9004434 >> >> Random read 7156353 8957932 9167098 8980465 8940476 >> >> Mixed workload 4172747 5680814 5927825 5489578 5972253 >> >> Random write 1483044 1605588 1594329 1600453 1596010 >> >> Pwrite 1276644 1303108 1311612 1314228 1300960 >> >> Pread 4324337 4632869 4618386 4457870 4500166 >> >> >> >> To enhance the possibility of access the same table[index] concurrently, >> >> set zram a small disksize(10MB) and let threads run with large loop count. >> >> >> >> fio test: >> >> fio --bs=32k --randrepeat=1 --randseed=100 --refill_buffers >> >> --scramble_buffers=1 --direct=1 --loops=3000 --numjobs=4 >> >> --filename=/dev/zram0 --name=seq-write --rw=write --stonewall >> >> --name=seq-read --rw=read --stonewall --name=seq-readwrite >> >> --rw=rw --stonewall --name=rand-readwrite --rw=randrw --stonewall >> >> (10MB zram raw block device, take the average of 10 tests, KB/s) >> >> >> >> Test base CAS spinlock rwlock bit_spinlock >> >> ------------------------------------------------------------- >> >> seq-write 933789 999357 1003298 995961 1001958 >> >> seq-read 5634130 6577930 6380861 6243912 6230006 >> >> seq-rw 1405687 1638117 1640256 1633903 1634459 >> >> rand-rw 1386119 1614664 1617211 1609267 1612471 >> >> >> >> All the optimization methods show a higher performance than the base, >> >> however, it is hard to say which method is the most appropriate. >> >> >> >> On the other hand, zram is mostly used on small embedded system, so we >> >> don't want to increase any memory footprint. >> >> >> >> This patch pick the bit_spinlock method, pack object size and page_flag >> >> into an unsigned long table.value, so as to not increase any memory >> >> overhead on both 32-bit and 64-bit system. >> >> >> >> On the third hand, even though different kinds of locks have different >> >> performances, we can ignore this difference, because: >> >> if zram is used as zram swapfile, the swap subsystem can prevent concurrent >> >> access to the same swapslot; >> >> if zram is used as zram-blk for set up filesystem on it, the upper filesystem >> >> and the page cache also prevent concurrent access of the same block mostly. >> >> So we can ignore the different performances among locks. >> > >> > Nice description. :) >> > >> >> >> >> Changes since v1: https://lkml.org/lkml/2014/5/5/1 >> >> - replace CAS method with bit_spinlock method >> >> - rename zram_test_flag() to zram_test_zero() >> >> - add some comments >> >> >> >> Changes since v2: https://lkml.org/lkml/2014/5/15/113 >> >> - change size type from int to size_t in zram_set_obj_size() >> >> - refactor zram_set_obj_size() to make it readable >> >> - add comments >> >> >> >> Signed-off-by: Weijie Yang <weijie.yang@xxxxxxxxxxx> >> >> --- >> >> drivers/block/zram/zram_drv.c | 89 ++++++++++++++++++++++++----------------- >> >> drivers/block/zram/zram_drv.h | 22 +++++++--- >> >> 2 files changed, 68 insertions(+), 43 deletions(-) >> >> >> >> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c >> >> index 9849b52..166e882 100644 >> >> --- a/drivers/block/zram/zram_drv.c >> >> +++ b/drivers/block/zram/zram_drv.c >> >> @@ -179,23 +179,32 @@ static ssize_t comp_algorithm_store(struct device *dev, >> >> return len; >> >> } >> >> >> >> -/* flag operations needs meta->tb_lock */ >> >> -static int zram_test_flag(struct zram_meta *meta, u32 index, >> >> - enum zram_pageflags flag) >> >> +static int zram_test_zero(struct zram_meta *meta, u32 index) >> > >> > Why do you want to create specific function for zero? >> > It would be one of usecase for various potential flags. >> > Do you want to create new functions whenever we define new flag? >> > Or something do you have a mind? >> > >> >> As you see, this patch adds a new flag ZRAM_ACCESS, which is >> accessed through different method from ZRAM_ZERO. >> I think it is hard to use a general method to access all kinds of flags, >> to eliminate some potential ambiguity or wrong usage, I use specific >> function to access different flags. > > Although I am not against that, we can do it when it's really messy > as another patch in future. I don't want to bloat patch size without > the goal which is to enhance locking contention. OK, let's focus on this patch's goal. I will restore it. >> >> > >> >> { >> >> - return meta->table[index].flags & BIT(flag); >> >> + return meta->table[index].value & BIT(ZRAM_ZERO); >> >> } >> >> >> >> -static void zram_set_flag(struct zram_meta *meta, u32 index, >> >> - enum zram_pageflags flag) >> >> +static void zram_set_zero(struct zram_meta *meta, u32 index) >> >> { >> >> - meta->table[index].flags |= BIT(flag); >> >> + meta->table[index].value |= BIT(ZRAM_ZERO); >> >> } >> >> >> >> -static void zram_clear_flag(struct zram_meta *meta, u32 index, >> >> - enum zram_pageflags flag) >> >> +static void zram_clear_zero(struct zram_meta *meta, u32 index) >> >> { >> >> - meta->table[index].flags &= ~BIT(flag); >> >> + meta->table[index].value &= ~BIT(ZRAM_ZERO); >> >> +} >> >> + >> >> +static size_t zram_get_obj_size(struct zram_meta *meta, u32 index) >> >> +{ >> >> + return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1); >> >> +} >> >> + >> >> +static void zram_set_obj_size(struct zram_meta *meta, >> >> + u32 index, size_t size) >> >> +{ >> >> + unsigned long flags = meta->table[index].value >> ZRAM_FLAG_SHIFT; >> >> + >> >> + meta->table[index].value = (flags << ZRAM_FLAG_SHIFT) | size; >> >> } >> >> >> >> static inline int is_partial_io(struct bio_vec *bvec) >> >> @@ -255,7 +264,6 @@ static struct zram_meta *zram_meta_alloc(u64 disksize) >> >> goto free_table; >> >> } >> >> >> >> - rwlock_init(&meta->tb_lock); >> >> return meta; >> >> >> >> free_table: >> >> @@ -304,19 +312,24 @@ static void handle_zero_page(struct bio_vec *bvec) >> >> flush_dcache_page(page); >> >> } >> >> >> >> -/* NOTE: caller should hold meta->tb_lock with write-side */ >> >> +/* >> >> + * To protect concurrent access to the same index entry, >> >> + * caller should hold this table index entry's bit_spinlock to >> >> + * indicate this index entry is accessing. >> >> + */ >> >> static void zram_free_page(struct zram *zram, size_t index) >> >> { >> >> struct zram_meta *meta = zram->meta; >> >> unsigned long handle = meta->table[index].handle; >> >> + size_t size; >> >> >> >> if (unlikely(!handle)) { >> >> /* >> >> * No memory is allocated for zero filled pages. >> >> * Simply clear zero page flag. >> >> */ >> >> - if (zram_test_flag(meta, index, ZRAM_ZERO)) { >> >> - zram_clear_flag(meta, index, ZRAM_ZERO); >> >> + if (zram_test_zero(meta, index)) { >> >> + zram_clear_zero(meta, index); >> >> atomic64_dec(&zram->stats.zero_pages); >> >> } >> >> return; >> >> @@ -324,27 +337,28 @@ static void zram_free_page(struct zram *zram, size_t index) >> >> >> >> zs_free(meta->mem_pool, handle); >> >> >> >> - atomic64_sub(meta->table[index].size, &zram->stats.compr_data_size); >> >> + size = zram_get_obj_size(meta, index); >> >> + atomic64_sub(size, &zram->stats.compr_data_size); >> >> atomic64_dec(&zram->stats.pages_stored); >> >> >> >> meta->table[index].handle = 0; >> >> - meta->table[index].size = 0; >> >> + zram_set_obj_size(meta, index, 0); >> >> } >> >> >> >> static int zram_decompress_page(struct zram *zram, char *mem, u32 index) >> >> { >> >> - int ret = 0; >> > >> > Unnecessary change. >> >> I want to compact the memory usage on stack, so I put ret and size variables >> together. On 64-bit system, it will be helpful. > > Please, do it as another patchset if you think it's worthy. I will send a specific patch on this topic. >> >> >> unsigned char *cmem; >> >> struct zram_meta *meta = zram->meta; >> >> unsigned long handle; >> >> - u16 size; >> > >> > I'm not sure it's good idea to use size_t instead of u16 because we apparently >> > have a limitation to express range of size due to packing it into unsigned long >> > so u16 is more clear to show the limiation and someone might find a problem >> > more easily in future if we break something subtle. >> > >> >> + size_t size; >> >> + int ret = 0; >> >> >> >> - read_lock(&meta->tb_lock); >> >> + bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value); >> >> handle = meta->table[index].handle; >> >> - size = meta->table[index].size; >> >> + size = zram_get_obj_size(meta, index); >> >> >> >> - if (!handle || zram_test_flag(meta, index, ZRAM_ZERO)) { >> >> - read_unlock(&meta->tb_lock); >> >> + if (!handle || zram_test_zero(meta, index)) { >> >> + bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value); >> >> clear_page(mem); >> >> return 0; >> >> } >> >> @@ -355,7 +369,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index) >> >> else >> >> ret = zcomp_decompress(zram->comp, cmem, size, mem); >> >> zs_unmap_object(meta->mem_pool, handle); >> >> - read_unlock(&meta->tb_lock); >> >> + bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value); >> >> >> >> /* Should NEVER happen. Return bio error if it does. */ >> >> if (unlikely(ret)) { >> >> @@ -376,14 +390,14 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec, >> >> struct zram_meta *meta = zram->meta; >> >> page = bvec->bv_page; >> >> >> >> - read_lock(&meta->tb_lock); >> >> + bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value); >> >> if (unlikely(!meta->table[index].handle) || >> >> - zram_test_flag(meta, index, ZRAM_ZERO)) { >> >> - read_unlock(&meta->tb_lock); >> >> + zram_test_zero(meta, index)) { >> >> + bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value); >> >> handle_zero_page(bvec); >> >> return 0; >> >> } >> >> - read_unlock(&meta->tb_lock); >> >> + bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value); >> >> >> >> if (is_partial_io(bvec)) >> >> /* Use a temporary buffer to decompress the page */ >> >> @@ -461,10 +475,10 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index, >> >> if (page_zero_filled(uncmem)) { >> >> kunmap_atomic(user_mem); >> >> /* Free memory associated with this sector now. */ >> >> - write_lock(&zram->meta->tb_lock); >> >> + bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value); >> >> zram_free_page(zram, index); >> >> - zram_set_flag(meta, index, ZRAM_ZERO); >> >> - write_unlock(&zram->meta->tb_lock); >> >> + zram_set_zero(meta, index); >> >> + bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value); >> >> >> >> atomic64_inc(&zram->stats.zero_pages); >> >> ret = 0; >> >> @@ -514,12 +528,12 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index, >> >> * Free memory associated with this sector >> >> * before overwriting unused sectors. >> >> */ >> >> - write_lock(&zram->meta->tb_lock); >> >> + bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value); >> >> zram_free_page(zram, index); >> >> >> >> meta->table[index].handle = handle; >> >> - meta->table[index].size = clen; >> >> - write_unlock(&zram->meta->tb_lock); >> >> + zram_set_obj_size(meta, index, clen); >> >> + bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value); >> >> >> >> /* Update stats */ >> >> atomic64_add(clen, &zram->stats.compr_data_size); >> >> @@ -560,6 +574,7 @@ 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 >> >> @@ -584,9 +599,9 @@ static void zram_bio_discard(struct zram *zram, u32 index, >> >> * Discard request can be large so the lock hold times could be >> >> * lengthy. So take the lock once per page. >> >> */ >> >> - write_lock(&zram->meta->tb_lock); >> >> + bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value); >> >> zram_free_page(zram, index); >> >> - write_unlock(&zram->meta->tb_lock); >> >> + bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value); >> >> index++; >> >> n -= PAGE_SIZE; >> >> } >> >> @@ -804,9 +819,9 @@ static void zram_slot_free_notify(struct block_device *bdev, >> >> zram = bdev->bd_disk->private_data; >> >> meta = zram->meta; >> >> >> >> - write_lock(&meta->tb_lock); >> >> + bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value); >> >> zram_free_page(zram, index); >> >> - write_unlock(&meta->tb_lock); >> >> + bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value); >> >> atomic64_inc(&zram->stats.notify_free); >> >> } >> >> >> >> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h >> >> index 7f21c14..71bc4ad 100644 >> >> --- a/drivers/block/zram/zram_drv.h >> >> +++ b/drivers/block/zram/zram_drv.h >> >> @@ -51,10 +51,22 @@ static const size_t max_zpage_size = PAGE_SIZE / 4 * 3; >> >> #define ZRAM_SECTOR_PER_LOGICAL_BLOCK \ >> >> (1 << (ZRAM_LOGICAL_BLOCK_SHIFT - SECTOR_SHIFT)) >> >> >> >> -/* Flags for zram pages (table[page_no].flags) */ >> >> +/* >> >> + * The lower ZRAM_FLAG_SHIFT bits of table.value is for >> >> + * object size (excluding header), the higher bits is for >> >> + * zram_pageflags. By this means, it won't increase any >> >> + * memory overhead on both 32-bit and 64-bit system. >> > >> > Comment on "By this means, ~ 64 bit system" is unncessary because >> > someone read this line but don't know history couldn't understand >> > what's the old structure. >> >> I will remove it. >> >> >> + * zram is mostly used on small embedded system, so we >> >> + * don't want to increase memory footprint. That is why >> >> + * we pack size and flag into table.value. >> >> + */ >> > >> > IMHO, it would be more clear but not sure if native speakers look at. ;-) >> > >> > * zram is mainly used for memory efficiency so we want to keep memory >> > * footprint small so we can squeeze size and flags into a field. >> > * The lower ZRAM_FLAG_SHIFT bits is for object size (excluding header), >> > * the higher bits is for zram_pageflags. >> > >> >> That is a better comment. >> >> >> +#define ZRAM_FLAG_SHIFT 24 >> > >> > Why is it 24? We have used for 16-bit for size. >> > Do you think it's too small for size? >> >> The reason why I choose 24-bit and size_t is that when I checked the PAGE_SHIFT >> on all kinds of architectures, I found on some architectures such as powerpc and >> hexagon, PAGE_SHIFT would be 18 or 20, so I think 16-bit could be small to use. > > If it's a problem, could you do it as another patch? Because it's bug fix, > not related to this patchset. OK, I will send a specific patch on this. Thank you very much for your review and suggestion. > Thanks. > > -- > Kind regards, > Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>