| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 2 Oct 2015 19:02:02 +0200
From: Roger Pau Monné <roger.pau@...rix.com>
To: Bob Liu <bob.liu@...cle.com>, <xen-devel@...ts.xen.org>
CC: <david.vrabel@...rix.com>, <linux-kernel@...r.kernel.org>,
<konrad.wilk@...cle.com>, <felipe.franciosi@...rix.com>,
<axboe@...com>, <hch@...radead.org>, <avanzini.arianna@...il.com>,
<rafal.mielniczuk@...rix.com>, <boris.ostrovsky@...cle.com>,
<jonathan.davies@...rix.com>
Subject: Re: [PATCH v3 3/9] xen/blkfront: separate per ring information out of
device info
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@...il.com>
> Signed-off-by: Bob Liu <bob.liu@...cle.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists