From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753740AbbJGKe5 (ORCPT ); Wed, 7 Oct 2015 06:34:57 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:25507 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751915AbbJGKez (ORCPT ); Wed, 7 Oct 2015 06:34:55 -0400 Message-ID: <5614F51E.1020404@oracle.com> Date: Wed, 07 Oct 2015 18:34:06 +0800 From: Bob Liu User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130308 Thunderbird/17.0.4 MIME-Version: 1.0 To: =?windows-1252?Q?Roger_Pau_Monn=E9?= CC: xen-devel@lists.xen.org, david.vrabel@citrix.com, linux-kernel@vger.kernel.org, konrad.wilk@oracle.com, felipe.franciosi@citrix.com, axboe@fb.com, hch@infradead.org, avanzini.arianna@gmail.com, rafal.mielniczuk@citrix.com, boris.ostrovsky@oracle.com, jonathan.davies@citrix.com Subject: Re: [PATCH v3 5/9] xen/blkfront: convert per device io_lock to per ring ring_lock References: <1441456782-31318-1-git-send-email-bob.liu@oracle.com> <1441456782-31318-6-git-send-email-bob.liu@oracle.com> <56128587.7000804@citrix.com> In-Reply-To: <56128587.7000804@citrix.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-Source-IP: aserv0021.oracle.com [141.146.126.233] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/05/2015 10:13 PM, Roger Pau Monné wrote: > El 05/09/15 a les 14.39, Bob Liu ha escrit: >> The per device io_lock became a coarser grained lock after multi-queues/rings >> was introduced, this patch converts it to a fine-grained per ring lock. >> >> NOTE: The per dev_info structure was no more protected by any lock. > > I would rewrite this as: > > Note that the per-device blkfront_dev_info structure is no longer > protected by any lock. > Will update. >> >> Signed-off-by: Bob Liu >> --- >> drivers/block/xen-blkfront.c | 44 +++++++++++++++++++----------------------- >> 1 file changed, 20 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c >> index bf45c99..1cae76b 100644 >> --- a/drivers/block/xen-blkfront.c >> +++ b/drivers/block/xen-blkfront.c >> @@ -123,6 +123,7 @@ MODULE_PARM_DESC(max_ring_page_order, "Maximum order of pages to be used for the >> struct blkfront_ring_info >> { >> struct blkif_front_ring ring; >> + spinlock_t ring_lock; >> unsigned int ring_ref[XENBUS_MAX_RING_PAGES]; >> unsigned int evtchn, irq; >> struct work_struct work; >> @@ -141,7 +142,6 @@ struct blkfront_ring_info >> * putting all kinds of interesting stuff here :-) >> */ >> struct blkfront_dev_info { >> - spinlock_t io_lock; >> struct mutex mutex; >> struct xenbus_device *xbdev; >> struct gendisk *gd; >> @@ -637,29 +637,28 @@ static int blkif_queue_rq(struct blk_mq_hw_ctx *hctx, >> const struct blk_mq_queue_data *qd) >> { >> struct blkfront_ring_info *rinfo = (struct blkfront_ring_info *)hctx->driver_data; >> - struct blkfront_dev_info *dinfo = rinfo->dinfo; >> >> blk_mq_start_request(qd->rq); >> - spin_lock_irq(&dinfo->io_lock); >> + spin_lock_irq(&rinfo->ring_lock); >> if (RING_FULL(&rinfo->ring)) >> goto out_busy; >> >> - if (blkif_request_flush_invalid(qd->rq, dinfo)) >> + if (blkif_request_flush_invalid(qd->rq, rinfo->dinfo)) >> goto out_err; >> >> if (blkif_queue_request(qd->rq, rinfo)) >> goto out_busy; >> >> flush_requests(rinfo); >> - spin_unlock_irq(&dinfo->io_lock); >> + spin_unlock_irq(&rinfo->ring_lock); >> return BLK_MQ_RQ_QUEUE_OK; >> >> out_err: >> - spin_unlock_irq(&dinfo->io_lock); >> + spin_unlock_irq(&rinfo->ring_lock); >> return BLK_MQ_RQ_QUEUE_ERROR; >> >> out_busy: >> - spin_unlock_irq(&dinfo->io_lock); >> + spin_unlock_irq(&rinfo->ring_lock); >> blk_mq_stop_hw_queue(hctx); >> return BLK_MQ_RQ_QUEUE_BUSY; >> } >> @@ -961,7 +960,7 @@ static void xlvbd_release_gendisk(struct blkfront_dev_info *dinfo) >> dinfo->gd = NULL; >> } >> >> -/* Must be called with io_lock holded */ >> +/* Must be called with ring_lock holded */ > ^ held. >> static void kick_pending_request_queues(struct blkfront_ring_info *rinfo) >> { >> if (!RING_FULL(&rinfo->ring)) >> @@ -972,10 +971,10 @@ static void blkif_restart_queue(struct work_struct *work) >> { >> struct blkfront_ring_info *rinfo = container_of(work, struct blkfront_ring_info, work); >> >> - spin_lock_irq(&rinfo->dinfo->io_lock); >> + spin_lock_irq(&rinfo->ring_lock); >> if (rinfo->dinfo->connected == BLKIF_STATE_CONNECTED) >> kick_pending_request_queues(rinfo); > > This seems wrong, why are you acquiring a per-ring lock in order to > check a per-device field? IMHO, I think you need to introduce a > per-device lock or drop the locking around this chunk if it's really not > needed. > The lock here is to protect kick_pending_request_queues() where we will check RING_FULL(). Will move this lock after the checking of per-device field. Thanks, -Bob >> - spin_unlock_irq(&rinfo->dinfo->io_lock); >> + spin_unlock_irq(&rinfo->ring_lock); >> } >>