Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932426AbbFRP7k (ORCPT ); Thu, 18 Jun 2015 11:59:40 -0400 Received: from mga01.intel.com ([192.55.52.88]:20023 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753051AbbFRP7c (ORCPT ); Thu, 18 Jun 2015 11:59:32 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,638,1427785200"; d="scan'208";a="713370087" Date: Thu, 18 Jun 2015 09:59:32 -0600 From: Jon Derrick To: Parav Pandit Cc: linux-nvme@lists.infradead.org, willy@linux.intel.com, axboe@kernel.dk, keith.busch@intel.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] NVMe: Fixed race between nvme_thread & probe path. Message-ID: <20150618155932.GA1276@localhost.localdomain> References: <1434624230-25050-1-git-send-email-Parav.pandit@avagotech.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1434624230-25050-1-git-send-email-Parav.pandit@avagotech.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2085 Lines: 55 On Thu, Jun 18, 2015 at 04:13:50PM +0530, Parav Pandit wrote: > Kernel thread nvme_thread and driver load process can be executing > in parallel on different CPU. This leads to race condition whenever > nvme_alloc_queue() instructions are executed out of order that can > reflects incorrect value for nvme_thread. > Memory barrier in nvme_alloc_queue() ensures that it maintains the > order and and data dependency read barrier in reader thread ensures > that cpu cache is synced. > > Signed-off-by: Parav Pandit > --- > drivers/block/nvme-core.c | 12 ++++++++++-- > 1 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c > index 5961ed7..90fb0ce 100644 > --- a/drivers/block/nvme-core.c > +++ b/drivers/block/nvme-core.c > @@ -1403,8 +1403,10 @@ static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid, > nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride]; > nvmeq->q_depth = depth; > nvmeq->qid = qid; > - dev->queue_count++; > dev->queues[qid] = nvmeq; > + /* update queues first before updating queue_count */ > + smp_wmb(); > + dev->queue_count++; > > return nvmeq; > This has been applied already as an explicit mb() > @@ -2073,7 +2075,13 @@ static int nvme_kthread(void *data) > continue; > } > for (i = 0; i < dev->queue_count; i++) { > - struct nvme_queue *nvmeq = dev->queues[i]; > + struct nvme_queue *nvmeq; > + > + /* make sure to read queue_count before > + * traversing queues. > + */ > + smp_read_barrier_depends(); > + nvmeq = dev->queues[i]; > if (!nvmeq) > continue; > spin_lock_irq(&nvmeq->q_lock); I don't think this is necessary. If queue_count is incremented while in this loop, it will be picked up the next time the kthread runs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/