| Message ID | 20250131090658.3386285-3-senozhatsky@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | zsmalloc/zram: there be preemption | expand |
Hi Sergey, On Fri, Jan 31, 2025 at 5:07 PM Sergey Senozhatsky <senozhatsky@chromium.org> wrote: > > Similarly to per-entry spin-lock per-CPU compression streams > also have a number of shortcoming. > > First, per-CPU stream access has to be done from a non-preemptible > (atomic) section, which imposes the same atomicity requirements on > compression backends as entry spin-lock do and makes it impossible > to use algorithms that can schedule/wait/sleep during compression > and decompression. > > Second, per-CPU streams noticeably increase memory usage (actually > more like wastage) of secondary compression streams. The problem > is that secondary compression streams are allocated per-CPU, just > like the primary streams are. Yet we never use more that one > secondary stream at a time, because recompression is a single > threaded action. Which means that remaining num_online_cpu() - 1 > streams are allocated for nothing, and this is per-priority list > (we can have several secondary compression algorithms). Depending > on the algorithm this may lead to a significant memory wastage, in > addition each stream also carries a workmem buffer (2 physical > pages). > > Instead of per-CPU streams, maintain a list of idle compression > streams and allocate new streams on-demand (something that we > used to do many years ago). So that zram read() and write() become > non-atomic and ease requirements on the compression algorithm > implementation. This also means that we now should have only one > secondary stream per-priority list. > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > drivers/block/zram/zcomp.c | 164 +++++++++++++++++++--------------- > drivers/block/zram/zcomp.h | 17 ++-- > drivers/block/zram/zram_drv.c | 29 +++--- > include/linux/cpuhotplug.h | 1 - > 4 files changed, 109 insertions(+), 102 deletions(-) > > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c > index bb514403e305..982c769d5831 100644 > --- a/drivers/block/zram/zcomp.c > +++ b/drivers/block/zram/zcomp.c > @@ -6,7 +6,7 @@ > #include <linux/slab.h> > #include <linux/wait.h> > #include <linux/sched.h> > -#include <linux/cpu.h> > +#include <linux/cpumask.h> > #include <linux/crypto.h> > #include <linux/vmalloc.h> > > @@ -43,31 +43,40 @@ static const struct zcomp_ops *backends[] = { > NULL > }; > > -static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *zstrm) > +static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *strm) > { > - comp->ops->destroy_ctx(&zstrm->ctx); > - vfree(zstrm->buffer); > - zstrm->buffer = NULL; > + comp->ops->destroy_ctx(&strm->ctx); > + vfree(strm->buffer); > + kfree(strm); > } > > -static int zcomp_strm_init(struct zcomp *comp, struct zcomp_strm *zstrm) > +static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp) > { > + struct zcomp_strm *strm; > int ret; > > - ret = comp->ops->create_ctx(comp->params, &zstrm->ctx); > - if (ret) > - return ret; > + strm = kzalloc(sizeof(*strm), GFP_KERNEL); > + if (!strm) > + return NULL; > + > + INIT_LIST_HEAD(&strm->entry); > + > + ret = comp->ops->create_ctx(comp->params, &strm->ctx); > + if (ret) { > + kfree(strm); > + return NULL; > + } > > /* > - * allocate 2 pages. 1 for compressed data, plus 1 extra for the > - * case when compressed size is larger than the original one > + * allocate 2 pages. 1 for compressed data, plus 1 extra in case if > + * compressed data is larger than the original one. > */ > - zstrm->buffer = vzalloc(2 * PAGE_SIZE); > - if (!zstrm->buffer) { > - zcomp_strm_free(comp, zstrm); > - return -ENOMEM; > + strm->buffer = vzalloc(2 * PAGE_SIZE); > + if (!strm->buffer) { > + zcomp_strm_free(comp, strm); > + return NULL; > } > - return 0; > + return strm; > } > > static const struct zcomp_ops *lookup_backend_ops(const char *comp) > @@ -109,13 +118,59 @@ ssize_t zcomp_available_show(const char *comp, char *buf) > > struct zcomp_strm *zcomp_stream_get(struct zcomp *comp) > { > - local_lock(&comp->stream->lock); > - return this_cpu_ptr(comp->stream); > + struct zcomp_strm *strm; > + > + might_sleep(); > + > + while (1) { > + spin_lock(&comp->strm_lock); > + if (!list_empty(&comp->idle_strm)) { > + strm = list_first_entry(&comp->idle_strm, > + struct zcomp_strm, > + entry); > + list_del(&strm->entry); > + spin_unlock(&comp->strm_lock); > + return strm; > + } > + > + /* cannot allocate new stream, wait for an idle one */ > + if (comp->avail_strm >= num_online_cpus()) { > + spin_unlock(&comp->strm_lock); > + wait_event(comp->strm_wait, > + !list_empty(&comp->idle_strm)); > + continue; > + } > + > + /* allocate new stream */ > + comp->avail_strm++; > + spin_unlock(&comp->strm_lock); > + > + strm = zcomp_strm_alloc(comp); > + if (strm) > + break; > + > + spin_lock(&comp->strm_lock); > + comp->avail_strm--; > + spin_unlock(&comp->strm_lock); > + wait_event(comp->strm_wait, !list_empty(&comp->idle_strm)); > + } > + > + return strm; > } > > -void zcomp_stream_put(struct zcomp *comp) > +void zcomp_stream_put(struct zcomp *comp, struct zcomp_strm *strm) > { > - local_unlock(&comp->stream->lock); > + spin_lock(&comp->strm_lock); > + if (comp->avail_strm <= num_online_cpus()) { > + list_add(&strm->entry, &comp->idle_strm); > + spin_unlock(&comp->strm_lock); > + wake_up(&comp->strm_wait); > + return; > + } > + > + comp->avail_strm--; > + spin_unlock(&comp->strm_lock); > + zcomp_strm_free(comp, strm); > } > > int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm, > @@ -148,61 +203,19 @@ int zcomp_decompress(struct zcomp *comp, struct zcomp_strm *zstrm, > return comp->ops->decompress(comp->params, &zstrm->ctx, &req); > } > > -int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node) > -{ > - struct zcomp *comp = hlist_entry(node, struct zcomp, node); > - struct zcomp_strm *zstrm; > - int ret; > - > - zstrm = per_cpu_ptr(comp->stream, cpu); > - local_lock_init(&zstrm->lock); > - > - ret = zcomp_strm_init(comp, zstrm); > - if (ret) > - pr_err("Can't allocate a compression stream\n"); > - return ret; > -} > - > -int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node) > -{ > - struct zcomp *comp = hlist_entry(node, struct zcomp, node); > - struct zcomp_strm *zstrm; > - > - zstrm = per_cpu_ptr(comp->stream, cpu); > - zcomp_strm_free(comp, zstrm); > - return 0; > -} > - > -static int zcomp_init(struct zcomp *comp, struct zcomp_params *params) > -{ > - int ret; > - > - comp->stream = alloc_percpu(struct zcomp_strm); > - if (!comp->stream) > - return -ENOMEM; > - > - comp->params = params; > - ret = comp->ops->setup_params(comp->params); > - if (ret) > - goto cleanup; > - > - ret = cpuhp_state_add_instance(CPUHP_ZCOMP_PREPARE, &comp->node); > - if (ret < 0) > - goto cleanup; > - > - return 0; > - > -cleanup: > - comp->ops->release_params(comp->params); > - free_percpu(comp->stream); > - return ret; > -} > - > void zcomp_destroy(struct zcomp *comp) > { > - cpuhp_state_remove_instance(CPUHP_ZCOMP_PREPARE, &comp->node); > + struct zcomp_strm *strm; > + > + while (!list_empty(&comp->idle_strm)) { > + strm = list_first_entry(&comp->idle_strm, > + struct zcomp_strm, > + entry); > + list_del(&strm->entry); > + zcomp_strm_free(comp, strm); > + } > + > comp->ops->release_params(comp->params); > - free_percpu(comp->stream); > kfree(comp); > } > > @@ -229,7 +242,12 @@ struct zcomp *zcomp_create(const char *alg, struct zcomp_params *params) > return ERR_PTR(-EINVAL); > } > > - error = zcomp_init(comp, params); > + INIT_LIST_HEAD(&comp->idle_strm); > + init_waitqueue_head(&comp->strm_wait); > + spin_lock_init(&comp->strm_lock); > + > + comp->params = params; > + error = comp->ops->setup_params(comp->params); > if (error) { > kfree(comp); > return ERR_PTR(error); > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h > index ad5762813842..62330829db3f 100644 > --- a/drivers/block/zram/zcomp.h > +++ b/drivers/block/zram/zcomp.h > @@ -3,10 +3,10 @@ > #ifndef _ZCOMP_H_ > #define _ZCOMP_H_ > > -#include <linux/local_lock.h> > - > #define ZCOMP_PARAM_NO_LEVEL INT_MIN > > +#include <linux/wait.h> > + > /* > * Immutable driver (backend) parameters. The driver may attach private > * data to it (e.g. driver representation of the dictionary, etc.). > @@ -31,7 +31,7 @@ struct zcomp_ctx { > }; > > struct zcomp_strm { > - local_lock_t lock; > + struct list_head entry; > /* compression buffer */ > void *buffer; > struct zcomp_ctx ctx; > @@ -60,16 +60,15 @@ struct zcomp_ops { > const char *name; > }; > > -/* dynamic per-device compression frontend */ > struct zcomp { > - struct zcomp_strm __percpu *stream; > + struct list_head idle_strm; > + spinlock_t strm_lock; > + u32 avail_strm; > + wait_queue_head_t strm_wait; > const struct zcomp_ops *ops; > struct zcomp_params *params; > - struct hlist_node node; > }; > > -int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node); > -int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node); > ssize_t zcomp_available_show(const char *comp, char *buf); > bool zcomp_available_algorithm(const char *comp); > > @@ -77,7 +76,7 @@ struct zcomp *zcomp_create(const char *alg, struct zcomp_params *params); > void zcomp_destroy(struct zcomp *comp); > > struct zcomp_strm *zcomp_stream_get(struct zcomp *comp); > -void zcomp_stream_put(struct zcomp *comp); > +void zcomp_stream_put(struct zcomp *comp, struct zcomp_strm *strm); > > int zcomp_compress(struct zcomp *comp, struct zcomp_strm *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 1c2df2341704..8d5974ea8ff8 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -31,7 +31,6 @@ > #include <linux/idr.h> > #include <linux/sysfs.h> > #include <linux/debugfs.h> > -#include <linux/cpuhotplug.h> > #include <linux/part_stat.h> > #include <linux/kernel_read_file.h> > > @@ -1601,7 +1600,7 @@ static int read_compressed_page(struct zram *zram, struct page *page, u32 index) > ret = zcomp_decompress(zram->comps[prio], zstrm, src, size, dst); > kunmap_local(dst); > zs_unmap_object(zram->mem_pool, handle); > - zcomp_stream_put(zram->comps[prio]); > + zcomp_stream_put(zram->comps[prio], zstrm); > > return ret; > } > @@ -1762,14 +1761,14 @@ static int zram_write_page(struct zram *zram, struct page *page, u32 index) > kunmap_local(mem); > > if (unlikely(ret)) { > - zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]); > + zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP], zstrm); > pr_err("Compression failed! err=%d\n", ret); > zs_free(zram->mem_pool, handle); > return ret; > } > > if (comp_len >= huge_class_size) { > - zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]); > + zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP], zstrm); > return write_incompressible_page(zram, page, index); > } > > @@ -1793,7 +1792,7 @@ static int zram_write_page(struct zram *zram, struct page *page, u32 index) > __GFP_HIGHMEM | > __GFP_MOVABLE); > if (IS_ERR_VALUE(handle)) { > - zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]); > + zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP], zstrm); > atomic64_inc(&zram->stats.writestall); > handle = zs_malloc(zram->mem_pool, comp_len, > GFP_NOIO | __GFP_HIGHMEM | > @@ -1805,7 +1804,7 @@ static int zram_write_page(struct zram *zram, struct page *page, u32 index) > } > > if (!zram_can_store_page(zram)) { > - zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]); > + zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP], zstrm); > zs_free(zram->mem_pool, handle); > return -ENOMEM; > } > @@ -1813,7 +1812,7 @@ static int zram_write_page(struct zram *zram, struct page *page, u32 index) > dst = zs_map_object(zram->mem_pool, handle, ZS_MM_WO); > > memcpy(dst, zstrm->buffer, comp_len); > - zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]); > + zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP], zstrm); > zs_unmap_object(zram->mem_pool, handle); > > zram_slot_write_lock(zram, index); > @@ -1972,7 +1971,7 @@ static int recompress_slot(struct zram *zram, u32 index, struct page *page, > kunmap_local(src); > > if (ret) { > - zcomp_stream_put(zram->comps[prio]); > + zcomp_stream_put(zram->comps[prio], zstrm); > return ret; > } > > @@ -1982,7 +1981,7 @@ static int recompress_slot(struct zram *zram, u32 index, struct page *page, > /* Continue until we make progress */ > if (class_index_new >= class_index_old || > (threshold && comp_len_new >= threshold)) { > - zcomp_stream_put(zram->comps[prio]); > + zcomp_stream_put(zram->comps[prio], zstrm); > continue; > } > > @@ -2040,13 +2039,13 @@ static int recompress_slot(struct zram *zram, u32 index, struct page *page, > __GFP_HIGHMEM | > __GFP_MOVABLE); > if (IS_ERR_VALUE(handle_new)) { > - zcomp_stream_put(zram->comps[prio]); > + zcomp_stream_put(zram->comps[prio], zstrm); > return PTR_ERR((void *)handle_new); > } > > dst = zs_map_object(zram->mem_pool, handle_new, ZS_MM_WO); > memcpy(dst, zstrm->buffer, comp_len_new); > - zcomp_stream_put(zram->comps[prio]); > + zcomp_stream_put(zram->comps[prio], zstrm); > > zs_unmap_object(zram->mem_pool, handle_new); > > @@ -2794,7 +2793,6 @@ static void destroy_devices(void) > zram_debugfs_destroy(); > idr_destroy(&zram_index_idr); > unregister_blkdev(zram_major, "zram"); > - cpuhp_remove_multi_state(CPUHP_ZCOMP_PREPARE); > } > > static int __init zram_init(void) > @@ -2804,15 +2802,9 @@ static int __init zram_init(void) > > BUILD_BUG_ON(__NR_ZRAM_PAGEFLAGS > sizeof(zram_te.flags) * 8); > > - ret = cpuhp_setup_state_multi(CPUHP_ZCOMP_PREPARE, "block/zram:prepare", > - zcomp_cpu_up_prepare, zcomp_cpu_dead); > - if (ret < 0) > - return ret; > - > ret = class_register(&zram_control_class); > if (ret) { > pr_err("Unable to register zram-control class\n"); > - cpuhp_remove_multi_state(CPUHP_ZCOMP_PREPARE); > return ret; > } > > @@ -2821,7 +2813,6 @@ static int __init zram_init(void) > if (zram_major <= 0) { > pr_err("Unable to get major number\n"); > class_unregister(&zram_control_class); > - cpuhp_remove_multi_state(CPUHP_ZCOMP_PREPARE); > return -EBUSY; > } > > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h > index 6cc5e484547c..092ace7db8ee 100644 > --- a/include/linux/cpuhotplug.h > +++ b/include/linux/cpuhotplug.h > @@ -119,7 +119,6 @@ enum cpuhp_state { > CPUHP_MM_ZS_PREPARE, > CPUHP_MM_ZSWP_POOL_PREPARE, > CPUHP_KVM_PPC_BOOK3S_PREPARE, > - CPUHP_ZCOMP_PREPARE, > CPUHP_TIMERS_PREPARE, > CPUHP_TMIGR_PREPARE, > CPUHP_MIPS_SOC_PREPARE, > -- > 2.48.1.362.g079036d154-goog This seems will cause a huge regression of performance on multi core systems, this is especially significant as the number of concurrent tasks increases: Test build linux kernel using ZRAM as SWAP (1G memcg): Before: + /usr/bin/time make -s -j48 2495.77user 2604.77system 2:12.95elapsed 3836%CPU (0avgtext+0avgdata 863304maxresident)k After: + /usr/bin/time make -s -j48 2403.60user 6676.09system 3:38.22elapsed 4160%CPU (0avgtext+0avgdata 863276maxresident)k `perf lock contention -ab sleep 3` also indicates the big spin lock in zcomp_stream_get/put is having significant contention: contended total wait max wait avg wait type caller 793357 28.71 s 2.66 ms 36.19 us spinlock zcomp_stream_get+0x37 793170 28.60 s 2.65 ms 36.06 us spinlock zcomp_stream_put+0x1f 444007 15.26 s 2.58 ms 34.37 us spinlock zcomp_stream_put+0x1f 443960 15.21 s 2.68 ms 34.25 us spinlock zcomp_stream_get+0x37 5516 152.50 ms 3.30 ms 27.65 us spinlock evict_folios+0x7e 4523 137.47 ms 3.66 ms 30.39 us spinlock folio_lruvec_lock_irqsave+0xc3 4253 108.93 ms 2.92 ms 25.61 us spinlock folio_lruvec_lock_irqsave+0xc3 49294 71.73 ms 15.87 us 1.46 us spinlock list_lru_del+0x7c 2327 51.35 ms 3.48 ms 22.07 us spinlock evict_folios+0x5c0
On (25/02/01 17:21), Kairui Song wrote: > This seems will cause a huge regression of performance on multi core > systems, this is especially significant as the number of concurrent > tasks increases: > > Test build linux kernel using ZRAM as SWAP (1G memcg): > > Before: > + /usr/bin/time make -s -j48 > 2495.77user 2604.77system 2:12.95elapsed 3836%CPU (0avgtext+0avgdata > 863304maxresident)k > > After: > + /usr/bin/time make -s -j48 > 2403.60user 6676.09system 3:38.22elapsed 4160%CPU (0avgtext+0avgdata > 863276maxresident)k How many CPUs do you have? I assume, preemption gets into way which is sort of expected, to be honest... Using per-CPU compression streams disables preemption and uses CPU exclusively at a price of other tasks not being able to run. I do tend to think that I made a mistake by switching zram to per-CPU compression streams. What preemption model do you use and to what extent do you overload your system? My tests don't show anything unusual (but I don't overload the system) CONFIG_PREEMPT before 1371.96user 156.21system 1:30.91elapsed 1680%CPU (0avgtext+0avgdata 825636maxresident)k 32688inputs+1768416outputs (259major+51539861minor)pagefaults 0swaps after 1372.05user 155.79system 1:30.82elapsed 1682%CPU (0avgtext+0avgdata 825684maxresident)k 32680inputs+1768416outputs (273major+51541815minor)pagefaults 0swaps (I use zram as a block device with ext4 on it.) > `perf lock contention -ab sleep 3` also indicates the big spin lock in > zcomp_stream_get/put is having significant contention: Hmm it's just spin_lock() list first entry spin_unlock() Shouldn't be "a big spin lock", that's very odd. I'm not familiar with perf lock contention, let me take a look.
On Mon, Feb 03, 2025 at 12:49:42PM +0900, Sergey Senozhatsky wrote: > On (25/02/01 17:21), Kairui Song wrote: > > This seems will cause a huge regression of performance on multi core > > systems, this is especially significant as the number of concurrent > > tasks increases: > > > > Test build linux kernel using ZRAM as SWAP (1G memcg): > > > > Before: > > + /usr/bin/time make -s -j48 > > 2495.77user 2604.77system 2:12.95elapsed 3836%CPU (0avgtext+0avgdata > > 863304maxresident)k > > > > After: > > + /usr/bin/time make -s -j48 > > 2403.60user 6676.09system 3:38.22elapsed 4160%CPU (0avgtext+0avgdata > > 863276maxresident)k > > How many CPUs do you have? I assume, preemption gets into way which is > sort of expected, to be honest... Using per-CPU compression streams > disables preemption and uses CPU exclusively at a price of other tasks > not being able to run. I do tend to think that I made a mistake by > switching zram to per-CPU compression streams. FWIW, I am not familiar at all with the zram code but zswap uses per-CPU acomp contexts with a mutex instead of a spinlock. So the task uses the context of the CPU that it started on, but it can be preempted or migrated and end up running on a different CPU. This means that contention is still possible, but probably much lower than having a shared pool of contexts that all CPUs compete on. Again, this could be irrelevant as I am not very familiar with the zram code, just thought this may be useful.
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c index bb514403e305..982c769d5831 100644 --- a/drivers/block/zram/zcomp.c +++ b/drivers/block/zram/zcomp.c @@ -6,7 +6,7 @@ #include <linux/slab.h> #include <linux/wait.h> #include <linux/sched.h> -#include <linux/cpu.h> +#include <linux/cpumask.h> #include <linux/crypto.h> #include <linux/vmalloc.h> @@ -43,31 +43,40 @@ static const struct zcomp_ops *backends[] = { NULL }; -static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *zstrm) +static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *strm) { - comp->ops->destroy_ctx(&zstrm->ctx); - vfree(zstrm->buffer); - zstrm->buffer = NULL; + comp->ops->destroy_ctx(&strm->ctx); + vfree(strm->buffer); + kfree(strm); } -static int zcomp_strm_init(struct zcomp *comp, struct zcomp_strm *zstrm) +static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp) { + struct zcomp_strm *strm; int ret; - ret = comp->ops->create_ctx(comp->params, &zstrm->ctx); - if (ret) - return ret; + strm = kzalloc(sizeof(*strm), GFP_KERNEL); + if (!strm) + return NULL; + + INIT_LIST_HEAD(&strm->entry); + + ret = comp->ops->create_ctx(comp->params, &strm->ctx); + if (ret) { + kfree(strm); + return NULL; + } /* - * allocate 2 pages. 1 for compressed data, plus 1 extra for the - * case when compressed size is larger than the original one + * allocate 2 pages. 1 for compressed data, plus 1 extra in case if + * compressed data is larger than the original one. */ - zstrm->buffer = vzalloc(2 * PAGE_SIZE); - if (!zstrm->buffer) { - zcomp_strm_free(comp, zstrm); - return -ENOMEM; + strm->buffer = vzalloc(2 * PAGE_SIZE); + if (!strm->buffer) { + zcomp_strm_free(comp, strm); + return NULL; } - return 0; + return strm; } static const struct zcomp_ops *lookup_backend_ops(const char *comp) @@ -109,13 +118,59 @@ ssize_t zcomp_available_show(const char *comp, char *buf) struct zcomp_strm *zcomp_stream_get(struct zcomp *comp) { - local_lock(&comp->stream->lock); - return this_cpu_ptr(comp->stream); + struct zcomp_strm *strm; + + might_sleep(); + + while (1) { + spin_lock(&comp->strm_lock); + if (!list_empty(&comp->idle_strm)) { + strm = list_first_entry(&comp->idle_strm, + struct zcomp_strm, + entry); + list_del(&strm->entry); + spin_unlock(&comp->strm_lock); + return strm; + } + + /* cannot allocate new stream, wait for an idle one */ + if (comp->avail_strm >= num_online_cpus()) { + spin_unlock(&comp->strm_lock); + wait_event(comp->strm_wait, + !list_empty(&comp->idle_strm)); + continue; + } + + /* allocate new stream */ + comp->avail_strm++; + spin_unlock(&comp->strm_lock); + + strm = zcomp_strm_alloc(comp); + if (strm) + break; + + spin_lock(&comp->strm_lock); + comp->avail_strm--; + spin_unlock(&comp->strm_lock); + wait_event(comp->strm_wait, !list_empty(&comp->idle_strm)); + } + + return strm; } -void zcomp_stream_put(struct zcomp *comp) +void zcomp_stream_put(struct zcomp *comp, struct zcomp_strm *strm) { - local_unlock(&comp->stream->lock); + spin_lock(&comp->strm_lock); + if (comp->avail_strm <= num_online_cpus()) { + list_add(&strm->entry, &comp->idle_strm); + spin_unlock(&comp->strm_lock); + wake_up(&comp->strm_wait); + return; + } + + comp->avail_strm--; + spin_unlock(&comp->strm_lock); + zcomp_strm_free(comp, strm); } int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm, @@ -148,61 +203,19 @@ int zcomp_decompress(struct zcomp *comp, struct zcomp_strm *zstrm, return comp->ops->decompress(comp->params, &zstrm->ctx, &req); } -int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node) -{ - struct zcomp *comp = hlist_entry(node, struct zcomp, node); - struct zcomp_strm *zstrm; - int ret; - - zstrm = per_cpu_ptr(comp->stream, cpu); - local_lock_init(&zstrm->lock); - - ret = zcomp_strm_init(comp, zstrm); - if (ret) - pr_err("Can't allocate a compression stream\n"); - return ret; -} - -int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node) -{ - struct zcomp *comp = hlist_entry(node, struct zcomp, node); - struct zcomp_strm *zstrm; - - zstrm = per_cpu_ptr(comp->stream, cpu); - zcomp_strm_free(comp, zstrm); - return 0; -} - -static int zcomp_init(struct zcomp *comp, struct zcomp_params *params) -{ - int ret; - - comp->stream = alloc_percpu(struct zcomp_strm); - if (!comp->stream) - return -ENOMEM; - - comp->params = params; - ret = comp->ops->setup_params(comp->params); - if (ret) - goto cleanup; - - ret = cpuhp_state_add_instance(CPUHP_ZCOMP_PREPARE, &comp->node); - if (ret < 0) - goto cleanup; - - return 0; - -cleanup: - comp->ops->release_params(comp->params); - free_percpu(comp->stream); - return ret; -} - void zcomp_destroy(struct zcomp *comp) { - cpuhp_state_remove_instance(CPUHP_ZCOMP_PREPARE, &comp->node); + struct zcomp_strm *strm; + + while (!list_empty(&comp->idle_strm)) { + strm = list_first_entry(&comp->idle_strm, + struct zcomp_strm, + entry); + list_del(&strm->entry); + zcomp_strm_free(comp, strm); + } + comp->ops->release_params(comp->params); - free_percpu(comp->stream); kfree(comp); } @@ -229,7 +242,12 @@ struct zcomp *zcomp_create(const char *alg, struct zcomp_params *params) return ERR_PTR(-EINVAL); } - error = zcomp_init(comp, params); + INIT_LIST_HEAD(&comp->idle_strm); + init_waitqueue_head(&comp->strm_wait); + spin_lock_init(&comp->strm_lock); + + comp->params = params; + error = comp->ops->setup_params(comp->params); if (error) { kfree(comp); return ERR_PTR(error); diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h index ad5762813842..62330829db3f 100644 --- a/drivers/block/zram/zcomp.h +++ b/drivers/block/zram/zcomp.h @@ -3,10 +3,10 @@ #ifndef _ZCOMP_H_ #define _ZCOMP_H_ -#include <linux/local_lock.h> - #define ZCOMP_PARAM_NO_LEVEL INT_MIN +#include <linux/wait.h> + /* * Immutable driver (backend) parameters. The driver may attach private * data to it (e.g. driver representation of the dictionary, etc.). @@ -31,7 +31,7 @@ struct zcomp_ctx { }; struct zcomp_strm { - local_lock_t lock; + struct list_head entry; /* compression buffer */ void *buffer; struct zcomp_ctx ctx; @@ -60,16 +60,15 @@ struct zcomp_ops { const char *name; }; -/* dynamic per-device compression frontend */ struct zcomp { - struct zcomp_strm __percpu *stream; + struct list_head idle_strm; + spinlock_t strm_lock; + u32 avail_strm; + wait_queue_head_t strm_wait; const struct zcomp_ops *ops; struct zcomp_params *params; - struct hlist_node node; }; -int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node); -int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node); ssize_t zcomp_available_show(const char *comp, char *buf); bool zcomp_available_algorithm(const char *comp); @@ -77,7 +76,7 @@ struct zcomp *zcomp_create(const char *alg, struct zcomp_params *params); void zcomp_destroy(struct zcomp *comp); struct zcomp_strm *zcomp_stream_get(struct zcomp *comp); -void zcomp_stream_put(struct zcomp *comp); +void zcomp_stream_put(struct zcomp *comp, struct zcomp_strm *strm); int zcomp_compress(struct zcomp *comp, struct zcomp_strm *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 1c2df2341704..8d5974ea8ff8 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -31,7 +31,6 @@ #include <linux/idr.h> #include <linux/sysfs.h> #include <linux/debugfs.h> -#include <linux/cpuhotplug.h> #include <linux/part_stat.h> #include <linux/kernel_read_file.h> @@ -1601,7 +1600,7 @@ static int read_compressed_page(struct zram *zram, struct page *page, u32 index) ret = zcomp_decompress(zram->comps[prio], zstrm, src, size, dst); kunmap_local(dst); zs_unmap_object(zram->mem_pool, handle); - zcomp_stream_put(zram->comps[prio]); + zcomp_stream_put(zram->comps[prio], zstrm); return ret; } @@ -1762,14 +1761,14 @@ static int zram_write_page(struct zram *zram, struct page *page, u32 index) kunmap_local(mem); if (unlikely(ret)) { - zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]); + zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP], zstrm); pr_err("Compression failed! err=%d\n", ret); zs_free(zram->mem_pool, handle); return ret; } if (comp_len >= huge_class_size) { - zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]); + zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP], zstrm); return write_incompressible_page(zram, page, index); } @@ -1793,7 +1792,7 @@ static int zram_write_page(struct zram *zram, struct page *page, u32 index) __GFP_HIGHMEM | __GFP_MOVABLE); if (IS_ERR_VALUE(handle)) { - zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]); + zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP], zstrm); atomic64_inc(&zram->stats.writestall); handle = zs_malloc(zram->mem_pool, comp_len, GFP_NOIO | __GFP_HIGHMEM | @@ -1805,7 +1804,7 @@ static int zram_write_page(struct zram *zram, struct page *page, u32 index) } if (!zram_can_store_page(zram)) { - zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]); + zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP], zstrm); zs_free(zram->mem_pool, handle); return -ENOMEM; } @@ -1813,7 +1812,7 @@ static int zram_write_page(struct zram *zram, struct page *page, u32 index) dst = zs_map_object(zram->mem_pool, handle, ZS_MM_WO); memcpy(dst, zstrm->buffer, comp_len); - zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]); + zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP], zstrm); zs_unmap_object(zram->mem_pool, handle); zram_slot_write_lock(zram, index); @@ -1972,7 +1971,7 @@ static int recompress_slot(struct zram *zram, u32 index, struct page *page, kunmap_local(src); if (ret) { - zcomp_stream_put(zram->comps[prio]); + zcomp_stream_put(zram->comps[prio], zstrm); return ret; } @@ -1982,7 +1981,7 @@ static int recompress_slot(struct zram *zram, u32 index, struct page *page, /* Continue until we make progress */ if (class_index_new >= class_index_old || (threshold && comp_len_new >= threshold)) { - zcomp_stream_put(zram->comps[prio]); + zcomp_stream_put(zram->comps[prio], zstrm); continue; } @@ -2040,13 +2039,13 @@ static int recompress_slot(struct zram *zram, u32 index, struct page *page, __GFP_HIGHMEM | __GFP_MOVABLE); if (IS_ERR_VALUE(handle_new)) { - zcomp_stream_put(zram->comps[prio]); + zcomp_stream_put(zram->comps[prio], zstrm); return PTR_ERR((void *)handle_new); } dst = zs_map_object(zram->mem_pool, handle_new, ZS_MM_WO); memcpy(dst, zstrm->buffer, comp_len_new); - zcomp_stream_put(zram->comps[prio]); + zcomp_stream_put(zram->comps[prio], zstrm); zs_unmap_object(zram->mem_pool, handle_new); @@ -2794,7 +2793,6 @@ static void destroy_devices(void) zram_debugfs_destroy(); idr_destroy(&zram_index_idr); unregister_blkdev(zram_major, "zram"); - cpuhp_remove_multi_state(CPUHP_ZCOMP_PREPARE); } static int __init zram_init(void) @@ -2804,15 +2802,9 @@ static int __init zram_init(void) BUILD_BUG_ON(__NR_ZRAM_PAGEFLAGS > sizeof(zram_te.flags) * 8); - ret = cpuhp_setup_state_multi(CPUHP_ZCOMP_PREPARE, "block/zram:prepare", - zcomp_cpu_up_prepare, zcomp_cpu_dead); - if (ret < 0) - return ret; - ret = class_register(&zram_control_class); if (ret) { pr_err("Unable to register zram-control class\n"); - cpuhp_remove_multi_state(CPUHP_ZCOMP_PREPARE); return ret; } @@ -2821,7 +2813,6 @@ static int __init zram_init(void) if (zram_major <= 0) { pr_err("Unable to get major number\n"); class_unregister(&zram_control_class); - cpuhp_remove_multi_state(CPUHP_ZCOMP_PREPARE); return -EBUSY; } diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h index 6cc5e484547c..092ace7db8ee 100644 --- a/include/linux/cpuhotplug.h +++ b/include/linux/cpuhotplug.h @@ -119,7 +119,6 @@ enum cpuhp_state { CPUHP_MM_ZS_PREPARE, CPUHP_MM_ZSWP_POOL_PREPARE, CPUHP_KVM_PPC_BOOK3S_PREPARE, - CPUHP_ZCOMP_PREPARE, CPUHP_TIMERS_PREPARE, CPUHP_TMIGR_PREPARE, CPUHP_MIPS_SOC_PREPARE,
Similarly to per-entry spin-lock per-CPU compression streams also have a number of shortcoming. First, per-CPU stream access has to be done from a non-preemptible (atomic) section, which imposes the same atomicity requirements on compression backends as entry spin-lock do and makes it impossible to use algorithms that can schedule/wait/sleep during compression and decompression. Second, per-CPU streams noticeably increase memory usage (actually more like wastage) of secondary compression streams. The problem is that secondary compression streams are allocated per-CPU, just like the primary streams are. Yet we never use more that one secondary stream at a time, because recompression is a single threaded action. Which means that remaining num_online_cpu() - 1 streams are allocated for nothing, and this is per-priority list (we can have several secondary compression algorithms). Depending on the algorithm this may lead to a significant memory wastage, in addition each stream also carries a workmem buffer (2 physical pages). Instead of per-CPU streams, maintain a list of idle compression streams and allocate new streams on-demand (something that we used to do many years ago). So that zram read() and write() become non-atomic and ease requirements on the compression algorithm implementation. This also means that we now should have only one secondary stream per-priority list. Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- drivers/block/zram/zcomp.c | 164 +++++++++++++++++++--------------- drivers/block/zram/zcomp.h | 17 ++-- drivers/block/zram/zram_drv.c | 29 +++--- include/linux/cpuhotplug.h | 1 - 4 files changed, 109 insertions(+), 102 deletions(-)