* Preempt & smp_processor_id in __make_request
@ 2011-07-26 9:32 Steven Whitehouse
2011-07-26 10:56 ` Sergey Senozhatsky
2011-07-26 11:43 ` Christoph Hellwig
0 siblings, 2 replies; 5+ messages in thread
From: Steven Whitehouse @ 2011-07-26 9:32 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-kernel
Hi,
On the latest upstream kernel, I'm getting large quantities of messages
as per the following:
Jul 26 09:54:04 chywoon kernel: BUG: using smp_processor_id() in preemptible [00
000000] code: jbd2/dm-0-8/1546
Jul 26 09:54:04 chywoon kernel: caller is __make_request+0x209/0x350
Jul 26 09:54:04 chywoon kernel: Pid: 1546, comm: jbd2/dm-0-8 Tainted: G W
3.0.0+ #252
Jul 26 09:54:04 chywoon kernel: Call Trace:
Jul 26 09:54:04 chywoon kernel: [<ffffffff813db897>] debug_smp_processor_id+0xe7
/0x100
Jul 26 09:54:04 chywoon kernel: [<ffffffff813b6f29>] __make_request+0x209/0x350
Jul 26 09:54:04 chywoon kernel: [<ffffffff8154635e>] ? dm_request+0x2e/0x280
Jul 26 09:54:04 chywoon kernel: [<ffffffff813b3ffb>] generic_make_request+0x27b/
0x550
Jul 26 09:54:04 chywoon kernel: [<ffffffff8123ef7e>] ? jbd2_journal_file_buffer+
0x8e/0x130
Jul 26 09:54:04 chywoon kernel: [<ffffffff813b432f>] submit_bio+0x5f/0xd0
Jul 26 09:54:04 chywoon kernel: [<ffffffff811b14a6>] submit_bh+0xe6/0x120
etc.
The (trivial) fix appears to be the following:
diff --git a/block/blk-core.c b/block/blk-core.c
index f8cb099..f925581 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1283,7 +1283,7 @@ get_rq:
if (test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags) ||
bio_flagged(bio, BIO_CPU_AFFINE))
- req->cpu = smp_processor_id();
+ req->cpu = raw_smp_processor_id();
plug = current->plug;
if (plug) {
However this fixes the symptoms, rather than the cause, so I'm not at
all sure that this is the correct solution,
Steve.
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: Preempt & smp_processor_id in __make_request
2011-07-26 9:32 Preempt & smp_processor_id in __make_request Steven Whitehouse
@ 2011-07-26 10:56 ` Sergey Senozhatsky
2011-07-26 16:15 ` Williams, Dan J
2011-07-26 11:43 ` Christoph Hellwig
1 sibling, 1 reply; 5+ messages in thread
From: Sergey Senozhatsky @ 2011-07-26 10:56 UTC (permalink / raw)
To: Steven Whitehouse
Cc: Jens Axboe, linux-kernel, Christoph Hellwig, Dan Williams,
Roland Dreier
On (07/26/11 10:32), Steven Whitehouse wrote:
> Jul 26 09:54:04 chywoon kernel: BUG: using smp_processor_id() in preemptible [00
> 000000] code: jbd2/dm-0-8/1546
> Jul 26 09:54:04 chywoon kernel: caller is __make_request+0x209/0x350
> Jul 26 09:54:04 chywoon kernel: Pid: 1546, comm: jbd2/dm-0-8 Tainted: G W
> 3.0.0+ #252
> Jul 26 09:54:04 chywoon kernel: Call Trace:
> Jul 26 09:54:04 chywoon kernel: [<ffffffff813db897>] debug_smp_processor_id+0xe7
> /0x100
> Jul 26 09:54:04 chywoon kernel: [<ffffffff813b6f29>] __make_request+0x209/0x350
> Jul 26 09:54:04 chywoon kernel: [<ffffffff8154635e>] ? dm_request+0x2e/0x280
> Jul 26 09:54:04 chywoon kernel: [<ffffffff813b3ffb>] generic_make_request+0x27b/
> 0x550
> Jul 26 09:54:04 chywoon kernel: [<ffffffff8123ef7e>] ? jbd2_journal_file_buffer+
> 0x8e/0x130
> Jul 26 09:54:04 chywoon kernel: [<ffffffff813b432f>] submit_bio+0x5f/0xd0
> Jul 26 09:54:04 chywoon kernel: [<ffffffff811b14a6>] submit_bh+0xe6/0x120
> etc.
>
> The (trivial) fix appears to be the following:
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index f8cb099..f925581 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1283,7 +1283,7 @@ get_rq:
>
> if (test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags) ||
> bio_flagged(bio, BIO_CPU_AFFINE))
> - req->cpu = smp_processor_id();
> + req->cpu = raw_smp_processor_id();
>
> plug = current->plug;
> if (plug) {
>
> However this fixes the symptoms, rather than the cause, so I'm not at
> all sure that this is the correct solution,
>
Or we can switch back to get_cpu()/put_cpu() pair as it's been prior to
commit 5757a6d76cdf6dda2a492c09b985c015e86779b1
Author: Dan Williams <dan.j.williams@intel.com>
block: strict rq_affinity
Some systems benefit from completions always being steered to the strict
requester cpu rather than the looser "per-socket" steering that
blk_cpu_to_group() attempts by default. This is because the first
CPU in the group mask ends up being completely overloaded with work,
while the others (including the original submitter) has power left
to spare.
that changed CPU affinity logic from using blk_cpu_to_group(get_cpu()) to
smp_processor_id():
[ 573.599424] BUG: using smp_processor_id() in preemptible [00000000]
[ 573.599429] caller is __make_request+0x19c/0x344
[ 573.599437] Call Trace:
[ 573.599443] [<ffffffff81249a5f>] debug_smp_processor_id+0xc7/0xe0
[ 573.599449] [<ffffffff812298bc>] __make_request+0x19c/0x344
[ 573.599455] [<ffffffff81227873>] generic_make_request+0x4b8/0x60f
[ 573.599462] [<ffffffff81227aa9>] submit_bio+0xdf/0xfe
[ 573.599467] [<ffffffff811352d1>] ? bio_alloc_bioset+0x47/0xbe
[ 573.599473] [<ffffffff81130886>] submit_bh+0xda/0xf9
[ 573.599480] [<ffffffff811efcc0>] jbd2_journal_commit_transaction+0xb8f/0x1963
[ 573.599486] [<ffffffff8104cb00>] ? lock_timer_base.isra.30+0x26/0x4b
[ 573.599493] [<ffffffff8104cc9c>] ? try_to_del_timer_sync+0x177/0x177
[ 573.599499] [<ffffffff811f46b0>] kjournald2+0xce/0x215
[ 573.599505] [<ffffffff8105d91a>] ? __init_waitqueue_head+0x46/0x46
[ 573.599510] [<ffffffff811f45e2>] ? commit_timeout+0xb/0xb
[ 573.599516] [<ffffffff8105d0ce>] kthread+0x9a/0xa2
[ 573.599522] [<ffffffff81487824>] kernel_thread_helper+0x4/0x10
[ 573.599528] [<ffffffff8102d4d1>] ? finish_task_switch+0x76/0xf0
[ 573.599533] [<ffffffff814805b8>] ? retint_restore_args+0x13/0x13
[ 573.599540] [<ffffffff8105d034>] ? __init_kthread_worker+0x53/0x53
[ 573.599545] [<ffffffff81487820>] ? gs_change+0x13/0x13
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
block/blk-core.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index f8cb099..7e98677 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1282,8 +1282,10 @@ get_rq:
init_request_from_bio(req, bio);
if (test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags) ||
- bio_flagged(bio, BIO_CPU_AFFINE))
- req->cpu = smp_processor_id();
+ bio_flagged(bio, BIO_CPU_AFFINE)) {
+ req->cpu = get_cpu();
+ put_cpu();
+ }
plug = current->plug;
if (plug) {
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: Preempt & smp_processor_id in __make_request
2011-07-26 10:56 ` Sergey Senozhatsky
@ 2011-07-26 16:15 ` Williams, Dan J
0 siblings, 0 replies; 5+ messages in thread
From: Williams, Dan J @ 2011-07-26 16:15 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Steven Whitehouse, Jens Axboe, linux-kernel, Christoph Hellwig,
Roland Dreier
On Tue, Jul 26, 2011 at 3:56 AM, Sergey Senozhatsky
<sergey.senozhatsky@gmail.com> wrote:
> On (07/26/11 10:32), Steven Whitehouse wrote:
>> Jul 26 09:54:04 chywoon kernel: BUG: using smp_processor_id() in preemptible [00
>> 000000] code: jbd2/dm-0-8/1546
>> Jul 26 09:54:04 chywoon kernel: caller is __make_request+0x209/0x350
>> Jul 26 09:54:04 chywoon kernel: Pid: 1546, comm: jbd2/dm-0-8 Tainted: G W
>> 3.0.0+ #252
>> Jul 26 09:54:04 chywoon kernel: Call Trace:
>> Jul 26 09:54:04 chywoon kernel: [<ffffffff813db897>] debug_smp_processor_id+0xe7
>> /0x100
>> Jul 26 09:54:04 chywoon kernel: [<ffffffff813b6f29>] __make_request+0x209/0x350
>> Jul 26 09:54:04 chywoon kernel: [<ffffffff8154635e>] ? dm_request+0x2e/0x280
>> Jul 26 09:54:04 chywoon kernel: [<ffffffff813b3ffb>] generic_make_request+0x27b/
>> 0x550
>> Jul 26 09:54:04 chywoon kernel: [<ffffffff8123ef7e>] ? jbd2_journal_file_buffer+
>> 0x8e/0x130
>> Jul 26 09:54:04 chywoon kernel: [<ffffffff813b432f>] submit_bio+0x5f/0xd0
>> Jul 26 09:54:04 chywoon kernel: [<ffffffff811b14a6>] submit_bh+0xe6/0x120
>> etc.
>>
>> The (trivial) fix appears to be the following:
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index f8cb099..f925581 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1283,7 +1283,7 @@ get_rq:
>>
>> if (test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags) ||
>> bio_flagged(bio, BIO_CPU_AFFINE))
>> - req->cpu = smp_processor_id();
>> + req->cpu = raw_smp_processor_id();
>>
>> plug = current->plug;
>> if (plug) {
>>
>> However this fixes the symptoms, rather than the cause, so I'm not at
>> all sure that this is the correct solution,
I did not realize smp_processor_id() enforced that restriction. It
seems like we want raw_smp_processor_id() since it is an affinity
hint, and not an affinity guarantee.
--
Dan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Preempt & smp_processor_id in __make_request
2011-07-26 9:32 Preempt & smp_processor_id in __make_request Steven Whitehouse
2011-07-26 10:56 ` Sergey Senozhatsky
@ 2011-07-26 11:43 ` Christoph Hellwig
2011-07-26 13:05 ` Jens Axboe
1 sibling, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2011-07-26 11:43 UTC (permalink / raw)
To: Steven Whitehouse; +Cc: Jens Axboe, linux-kernel
On Tue, Jul 26, 2011 at 10:32:06AM +0100, Steven Whitehouse wrote:
> diff --git a/block/blk-core.c b/block/blk-core.c
> index f8cb099..f925581 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1283,7 +1283,7 @@ get_rq:
>
> if (test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags) ||
> bio_flagged(bio, BIO_CPU_AFFINE))
> - req->cpu = smp_processor_id();
> + req->cpu = raw_smp_processor_id();
>
> plug = current->plug;
> if (plug) {
>
> However this fixes the symptoms, rather than the cause, so I'm not at
> all sure that this is the correct solution,
It doesn't fix the symptoms - the warning is one of those 98% percent
right types of warnings. In this case get_cpu/put_cpu doesn't buy you
anthing - we want to stash away the cpu id that we submitted the I/O
from, to compare it when we later process the I/O completion from
a different context. With some PREEMPT options we might actually get
rescheduled to a different CPU during the rest of this function, but
as we submit the plugged requests at this point we'll still be correct.
Even if wasn't we would already thrash the cache badly enough for this
optimization not to matter in that case.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: Preempt & smp_processor_id in __make_request
2011-07-26 11:43 ` Christoph Hellwig
@ 2011-07-26 13:05 ` Jens Axboe
0 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2011-07-26 13:05 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Steven Whitehouse, linux-kernel
On 2011-07-26 13:43, Christoph Hellwig wrote:
> On Tue, Jul 26, 2011 at 10:32:06AM +0100, Steven Whitehouse wrote:
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index f8cb099..f925581 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1283,7 +1283,7 @@ get_rq:
>>
>> if (test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags) ||
>> bio_flagged(bio, BIO_CPU_AFFINE))
>> - req->cpu = smp_processor_id();
>> + req->cpu = raw_smp_processor_id();
>>
>> plug = current->plug;
>> if (plug) {
>>
>> However this fixes the symptoms, rather than the cause, so I'm not at
>> all sure that this is the correct solution,
>
> It doesn't fix the symptoms - the warning is one of those 98% percent
> right types of warnings. In this case get_cpu/put_cpu doesn't buy you
> anthing - we want to stash away the cpu id that we submitted the I/O
> from, to compare it when we later process the I/O completion from
> a different context. With some PREEMPT options we might actually get
> rescheduled to a different CPU during the rest of this function, but
> as we submit the plugged requests at this point we'll still be correct.
>
> Even if wasn't we would already thrash the cache badly enough for this
> optimization not to matter in that case.
This was reported earlier today as well, and I suggested the same fix.
raw_smp_processor_id() is fine, since it's just a hint.
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-07-26 16:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-26 9:32 Preempt & smp_processor_id in __make_request Steven Whitehouse
2011-07-26 10:56 ` Sergey Senozhatsky
2011-07-26 16:15 ` Williams, Dan J
2011-07-26 11:43 ` Christoph Hellwig
2011-07-26 13:05 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox