Skip to contentSkip to conversation

Staging: zram: Turn lockdep off during zram_init_device()

PATCH In review Jerome Marchand 3 participants 4 messages Nov 30, 2011 – Dec 3, 2011 Staging

lore
Share & tools
lore
#0 lore raw

PATCH 1/2 Staging: zram: Turn lockdep off during zram_init_device()

zram->init_lock can be hold over an allocation that may try to reclaim memory in zram_init_device(). The same lock can later be taken from a reclaim context in zram_make_request(), thus triggering a lockdep warning. However, memory can not be reclaimed to an uninitialized zram device. Therefore, this warning is a false positive.
To prevent the warning to occur, we turn lockdep off during while the device is initialized.

---
 zram_drv.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)
drivers/staging/zram/zram_drv.c +14 -0
index 09de99f..34f4b96 100644
@@ -637,10 +637,22 @@ int zram_init_device(struct zram *zram)
 	int ret;
 	size_t num_pages;
 
+	/*
+	 * Here we hold init_lock over allocations that may try to reclaim
+	 * memory. The same lock is also taken from a reclaim context in
+	 * zram_make_request(). That end up in a lockdep warning.
+	 * Moreover the table can be quite big an it wouldn't be reasonable to
+	 * use kmalloc(GFP_NOFS,...) to allocate it.
+	 * Fortunately, memory can not be reclaimed to an uninitialized zram
+	 * device, so this warning is a false positive. Therefore, to prevent
+	 * the warning, we turn lockdep off here.
+	 */
+	lockdep_off();
 	down_write(&zram->init_lock);
 
 	if (zram->init_done) {
 		up_write(&zram->init_lock);
+		lockdep_on();
 		return 0;
 	}
 
@@ -682,6 +694,7 @@ int zram_init_device(struct zram *zram)
 
 	zram->init_done = 1;
 	up_write(&zram->init_lock);
+	lockdep_on();
 
 	pr_debug("Initialization done!\n");
 	return 0;
@@ -692,6 +705,7 @@ fail_no_table:
 fail:
 	__zram_reset_device(zram);
 	up_write(&zram->init_lock);
+	lockdep_on();
 	pr_err("Initialization failed: err=%d\n", ret);
 	return ret;
 }
#1 lore raw

PATCH 2/2 Staging: zram: Add a missing GFP_KERNEL specifier in zram_init_device()

The allocation of zram->compress_buffer is misssing a GFP_* specifier. This is equivalent to GFP_NOWAIT but it is more likely a omission. Since the allocation just above it uses GFP_KERNEL, there is no reason to use GFP_NOWAIT here. Therefore, add GFP_KERNEL.

---
 zram_drv.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/staging/zram/zram_drv.c +2 -1
index 34f4b96..a263a1b 100644
@@ -665,7 +665,8 @@ int zram_init_device(struct zram *zram)
 		goto fail_no_table;
 	}
 
-	zram->compress_buffer = (void *)__get_free_pages(__GFP_ZERO, 1);
+	zram->compress_buffer =
+		(void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
 	if (!zram->compress_buffer) {
 		pr_err("Error allocating compressor buffer space\n");
 		ret = -ENOMEM;
#2 lore raw
In reply to Jerome Marchand
Quoted 7 lines · Jerome Marchand

zram->init_lock can be hold over an allocation that may try to reclaim memory in zram_init_device(). The same lock can later be taken from a reclaim context in zram_make_request(), thus triggering a lockdep warning. However, memory can not be reclaimed to an uninitialized zram device. Therefore, this warning is a false positive.
To prevent the warning to occur, we turn lockdep off during while the device is initialized.

fuck no! why do you even remotely consider this a sane thing to do?

There's tons of lockdep annotations, try one of those. If you have, your changelog utterly fails to explain why non of those are suitable and how your problem is special.

#3 lore raw
Quoted 9 lines · Peter Zijlstra
Quoted 7 lines · Jerome Marchand

zram->init_lock can be hold over an allocation that may try to reclaim memory in zram_init_device(). The same lock can later be taken from a reclaim context in zram_make_request(), thus triggering a lockdep warning. However, memory can not be reclaimed to an uninitialized zram device. Therefore, this warning is a false positive.
To prevent the warning to occur, we turn lockdep off during while the device is initialized.

Quoted 6 lines

fuck no! why do you even remotely consider this a sane thing to do?

There's tons of lockdep annotations, try one of those. If you have, your changelog utterly fails to explain why non of those are suitable and how your problem is special.

I must agree. Propsed patch is a hack, and actually can make more trouble to other subsystems. It is better to live with warning than perform temporary lockdep off/on.

Signature · 1 line
Witold Baryluk