Re: [PATCH] zsmalloc: do not use bit_spin_lock

[tiantao (H)]

在 2020/12/23 8:11, Vitaly Wool 写道:
> On Tue, 22 Dec 2020, 22:06 Song Bao Hua (Barry Song),
> <song.bao.hua@xxxxxxxxxxxxx> wrote:
> > > -----Original Message-----
> > > From: Vitaly Wool [mailto:vitaly.wool@xxxxxxxxxxxx]
> > > Sent: Tuesday, December 22, 2020 10:44 PM
> > > To: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>
> > > Cc: Shakeel Butt <shakeelb@xxxxxxxxxx>; Minchan Kim <minchan@xxxxxxxxxx>; Mike
> > > Galbraith <efault@xxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>; linux-mm
> > > <linux-mm@xxxxxxxxx>; Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>;
> > > NitinGupta <ngupta@xxxxxxxxxx>; Sergey Senozhatsky
> > > <sergey.senozhatsky.work@xxxxxxxxx>; Andrew Morton
> > > <akpm@xxxxxxxxxxxxxxxxxxxx>; tiantao (H) <tiantao6@xxxxxxxxxxxxx>
> > > Subject: Re: [PATCH] zsmalloc: do not use bit_spin_lock
> > >
> > > On Tue, 22 Dec 2020, 03:11 Song Bao Hua (Barry Song),
> > > <song.bao.hua@xxxxxxxxxxxxx> wrote:
> > > > > -----Original Message-----
> > > > > From: Song Bao Hua (Barry Song)
> > > > > Sent: Tuesday, December 22, 2020 3:03 PM
> > > > > To: 'Vitaly Wool' <vitaly.wool@xxxxxxxxxxxx>
> > > > > Cc: Shakeel Butt <shakeelb@xxxxxxxxxx>; Minchan Kim <minchan@xxxxxxxxxx>;
> > > Mike
> > > > > Galbraith <efault@xxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>; linux-mm
> > > > > <linux-mm@xxxxxxxxx>; Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>;
> > > > > NitinGupta <ngupta@xxxxxxxxxx>; Sergey Senozhatsky
> > > > > <sergey.senozhatsky.work@xxxxxxxxx>; Andrew Morton
> > > > > <akpm@xxxxxxxxxxxxxxxxxxxx>; tiantao (H) <tiantao6@xxxxxxxxxxxxx>
> > > > > Subject: RE: [PATCH] zsmalloc: do not use bit_spin_lock
> > > > >
> > > > >
> > > > > > I'm still not convinced. Will kmap what, src? At this point src might
> > > become
> > > > > just a bogus pointer.
> > > > >
> > > > > As long as the memory is still there, we can kmap it by its page struct.
> > > But
> > > > > if
> > > > > it is not there anymore, we have no way.
> > > > >
> > > > > > Why couldn't the object have been moved somewhere else (due to the compaction
> > > > > mechanism for instance)
> > > > > > at the time DMA kicks in?
> > > > > So zs_map_object() will guarantee the src won't be moved by holding those
> > > > > preemption-disabled lock?
> > > > > If so, it seems we have to drop the MOVABLE gfp in zswap for zsmalloc case?
> > > > Or we can do get_page() to avoid the movement of the page.
> > >
> > > I would like to discuss this more in zswap context than zsmalloc's.
> > > Since zsmalloc does not implement reclaim callback, using it in zswap
> > > is a corner case anyway.
> > I see. But it seems we still need a solution for the compatibility
> > of zsmalloc and zswap? this will require change in either zsmalloc
> > or zswap.
> > or do you want to make zswap depend on !ZSMALLOC?
> No, I really don't think we should go that far. What if we add a flag
> to zpool, named like "can_sleep_mapped", and have it set for
> zbud/z3fold?
> Then zswap could go the current path if the flag is set; and if it's
> not set, and mutex_trylock fails, copy data from src to a temporary
> buffer, then unmap the handle, take the mutex, process the buffer
> instead of src. Not the nicest thing to do but at least it won't break
> anything.

write the following patch according to your idea, what do you think ?

--- 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;

@@ -1262,16 +1262,26 @@ static int zswap_frontswap_load(unsigned type, 
pgoff_t offset,
        if (zpool_evictable(entry->pool->zpool))
                src += sizeof(struct zswap_header);

+       if (!zpool_can_sleep_mapped(entry->pool->zpool) && 
!mutex_trylock(acomp_ctx->mutex)) {
+               tmp = kmemdup(src, entry->length, GFP_ATOMIC);
+               zpool_unmap_handle(entry->pool->zpool, entry->handle); ???
+               if (!tmp)
+                       goto freeentry;
+       }
        acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
        mutex_lock(acomp_ctx->mutex);
-       sg_init_one(&input, src, entry->length);
+       sg_init_one(&input, zpool_can_sleep_mapped(entry->pool->zpool) ? 
src : tmp, 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);
        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);
+
--- 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,

> ~Vitaly
>
> > > zswap, on the other hand, may be dealing with some new backends in
> > > future which have more chances to become mainstream. Imagine typical
> > > NUMA-like cases, i. e. a zswap pool allocated in some kind SRAM, or in
> > > unused video memory. In such a case if you try to use a pointer to an
> > > invalidated zpool mapping, you are on the way to thrash the system.
> > > So: no assumptions that the zswap pool is in regular linear RAM should
> > > be made.
> > >
> > > ~Vitaly
> > Thanks
> > Barry