Re: [PATCH] zsmalloc: do not use bit_spin_lock

[Vitaly Wool <vitaly.wool@konsulko.com>]

On Wed, Dec 23, 2020 at 1:44 PM tiantao (H) <tiantao6@huawei.com> wrote:
>
>
> 在 2020/12/23 8:11, Vitaly Wool 写道:
> > On Tue, 22 Dec 2020, 22:06 Song Bao Hua (Barry Song),
> > <song.bao.hua@hisilicon.com> wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Vitaly Wool [mailto:vitaly.wool@konsulko.com]
> >>> Sent: Tuesday, December 22, 2020 10:44 PM
> >>> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> >>> Cc: Shakeel Butt <shakeelb@google.com>; Minchan Kim <minchan@kernel.org>; Mike
> >>> Galbraith <efault@gmx.de>; LKML <linux-kernel@vger.kernel.org>; linux-mm
> >>> <linux-mm@kvack.org>; Sebastian Andrzej Siewior <bigeasy@linutronix.de>;
> >>> NitinGupta <ngupta@vflare.org>; Sergey Senozhatsky
> >>> <sergey.senozhatsky.work@gmail.com>; Andrew Morton
> >>> <akpm@linux-foundation.org>; tiantao (H) <tiantao6@hisilicon.com>
> >>> 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@hisilicon.com> wrote:
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Song Bao Hua (Barry Song)
> >>>>> Sent: Tuesday, December 22, 2020 3:03 PM
> >>>>> To: 'Vitaly Wool' <vitaly.wool@konsulko.com>
> >>>>> Cc: Shakeel Butt <shakeelb@google.com>; Minchan Kim <minchan@kernel.org>;
> >>> Mike
> >>>>> Galbraith <efault@gmx.de>; LKML <linux-kernel@vger.kernel.org>; linux-mm
> >>>>> <linux-mm@kvack.org>; Sebastian Andrzej Siewior <bigeasy@linutronix.de>;
> >>>>> NitinGupta <ngupta@vflare.org>; Sergey Senozhatsky
> >>>>> <sergey.senozhatsky.work@gmail.com>; Andrew Morton
> >>>>> <akpm@linux-foundation.org>; tiantao (H) <tiantao6@hisilicon.com>
> >>>>> 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 ?

Yep, that is basically what I was thinking of. Some nitpicks below:

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

kmemdump? just use memcpy :)

> +               zpool_unmap_handle(entry->pool->zpool, entry->handle); ???
> +               if (!tmp)
> +                       goto freeentry;

Jumping to freentry results in returning success which isn't
appropriate here. You should return -ENOMEM in this case.

> +       }
>          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);

This is kind of hard to read, I would rather assign src to tmp after
memcpy and then no condition check would be needed here.

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

Best regards,
   Vitaly

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