lkml.org 
[lkml]   [2015]   [Oct]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 3/9] xen/blkfront: separate per ring information out of device info
From
Date
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.


\
 
 \ /
  Last update: 2015-10-02 19:21    [W:1.260 / U:0.488 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site