| Message ID | 1608812141-20238-1-git-send-email-tiantao6@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/zswap: Fix the compatibility of zsmalloc and zswap | expand |
On Thu, Dec 24, 2020 at 1:16 PM Tian Tao <tiantao6@hisilicon.com> wrote: > > add a flag to zpool, named is "can_sleep_mapped", and have it set true > for zbud/z3fold, set false for zsmalloc. Then zswap could go the current > path if the flag is true; and if it's false, copy data from src to a > temporary buffer, then unmap the handle, take the mutex, process the > buffer instead of src to avoid sleeping function called from atomic > context. > > Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > --- > include/linux/zpool.h | 2 ++ > mm/z3fold.c | 1 + > mm/zbud.c | 1 + > mm/zpool.c | 13 +++++++++++++ > mm/zsmalloc.c | 1 + > mm/zswap.c | 23 +++++++++++++++++++++-- > 6 files changed, 39 insertions(+), 2 deletions(-) > > diff --git a/include/linux/zpool.h b/include/linux/zpool.h > index 51bf430..a354f4fe 100644 > --- a/include/linux/zpool.h > +++ b/include/linux/zpool.h > @@ -82,6 +82,7 @@ u64 zpool_get_total_size(struct zpool *pool); > */ > struct zpool_driver { > char *type; > + bool sleep_mapped; Could you please add this somewhat further down, close to map / unmap callbacks, and add a description of the field above, too? > struct module *owner; > atomic_t refcount; > struct list_head list; > @@ -112,5 +113,6 @@ void zpool_register_driver(struct zpool_driver *driver); > int zpool_unregister_driver(struct zpool_driver *driver); > > bool zpool_evictable(struct zpool *pool); > +bool zpool_can_sleep_mapped(struct zpool *pool); > > #endif > diff --git a/mm/z3fold.c b/mm/z3fold.c > index dacb0d7..234b46f 100644 > --- a/mm/z3fold.c > +++ b/mm/z3fold.c > @@ -1778,6 +1778,7 @@ static u64 z3fold_zpool_total_size(void *pool) > > static struct zpool_driver z3fold_zpool_driver = { > .type = "z3fold", > + .sleep_mapped = true, > .owner = THIS_MODULE, > .create = z3fold_zpool_create, > .destroy = z3fold_zpool_destroy, > diff --git a/mm/zbud.c b/mm/zbud.c > index c49966e..7ec5f27 100644 > --- a/mm/zbud.c > +++ b/mm/zbud.c > @@ -203,6 +203,7 @@ static u64 zbud_zpool_total_size(void *pool) > > static struct zpool_driver zbud_zpool_driver = { > .type = "zbud", > + .sleep_mapped = true, > .owner = THIS_MODULE, > .create = zbud_zpool_create, > .destroy = zbud_zpool_destroy, > diff --git a/mm/zpool.c b/mm/zpool.c > index 3744a2d..5ed7120 100644 > --- a/mm/zpool.c > +++ b/mm/zpool.c > @@ -23,6 +23,7 @@ struct zpool { > void *pool; > const struct zpool_ops *ops; > bool evictable; > + bool can_sleep_mapped; > > struct list_head list; > }; > @@ -183,6 +184,7 @@ struct zpool *zpool_create_pool(const char *type, const char *name, gfp_t gfp, > zpool->pool = driver->create(name, gfp, ops, zpool); > zpool->ops = ops; > zpool->evictable = driver->shrink && ops && ops->evict; > + zpool->can_sleep_mapped = driver->sleep_mapped; > > if (!zpool->pool) { > pr_err("couldn't create %s pool\n", type); > @@ -393,6 +395,17 @@ bool zpool_evictable(struct zpool *zpool) > return zpool->evictable; > } > > +/** > + * zpool_can_sleep_mapped - Test if zpool can sleep when do mapped. > + * @zpool: The zpool to test > + * > + * Returns: true if zpool can sleep; false otherwise. > + */ > +bool zpool_can_sleep_mapped(struct zpool *zpool) > +{ > + return zpool->can_sleep_mapped; > +} > + > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Dan Streetman <ddstreet@ieee.org>"); > MODULE_DESCRIPTION("Common API for compressed memory storage"); > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c > index 7289f50..a991d3c 100644 > --- a/mm/zsmalloc.c > +++ b/mm/zsmalloc.c > @@ -440,6 +440,7 @@ static u64 zs_zpool_total_size(void *pool) > > static struct zpool_driver zs_zpool_driver = { > .type = "zsmalloc", > + .sleep_mapped = false, You don't have to add this, it will be false if not explicitly specified. > .owner = THIS_MODULE, > .create = zs_zpool_create, > .destroy = zs_zpool_destroy, > diff --git a/mm/zswap.c b/mm/zswap.c > index 182f6ad..51b033a 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -1235,7 +1235,7 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, > struct zswap_entry *entry; > struct scatterlist input, output; > struct crypto_acomp_ctx *acomp_ctx; > - u8 *src, *dst; > + u8 *src, *dst, *tmp; > unsigned int dlen; > int ret; > > @@ -1256,12 +1256,27 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, > goto freeentry; > } > > + if (!zpool_can_sleep_mapped(entry->pool->zpool)) { > + > + tmp = kzalloc(entry->length, GFP_KERNEL); Do we really need kzalloc here? the buffer will be written to in a few moments anyway. Also, we allocate a small buffer for a short while, and this is likely to be a performance critical path, so GFP_ATOMIC looks more appropriate. > + if (!tmp) > + return -ENOMEM; You can't just return here, you need to call zswap_entry_put() first. Since this is obviously not the final version of your patch, could you please split it in 2 the next time? I'd prefer setting flags to specific backends in a separate patch. Best regards, Vitaly > + } > + > /* decompress */ > dlen = PAGE_SIZE; > src = zpool_map_handle(entry->pool->zpool, entry->handle, ZPOOL_MM_RO); > if (zpool_evictable(entry->pool->zpool)) > src += sizeof(struct zswap_header); > > + if (!zpool_can_sleep_mapped(entry->pool->zpool)) { > + > + memcpy(tmp, src, entry->length); > + src = tmp; > + > + zpool_unmap_handle(entry->pool->zpool, entry->handle); > + } > + > acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); > mutex_lock(acomp_ctx->mutex); > sg_init_one(&input, src, entry->length); > @@ -1271,7 +1286,11 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, > ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait); > mutex_unlock(acomp_ctx->mutex); > > - zpool_unmap_handle(entry->pool->zpool, entry->handle); > + if (zpool_can_sleep_mapped(entry->pool->zpool)) > + zpool_unmap_handle(entry->pool->zpool, entry->handle); > + else > + kfree(tmp); > + > BUG_ON(ret); > > freeentry: > -- > 2.7.4 >
在 2020/12/24 22:16, Vitaly Wool 写道: > On Thu, Dec 24, 2020 at 1:16 PM Tian Tao <tiantao6@hisilicon.com> wrote: >> add a flag to zpool, named is "can_sleep_mapped", and have it set true >> for zbud/z3fold, set false for zsmalloc. Then zswap could go the current >> path if the flag is true; and if it's false, copy data from src to a >> temporary buffer, then unmap the handle, take the mutex, process the >> buffer instead of src to avoid sleeping function called from atomic >> context. >> >> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> >> --- >> include/linux/zpool.h | 2 ++ >> mm/z3fold.c | 1 + >> mm/zbud.c | 1 + >> mm/zpool.c | 13 +++++++++++++ >> mm/zsmalloc.c | 1 + >> mm/zswap.c | 23 +++++++++++++++++++++-- >> 6 files changed, 39 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/zpool.h b/include/linux/zpool.h >> index 51bf430..a354f4fe 100644 >> --- a/include/linux/zpool.h >> +++ b/include/linux/zpool.h >> @@ -82,6 +82,7 @@ u64 zpool_get_total_size(struct zpool *pool); >> */ >> struct zpool_driver { >> char *type; >> + bool sleep_mapped; > Could you please add this somewhat further down, close to map / unmap > callbacks, and add a description of the field above, too? >> struct module *owner; >> atomic_t refcount; >> struct list_head list; >> @@ -112,5 +113,6 @@ void zpool_register_driver(struct zpool_driver *driver); >> int zpool_unregister_driver(struct zpool_driver *driver); >> >> bool zpool_evictable(struct zpool *pool); >> +bool zpool_can_sleep_mapped(struct zpool *pool); >> >> #endif >> diff --git a/mm/z3fold.c b/mm/z3fold.c >> index dacb0d7..234b46f 100644 >> --- a/mm/z3fold.c >> +++ b/mm/z3fold.c >> @@ -1778,6 +1778,7 @@ static u64 z3fold_zpool_total_size(void *pool) >> >> static struct zpool_driver z3fold_zpool_driver = { >> .type = "z3fold", >> + .sleep_mapped = true, >> .owner = THIS_MODULE, >> .create = z3fold_zpool_create, >> .destroy = z3fold_zpool_destroy, >> diff --git a/mm/zbud.c b/mm/zbud.c >> index c49966e..7ec5f27 100644 >> --- a/mm/zbud.c >> +++ b/mm/zbud.c >> @@ -203,6 +203,7 @@ static u64 zbud_zpool_total_size(void *pool) >> >> static struct zpool_driver zbud_zpool_driver = { >> .type = "zbud", >> + .sleep_mapped = true, >> .owner = THIS_MODULE, >> .create = zbud_zpool_create, >> .destroy = zbud_zpool_destroy, >> diff --git a/mm/zpool.c b/mm/zpool.c >> index 3744a2d..5ed7120 100644 >> --- a/mm/zpool.c >> +++ b/mm/zpool.c >> @@ -23,6 +23,7 @@ struct zpool { >> void *pool; >> const struct zpool_ops *ops; >> bool evictable; >> + bool can_sleep_mapped; >> >> struct list_head list; >> }; >> @@ -183,6 +184,7 @@ struct zpool *zpool_create_pool(const char *type, const char *name, gfp_t gfp, >> zpool->pool = driver->create(name, gfp, ops, zpool); >> zpool->ops = ops; >> zpool->evictable = driver->shrink && ops && ops->evict; >> + zpool->can_sleep_mapped = driver->sleep_mapped; >> >> if (!zpool->pool) { >> pr_err("couldn't create %s pool\n", type); >> @@ -393,6 +395,17 @@ bool zpool_evictable(struct zpool *zpool) >> return zpool->evictable; >> } >> >> +/** >> + * zpool_can_sleep_mapped - Test if zpool can sleep when do mapped. >> + * @zpool: The zpool to test >> + * >> + * Returns: true if zpool can sleep; false otherwise. >> + */ >> +bool zpool_can_sleep_mapped(struct zpool *zpool) >> +{ >> + return zpool->can_sleep_mapped; >> +} >> + >> MODULE_LICENSE("GPL"); >> MODULE_AUTHOR("Dan Streetman <ddstreet@ieee.org>"); >> MODULE_DESCRIPTION("Common API for compressed memory storage"); >> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c >> index 7289f50..a991d3c 100644 >> --- a/mm/zsmalloc.c >> +++ b/mm/zsmalloc.c >> @@ -440,6 +440,7 @@ static u64 zs_zpool_total_size(void *pool) >> >> static struct zpool_driver zs_zpool_driver = { >> .type = "zsmalloc", >> + .sleep_mapped = false, > You don't have to add this, it will be false if not explicitly specified. > >> .owner = THIS_MODULE, >> .create = zs_zpool_create, >> .destroy = zs_zpool_destroy, >> diff --git a/mm/zswap.c b/mm/zswap.c >> index 182f6ad..51b033a 100644 >> --- a/mm/zswap.c >> +++ b/mm/zswap.c >> @@ -1235,7 +1235,7 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, >> struct zswap_entry *entry; >> struct scatterlist input, output; >> struct crypto_acomp_ctx *acomp_ctx; >> - u8 *src, *dst; >> + u8 *src, *dst, *tmp; >> unsigned int dlen; >> int ret; >> >> @@ -1256,12 +1256,27 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, >> goto freeentry; >> } >> >> + if (!zpool_can_sleep_mapped(entry->pool->zpool)) { >> + >> + tmp = kzalloc(entry->length, GFP_KERNEL); > Do we really need kzalloc here? the buffer will be written to in a few > moments anyway. > Also, we allocate a small buffer for a short while, and this is likely > to be a performance critical path, so GFP_ATOMIC looks more > appropriate. > >> + if (!tmp) >> + return -ENOMEM; > You can't just return here, you need to call zswap_entry_put() first. > > Since this is obviously not the final version of your patch, could you > please split it in 2 the next time? I'd prefer setting flags to > specific backends in a separate patch. Thanks for helping to review the code. I will post a new patch of my understanding according to you advice, if I do not understand the right, you can help point out, I continue to modify > > Best regards, > Vitaly > >> + } >> + >> /* decompress */ >> dlen = PAGE_SIZE; >> src = zpool_map_handle(entry->pool->zpool, entry->handle, ZPOOL_MM_RO); >> if (zpool_evictable(entry->pool->zpool)) >> src += sizeof(struct zswap_header); >> >> + if (!zpool_can_sleep_mapped(entry->pool->zpool)) { >> + >> + memcpy(tmp, src, entry->length); >> + src = tmp; >> + >> + zpool_unmap_handle(entry->pool->zpool, entry->handle); >> + } >> + >> acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); >> mutex_lock(acomp_ctx->mutex); >> sg_init_one(&input, src, entry->length); >> @@ -1271,7 +1286,11 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, >> ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait); >> mutex_unlock(acomp_ctx->mutex); >> >> - zpool_unmap_handle(entry->pool->zpool, entry->handle); >> + if (zpool_can_sleep_mapped(entry->pool->zpool)) >> + zpool_unmap_handle(entry->pool->zpool, entry->handle); >> + else >> + kfree(tmp); >> + >> BUG_ON(ret); >> >> freeentry: >> -- >> 2.7.4 >> > . >
diff --git a/include/linux/zpool.h b/include/linux/zpool.h index 51bf430..a354f4fe 100644 --- a/include/linux/zpool.h +++ b/include/linux/zpool.h @@ -82,6 +82,7 @@ u64 zpool_get_total_size(struct zpool *pool); */ struct zpool_driver { char *type; + bool sleep_mapped; struct module *owner; atomic_t refcount; struct list_head list; @@ -112,5 +113,6 @@ void zpool_register_driver(struct zpool_driver *driver); int zpool_unregister_driver(struct zpool_driver *driver); bool zpool_evictable(struct zpool *pool); +bool zpool_can_sleep_mapped(struct zpool *pool); #endif diff --git a/mm/z3fold.c b/mm/z3fold.c index dacb0d7..234b46f 100644 --- a/mm/z3fold.c +++ b/mm/z3fold.c @@ -1778,6 +1778,7 @@ static u64 z3fold_zpool_total_size(void *pool) static struct zpool_driver z3fold_zpool_driver = { .type = "z3fold", + .sleep_mapped = true, .owner = THIS_MODULE, .create = z3fold_zpool_create, .destroy = z3fold_zpool_destroy, diff --git a/mm/zbud.c b/mm/zbud.c index c49966e..7ec5f27 100644 --- a/mm/zbud.c +++ b/mm/zbud.c @@ -203,6 +203,7 @@ static u64 zbud_zpool_total_size(void *pool) static struct zpool_driver zbud_zpool_driver = { .type = "zbud", + .sleep_mapped = true, .owner = THIS_MODULE, .create = zbud_zpool_create, .destroy = zbud_zpool_destroy, diff --git a/mm/zpool.c b/mm/zpool.c index 3744a2d..5ed7120 100644 --- a/mm/zpool.c +++ b/mm/zpool.c @@ -23,6 +23,7 @@ struct zpool { void *pool; const struct zpool_ops *ops; bool evictable; + bool can_sleep_mapped; struct list_head list; }; @@ -183,6 +184,7 @@ struct zpool *zpool_create_pool(const char *type, const char *name, gfp_t gfp, zpool->pool = driver->create(name, gfp, ops, zpool); zpool->ops = ops; zpool->evictable = driver->shrink && ops && ops->evict; + zpool->can_sleep_mapped = driver->sleep_mapped; if (!zpool->pool) { pr_err("couldn't create %s pool\n", type); @@ -393,6 +395,17 @@ bool zpool_evictable(struct zpool *zpool) return zpool->evictable; } +/** + * zpool_can_sleep_mapped - Test if zpool can sleep when do mapped. + * @zpool: The zpool to test + * + * Returns: true if zpool can sleep; false otherwise. + */ +bool zpool_can_sleep_mapped(struct zpool *zpool) +{ + return zpool->can_sleep_mapped; +} + MODULE_LICENSE("GPL"); MODULE_AUTHOR("Dan Streetman <ddstreet@ieee.org>"); MODULE_DESCRIPTION("Common API for compressed memory storage"); diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c index 7289f50..a991d3c 100644 --- a/mm/zsmalloc.c +++ b/mm/zsmalloc.c @@ -440,6 +440,7 @@ static u64 zs_zpool_total_size(void *pool) static struct zpool_driver zs_zpool_driver = { .type = "zsmalloc", + .sleep_mapped = false, .owner = THIS_MODULE, .create = zs_zpool_create, .destroy = zs_zpool_destroy, diff --git a/mm/zswap.c b/mm/zswap.c index 182f6ad..51b033a 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -1235,7 +1235,7 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, struct zswap_entry *entry; struct scatterlist input, output; struct crypto_acomp_ctx *acomp_ctx; - u8 *src, *dst; + u8 *src, *dst, *tmp; unsigned int dlen; int ret; @@ -1256,12 +1256,27 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, goto freeentry; } + if (!zpool_can_sleep_mapped(entry->pool->zpool)) { + + tmp = kzalloc(entry->length, GFP_KERNEL); + if (!tmp) + return -ENOMEM; + } + /* decompress */ dlen = PAGE_SIZE; src = zpool_map_handle(entry->pool->zpool, entry->handle, ZPOOL_MM_RO); if (zpool_evictable(entry->pool->zpool)) src += sizeof(struct zswap_header); + if (!zpool_can_sleep_mapped(entry->pool->zpool)) { + + memcpy(tmp, src, entry->length); + src = tmp; + + zpool_unmap_handle(entry->pool->zpool, entry->handle); + } + acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); mutex_lock(acomp_ctx->mutex); sg_init_one(&input, src, entry->length); @@ -1271,7 +1286,11 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait); mutex_unlock(acomp_ctx->mutex); - zpool_unmap_handle(entry->pool->zpool, entry->handle); + if (zpool_can_sleep_mapped(entry->pool->zpool)) + zpool_unmap_handle(entry->pool->zpool, entry->handle); + else + kfree(tmp); + BUG_ON(ret); freeentry:
add a flag to zpool, named is "can_sleep_mapped", and have it set true for zbud/z3fold, set false for zsmalloc. Then zswap could go the current path if the flag is true; and if it's false, copy data from src to a temporary buffer, then unmap the handle, take the mutex, process the buffer instead of src to avoid sleeping function called from atomic context. Signed-off-by: Tian Tao <tiantao6@hisilicon.com> --- include/linux/zpool.h | 2 ++ mm/z3fold.c | 1 + mm/zbud.c | 1 + mm/zpool.c | 13 +++++++++++++ mm/zsmalloc.c | 1 + mm/zswap.c | 23 +++++++++++++++++++++-- 6 files changed, 39 insertions(+), 2 deletions(-)