From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752207AbcHHQja (ORCPT ); Mon, 8 Aug 2016 12:39:30 -0400 Received: from mail-it0-f44.google.com ([209.85.214.44]:35630 "EHLO mail-it0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752054AbcHHQj2 (ORCPT ); Mon, 8 Aug 2016 12:39:28 -0400 Subject: Re: [dm-devel] bug: using smp_processor_id() in preemptible code in rr_select_path() To: Benjamin Block , Mike Snitzer References: <20160803153545.GE20370@bblock-ThinkPad-W530> <20160805152705.GA17480@redhat.com> <497f02e5-4f3d-8590-3096-fb5e2422bff4@kernel.dk> <20160805154223.GA2340@redhat.com> <107dc81f-675d-8ae4-0ac2-99c248a23c60@kernel.dk> <20160805160638.GA17651@redhat.com> <20160808163236.GB31352@bblock-ThinkPad-W530> Cc: linux-block@vger.kernel.org, dm-devel@redhat.com, linux-kernel@vger.kernel.org, hch@lst.de From: Jens Axboe Message-ID: <4b62efae-c6e2-e552-5c26-de7a5107dfd4@kernel.dk> Date: Mon, 8 Aug 2016 10:39:25 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160808163236.GB31352@bblock-ThinkPad-W530> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 wrote: >> >>> On 08/05/2016 09:42 AM, Mike Snitzer wrote: >>>> On Fri, Aug 05 2016 at 11:33P -0400, >>>> Jens Axboe wrote: >>>> >>>>> On 08/05/2016 09:27 AM, Mike Snitzer wrote: >>>>>> On Wed, Aug 03 2016 at 11:35am -0400, >>>>>> Benjamin Block 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