Re: [Xen-devel] [PATCH v3 3/9] xen/blkfront: separate per ring information out of device info

[Roger Pau Monnà <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxx>
> Signed-off-by: Bob Liu <bob.liu@xxxxxxxxxx>
> ---
>  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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel