block: blk_queue_bounce_limits can actually sleep

PATCH In review Arjan van de Ven 3 participants 6 messages May 20, 2008 Block layer

lore

Share & tools

lore

Arjan van de Ven

author

2008-05-20 13:36 UTC #0 lore raw

PATCH 1/1 block: blk_queue_bounce_limits can actually sleep

From: Arjan van de Ven <[email protected]>
Subject: [PATCH] block: blk_queue_bounce_limits can actually sleep

blk_queue_bounce_limit can call init_emergency_isa_pool, which
does sleeping allocations... document it as such by adding might_sleep() to the driver

Signed-off-by Arjan van de Ven <[email protected]>

---
 block/blk-settings.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

block/blk-settings.c +1 -0

index 8dd8641..c420a67 100644
@@ -148,6 +148,7 @@ void blk_queue_bounce_limit(struct request_queue *q, u64 dma_addr)
 	q->bounce_pfn = b_pfn;
 #endif
 	if (dma) {
+		might_sleep();
 		init_emergency_isa_pool();
 		q->bounce_gfp = GFP_NOIO | GFP_DMA;
 		q->bounce_pfn = b_pfn;

Signature · 1 line

1.5.4.5

Jens Axboe

2008-05-20 19:30 UTC #1 lore raw

Quoted 6 lines · Arjan van de Ven

From: Arjan van de Ven <[email protected]>
Subject: [PATCH] block: blk_queue_bounce_limits can actually sleep
blk_queue_bounce_limit can call init_emergency_isa_pool, which does sleeping allocations... document it as such by adding might_sleep() to the driver

Isn't that superflous, as mempool_create() -> kmalloc(..., __GFP_WAIT) ends up spewing that warning anyway?

Signature · 1 line

Jens Axboe

Andrew Morton

2008-05-20 19:46 UTC #2 lore raw

Quoted 10 lines · On Tue, 20 May 2008 21:29:59 +0200 Jens Axboe <[email protected]> wrote:

Quoted 6 lines · Arjan van de Ven
From: Arjan van de Ven <[email protected]>
Subject: [PATCH] block: blk_queue_bounce_limits can actually sleep
blk_queue_bounce_limit can call init_emergency_isa_pool, which does sleeping allocations... document it as such by adding might_sleep() to the driver
Isn't that superflous, as mempool_create() -> kmalloc(..., __GFP_WAIT) ends up spewing that warning anyway?

It's largely superfluous given the way in which Arjan implemented it.

One situation which we regularly hit is:

foo()

{
	...
	if (some_unlikely_condition())
		do_something_which_sleeps();
	...
}

and then we go and call that code under spinlock and ship it out, when of course a handful of testers hit the unlikely condition.

The solution to that is to add a might_sleep() _outside_ the test of some_unlikely_condition(). ie:

block/blk-settings.c renamed from block/blk-settings.c~a +2 -0

@@ -140,6 +140,8 @@ void blk_queue_bounce_limit(struct reque
 	unsigned long b_pfn = dma_addr >> PAGE_SHIFT;
 	int dma = 0;
 
+	might_sleep();
+
 	q->bounce_gfp = GFP_NOIO;
 #if BITS_PER_LONG == 64
 	/* Assume anything <= 4GB can be handled by IOMMU.

but it's all vague and waffly because Arjan forgot to tell us why he's bothering to patch this code at all???

Jens Axboe

2008-05-20 19:59 UTC #3 lore raw

Quoted 35 lines · Andrew Morton

Quoted 10 lines · On Tue, 20 May 2008 21:29:59 +0200 Jens Axboe <[email protected]> wrote:
Quoted 6 lines · Arjan van de Ven
From: Arjan van de Ven <[email protected]>
Subject: [PATCH] block: blk_queue_bounce_limits can actually sleep
blk_queue_bounce_limit can call init_emergency_isa_pool, which does sleeping allocations... document it as such by adding might_sleep() to the driver
Isn't that superflous, as mempool_create() -> kmalloc(..., __GFP_WAIT) ends up spewing that warning anyway?
It's largely superfluous given the way in which Arjan implemented it.
One situation which we regularly hit is:
foo()
{
	...
	if (some_unlikely_condition())
		do_something_which_sleeps();
	...
}
and then we go and call that code under spinlock and ship it out, when of course a handful of testers hit the unlikely condition.
The solution to that is to add a might_sleep() _outside_ the test of some_unlikely_condition(). ie:

block/blk-settings.c void blk_queue_bounce_limit(struct reque · line 147

Quoted 8 lines

block/blk-settings.c renamed from block/blk-settings.c~a +2 -0

@@ -140,6 +140,8 @@ void blk_queue_bounce_limit(struct reque
 	unsigned long b_pfn = dma_addr >> PAGE_SHIFT;
 	int dma = 0;
 
+	might_sleep();
+
 	q->bounce_gfp = GFP_NOIO;
 #if BITS_PER_LONG == 64
 	/* Assume anything <= 4GB can be handled by IOMMU.

Yeah, THAT I agree with in general, but it's probably too much here since most callers will not block and probably do call it under the queue lock already (just guessing here, didn't audit any callers).

Quoted 2 lines

but it's all vague and waffly because Arjan forgot to tell us why he's bothering to patch this code at all???

Probably the math still isn't quite correct, so it ends up setting up the isa pool for no good reason :-(

Signature · 1 line

Jens Axboe

Arjan van de Ven

author

2008-05-20 21:03 UTC #4 lore raw

On Tue, 20 May 2008 21:58:57 +0200

Quoted 10 lines

Yeah, THAT I agree with in genereal, but it's probably too much here since most callers will not block and probably do call it under the queue lock already (just guessing here, didn't audit any callers).
Quoted 2 lines
but it's all vague and waffly because Arjan forgot to tell us why he's bothering to patch this code at all???
Probably the math still isn't quite correct, so it ends up setting up the isa pool for no good reason :-(

well either it sleeps or it doesn't.....
if this guy should sleep (and right now it does) we shouldn't call it from such contexts.
If we do the right thing and allocate the isa pools in a sane context, it wouldn't ever sleep and the patch isn't needed...

Arjan van de Ven

author

2008-05-20 20:04 UTC #5 lore raw

In reply to Andrew Morton

Quoted 45 lines · On Tue, 20 May 2008 12:45:56 -0700 Andrew Morton <[email protected]>…

Quoted 10 lines · On Tue, 20 May 2008 21:29:59 +0200 Jens Axboe <[email protected]> wrote:
Quoted 6 lines · Arjan van de Ven
From: Arjan van de Ven <[email protected]>
Subject: [PATCH] block: blk_queue_bounce_limits can actually sleep
blk_queue_bounce_limit can call init_emergency_isa_pool, which does sleeping allocations... document it as such by adding might_sleep() to the driver
Isn't that superflous, as mempool_create() -> kmalloc(..., __GFP_WAIT) ends up spewing that warning anyway?
It's largely superfluous given the way in which Arjan implemented it.
One situation which we regularly hit is:
foo()
{
	...
	if (some_unlikely_condition())
		do_something_which_sleeps();
	...
}
and then we go and call that code under spinlock and ship it out, when of course a handful of testers hit the unlikely condition.
The solution to that is to add a might_sleep() _outside_ the test of some_unlikely_condition(). ie:
block/blk-settings.c renamed from block/blk-settings.c~a +2 -0
@@ -140,6 +140,8 @@ void blk_queue_bounce_limit(struct reque
 	unsigned long b_pfn = dma_addr >> PAGE_SHIFT;
 	int dma = 0;
 
+	might_sleep();
+
 	q->bounce_gfp = GFP_NOIO;
 #if BITS_PER_LONG == 64
 	/* Assume anything <= 4GB can be handled by IOMMU.
_

Quoted 2 lines

but it's all vague and waffly because Arjan forgot to tell us why he's bothering to patch this code at all???

the sata_nv driver calls this from an invalid context ... and spews a ton of warnings as a result... made me think this is a common mistake to make.

I'd love to make it do your version instead, but I was afraid it would trigger too often....