> -----Original Message----- > From: Vitaly Wool [mailto:vitaly.wool@xxxxxxxxxxxx] > Sent: Thursday, January 21, 2021 10:17 PM > To: tiantao (H) <tiantao6@xxxxxxxxxxxxx> > Cc: Seth Jennings <sjenning@xxxxxxxxxx>; Dan Streetman <ddstreet@xxxxxxxx>; > Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>; Song Bao Hua (Barry Song) > <song.bao.hua@xxxxxxxxxxxxx>; Linux-MM <linux-mm@xxxxxxxxx> > Subject: Re: [RFC mm/zswap 1/2] mm/zswap: add the flag can_sleep_mapped > > On Fri, Dec 25, 2020 at 12:02 PM Tian Tao <tiantao6@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> > > --- > > include/linux/zpool.h | 3 +++ > > mm/zpool.c | 13 +++++++++++++ > > mm/zswap.c | 50 > +++++++++++++++++++++++++++++++++++++++++++++----- > > 3 files changed, 61 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/zpool.h b/include/linux/zpool.h > > index 51bf430..e899701 100644 > > --- a/include/linux/zpool.h > > +++ b/include/linux/zpool.h > > @@ -73,6 +73,7 @@ u64 zpool_get_total_size(struct zpool *pool); > > * @malloc: allocate mem from a pool. > > * @free: free mem from a pool. > > * @shrink: shrink the pool. > > + * @sleep_mapped: whether zpool driver can sleep during map. > > * @map: map a handle. > > * @unmap: unmap a handle. > > * @total_size: get total size of a pool. > > @@ -100,6 +101,7 @@ struct zpool_driver { > > int (*shrink)(void *pool, unsigned int pages, > > unsigned int *reclaimed); > > > > + bool sleep_mapped; > > void *(*map)(void *pool, unsigned long handle, > > enum zpool_mapmode mm); > > void (*unmap)(void *pool, unsigned long handle); > > @@ -112,5 +114,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/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@xxxxxxxx>"); > > MODULE_DESCRIPTION("Common API for compressed memory storage"); > > diff --git a/mm/zswap.c b/mm/zswap.c > > index 182f6ad..67d4555 100644 > > --- a/mm/zswap.c > > +++ b/mm/zswap.c > > @@ -935,13 +935,20 @@ static int zswap_writeback_entry(struct zpool *pool, > unsigned long handle) > > struct scatterlist input, output; > > struct crypto_acomp_ctx *acomp_ctx; > > > > - u8 *src; > > + u8 *src, *tmp; > > unsigned int dlen; > > int ret; > > struct writeback_control wbc = { > > .sync_mode = WB_SYNC_NONE, > > }; > > > > + if (!zpool_can_sleep_mapped(pool)) { > > + > > + tmp = kmalloc(entry->length, GFP_ATOMIC); > > This has escaped my attention, but this is obviously a bug. 'entry' is > not initialized at this point. > You either have to move memory allocation further down when > entry->length starts making sense, or allocate PAGE_SIZE. > Since zsmalloc has no evict entry, we have never arrived here. That's why the testing by both Tiantao and Sebastian didn't show the problem. > Best regards, > Vitaly > > > + if (!tmp) > > + return -ENOMEM; > > + } > > + > > /* extract swpentry from data */ > > zhdr = zpool_map_handle(pool, handle, ZPOOL_MM_RO); > > swpentry = zhdr->swpentry; /* here */ > > @@ -979,6 +986,14 @@ static int zswap_writeback_entry(struct zpool *pool, > unsigned long handle) > > dlen = PAGE_SIZE; > > src = (u8 *)zhdr + sizeof(struct zswap_header); > > > > + if (!zpool_can_sleep_mapped(pool)) { > > + > > + memcpy(tmp, src, entry->length); > > + src = tmp; > > + > > + zpool_unmap_handle(pool, handle); > > + } > > + > > mutex_lock(acomp_ctx->mutex); > > sg_init_one(&input, src, entry->length); > > sg_init_table(&output, 1); > > @@ -1033,7 +1048,11 @@ static int zswap_writeback_entry(struct zpool *pool, > unsigned long handle) > > spin_unlock(&tree->lock); > > > > end: > > - zpool_unmap_handle(pool, handle); > > + if (zpool_can_sleep_mapped(pool)) > > + zpool_unmap_handle(pool, handle); > > + else > > + kfree(tmp); > > + > > return ret; > > } > > > > @@ -1235,7 +1254,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 +1275,29 @@ static int zswap_frontswap_load(unsigned type, pgoff_t > offset, > > goto freeentry; > > } > > > > + if (!zpool_can_sleep_mapped(entry->pool->zpool)) { > > + > > + tmp = kmalloc(entry->length, GFP_ATOMIC); > > + if (!tmp) { > > + ret = -ENOMEM; > > + goto freeentry; > > + } > > + } > > + > > /* 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 +1307,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: > > @@ -1279,7 +1319,7 @@ static int zswap_frontswap_load(unsigned type, pgoff_t > offset, > > zswap_entry_put(tree, entry); > > spin_unlock(&tree->lock); > > > > - return 0; > > + return ret; > > } > > > > /* frees an entry in zswap */ > > -- > > 2.7.4 > > Thanks Barry
