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@kernel.org 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@vger.kernel.org # 4.14+ Signed-off-by: Minchan Kim minchan@kernel.org
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.