RE: [PATCH] zsmalloc: do not use bit_spin_lock

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Mike Galbraith [mailto:efault@xxxxxx]
> Sent: Sunday, December 20, 2020 8:48 PM
> To: Vitaly Wool <vitaly.wool@xxxxxxxxxxxx>; LKML
> <linux-kernel@xxxxxxxxxxxxxxx>; linux-mm <linux-mm@xxxxxxxxx>
> Cc: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>; Sebastian Andrzej
> Siewior <bigeasy@xxxxxxxxxxxxx>; Minchan Kim <minchan@xxxxxxxxxx>; NitinGupta
> <ngupta@xxxxxxxxxx>
> Subject: Re: [PATCH] zsmalloc: do not use bit_spin_lock
> 
> On Sun, 2020-12-20 at 02:23 +0100, Mike Galbraith wrote:
> > On Sun, 2020-12-20 at 02:22 +0200, Vitaly Wool wrote:
> > > zsmalloc takes bit spinlock in its _map() callback and releases it
> > > only in unmap() which is unsafe and leads to zswap complaining
> > > about scheduling in atomic context.
> > >
> > > To fix that and to improve RT properties of zsmalloc, remove that
> > > bit spinlock completely and use a bit flag instead.
> >
> > It also does get_cpu_var() in map(), put_cpu_var() in unmap().
> 
> That aside, the bit spinlock removal seems to hold up to beating in RT.
> I stripped out the RT changes to replace the bit spinlocks, applied the
> still needed atm might_sleep() fix, and ltp zram and zswap test are
> running in a loop with no signs that it's a bad idea, so I hope that
> makes it in (minus the preempt disabled spin which I whacked), as it
> makes zsmalloc markedly more RT friendly.
> 
> RT changes go from:
>  1 file changed, 79 insertions(+), 6 deletions(-)
> to:
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 

Sorry, would you like to show the change for 
"8 insertions(+), 3 deletions(-)"?

BTW, your original patch looks not right as 
crypto_wait_req(crypto_acomp_decompress()...)
can sleep too.

[copy from your original patch with comment]
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1258,20 +1258,20 @@ static int zswap_frontswap_load(unsigned

 	/* decompress */
 	dlen = PAGE_SIZE;
+	acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
+	mutex_lock(acomp_ctx->mutex);
 	src = zpool_map_handle(entry->pool->zpool, entry->handle, ZPOOL_MM_RO);
 	if (zpool_evictable(entry->pool->zpool))
 		src += sizeof(struct zswap_header);

-	acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
-	mutex_lock(acomp_ctx->mutex);
 	sg_init_one(&input, src, entry->length);
 	sg_init_table(&output, 1);
 	sg_set_page(&output, page, PAGE_SIZE, 0);
 	acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen);

/*!!!!!!!!!!!!!!!!
 * here crypto could sleep
 !!!!!!!!!!!!!!*/

 	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);
+	mutex_unlock(acomp_ctx->mutex);
 	BUG_ON(ret);

 freeentry:

[end]

so I guess we have to fix zsmalloc.


> 	-Mike

Thanks
Barry