RE: [PATCH] zsmalloc: do not use bit_spin_lock

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Shakeel Butt [mailto:shakeelb@xxxxxxxxxx]
> Sent: Tuesday, December 22, 2020 8:50 AM
> To: Vitaly Wool <vitaly.wool@xxxxxxxxxxxx>
> Cc: Minchan Kim <minchan@xxxxxxxxxx>; Mike Galbraith <efault@xxxxxx>; LKML
> <linux-kernel@xxxxxxxxxxxxxxx>; linux-mm <linux-mm@xxxxxxxxx>; Song Bao Hua
> (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>; Sebastian Andrzej Siewior
> <bigeasy@xxxxxxxxxxxxx>; NitinGupta <ngupta@xxxxxxxxxx>; Sergey Senozhatsky
> <sergey.senozhatsky.work@xxxxxxxxx>; Andrew Morton
> <akpm@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH] zsmalloc: do not use bit_spin_lock
> 
> On Mon, Dec 21, 2020 at 11:20 AM Vitaly Wool <vitaly.wool@xxxxxxxxxxxx> wrote:
> >
> > On Mon, Dec 21, 2020 at 6:24 PM Minchan Kim <minchan@xxxxxxxxxx> wrote:
> > >
> > > On Sun, Dec 20, 2020 at 02:22:28AM +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.
> > >
> > > I don't want to use such open code for the lock.
> > >
> > > I see from Mike's patch, recent zswap change introduced the lockdep
> > > splat bug and you want to improve zsmalloc to fix the zswap bug and
> > > introduce this patch with allowing preemption enabling.
> >
> > This understanding is upside down. The code in zswap you are referring
> > to is not buggy.  You may claim that it is suboptimal but there is
> > nothing wrong in taking a mutex.
> >
> 
> Is this suboptimal for all or just the hardware accelerators? Sorry, I
> am not very familiar with the crypto API. If I select lzo or lz4 as a
> zswap compressor will the [de]compression be async or sync?

Right now, in crypto subsystem, new drivers are required to write based on
async APIs. The old sync API can't work in new accelerator drivers as they
are not supported at all.

Old drivers are used to sync, but they've got async wrappers to support async
APIs. Eg.
crypto: acomp - add support for lz4 via scomp
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/crypto/lz4.c?id=8cd9330e0a615c931037d4def98b5ce0d540f08d

crypto: acomp - add support for lzo via scomp
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/crypto/lzo.c?id=ac9d2c4b39e022d2c61486bfc33b730cfd02898e

so they are supporting async APIs but they are still working in sync mode as
those old drivers don't sleep.

> 
> > >
> https://lore.kernel.org/linux-mm/fae85e4440a8ef6f13192476bd33a4826416fc58.
> camel@xxxxxx/
> > >
> > > zs_[un/map]_object is designed to be used in fast path(i.e.,
> > > zs_map_object/4K page copy/zs_unmap_object) so the spinlock is
> > > perfectly fine for API point of view. However, zswap introduced
> > > using the API with mutex_lock/crypto_wait_req where allowing
> > > preemption, which was wrong.
> >
> > Taking a spinlock in one callback and releasing it in another is
> > unsafe and error prone. What if unmap was called on completion of a
> > DMA-like transfer from another context, like a threaded IRQ handler?
> > In that case this spinlock might never be released.
> >
> > Anyway I can come up with a zswap patch explicitly stating that
> > zsmalloc is not fully compliant with zswap / zpool API
> 
> The documentation of zpool_map_handle() clearly states "This may hold
> locks, disable interrupts, and/or preemption, ...", so how come
> zsmalloc is not fully compliant?

Zbud, z3fold haven't really done this. If we hold spinlock before
entering zswap and release spinlock after calling zswap, this will
put zswap in an atomic context which isn't necessarily needed.

> 
> > to avoid
> > confusion for the time being. Would that be ok with you?
> >
> > Best regards,
> >    Vitaly
> >

Thanks
Barry