Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965157AbbKDBB2 (ORCPT ); Tue, 3 Nov 2015 20:01:28 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:33052 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755445AbbKDBB0 (ORCPT ); Tue, 3 Nov 2015 20:01:26 -0500 Message-ID: <563958D7.6050304@oracle.com> Date: Wed, 04 Nov 2015 09:01:11 +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: Konrad Rzeszutek Wilk CC: xen-devel@lists.xen.org, linux-kernel@vger.kernel.org, roger.pau@citrix.com, felipe.franciosi@citrix.com, axboe@fb.com, avanzini.arianna@gmail.com, rafal.mielniczuk@citrix.com, jonathan.davies@citrix.com, david.vrabel@citrix.com Subject: Re: [PATCH v4 03/10] xen/blkfront: pseudo support for multi hardware queues/rings References: <1446438106-20171-1-git-send-email-bob.liu@oracle.com> <1446438106-20171-4-git-send-email-bob.liu@oracle.com> <20151103194436.GE28527@char.us.oracle.com> In-Reply-To: <20151103194436.GE28527@char.us.oracle.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 Content-Length: 3800 Lines: 105 On 11/04/2015 03:44 AM, Konrad Rzeszutek Wilk wrote: > On Mon, Nov 02, 2015 at 12:21:39PM +0800, Bob Liu wrote: >> Preparatory patch for multiple hardware queues (rings). The number of >> rings is unconditionally set to 1, larger number will be enabled in next >> patch so as to make every single patch small and readable. > > s/next patch/"xen/blkfront: negotiate number of queues/rings to be used with backend" > >> >> Signed-off-by: Bob Liu >> --- >> drivers/block/xen-blkfront.c | 327 +++++++++++++++++++++++++------------------ >> 1 file changed, 188 insertions(+), 139 deletions(-) >> >> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c >> index 2a557e4..eab78e7 100644 >> --- a/drivers/block/xen-blkfront.c >> +++ b/drivers/block/xen-blkfront.c >> @@ -145,6 +145,7 @@ struct blkfront_info >> int vdevice; >> blkif_vdev_t handle; >> enum blkif_state connected; >> + /* Number of pages per ring buffer */ > > Missing full stop, aka '.'. > >> unsigned int nr_ring_pages; >> struct request_queue *rq; >> struct list_head grants; >> @@ -158,7 +159,8 @@ struct blkfront_info >> unsigned int max_indirect_segments; >> int is_ready; >> struct blk_mq_tag_set tag_set; >> - struct blkfront_ring_info rinfo; >> + struct blkfront_ring_info *rinfo; >> + unsigned int nr_rings; >> }; >> >> static unsigned int nr_minors; >> @@ -190,7 +192,7 @@ static DEFINE_SPINLOCK(minor_lock); >> ((_segs + SEGS_PER_INDIRECT_FRAME - 1)/SEGS_PER_INDIRECT_FRAME) >> >> static int blkfront_setup_indirect(struct blkfront_ring_info *rinfo); >> -static int blkfront_gather_backend_features(struct blkfront_info *info); >> +static void blkfront_gather_backend_features(struct blkfront_info *info); >> >> static int get_id_from_freelist(struct blkfront_ring_info *rinfo) >> { >> @@ -443,12 +445,13 @@ static int blkif_queue_request(struct request *req, struct blkfront_ring_info *r >> */ >> max_grefs += INDIRECT_GREFS(req->nr_phys_segments); >> >> - /* Check if we have enough grants to allocate a requests */ >> - if (info->persistent_gnts_c < max_grefs) { >> + /* Check if we have enough grants to allocate a requests, we have to >> + * reserve 'max_grefs' grants because persistent grants are shared by all >> + * rings */ > > Missing full stop. > >> + if (0 < max_grefs) { > > ? 0!? > > max_grefs will at least be BLKIF_MAX_SEGMENTS_PER_REQUEST > so this will always be true. > No, max_grefs = req->nr_phys_segments; It's 0 in some cases(flush req?), and gnttable_alloc_grant_references() can not handle 0 as the parameter. > In which ase why not just .. >> new_persistent_gnts = 1; >> if (gnttab_alloc_grant_references( >> - max_grefs - info->persistent_gnts_c, >> - &gref_head) < 0) { >> + max_grefs, &gref_head) < 0) { >> gnttab_request_free_callback( >> &rinfo->callback, >> blkif_restart_queue_callback, > > .. move this whole code down? And get rid of 'new_persistent_gnts' > since it will always be true? > Unless we fix gnttable_alloc_grant_references(0). > But more importantly, why do we not check for 'info->persistent_gnts_c' anymore? > Info->persistent_gnts_c is for per-device not per-ring, the persistent grants may be taken by other queues/rings after we checked the value here. Which would make get_grant() fail, so we have to reserved enough grants in advance. Those new-allocated grants will be freed if there are enough grants in persistent list. Will fix all other comments for this patch. Thanks, Bob -- 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/