From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755112AbbJIPLW (ORCPT ); Fri, 9 Oct 2015 11:11:22 -0400 Received: from mail-ig0-f174.google.com ([209.85.213.174]:38739 "EHLO mail-ig0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754203AbbJIPLU (ORCPT ); Fri, 9 Oct 2015 11:11:20 -0400 Subject: Re: [PATCH] blk-mq: fix waitqueue_active without memory barrier in block/blk-mq-tag.c To: Kosuke Tatsukawa References: <17EC94B0A072C34B8DCF0D30AD16044A02874753@BPXM09GP.gisp.nec.co.jp> Cc: "linux-kernel@vger.kernel.org" From: Jens Axboe Message-ID: <5617D914.4060700@kernel.dk> Date: Fri, 9 Oct 2015 09:11:16 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <17EC94B0A072C34B8DCF0D30AD16044A02874753@BPXM09GP.gisp.nec.co.jp> Content-Type: text/plain; charset=iso-2022-jp Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/08/2015 06:35 PM, Kosuke Tatsukawa wrote: > blk_mq_tag_update_depth() seems to be missing a memory barrier which > might cause the waker to not notice the waiter and fail to send a > wake_up as in the following figure. > > blk_mq_tag_update_depth bt_get > ------------------------------------------------------------------------ > if (waitqueue_active(&bs->wait)) > /* The CPU might reorder the test for > the waitqueue up here, before > prior writes complete */ > prepare_to_wait(&bs->wait, &wait, > TASK_UNINTERRUPTIBLE); > tag = __bt_get(hctx, bt, last_tag, > tags); > /* Value set in bt_update_count not > visible yet */ > bt_update_count(&tags->bitmap_tags, tdepth); > /* blk_mq_tag_wakeup_all(tags, false); */ > bt = &tags->bitmap_tags; > wake_index = atomic_read(&bt->wake_index); > ... > io_schedule(); > ------------------------------------------------------------------------ > > This patch adds the missing memory barrier. > > I found this issue when I was looking through the linux source code > for places calling waitqueue_active() before wake_up*(), but without > preceding memory barriers, after sending a patch to fix a similar > issue in drivers/tty/n_tty.c (Details about the original issue can be > found here: https://lkml.org/lkml/2015/9/28/849). > > Signed-off-by: Kosuke Tatsukawa > --- > block/blk-mq-tag.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c > index ed96474..7a6b6e2 100644 > --- a/block/blk-mq-tag.c > +++ b/block/blk-mq-tag.c > @@ -75,6 +75,10 @@ void blk_mq_tag_wakeup_all(struct blk_mq_tags *tags, bool include_reserve) > struct blk_mq_bitmap_tags *bt; > int i, wake_index; > > + /* > + * Make sure all changes prior to this are visible from other CPUs. > + */ > + smp_mb(); > bt = &tags->bitmap_tags; > wake_index = atomic_read(&bt->wake_index); > for (i = 0; i < BT_WAIT_QUEUES; i++) { > Thanks, after looking at this, I think this patch is fine. It's not a super hot path, so not worth it to further optimize this or look into ways to avoid the barrier. I do wonder if there are archs where atomic_read() is a memory barrier, in that case we need not do it at all. And perhaps we have some weird smp_before_bla variant that could be used here instead fo improve upon that case. -- Jens Axboe