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? > > 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? > to avoid > confusion for the time being. Would that be ok with you? > > Best regards, > Vitaly >
