From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Bob Liu <bob.liu@oracle.com>, <xen-devel@lists.xen.org>
Cc: <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 3/9] xen/blkfront: separate per ring information out of device info
Date: Fri, 2 Oct 2015 19:02:02 +0200 [thread overview]
Message-ID: <560EB88A.9070905@citrix.com> (raw)
In-Reply-To: <1441456782-31318-4-git-send-email-bob.liu@oracle.com>
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 <avanzini.arianna@gmail.com>
> Signed-off-by: Bob Liu <bob.liu@oracle.com>
> ---
> 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.
next prev parent reply other threads:[~2015-10-02 17:02 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-05 12:39 [PATCH v3 0/9] xen-block: support multi hardware-queues/rings Bob Liu
2015-09-05 12:39 ` [PATCH v3 1/9] xen-blkfront: convert to blk-mq APIs Bob Liu
2015-09-23 20:31 ` Konrad Rzeszutek Wilk
2015-09-23 21:12 ` Konrad Rzeszutek Wilk
2015-09-05 12:39 ` [PATCH v3 2/9] xen-block: add document for mutli hardware queues/rings Bob Liu
2015-09-23 20:32 ` Konrad Rzeszutek Wilk
2015-10-02 16:04 ` Roger Pau Monné
2015-10-02 16:12 ` [Xen-devel] " Wei Liu
2015-10-02 16:22 ` Roger Pau Monné
2015-10-02 23:55 ` Bob Liu
2015-09-05 12:39 ` [PATCH v3 3/9] xen/blkfront: separate per ring information out of device info Bob Liu
2015-10-02 17:02 ` Roger Pau Monné [this message]
2015-10-03 0:34 ` Bob Liu
2015-10-05 15:17 ` Roger Pau Monné
2015-10-10 8:30 ` Bob Liu
2015-10-19 9:42 ` Roger Pau Monné
2015-09-05 12:39 ` [PATCH v3 4/9] xen/blkfront: pseudo support for multi hardware queues/rings Bob Liu
2015-10-05 10:52 ` Roger Pau Monné
2015-10-07 10:28 ` Bob Liu
2015-09-05 12:39 ` [PATCH v3 5/9] xen/blkfront: convert per device io_lock to per ring ring_lock Bob Liu
2015-10-05 14:13 ` Roger Pau Monné
2015-10-07 10:34 ` Bob Liu
2015-09-05 12:39 ` [PATCH v3 6/9] xen/blkfront: negotiate the number of hw queues/rings with backend Bob Liu
2015-10-05 14:40 ` Roger Pau Monné
2015-10-07 10:39 ` Bob Liu
2015-10-07 11:46 ` Roger Pau Monné
2015-10-07 12:19 ` Bob Liu
2015-09-05 12:39 ` [PATCH v3 7/9] xen/blkback: separate ring information out of struct xen_blkif Bob Liu
2015-10-05 14:55 ` Roger Pau Monné
2015-10-07 10:41 ` Bob Liu
2015-10-10 4:08 ` Bob Liu
2015-10-19 9:36 ` Roger Pau Monné
2015-10-19 10:03 ` Bob Liu
2015-10-05 14:55 ` Roger Pau Monné
2015-09-05 12:39 ` [PATCH v3 8/9] xen/blkback: pseudo support for multi hardware queues/rings Bob Liu
2015-10-05 15:08 ` Roger Pau Monné
2015-10-07 10:50 ` Bob Liu
2015-10-07 11:49 ` Roger Pau Monné
2015-09-05 12:39 ` [PATCH v3 9/9] xen/blkback: get number of hardware queues/rings from blkfront Bob Liu
2015-10-05 15:15 ` Roger Pau Monné
2015-10-07 10:54 ` Bob Liu
2015-10-02 9:57 ` [PATCH v3 0/9] xen-block: support multi hardware-queues/rings Rafal Mielniczuk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=560EB88A.9070905@citrix.com \
--to=roger.pau@citrix.com \
--cc=avanzini.arianna@gmail.com \
--cc=axboe@fb.com \
--cc=bob.liu@oracle.com \
--cc=boris.ostrovsky@oracle.com \
--cc=david.vrabel@citrix.com \
--cc=felipe.franciosi@citrix.com \
--cc=hch@infradead.org \
--cc=jonathan.davies@citrix.com \
--cc=konrad.wilk@oracle.com \
--cc=linux-kernel@vger.kernel.org \
--cc=rafal.mielniczuk@citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).