Re: [PATCHv4 01/17] zram: switch to non-atomic entry locking

[Sebastian Andrzej Siewior]

On 2025-02-06 16:01:12 [+0900], Sergey Senozhatsky wrote:
> So I completely reworked this bit and we don't have that problem
> anymore, nor the problem of "inventing locking schemes in 2025".
> 
> In short - we are returning back to bit-locks, what zram has been using
> until commit 9518e5bfaae19 ("zram: Replace bit spinlocks with a spinlock_t),
> not bit-spinlock these time around, that won't work with linux-rt, but
> wait_on_bit and friends.  Entry lock is exclusive, just like before,
> but lock owner can sleep now, any task wishing to lock that same entry
> will wait to be woken up by the current lock owner once it unlocks the
> entry.  For cases when lock is taken from atomic context (e.g. slot-free
> notification from softirq) we continue using TRY lock, which has been
> introduced by commit 3c9959e025472 ("zram: fix lockdep warning of free block
> handling"), so there's nothing new here.  In addition I added some lockdep
> annotations, just to be safe.
> 
> There shouldn't be too many tasks competing for the same entry.  I can
> only think of cases when read/write (or slot-free notification if zram
> is used as a swap device) vs. writeback or recompression (we cannot have
> writeback and recompression simultaneously).

So if I understand, you want to get back to bit spinlocks but sleeping
instead of polling. But why? Do you intend to have more locks per entry
so that you use the additional bits with the "lock"?

> It currently looks like this:
> 
…
> static __must_check bool zram_slot_try_lock(struct zram *zram, u32 index)
> {
>         unsigned long *lock = &zram->table[index].flags;
> 
>         if (!test_and_set_bit_lock(ZRAM_ENTRY_LOCK, lock)) {
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
>                 mutex_acquire(&zram->table[index].lockdep_map, 0, 0, _RET_IP_);
> #endif
>                 return true;
>         }
>         return false;
> }

I hope the caller does not poll.

> static void zram_slot_lock(struct zram *zram, u32 index)
> {
>         unsigned long *lock = &zram->table[index].flags;
> 
>         WARN_ON_ONCE(!preemptible());

you want might_sleep() here instead. preemptible() works only on
preemptible kernels. And might_sleep() is already provided by
wait_on_bit_lock(). So this can go.

>         wait_on_bit_lock(lock, ZRAM_ENTRY_LOCK, TASK_UNINTERRUPTIBLE);
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
>         mutex_acquire(&zram->table[index].lockdep_map, 0, 0, _RET_IP_);
> #endif

I would argue that you want this before the wait_on_bit_lock() simply
because you want to see a possible deadlock before it happens.

> }
> 
> static void zram_slot_unlock(struct zram *zram, u32 index)
> {
>         unsigned long *lock = &zram->table[index].flags;
> 
>         clear_and_wake_up_bit(ZRAM_ENTRY_LOCK, lock);
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
>         mutex_release(&zram->table[index].lockdep_map, _RET_IP_);
> #endif
Also before. So it complains about release a not locked lock before it
happens.
> }

Sebastian