From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752627AbbJBRCK (ORCPT ); Fri, 2 Oct 2015 13:02:10 -0400 Received: from smtp02.citrix.com ([66.165.176.63]:39959 "EHLO SMTP02.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750846AbbJBRCI (ORCPT ); Fri, 2 Oct 2015 13:02:08 -0400 X-IronPort-AV: E=Sophos;i="5.17,624,1437436800"; d="scan'208";a="307723409" Subject: Re: [PATCH v3 3/9] xen/blkfront: separate per ring information out of device info To: Bob Liu , References: <1441456782-31318-1-git-send-email-bob.liu@oracle.com> <1441456782-31318-4-git-send-email-bob.liu@oracle.com> CC: , , , , , , , , , From: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= X-Enigmail-Draft-Status: N1110 Message-ID: <560EB88A.9070905@citrix.com> Date: Fri, 2 Oct 2015 19:02:02 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <1441456782-31318-4-git-send-email-bob.liu@oracle.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-DLP: MIA1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org El 05/09/15 a les 14.39, Bob Liu ha escrit: > Split per ring information to an new structure:blkfront_ring_info, also rename > per blkfront_info to blkfront_dev_info. ^ removed. > > A ring is the representation of a hardware queue, every vbd device can associate > with one or more blkfront_ring_info depending on how many hardware > queues/rings to be used. > > This patch is a preparation for supporting real multi hardware queues/rings. > > Signed-off-by: Arianna Avanzini > Signed-off-by: Bob Liu > --- > drivers/block/xen-blkfront.c | 854 ++++++++++++++++++++++-------------------- > 1 file changed, 445 insertions(+), 409 deletions(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index 5dd591d..bf416d5 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -107,7 +107,7 @@ static unsigned int xen_blkif_max_ring_order; > module_param_named(max_ring_page_order, xen_blkif_max_ring_order, int, S_IRUGO); > MODULE_PARM_DESC(max_ring_page_order, "Maximum order of pages to be used for the shared ring"); > > -#define BLK_RING_SIZE(info) __CONST_RING_SIZE(blkif, PAGE_SIZE * (info)->nr_ring_pages) > +#define BLK_RING_SIZE(dinfo) __CONST_RING_SIZE(blkif, PAGE_SIZE * (dinfo)->nr_ring_pages) This change looks pointless, any reason to use dinfo instead of info? > #define BLK_MAX_RING_SIZE __CONST_RING_SIZE(blkif, PAGE_SIZE * XENBUS_MAX_RING_PAGES) > /* > * ring-ref%i i=(-1UL) would take 11 characters + 'ring-ref' is 8, so 19 > @@ -116,12 +116,31 @@ MODULE_PARM_DESC(max_ring_page_order, "Maximum order of pages to be used for the > #define RINGREF_NAME_LEN (20) > > /* > + * Per-ring info. > + * Every blkfront device can associate with one or more blkfront_ring_info, > + * depending on how many hardware queues to be used. > + */ > +struct blkfront_ring_info > +{ > + struct blkif_front_ring ring; > + unsigned int ring_ref[XENBUS_MAX_RING_PAGES]; > + unsigned int evtchn, irq; > + struct work_struct work; > + struct gnttab_free_callback callback; > + struct blk_shadow shadow[BLK_MAX_RING_SIZE]; > + struct list_head grants; > + struct list_head indirect_pages; > + unsigned int persistent_gnts_c; persistent grants should be per-device, not per-queue IMHO. Is it really hard to make this global instead of per-queue? > + unsigned long shadow_free; > + struct blkfront_dev_info *dinfo; > +}; > + > +/* > * We have one of these per vbd, whether ide, scsi or 'other'. They > * hang in private_data off the gendisk structure. We may end up > * putting all kinds of interesting stuff here :-) > */ > -struct blkfront_info > -{ > +struct blkfront_dev_info { IMHO, you can leave this as blkfront_info (unless I'm missing something). > spinlock_t io_lock; Shouldn't the spinlock be per-queue instead of per-device? > struct mutex mutex; > struct xenbus_device *xbdev; > @@ -129,18 +148,7 @@ struct blkfront_info > int vdevice; > blkif_vdev_t handle; > enum blkif_state connected; > - int ring_ref[XENBUS_MAX_RING_PAGES]; > - unsigned int nr_ring_pages; > - struct blkif_front_ring ring; > - unsigned int evtchn, irq; > struct request_queue *rq; > - struct work_struct work; > - struct gnttab_free_callback callback; > - struct blk_shadow shadow[BLK_MAX_RING_SIZE]; > - struct list_head grants; > - struct list_head indirect_pages; > - unsigned int persistent_gnts_c; > - unsigned long shadow_free; > unsigned int feature_flush; > unsigned int feature_discard:1; > unsigned int feature_secdiscard:1; > @@ -149,7 +157,9 @@ struct blkfront_info > unsigned int feature_persistent:1; > unsigned int max_indirect_segments; > int is_ready; > + unsigned int nr_ring_pages; Spurious change? You are removing it in the chunk above and adding it back here. [...] > @@ -246,33 +257,33 @@ out_of_memory: > } > > static struct grant *get_grant(grant_ref_t *gref_head, > - unsigned long pfn, > - struct blkfront_info *info) > + unsigned long pfn, > + struct blkfront_ring_info *rinfo) Indentation? (or my email client is mangling emails one more time...) In order to make this easier to review, do you think you can leave blkfront_info as "info" for now, and do the renaming to dinfo in a later patch. That would help figuring out mechanical name changes from the actual meat of the patch. Roger.