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

[Bob Liu <bob.liu@oracle.com>]

On 10/03/2015 01:02 AM, Roger Pau Monné wrote:
> 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?
> 

The most important thing is keep changes minimal for better review at this stage.
I'll check which way has the least modification.

>> +	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?
> 

That's in another patch for better review.
'[PATCH v3 5/9] xen/blkfront: convert per device io_lock to per ring ring_lock' will do that.

>>  	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.
> 

Will be fix.

> [...]
> 
>> @@ -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...)
> 

Will be fix.

> 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.
> 

That's what I have done in v2, but believe me it's more difficult to read and review.
They are a lot of messed place combined with info and rinfo, when seeing an info you have to think 
whether is device info or ring info. It's more straightforward to use dinfo and rinfo to distinguish at the beginning.

-- 
Regards,
-Bob