On Mon, Nov 26, 2018 at 12:49:28PM -0800, Andrew Morton wrote:
> On Mon, 26 Nov 2018 17:28:07 +0900 Minchan Kim <minchan@xxxxxxxxxx> wrote:
>
> >
> > ...
> >
> > With writeback feature, zram_slot_free_notify could be called
> > in softirq context by end_swap_bio_read. However, bitmap_lock
> > is not aware of that so lockdep yell out. Thanks.
> >
> > The problem is not only bitmap_lock but it is also zram_slot_lock
> > so straightforward solution would disable irq on zram_slot_lock
> > which covers every bitmap_lock, too.
> > Although duration disabling the irq is short in many places
> > zram_slot_lock is used, a place(ie, decompress) is not fast
> > enough to hold irqlock on relying on compression algorithm
> > so it's not a option.
> >
> > The approach in this patch is just "best effort", not guarantee
> > "freeing orphan zpage". If the zram_slot_lock contention may happen,
> > kernel couldn't free the zpage until it recycles the block. However,
> > such contention between zram_slot_free_notify and other places to
> > hold zram_slot_lock should be very rare in real practice.
> > To see how often it happens, this patch adds new debug stat
> > "miss_free".
> >
> > It also adds irq lock in get/put_block_bdev to prevent deadlock
> > lockdep reported. The reason I used irq disable rather than bottom
> > half is swap_slot_free_notify could be called with irq disabled
> > so it breaks local_bh_enable's rule. The irqlock works on only
> > writebacked zram slot entry so it should be not frequent lock.
> >
> > Cc: stable@xxxxxxxxxxxxxxx # 4.14+
> > Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
> > ---
> > drivers/block/zram/zram_drv.c | 56 +++++++++++++++++++++++++----------
> > drivers/block/zram/zram_drv.h | 1 +
> > 2 files changed, 42 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 4879595200e1..472027eaed60 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -53,6 +53,11 @@ static size_t huge_class_size;
> >
> > static void zram_free_page(struct zram *zram, size_t index);
> >
> > +static int zram_slot_trylock(struct zram *zram, u32 index)
> > +{
> > + return bit_spin_trylock(ZRAM_LOCK, &zram->table[index].value);
> > +}
> > +
> > static void zram_slot_lock(struct zram *zram, u32 index)
> > {
> > bit_spin_lock(ZRAM_LOCK, &zram->table[index].value);
> > @@ -443,29 +448,45 @@ static ssize_t backing_dev_store(struct device *dev,
> >
> > static unsigned long get_entry_bdev(struct zram *zram)
> > {
> > - unsigned long entry;
> > + unsigned long blk_idx;
> > + unsigned long ret = 0;
> >
> > - spin_lock(&zram->bitmap_lock);
> > /* skip 0 bit to confuse zram.handle = 0 */
> > - entry = find_next_zero_bit(zram->bitmap, zram->nr_pages, 1);
> > - if (entry == zram->nr_pages) {
> > - spin_unlock(&zram->bitmap_lock);
> > - return 0;
> > + blk_idx = find_next_zero_bit(zram->bitmap, zram->nr_pages, 1);
> > + if (blk_idx == zram->nr_pages)
> > + goto retry;
> > +
> > + spin_lock_irq(&zram->bitmap_lock);
> > + if (test_bit(blk_idx, zram->bitmap)) {
> > + spin_unlock_irq(&zram->bitmap_lock);
> > + goto retry;
> > }
> >
> > - set_bit(entry, zram->bitmap);
> > - spin_unlock(&zram->bitmap_lock);
> > + set_bit(blk_idx, zram->bitmap);
>
> Here we could do
>
> if (test_and_set_bit(...)) {
> spin_unlock(...);
> goto retry;
>
> But it's weird to take the spinlock on behalf of bitops which are
> already atomic!
>
> It seems rather suspicious to me. Why are we doing this?
What I need is look_up_and_set operation. I don't see there is an
atomic operation for that. But I want to minimize irq disabled
area so first, it scans the bit lockless and if race happens,
i can try under the lock.
It seems __set_bit is enough under the lock.
>
> > + ret = blk_idx;
> > + goto out;
> > +retry:
> > + spin_lock_irq(&zram->bitmap_lock);
> > + blk_idx = find_next_zero_bit(zram->bitmap, zram->nr_pages, 1);
> > + if (blk_idx == zram->nr_pages)
> > + goto out;
> > +
> > + set_bit(blk_idx, zram->bitmap);
> > + ret = blk_idx;
> > +out:
> > + spin_unlock_irq(&zram->bitmap_lock);
> >
> > - return entry;
> > + return ret;
> > }
> >
> > static void put_entry_bdev(struct zram *zram, unsigned long entry)
> > {
> > int was_set;
> > + unsigned long flags;
> >
> > - spin_lock(&zram->bitmap_lock);
> > + spin_lock_irqsave(&zram->bitmap_lock, flags);
> > was_set = test_and_clear_bit(entry, zram->bitmap);
> > - spin_unlock(&zram->bitmap_lock);
> > + spin_unlock_irqrestore(&zram->bitmap_lock, flags);
>
> Here's another one. Surely that locking is unnecessary.
Indeed! although get_entry_bdev side can miss some bits, it's not a critical problem.
Benefit is we might remove irq disable for the lockdep problem.
Yes, I will cook and test.
Thanks, Andrew.
