Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1457161 > unrolled thread

Re: bug: using smp_processor_id() in preemptible code in rr_select_path()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: bug: using smp_processor_id() in preemptible code in  rr_select_path() Mike Snitzer <snitzer@redhat.com> - 2016-08-05 17:30 +0200
    Re: bug: using smp_processor_id() in preemptible code in  rr_select_path() Jens Axboe <axboe@kernel.dk> - 2016-08-05 17:40 +0200
      Re: bug: using smp_processor_id() in preemptible code in  rr_select_path() Mike Snitzer <snitzer@redhat.com> - 2016-08-05 17:50 +0200
        Re: bug: using smp_processor_id() in preemptible code in  rr_select_path() Jens Axboe <axboe@kernel.dk> - 2016-08-05 18:00 +0200
          Re: bug: using smp_processor_id() in preemptible code in  rr_select_path() Mike Snitzer <snitzer@redhat.com> - 2016-08-05 18:10 +0200
            Re: bug: using smp_processor_id() in preemptible code in  rr_select_path() Jens Axboe <axboe@kernel.dk> - 2016-08-05 18:20 +0200
            Re: bug: using smp_processor_id() in preemptible code in  rr_select_path() Benjamin Block <bblock@linux.vnet.ibm.com> - 2016-08-07 17:50 +0200
            Re: [dm-devel] bug: using smp_processor_id() in preemptible code in  rr_select_path() Jens Axboe <axboe@kernel.dk> - 2016-08-08 18:40 +0200
              Re: [dm-devel] bug: using smp_processor_id() in preemptible code in  rr_select_path() Benjamin Block <bblock@linux.vnet.ibm.com> - 2016-08-08 19:10 +0200
            Re: [dm-devel] bug: using smp_processor_id() in preemptible code in  rr_select_path() Benjamin Block <bblock@linux.vnet.ibm.com> - 2016-08-08 18:40 +0200

#1457161 — Re: bug: using smp_processor_id() in preemptible code in rr_select_path()

On Wed, Aug 03 2016 at 11:35am -0400,
Benjamin Block <bblock@linux.vnet.ibm.com> wrote:

> Hej Mike,
> 
> when running a debug-kernel today with several multipath-devices using
> the round-robin path selector I noticed that the kernel throws these
> warnings here:
> 
> BUG: using smp_processor_id() in preemptible [00000000] code: kdmwork-252:0/881
> caller is rr_select_path+0x36/0x108 [dm_round_robin]
> CPU: 1 PID: 881 Comm: kdmwork-252:0 Not tainted 4.7.0-debug #4
>        00000000617679b8 0000000061767a48 0000000000000002 0000000000000000
>        0000000061767ae8 0000000061767a60 0000000061767a60 00000000001145d0
>        0000000000000000 0000000000b962ae 0000000000bb291e 000000000000000b
>        0000000061767aa8 0000000061767a48 0000000000000000 0000000000000000
>        0700000000b962ae 00000000001145d0 0000000061767a48 0000000061767aa8
> Call Trace:
> ([<00000000001144a2>] show_trace+0x8a/0xe0)
> ([<0000000000114586>] show_stack+0x8e/0xf0)
> ([<00000000006c7fdc>] dump_stack+0x9c/0xe0)
> ([<00000000006fbbc0>] check_preemption_disabled+0x108/0x130)
> ([<000003ff80268646>] rr_select_path+0x36/0x108 [dm_round_robin])
> ([<000003ff80259a42>] choose_path_in_pg+0x42/0xc8 [dm_multipath])
> ([<000003ff80259b62>] choose_pgpath+0x9a/0x1a0 [dm_multipath])
> ([<000003ff8025b51a>] __multipath_map.isra.5+0x72/0x228 [dm_multipath])
> ([<000003ff8025b75e>] multipath_map+0x3e/0x50 [dm_multipath])
> ([<000003ff80225eb6>] map_request+0x66/0x458 [dm_mod])
> ([<000003ff802262ec>] map_tio_request+0x44/0x70 [dm_mod])
> ([<000000000016835a>] kthread_worker_fn+0xf2/0x1d8)
> ([<00000000001681da>] kthread+0x112/0x120)
> ([<000000000098378a>] kernel_thread_starter+0x6/0xc)
> ([<0000000000983784>] kernel_thread_starter+0x0/0xc)
> no locks held by kdmwork-252:0/881.
> 
> 
> There are several more of these warnings, but all have the same
> stack-trace (this is on s390x, but this looks like its only common code)
> - sometimes the process-context is multipath.
> 
> Looking at the changes in this function, it rather looks like this is
> caused by changes made in commit
> b0b477c7e0dd93f8916d106018ded1331b81bf61 (dm round robin: use percpu
> 'repeat_count' and 'current_path').
> 
> The kernel is a stock v4.7 with some debug options enabled (prominently
> DEBUG_PREEMPT). Need any more info?

As you can see from commit b0b477c7e0dd9 the round-robin path selector
is now using percpu data (pointer) and a percpu_counter.

I'm really not sure how else to access this percpu data.

Cc'ing LKML, linux-block, Jens and hch to cast a wider net in the hopes
of getting an answer to how to fix this.

[toc] | [next] | [standalone]

#1457169

On 08/05/2016 09:27 AM, Mike Snitzer wrote:
> On Wed, Aug 03 2016 at 11:35am -0400,
> Benjamin Block <bblock@linux.vnet.ibm.com> wrote:
>
>> Hej Mike,
>>
>> when running a debug-kernel today with several multipath-devices using
>> the round-robin path selector I noticed that the kernel throws these
>> warnings here:
>>
>> BUG: using smp_processor_id() in preemptible [00000000] code: kdmwork-252:0/881
>> caller is rr_select_path+0x36/0x108 [dm_round_robin]
>> CPU: 1 PID: 881 Comm: kdmwork-252:0 Not tainted 4.7.0-debug #4
>>        00000000617679b8 0000000061767a48 0000000000000002 0000000000000000
>>        0000000061767ae8 0000000061767a60 0000000061767a60 00000000001145d0
>>        0000000000000000 0000000000b962ae 0000000000bb291e 000000000000000b
>>        0000000061767aa8 0000000061767a48 0000000000000000 0000000000000000
>>        0700000000b962ae 00000000001145d0 0000000061767a48 0000000061767aa8
>> Call Trace:
>> ([<00000000001144a2>] show_trace+0x8a/0xe0)
>> ([<0000000000114586>] show_stack+0x8e/0xf0)
>> ([<00000000006c7fdc>] dump_stack+0x9c/0xe0)
>> ([<00000000006fbbc0>] check_preemption_disabled+0x108/0x130)
>> ([<000003ff80268646>] rr_select_path+0x36/0x108 [dm_round_robin])
>> ([<000003ff80259a42>] choose_path_in_pg+0x42/0xc8 [dm_multipath])
>> ([<000003ff80259b62>] choose_pgpath+0x9a/0x1a0 [dm_multipath])
>> ([<000003ff8025b51a>] __multipath_map.isra.5+0x72/0x228 [dm_multipath])
>> ([<000003ff8025b75e>] multipath_map+0x3e/0x50 [dm_multipath])
>> ([<000003ff80225eb6>] map_request+0x66/0x458 [dm_mod])
>> ([<000003ff802262ec>] map_tio_request+0x44/0x70 [dm_mod])
>> ([<000000000016835a>] kthread_worker_fn+0xf2/0x1d8)
>> ([<00000000001681da>] kthread+0x112/0x120)
>> ([<000000000098378a>] kernel_thread_starter+0x6/0xc)
>> ([<0000000000983784>] kernel_thread_starter+0x0/0xc)
>> no locks held by kdmwork-252:0/881.
>>
>>
>> There are several more of these warnings, but all have the same
>> stack-trace (this is on s390x, but this looks like its only common code)
>> - sometimes the process-context is multipath.
>>
>> Looking at the changes in this function, it rather looks like this is
>> caused by changes made in commit
>> b0b477c7e0dd93f8916d106018ded1331b81bf61 (dm round robin: use percpu
>> 'repeat_count' and 'current_path').
>>
>> The kernel is a stock v4.7 with some debug options enabled (prominently
>> DEBUG_PREEMPT). Need any more info?
>
> As you can see from commit b0b477c7e0dd9 the round-robin path selector
> is now using percpu data (pointer) and a percpu_counter.
>
> I'm really not sure how else to access this percpu data.
>
> Cc'ing LKML, linux-block, Jens and hch to cast a wider net in the hopes
> of getting an answer to how to fix this.

 From a quick look, looks like you are using this_cpu_ptr() without
having preemption disabled.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1457183

On Fri, Aug 05 2016 at 11:33P -0400,
Jens Axboe <axboe@kernel.dk> wrote:

> On 08/05/2016 09:27 AM, Mike Snitzer wrote:
> >On Wed, Aug 03 2016 at 11:35am -0400,
> >Benjamin Block <bblock@linux.vnet.ibm.com> wrote:
> >
> >>Hej Mike,
> >>
> >>when running a debug-kernel today with several multipath-devices using
> >>the round-robin path selector I noticed that the kernel throws these
> >>warnings here:
> >>
> >>BUG: using smp_processor_id() in preemptible [00000000] code: kdmwork-252:0/881
> >>caller is rr_select_path+0x36/0x108 [dm_round_robin]
> >>CPU: 1 PID: 881 Comm: kdmwork-252:0 Not tainted 4.7.0-debug #4
> >>       00000000617679b8 0000000061767a48 0000000000000002 0000000000000000
> >>       0000000061767ae8 0000000061767a60 0000000061767a60 00000000001145d0
> >>       0000000000000000 0000000000b962ae 0000000000bb291e 000000000000000b
> >>       0000000061767aa8 0000000061767a48 0000000000000000 0000000000000000
> >>       0700000000b962ae 00000000001145d0 0000000061767a48 0000000061767aa8
> >>Call Trace:
> >>([<00000000001144a2>] show_trace+0x8a/0xe0)
> >>([<0000000000114586>] show_stack+0x8e/0xf0)
> >>([<00000000006c7fdc>] dump_stack+0x9c/0xe0)
> >>([<00000000006fbbc0>] check_preemption_disabled+0x108/0x130)
> >>([<000003ff80268646>] rr_select_path+0x36/0x108 [dm_round_robin])
> >>([<000003ff80259a42>] choose_path_in_pg+0x42/0xc8 [dm_multipath])
> >>([<000003ff80259b62>] choose_pgpath+0x9a/0x1a0 [dm_multipath])
> >>([<000003ff8025b51a>] __multipath_map.isra.5+0x72/0x228 [dm_multipath])
> >>([<000003ff8025b75e>] multipath_map+0x3e/0x50 [dm_multipath])
> >>([<000003ff80225eb6>] map_request+0x66/0x458 [dm_mod])
> >>([<000003ff802262ec>] map_tio_request+0x44/0x70 [dm_mod])
> >>([<000000000016835a>] kthread_worker_fn+0xf2/0x1d8)
> >>([<00000000001681da>] kthread+0x112/0x120)
> >>([<000000000098378a>] kernel_thread_starter+0x6/0xc)
> >>([<0000000000983784>] kernel_thread_starter+0x0/0xc)
> >>no locks held by kdmwork-252:0/881.
> >>
> >>
> >>There are several more of these warnings, but all have the same
> >>stack-trace (this is on s390x, but this looks like its only common code)
> >>- sometimes the process-context is multipath.
> >>
> >>Looking at the changes in this function, it rather looks like this is
> >>caused by changes made in commit
> >>b0b477c7e0dd93f8916d106018ded1331b81bf61 (dm round robin: use percpu
> >>'repeat_count' and 'current_path').
> >>
> >>The kernel is a stock v4.7 with some debug options enabled (prominently
> >>DEBUG_PREEMPT). Need any more info?
> >
> >As you can see from commit b0b477c7e0dd9 the round-robin path selector
> >is now using percpu data (pointer) and a percpu_counter.
> >
> >I'm really not sure how else to access this percpu data.
> >
> >Cc'ing LKML, linux-block, Jens and hch to cast a wider net in the hopes
> >of getting an answer to how to fix this.
> 
> From a quick look, looks like you are using this_cpu_ptr() without
> having preemption disabled.

Right, that is what it looked like to me too.

I'm just not sure on what the proper pattern is to fix this.

I'll look closer though.

[toc] | [prev] | [next] | [standalone]

#1457194

On 08/05/2016 09:42 AM, Mike Snitzer wrote:
> On Fri, Aug 05 2016 at 11:33P -0400,
> Jens Axboe <axboe@kernel.dk> wrote:
>
>> On 08/05/2016 09:27 AM, Mike Snitzer wrote:
>>> On Wed, Aug 03 2016 at 11:35am -0400,
>>> Benjamin Block <bblock@linux.vnet.ibm.com> wrote:
>>>
>>>> Hej Mike,
>>>>
>>>> when running a debug-kernel today with several multipath-devices using
>>>> the round-robin path selector I noticed that the kernel throws these
>>>> warnings here:
>>>>
>>>> BUG: using smp_processor_id() in preemptible [00000000] code: kdmwork-252:0/881
>>>> caller is rr_select_path+0x36/0x108 [dm_round_robin]
>>>> CPU: 1 PID: 881 Comm: kdmwork-252:0 Not tainted 4.7.0-debug #4
>>>>       00000000617679b8 0000000061767a48 0000000000000002 0000000000000000
>>>>       0000000061767ae8 0000000061767a60 0000000061767a60 00000000001145d0
>>>>       0000000000000000 0000000000b962ae 0000000000bb291e 000000000000000b
>>>>       0000000061767aa8 0000000061767a48 0000000000000000 0000000000000000
>>>>       0700000000b962ae 00000000001145d0 0000000061767a48 0000000061767aa8
>>>> Call Trace:
>>>> ([<00000000001144a2>] show_trace+0x8a/0xe0)
>>>> ([<0000000000114586>] show_stack+0x8e/0xf0)
>>>> ([<00000000006c7fdc>] dump_stack+0x9c/0xe0)
>>>> ([<00000000006fbbc0>] check_preemption_disabled+0x108/0x130)
>>>> ([<000003ff80268646>] rr_select_path+0x36/0x108 [dm_round_robin])
>>>> ([<000003ff80259a42>] choose_path_in_pg+0x42/0xc8 [dm_multipath])
>>>> ([<000003ff80259b62>] choose_pgpath+0x9a/0x1a0 [dm_multipath])
>>>> ([<000003ff8025b51a>] __multipath_map.isra.5+0x72/0x228 [dm_multipath])
>>>> ([<000003ff8025b75e>] multipath_map+0x3e/0x50 [dm_multipath])
>>>> ([<000003ff80225eb6>] map_request+0x66/0x458 [dm_mod])
>>>> ([<000003ff802262ec>] map_tio_request+0x44/0x70 [dm_mod])
>>>> ([<000000000016835a>] kthread_worker_fn+0xf2/0x1d8)
>>>> ([<00000000001681da>] kthread+0x112/0x120)
>>>> ([<000000000098378a>] kernel_thread_starter+0x6/0xc)
>>>> ([<0000000000983784>] kernel_thread_starter+0x0/0xc)
>>>> no locks held by kdmwork-252:0/881.
>>>>
>>>>
>>>> There are several more of these warnings, but all have the same
>>>> stack-trace (this is on s390x, but this looks like its only common code)
>>>> - sometimes the process-context is multipath.
>>>>
>>>> Looking at the changes in this function, it rather looks like this is
>>>> caused by changes made in commit
>>>> b0b477c7e0dd93f8916d106018ded1331b81bf61 (dm round robin: use percpu
>>>> 'repeat_count' and 'current_path').
>>>>
>>>> The kernel is a stock v4.7 with some debug options enabled (prominently
>>>> DEBUG_PREEMPT). Need any more info?
>>>
>>> As you can see from commit b0b477c7e0dd9 the round-robin path selector
>>> is now using percpu data (pointer) and a percpu_counter.
>>>
>>> I'm really not sure how else to access this percpu data.
>>>
>>> Cc'ing LKML, linux-block, Jens and hch to cast a wider net in the hopes
>>> of getting an answer to how to fix this.
>>
>> From a quick look, looks like you are using this_cpu_ptr() without
>> having preemption disabled.
>
> Right, that is what it looked like to me too.
>
> I'm just not sure on what the proper pattern is to fix this.
>
> I'll look closer though.

I always forget the details (if this confuses lockdep or not), but you
could potentially turn it into:

local_irq_save(flags);
x = this_cpu_ptr();
[...]

spin_lock(&s->lock);
[...]

instead.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1457196

On Fri, Aug 05 2016 at 11:54am -0400,
Jens Axboe <axboe@kernel.dk> wrote:

> On 08/05/2016 09:42 AM, Mike Snitzer wrote:
> >On Fri, Aug 05 2016 at 11:33P -0400,
> >Jens Axboe <axboe@kernel.dk> wrote:
> >
> >>On 08/05/2016 09:27 AM, Mike Snitzer wrote:
> >>>On Wed, Aug 03 2016 at 11:35am -0400,
> >>>Benjamin Block <bblock@linux.vnet.ibm.com> wrote:
> >>>
> >>>>Hej Mike,
> >>>>
> >>>>when running a debug-kernel today with several multipath-devices using
> >>>>the round-robin path selector I noticed that the kernel throws these
> >>>>warnings here:
> >>>>
> >>>>BUG: using smp_processor_id() in preemptible [00000000] code: kdmwork-252:0/881
> >>>>caller is rr_select_path+0x36/0x108 [dm_round_robin]
> >>>>CPU: 1 PID: 881 Comm: kdmwork-252:0 Not tainted 4.7.0-debug #4
> >>>>      00000000617679b8 0000000061767a48 0000000000000002 0000000000000000
> >>>>      0000000061767ae8 0000000061767a60 0000000061767a60 00000000001145d0
> >>>>      0000000000000000 0000000000b962ae 0000000000bb291e 000000000000000b
> >>>>      0000000061767aa8 0000000061767a48 0000000000000000 0000000000000000
> >>>>      0700000000b962ae 00000000001145d0 0000000061767a48 0000000061767aa8
> >>>>Call Trace:
> >>>>([<00000000001144a2>] show_trace+0x8a/0xe0)
> >>>>([<0000000000114586>] show_stack+0x8e/0xf0)
> >>>>([<00000000006c7fdc>] dump_stack+0x9c/0xe0)
> >>>>([<00000000006fbbc0>] check_preemption_disabled+0x108/0x130)
> >>>>([<000003ff80268646>] rr_select_path+0x36/0x108 [dm_round_robin])
> >>>>([<000003ff80259a42>] choose_path_in_pg+0x42/0xc8 [dm_multipath])
> >>>>([<000003ff80259b62>] choose_pgpath+0x9a/0x1a0 [dm_multipath])
> >>>>([<000003ff8025b51a>] __multipath_map.isra.5+0x72/0x228 [dm_multipath])
> >>>>([<000003ff8025b75e>] multipath_map+0x3e/0x50 [dm_multipath])
> >>>>([<000003ff80225eb6>] map_request+0x66/0x458 [dm_mod])
> >>>>([<000003ff802262ec>] map_tio_request+0x44/0x70 [dm_mod])
> >>>>([<000000000016835a>] kthread_worker_fn+0xf2/0x1d8)
> >>>>([<00000000001681da>] kthread+0x112/0x120)
> >>>>([<000000000098378a>] kernel_thread_starter+0x6/0xc)
> >>>>([<0000000000983784>] kernel_thread_starter+0x0/0xc)
> >>>>no locks held by kdmwork-252:0/881.
> >>>>
> >>>>
> >>>>There are several more of these warnings, but all have the same
> >>>>stack-trace (this is on s390x, but this looks like its only common code)
> >>>>- sometimes the process-context is multipath.
> >>>>
> >>>>Looking at the changes in this function, it rather looks like this is
> >>>>caused by changes made in commit
> >>>>b0b477c7e0dd93f8916d106018ded1331b81bf61 (dm round robin: use percpu
> >>>>'repeat_count' and 'current_path').
> >>>>
> >>>>The kernel is a stock v4.7 with some debug options enabled (prominently
> >>>>DEBUG_PREEMPT). Need any more info?
> >>>
> >>>As you can see from commit b0b477c7e0dd9 the round-robin path selector
> >>>is now using percpu data (pointer) and a percpu_counter.
> >>>
> >>>I'm really not sure how else to access this percpu data.
> >>>
> >>>Cc'ing LKML, linux-block, Jens and hch to cast a wider net in the hopes
> >>>of getting an answer to how to fix this.
> >>
> >>From a quick look, looks like you are using this_cpu_ptr() without
> >>having preemption disabled.
> >
> >Right, that is what it looked like to me too.
> >
> >I'm just not sure on what the proper pattern is to fix this.
> >
> >I'll look closer though.
> 
> I always forget the details (if this confuses lockdep or not), but you
> could potentially turn it into:
> 
> local_irq_save(flags);
> x = this_cpu_ptr();
> [...]
> 
> spin_lock(&s->lock);
> [...]
> 
> instead.

Cool, I've coded up the patch (compile tested only).

Benjamin, any chance you could test this against your v4.7 kernel and
report back?

Thanks,
Mike

diff --git a/drivers/md/dm-round-robin.c b/drivers/md/dm-round-robin.c
index 4ace1da..ed446f8 100644
--- a/drivers/md/dm-round-robin.c
+++ b/drivers/md/dm-round-robin.c
@@ -210,14 +210,17 @@ static struct dm_path *rr_select_path(struct path_selector *ps, size_t nr_bytes)
 	struct path_info *pi = NULL;
 	struct dm_path *current_path = NULL;
 
+	local_irq_save(flags);
 	current_path = *this_cpu_ptr(s->current_path);
 	if (current_path) {
 		percpu_counter_dec(&s->repeat_count);
-		if (percpu_counter_read_positive(&s->repeat_count) > 0)
+		if (percpu_counter_read_positive(&s->repeat_count) > 0) {
+			local_irq_restore(flags);
 			return current_path;
+		}
 	}
 
-	spin_lock_irqsave(&s->lock, flags);
+	spin_lock(&s->lock);
 	if (!list_empty(&s->valid_paths)) {
 		pi = list_entry(s->valid_paths.next, struct path_info, list);
 		list_move_tail(&pi->list, &s->valid_paths);
@@ -225,7 +228,8 @@ static struct dm_path *rr_select_path(struct path_selector *ps, size_t nr_bytes)
 		set_percpu_current_path(s, pi->path);
 		current_path = pi->path;
 	}
-	spin_unlock_irqrestore(&s->lock, flags);
+	spin_unlock(&s->lock);
+	local_irq_restore(flags);
 
 	return current_path;
 }

[toc] | [prev] | [next] | [standalone]

#1457200

On 08/05/2016 10:06 AM, Mike Snitzer wrote:
> On Fri, Aug 05 2016 at 11:54am -0400,
> Jens Axboe <axboe@kernel.dk> wrote:
>
>> On 08/05/2016 09:42 AM, Mike Snitzer wrote:
>>> On Fri, Aug 05 2016 at 11:33P -0400,
>>> Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>>> On 08/05/2016 09:27 AM, Mike Snitzer wrote:
>>>>> On Wed, Aug 03 2016 at 11:35am -0400,
>>>>> Benjamin Block <bblock@linux.vnet.ibm.com> wrote:
>>>>>
>>>>>> Hej Mike,
>>>>>>
>>>>>> when running a debug-kernel today with several multipath-devices using
>>>>>> the round-robin path selector I noticed that the kernel throws these
>>>>>> warnings here:
>>>>>>
>>>>>> BUG: using smp_processor_id() in preemptible [00000000] code: kdmwork-252:0/881
>>>>>> caller is rr_select_path+0x36/0x108 [dm_round_robin]
>>>>>> CPU: 1 PID: 881 Comm: kdmwork-252:0 Not tainted 4.7.0-debug #4
>>>>>>      00000000617679b8 0000000061767a48 0000000000000002 0000000000000000
>>>>>>      0000000061767ae8 0000000061767a60 0000000061767a60 00000000001145d0
>>>>>>      0000000000000000 0000000000b962ae 0000000000bb291e 000000000000000b
>>>>>>      0000000061767aa8 0000000061767a48 0000000000000000 0000000000000000
>>>>>>      0700000000b962ae 00000000001145d0 0000000061767a48 0000000061767aa8
>>>>>> Call Trace:
>>>>>> ([<00000000001144a2>] show_trace+0x8a/0xe0)
>>>>>> ([<0000000000114586>] show_stack+0x8e/0xf0)
>>>>>> ([<00000000006c7fdc>] dump_stack+0x9c/0xe0)
>>>>>> ([<00000000006fbbc0>] check_preemption_disabled+0x108/0x130)
>>>>>> ([<000003ff80268646>] rr_select_path+0x36/0x108 [dm_round_robin])
>>>>>> ([<000003ff80259a42>] choose_path_in_pg+0x42/0xc8 [dm_multipath])
>>>>>> ([<000003ff80259b62>] choose_pgpath+0x9a/0x1a0 [dm_multipath])
>>>>>> ([<000003ff8025b51a>] __multipath_map.isra.5+0x72/0x228 [dm_multipath])
>>>>>> ([<000003ff8025b75e>] multipath_map+0x3e/0x50 [dm_multipath])
>>>>>> ([<000003ff80225eb6>] map_request+0x66/0x458 [dm_mod])
>>>>>> ([<000003ff802262ec>] map_tio_request+0x44/0x70 [dm_mod])
>>>>>> ([<000000000016835a>] kthread_worker_fn+0xf2/0x1d8)
>>>>>> ([<00000000001681da>] kthread+0x112/0x120)
>>>>>> ([<000000000098378a>] kernel_thread_starter+0x6/0xc)
>>>>>> ([<0000000000983784>] kernel_thread_starter+0x0/0xc)
>>>>>> no locks held by kdmwork-252:0/881.
>>>>>>
>>>>>>
>>>>>> There are several more of these warnings, but all have the same
>>>>>> stack-trace (this is on s390x, but this looks like its only common code)
>>>>>> - sometimes the process-context is multipath.
>>>>>>
>>>>>> Looking at the changes in this function, it rather looks like this is
>>>>>> caused by changes made in commit
>>>>>> b0b477c7e0dd93f8916d106018ded1331b81bf61 (dm round robin: use percpu
>>>>>> 'repeat_count' and 'current_path').
>>>>>>
>>>>>> The kernel is a stock v4.7 with some debug options enabled (prominently
>>>>>> DEBUG_PREEMPT). Need any more info?
>>>>>
>>>>> As you can see from commit b0b477c7e0dd9 the round-robin path selector
>>>>> is now using percpu data (pointer) and a percpu_counter.
>>>>>
>>>>> I'm really not sure how else to access this percpu data.
>>>>>
>>>>> Cc'ing LKML, linux-block, Jens and hch to cast a wider net in the hopes
>>>>> of getting an answer to how to fix this.
>>>>
>>> >From a quick look, looks like you are using this_cpu_ptr() without
>>>> having preemption disabled.
>>>
>>> Right, that is what it looked like to me too.
>>>
>>> I'm just not sure on what the proper pattern is to fix this.
>>>
>>> I'll look closer though.
>>
>> I always forget the details (if this confuses lockdep or not), but you
>> could potentially turn it into:
>>
>> local_irq_save(flags);
>> x = this_cpu_ptr();
>> [...]
>>
>> spin_lock(&s->lock);
>> [...]
>>
>> instead.
>
> Cool, I've coded up the patch (compile tested only).
>
> Benjamin, any chance you could test this against your v4.7 kernel and
> report back?
>
> Thanks,
> Mike
>
> diff --git a/drivers/md/dm-round-robin.c b/drivers/md/dm-round-robin.c
> index 4ace1da..ed446f8 100644
> --- a/drivers/md/dm-round-robin.c
> +++ b/drivers/md/dm-round-robin.c
> @@ -210,14 +210,17 @@ static struct dm_path *rr_select_path(struct path_selector *ps, size_t nr_bytes)
>  	struct path_info *pi = NULL;
>  	struct dm_path *current_path = NULL;
>
> +	local_irq_save(flags);
>  	current_path = *this_cpu_ptr(s->current_path);
>  	if (current_path) {
>  		percpu_counter_dec(&s->repeat_count);
> -		if (percpu_counter_read_positive(&s->repeat_count) > 0)
> +		if (percpu_counter_read_positive(&s->repeat_count) > 0) {
> +			local_irq_restore(flags);
>  			return current_path;
> +		}
>  	}
>
> -	spin_lock_irqsave(&s->lock, flags);
> +	spin_lock(&s->lock);
>  	if (!list_empty(&s->valid_paths)) {
>  		pi = list_entry(s->valid_paths.next, struct path_info, list);
>  		list_move_tail(&pi->list, &s->valid_paths);
> @@ -225,7 +228,8 @@ static struct dm_path *rr_select_path(struct path_selector *ps, size_t nr_bytes)
>  		set_percpu_current_path(s, pi->path);
>  		current_path = pi->path;

Just leave that as-is, should be fine.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1457470

On 12:06 Fri 05 Aug     , Mike Snitzer wrote:
> On Fri, Aug 05 2016 at 11:54am -0400,
> Jens Axboe <axboe@kernel.dk> wrote:
> 
> > On 08/05/2016 09:42 AM, Mike Snitzer wrote:
> > >On Fri, Aug 05 2016 at 11:33P -0400,
> > >Jens Axboe <axboe@kernel.dk> wrote:
> > >
> > >>On 08/05/2016 09:27 AM, Mike Snitzer wrote:
> > >>>On Wed, Aug 03 2016 at 11:35am -0400,
> > >>>Benjamin Block <bblock@linux.vnet.ibm.com> wrote:
> > >>>
> > >>>>Hej Mike,
> > >>>>
> > >>>>when running a debug-kernel today with several multipath-devices using
> > >>>>the round-robin path selector I noticed that the kernel throws these
> > >>>>warnings here:
> > >>>>
> > >>>>BUG: using smp_processor_id() in preemptible [00000000] code: kdmwork-252:0/881
> > >>>>caller is rr_select_path+0x36/0x108 [dm_round_robin]
> > >>>>CPU: 1 PID: 881 Comm: kdmwork-252:0 Not tainted 4.7.0-debug #4
> > >>>>      00000000617679b8 0000000061767a48 0000000000000002 0000000000000000
> > >>>>      0000000061767ae8 0000000061767a60 0000000061767a60 00000000001145d0
> > >>>>      0000000000000000 0000000000b962ae 0000000000bb291e 000000000000000b
> > >>>>      0000000061767aa8 0000000061767a48 0000000000000000 0000000000000000
> > >>>>      0700000000b962ae 00000000001145d0 0000000061767a48 0000000061767aa8
> > >>>>Call Trace:
> > >>>>([<00000000001144a2>] show_trace+0x8a/0xe0)
> > >>>>([<0000000000114586>] show_stack+0x8e/0xf0)
> > >>>>([<00000000006c7fdc>] dump_stack+0x9c/0xe0)
> > >>>>([<00000000006fbbc0>] check_preemption_disabled+0x108/0x130)
> > >>>>([<000003ff80268646>] rr_select_path+0x36/0x108 [dm_round_robin])
> > >>>>([<000003ff80259a42>] choose_path_in_pg+0x42/0xc8 [dm_multipath])
> > >>>>([<000003ff80259b62>] choose_pgpath+0x9a/0x1a0 [dm_multipath])
> > >>>>([<000003ff8025b51a>] __multipath_map.isra.5+0x72/0x228 [dm_multipath])
> > >>>>([<000003ff8025b75e>] multipath_map+0x3e/0x50 [dm_multipath])
> > >>>>([<000003ff80225eb6>] map_request+0x66/0x458 [dm_mod])
> > >>>>([<000003ff802262ec>] map_tio_request+0x44/0x70 [dm_mod])
> > >>>>([<000000000016835a>] kthread_worker_fn+0xf2/0x1d8)
> > >>>>([<00000000001681da>] kthread+0x112/0x120)
> > >>>>([<000000000098378a>] kernel_thread_starter+0x6/0xc)
> > >>>>([<0000000000983784>] kernel_thread_starter+0x0/0xc)
> > >>>>no locks held by kdmwork-252:0/881.
> > >>>>
> > >>>>
> > >>>>There are several more of these warnings, but all have the same
> > >>>>stack-trace (this is on s390x, but this looks like its only common code)
> > >>>>- sometimes the process-context is multipath.
> > >>>>
> > >>>>Looking at the changes in this function, it rather looks like this is
> > >>>>caused by changes made in commit
> > >>>>b0b477c7e0dd93f8916d106018ded1331b81bf61 (dm round robin: use percpu
> > >>>>'repeat_count' and 'current_path').
> > >>>>
> > >>>>The kernel is a stock v4.7 with some debug options enabled (prominently
> > >>>>DEBUG_PREEMPT). Need any more info?
> > >>>
> > >>>As you can see from commit b0b477c7e0dd9 the round-robin path selector
> > >>>is now using percpu data (pointer) and a percpu_counter.
> > >>>
> > >>>I'm really not sure how else to access this percpu data.
> > >>>
> > >>>Cc'ing LKML, linux-block, Jens and hch to cast a wider net in the hopes
> > >>>of getting an answer to how to fix this.
> > >>
> > >>From a quick look, looks like you are using this_cpu_ptr() without
> > >>having preemption disabled.
> > >
> > >Right, that is what it looked like to me too.
> > >
> > >I'm just not sure on what the proper pattern is to fix this.
> > >
> > >I'll look closer though.
> > 
> > I always forget the details (if this confuses lockdep or not), but you
> > could potentially turn it into:
> > 
> > local_irq_save(flags);
> > x = this_cpu_ptr();
> > [...]
> > 
> > spin_lock(&s->lock);
> > [...]
> > 
> > instead.
> 
> Cool, I've coded up the patch (compile tested only).
> 
> Benjamin, any chance you could test this against your v4.7 kernel and
> report back?
> 

I will give it a go on Monday. Thx for the quick fix.


- Benjamin

> 
> diff --git a/drivers/md/dm-round-robin.c b/drivers/md/dm-round-robin.c
> index 4ace1da..ed446f8 100644
> --- a/drivers/md/dm-round-robin.c
> +++ b/drivers/md/dm-round-robin.c
> @@ -210,14 +210,17 @@ static struct dm_path *rr_select_path(struct path_selector *ps, size_t nr_bytes)
>  	struct path_info *pi = NULL;
>  	struct dm_path *current_path = NULL;
> 
> +	local_irq_save(flags);
>  	current_path = *this_cpu_ptr(s->current_path);
>  	if (current_path) {
>  		percpu_counter_dec(&s->repeat_count);
> -		if (percpu_counter_read_positive(&s->repeat_count) > 0)
> +		if (percpu_counter_read_positive(&s->repeat_count) > 0) {
> +			local_irq_restore(flags);
>  			return current_path;
> +		}
>  	}
> 
> -	spin_lock_irqsave(&s->lock, flags);
> +	spin_lock(&s->lock);
>  	if (!list_empty(&s->valid_paths)) {
>  		pi = list_entry(s->valid_paths.next, struct path_info, list);
>  		list_move_tail(&pi->list, &s->valid_paths);
> @@ -225,7 +228,8 @@ static struct dm_path *rr_select_path(struct path_selector *ps, size_t nr_bytes)
>  		set_percpu_current_path(s, pi->path);
>  		current_path = pi->path;
>  	}
> -	spin_unlock_irqrestore(&s->lock, flags);
> +	spin_unlock(&s->lock);
> +	local_irq_restore(flags);
> 
>  	return current_path;
>  }
> 

-- 
Linux on z Systems Development         /         IBM Systems & Technology Group
		  IBM Deutschland Research & Development GmbH 
Vorsitz. AufsR.: Martina Koederitz     /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

[toc] | [prev] | [next] | [standalone]

#1457903 — Re: [dm-devel] bug: using smp_processor_id() in preemptible code in rr_select_path()

On 08/08/2016 10:32 AM, Benjamin Block wrote:
> On 12:06 Fri 05 Aug     , Mike Snitzer wrote:
>> On Fri, Aug 05 2016 at 11:54am -0400,
>> Jens Axboe <axboe@kernel.dk> wrote:
>>
>>> On 08/05/2016 09:42 AM, Mike Snitzer wrote:
>>>> On Fri, Aug 05 2016 at 11:33P -0400,
>>>> Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>>> On 08/05/2016 09:27 AM, Mike Snitzer wrote:
>>>>>> On Wed, Aug 03 2016 at 11:35am -0400,
>>>>>> Benjamin Block <bblock@linux.vnet.ibm.com> wrote:
>>>>>>
>>>>>>> Hej Mike,
>>>>>>>
>>>>>>> when running a debug-kernel today with several multipath-devices using
>>>>>>> the round-robin path selector I noticed that the kernel throws these
>>>>>>> warnings here:
>>>>>>>
>>>>>>> BUG: using smp_processor_id() in preemptible [00000000] code: kdmwork-252:0/881
>>>>>>> caller is rr_select_path+0x36/0x108 [dm_round_robin]
>>>>>>> CPU: 1 PID: 881 Comm: kdmwork-252:0 Not tainted 4.7.0-debug #4
>>>>>>>      00000000617679b8 0000000061767a48 0000000000000002 0000000000000000
>>>>>>>      0000000061767ae8 0000000061767a60 0000000061767a60 00000000001145d0
>>>>>>>      0000000000000000 0000000000b962ae 0000000000bb291e 000000000000000b
>>>>>>>      0000000061767aa8 0000000061767a48 0000000000000000 0000000000000000
>>>>>>>      0700000000b962ae 00000000001145d0 0000000061767a48 0000000061767aa8
>>>>>>> Call Trace:
>>>>>>> ([<00000000001144a2>] show_trace+0x8a/0xe0)
>>>>>>> ([<0000000000114586>] show_stack+0x8e/0xf0)
>>>>>>> ([<00000000006c7fdc>] dump_stack+0x9c/0xe0)
>>>>>>> ([<00000000006fbbc0>] check_preemption_disabled+0x108/0x130)
>>>>>>> ([<000003ff80268646>] rr_select_path+0x36/0x108 [dm_round_robin])
>>>>>>> ([<000003ff80259a42>] choose_path_in_pg+0x42/0xc8 [dm_multipath])
>>>>>>> ([<000003ff80259b62>] choose_pgpath+0x9a/0x1a0 [dm_multipath])
>>>>>>> ([<000003ff8025b51a>] __multipath_map.isra.5+0x72/0x228 [dm_multipath])
>>>>>>> ([<000003ff8025b75e>] multipath_map+0x3e/0x50 [dm_multipath])
>>>>>>> ([<000003ff80225eb6>] map_request+0x66/0x458 [dm_mod])
>>>>>>> ([<000003ff802262ec>] map_tio_request+0x44/0x70 [dm_mod])
>>>>>>> ([<000000000016835a>] kthread_worker_fn+0xf2/0x1d8)
>>>>>>> ([<00000000001681da>] kthread+0x112/0x120)
>>>>>>> ([<000000000098378a>] kernel_thread_starter+0x6/0xc)
>>>>>>> ([<0000000000983784>] kernel_thread_starter+0x0/0xc)
>>>>>>> no locks held by kdmwork-252:0/881.
>>>>>>>
> [:snip:]
>>>
>>> I always forget the details (if this confuses lockdep or not), but you
>>> could potentially turn it into:
>>>
>>> local_irq_save(flags);
>>> x = this_cpu_ptr();
>>> [...]
>>>
>>> spin_lock(&s->lock);
>>> [...]
>>>
>>> instead.
>>
>> Cool, I've coded up the patch (compile tested only).
>>
>> Benjamin, any chance you could test this against your v4.7 kernel and
>> report back?
>>
>> Thanks,
>> Mike
>>
>> diff --git a/drivers/md/dm-round-robin.c b/drivers/md/dm-round-robin.c
>> index 4ace1da..ed446f8 100644
>> --- a/drivers/md/dm-round-robin.c
>> +++ b/drivers/md/dm-round-robin.c
>> @@ -210,14 +210,17 @@ static struct dm_path *rr_select_path(struct path_selector *ps, size_t nr_bytes)
>>  	struct path_info *pi = NULL;
>>  	struct dm_path *current_path = NULL;
>>
>> +	local_irq_save(flags);
>>  	current_path = *this_cpu_ptr(s->current_path);
>>  	if (current_path) {
>>  		percpu_counter_dec(&s->repeat_count);
>> -		if (percpu_counter_read_positive(&s->repeat_count) > 0)
>> +		if (percpu_counter_read_positive(&s->repeat_count) > 0) {
>> +			local_irq_restore(flags);
>>  			return current_path;
>> +		}
>>  	}
>>
>> -	spin_lock_irqsave(&s->lock, flags);
>> +	spin_lock(&s->lock);
>>  	if (!list_empty(&s->valid_paths)) {
>>  		pi = list_entry(s->valid_paths.next, struct path_info, list);
>>  		list_move_tail(&pi->list, &s->valid_paths);
>> @@ -225,7 +228,8 @@ static struct dm_path *rr_select_path(struct path_selector *ps, size_t nr_bytes)
>>  		set_percpu_current_path(s, pi->path);
>>  		current_path = pi->path;
>>  	}
>> -	spin_unlock_irqrestore(&s->lock, flags);
>> +	spin_unlock(&s->lock);
>> +	local_irq_restore(flags);
>>
>>  	return current_path;
>>  }
>>
>
> Ok, this works as far as the warnings don't appear anymore. But while
> applying the patch and thinking about it, why local_irq_save() and not
> preempt_disable()? "Sounds" like this is the function you want, and I
> also stumbled across this in Documentation/preempt-locking.txt:
>
>   But keep in mind that 'irqs disabled' is a fundamentally unsafe way of
>   disabling preemption - any spin_unlock() decreasing the preemption
>   count to 0 might trigger a reschedule.
>
> The spinlock would do an other nested preempt_disable(), but those even
> out.

local_irq_save(), since we need to grab the lock irq safe very shortly
anyway. As long as they nest properly, the approach is fine, and it's
more efficient than first doing a preempt_disable(), then still needing
a irq safe spinlock.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1457929 — Re: [dm-devel] bug: using smp_processor_id() in preemptible code in rr_select_path()

On 10:39 Mon 08 Aug     , Jens Axboe wrote:
> On 08/08/2016 10:32 AM, Benjamin Block wrote:
> >On 12:06 Fri 05 Aug     , Mike Snitzer wrote:
> >>On Fri, Aug 05 2016 at 11:54am -0400,
> >>Jens Axboe <axboe@kernel.dk> wrote:
> >>
> >>>On 08/05/2016 09:42 AM, Mike Snitzer wrote:
> >>>>On Fri, Aug 05 2016 at 11:33P -0400,
> >>>>Jens Axboe <axboe@kernel.dk> wrote:
> >>>>
> >>>>>On 08/05/2016 09:27 AM, Mike Snitzer wrote:
> >>>>>>On Wed, Aug 03 2016 at 11:35am -0400,
> >>>>>>Benjamin Block <bblock@linux.vnet.ibm.com> wrote:
> >>>>>>
> >>>>>>>Hej Mike,
> >>>>>>>
> >>>>>>>when running a debug-kernel today with several multipath-devices using
> >>>>>>>the round-robin path selector I noticed that the kernel throws these
> >>>>>>>warnings here:
> >>>>>>>
> >>>>>>>BUG: using smp_processor_id() in preemptible [00000000] code: kdmwork-252:0/881
> >>>>>>>caller is rr_select_path+0x36/0x108 [dm_round_robin]
> >>>>>>>CPU: 1 PID: 881 Comm: kdmwork-252:0 Not tainted 4.7.0-debug #4
> >>>>>>>     00000000617679b8 0000000061767a48 0000000000000002 0000000000000000
> >>>>>>>     0000000061767ae8 0000000061767a60 0000000061767a60 00000000001145d0
> >>>>>>>     0000000000000000 0000000000b962ae 0000000000bb291e 000000000000000b
> >>>>>>>     0000000061767aa8 0000000061767a48 0000000000000000 0000000000000000
> >>>>>>>     0700000000b962ae 00000000001145d0 0000000061767a48 0000000061767aa8
> >>>>>>>Call Trace:
> >>>>>>>([<00000000001144a2>] show_trace+0x8a/0xe0)
> >>>>>>>([<0000000000114586>] show_stack+0x8e/0xf0)
> >>>>>>>([<00000000006c7fdc>] dump_stack+0x9c/0xe0)
> >>>>>>>([<00000000006fbbc0>] check_preemption_disabled+0x108/0x130)
> >>>>>>>([<000003ff80268646>] rr_select_path+0x36/0x108 [dm_round_robin])
> >>>>>>>([<000003ff80259a42>] choose_path_in_pg+0x42/0xc8 [dm_multipath])
> >>>>>>>([<000003ff80259b62>] choose_pgpath+0x9a/0x1a0 [dm_multipath])
> >>>>>>>([<000003ff8025b51a>] __multipath_map.isra.5+0x72/0x228 [dm_multipath])
> >>>>>>>([<000003ff8025b75e>] multipath_map+0x3e/0x50 [dm_multipath])
> >>>>>>>([<000003ff80225eb6>] map_request+0x66/0x458 [dm_mod])
> >>>>>>>([<000003ff802262ec>] map_tio_request+0x44/0x70 [dm_mod])
> >>>>>>>([<000000000016835a>] kthread_worker_fn+0xf2/0x1d8)
> >>>>>>>([<00000000001681da>] kthread+0x112/0x120)
> >>>>>>>([<000000000098378a>] kernel_thread_starter+0x6/0xc)
> >>>>>>>([<0000000000983784>] kernel_thread_starter+0x0/0xc)
> >>>>>>>no locks held by kdmwork-252:0/881.
> >>>>>>>
> >[:snip:]
> >>>
> >>>I always forget the details (if this confuses lockdep or not), but you
> >>>could potentially turn it into:
> >>>
> >>>local_irq_save(flags);
> >>>x = this_cpu_ptr();
> >>>[...]
> >>>
> >>>spin_lock(&s->lock);
> >>>[...]
> >>>
> >>>instead.
> >>
> >>Cool, I've coded up the patch (compile tested only).
> >>
> >>Benjamin, any chance you could test this against your v4.7 kernel and
> >>report back?
> >>
> >>Thanks,
> >>Mike
> >>
> >>diff --git a/drivers/md/dm-round-robin.c b/drivers/md/dm-round-robin.c
> >>index 4ace1da..ed446f8 100644
> >>--- a/drivers/md/dm-round-robin.c
> >>+++ b/drivers/md/dm-round-robin.c
> >>@@ -210,14 +210,17 @@ static struct dm_path *rr_select_path(struct path_selector *ps, size_t nr_bytes)
> >> 	struct path_info *pi = NULL;
> >> 	struct dm_path *current_path = NULL;
> >>
> >>+	local_irq_save(flags);
> >> 	current_path = *this_cpu_ptr(s->current_path);
> >> 	if (current_path) {
> >> 		percpu_counter_dec(&s->repeat_count);
> >>-		if (percpu_counter_read_positive(&s->repeat_count) > 0)
> >>+		if (percpu_counter_read_positive(&s->repeat_count) > 0) {
> >>+			local_irq_restore(flags);
> >> 			return current_path;
> >>+		}
> >> 	}
> >>
> >>-	spin_lock_irqsave(&s->lock, flags);
> >>+	spin_lock(&s->lock);
> >> 	if (!list_empty(&s->valid_paths)) {
> >> 		pi = list_entry(s->valid_paths.next, struct path_info, list);
> >> 		list_move_tail(&pi->list, &s->valid_paths);
> >>@@ -225,7 +228,8 @@ static struct dm_path *rr_select_path(struct path_selector *ps, size_t nr_bytes)
> >> 		set_percpu_current_path(s, pi->path);
> >> 		current_path = pi->path;
> >> 	}
> >>-	spin_unlock_irqrestore(&s->lock, flags);
> >>+	spin_unlock(&s->lock);
> >>+	local_irq_restore(flags);
> >>
> >> 	return current_path;
> >> }
> >>
> >
> >Ok, this works as far as the warnings don't appear anymore. But while
> >applying the patch and thinking about it, why local_irq_save() and not
> >preempt_disable()? "Sounds" like this is the function you want, and I
> >also stumbled across this in Documentation/preempt-locking.txt:
> >
> >  But keep in mind that 'irqs disabled' is a fundamentally unsafe way of
> >  disabling preemption - any spin_unlock() decreasing the preemption
> >  count to 0 might trigger a reschedule.
> >
> >The spinlock would do an other nested preempt_disable(), but those even
> >out.
> 
> local_irq_save(), since we need to grab the lock irq safe very shortly
> anyway. As long as they nest properly, the approach is fine, and it's
> more efficient than first doing a preempt_disable(), then still needing
> a irq safe spinlock.
> 

Fair enough, it also doesn't look like the code is doing anything that
could trigger a reschedule.


                                                    Beste Grüße / Best regards,
                                                      - Benjamin Block
-- 
Linux on z Systems Development         /         IBM Systems & Technology Group
		  IBM Deutschland Research & Development GmbH 
Vorsitz. AufsR.: Martina Koederitz     /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

[toc] | [prev] | [next] | [standalone]

#1457906 — Re: [dm-devel] bug: using smp_processor_id() in preemptible code in rr_select_path()

On 12:06 Fri 05 Aug     , Mike Snitzer wrote:
> On Fri, Aug 05 2016 at 11:54am -0400,
> Jens Axboe <axboe@kernel.dk> wrote:
> 
> > On 08/05/2016 09:42 AM, Mike Snitzer wrote:
> > >On Fri, Aug 05 2016 at 11:33P -0400,
> > >Jens Axboe <axboe@kernel.dk> wrote:
> > >
> > >>On 08/05/2016 09:27 AM, Mike Snitzer wrote:
> > >>>On Wed, Aug 03 2016 at 11:35am -0400,
> > >>>Benjamin Block <bblock@linux.vnet.ibm.com> wrote:
> > >>>
> > >>>>Hej Mike,
> > >>>>
> > >>>>when running a debug-kernel today with several multipath-devices using
> > >>>>the round-robin path selector I noticed that the kernel throws these
> > >>>>warnings here:
> > >>>>
> > >>>>BUG: using smp_processor_id() in preemptible [00000000] code: kdmwork-252:0/881
> > >>>>caller is rr_select_path+0x36/0x108 [dm_round_robin]
> > >>>>CPU: 1 PID: 881 Comm: kdmwork-252:0 Not tainted 4.7.0-debug #4
> > >>>>      00000000617679b8 0000000061767a48 0000000000000002 0000000000000000
> > >>>>      0000000061767ae8 0000000061767a60 0000000061767a60 00000000001145d0
> > >>>>      0000000000000000 0000000000b962ae 0000000000bb291e 000000000000000b
> > >>>>      0000000061767aa8 0000000061767a48 0000000000000000 0000000000000000
> > >>>>      0700000000b962ae 00000000001145d0 0000000061767a48 0000000061767aa8
> > >>>>Call Trace:
> > >>>>([<00000000001144a2>] show_trace+0x8a/0xe0)
> > >>>>([<0000000000114586>] show_stack+0x8e/0xf0)
> > >>>>([<00000000006c7fdc>] dump_stack+0x9c/0xe0)
> > >>>>([<00000000006fbbc0>] check_preemption_disabled+0x108/0x130)
> > >>>>([<000003ff80268646>] rr_select_path+0x36/0x108 [dm_round_robin])
> > >>>>([<000003ff80259a42>] choose_path_in_pg+0x42/0xc8 [dm_multipath])
> > >>>>([<000003ff80259b62>] choose_pgpath+0x9a/0x1a0 [dm_multipath])
> > >>>>([<000003ff8025b51a>] __multipath_map.isra.5+0x72/0x228 [dm_multipath])
> > >>>>([<000003ff8025b75e>] multipath_map+0x3e/0x50 [dm_multipath])
> > >>>>([<000003ff80225eb6>] map_request+0x66/0x458 [dm_mod])
> > >>>>([<000003ff802262ec>] map_tio_request+0x44/0x70 [dm_mod])
> > >>>>([<000000000016835a>] kthread_worker_fn+0xf2/0x1d8)
> > >>>>([<00000000001681da>] kthread+0x112/0x120)
> > >>>>([<000000000098378a>] kernel_thread_starter+0x6/0xc)
> > >>>>([<0000000000983784>] kernel_thread_starter+0x0/0xc)
> > >>>>no locks held by kdmwork-252:0/881.
> > >>>>
[:snip:]
> > 
> > I always forget the details (if this confuses lockdep or not), but you
> > could potentially turn it into:
> > 
> > local_irq_save(flags);
> > x = this_cpu_ptr();
> > [...]
> > 
> > spin_lock(&s->lock);
> > [...]
> > 
> > instead.
> 
> Cool, I've coded up the patch (compile tested only).
> 
> Benjamin, any chance you could test this against your v4.7 kernel and
> report back?
> 
> Thanks,
> Mike
> 
> diff --git a/drivers/md/dm-round-robin.c b/drivers/md/dm-round-robin.c
> index 4ace1da..ed446f8 100644
> --- a/drivers/md/dm-round-robin.c
> +++ b/drivers/md/dm-round-robin.c
> @@ -210,14 +210,17 @@ static struct dm_path *rr_select_path(struct path_selector *ps, size_t nr_bytes)
>  	struct path_info *pi = NULL;
>  	struct dm_path *current_path = NULL;
> 
> +	local_irq_save(flags);
>  	current_path = *this_cpu_ptr(s->current_path);
>  	if (current_path) {
>  		percpu_counter_dec(&s->repeat_count);
> -		if (percpu_counter_read_positive(&s->repeat_count) > 0)
> +		if (percpu_counter_read_positive(&s->repeat_count) > 0) {
> +			local_irq_restore(flags);
>  			return current_path;
> +		}
>  	}
> 
> -	spin_lock_irqsave(&s->lock, flags);
> +	spin_lock(&s->lock);
>  	if (!list_empty(&s->valid_paths)) {
>  		pi = list_entry(s->valid_paths.next, struct path_info, list);
>  		list_move_tail(&pi->list, &s->valid_paths);
> @@ -225,7 +228,8 @@ static struct dm_path *rr_select_path(struct path_selector *ps, size_t nr_bytes)
>  		set_percpu_current_path(s, pi->path);
>  		current_path = pi->path;
>  	}
> -	spin_unlock_irqrestore(&s->lock, flags);
> +	spin_unlock(&s->lock);
> +	local_irq_restore(flags);
> 
>  	return current_path;
>  }
> 

Ok, this works as far as the warnings don't appear anymore. But while
applying the patch and thinking about it, why local_irq_save() and not
preempt_disable()? "Sounds" like this is the function you want, and I
also stumbled across this in Documentation/preempt-locking.txt:

  But keep in mind that 'irqs disabled' is a fundamentally unsafe way of
  disabling preemption - any spin_unlock() decreasing the preemption
  count to 0 might trigger a reschedule. 

The spinlock would do an other nested preempt_disable(), but those even
out.


                                                    Beste Grüße / Best regards,
                                                      - Benjamin Block
-- 
Linux on z Systems Development         /         IBM Systems & Technology Group
		  IBM Deutschland Research & Development GmbH 
Vorsitz. AufsR.: Martina Koederitz     /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web