Re: [PATCH v3 8/9] xen/blkback: pseudo support for multi hardware queues/rings

["Roger Pau Monné" <roger.pau@citrix.com>]

El 07/10/15 a les 12.50, Bob Liu ha escrit:
> 
> On 10/05/2015 11:08 PM, Roger Pau Monné wrote:
>> El 05/09/15 a les 14.39, Bob Liu ha escrit:
>>> Prepare patch for multi hardware queues/rings, the ring number was set to 1 by
>>> force.
>>
>> This should be:
>>
>> Preparatory patch for multiple hardware queues (rings). The number of
>> rings is unconditionally set to 1.
>>
>> But frankly this description is not helpful at all, you should describe
>> the preparatory changes and why you need them.
>>
> 
> Will update, the purpose is just to make each patch smaller and more readable.
> 
>>>
>>> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
>>> Signed-off-by: Bob Liu <bob.liu@oracle.com>
>>> ---
>>>  drivers/block/xen-blkback/common.h |    3 +-
>>>  drivers/block/xen-blkback/xenbus.c |  328 +++++++++++++++++++++++-------------
>>>  2 files changed, 209 insertions(+), 122 deletions(-)
>>>
>>> diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
>>> index cc253d4..ba058a0 100644
>>> --- a/drivers/block/xen-blkback/common.h
>>> +++ b/drivers/block/xen-blkback/common.h
>>> @@ -339,7 +339,8 @@ struct xen_blkif {
>>>  	unsigned long long			st_wr_sect;
>>>  	unsigned int nr_ring_pages;
>>>  	/* All rings for this device */
>>> -	struct xen_blkif_ring ring;
>>> +	struct xen_blkif_ring *rings;
>>> +	unsigned int nr_rings;
>>>  };
>>>  
>>>  struct seg_buf {
>>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
>>> index 6482ee3..04b8d0d 100644
>>> --- a/drivers/block/xen-blkback/xenbus.c
>>> +++ b/drivers/block/xen-blkback/xenbus.c
>>> @@ -26,6 +26,7 @@
>>>  /* Enlarge the array size in order to fully show blkback name. */
>>>  #define BLKBACK_NAME_LEN (20)
>>>  #define RINGREF_NAME_LEN (20)
>>> +#define RINGREF_NAME_LEN (20)
>>
>> Duplicate define?
>>
> 
> Will update.
> 
>>>  
>>>  struct backend_info {
>>>  	struct xenbus_device	*dev;
>>> @@ -84,11 +85,13 @@ static int blkback_name(struct xen_blkif *blkif, char *buf)
>>>  
>>>  static void xen_update_blkif_status(struct xen_blkif *blkif)
>>>  {
>>> -	int err;
>>> +	int err, i;
>>>  	char name[BLKBACK_NAME_LEN];
>>> +	struct xen_blkif_ring *ring;
>>> +	char per_ring_name[BLKBACK_NAME_LEN + 2];
>>
>> Hm, why don't you just add + 2 to the place where BLKBACK_NAME_LEN is
>> defined and use the same character array ("name")? This is just a waste
>> of stack.
>>
> 
> per_ring_name = name + 'queue number';
> If reuse name[], I'm not sure whether
> snprintf(name, BLKBACK_NAME_LEN + 2, "%s-%d", name, i); can work.

What I would like to avoid is having two character arrays in the stack
when IMHO we can achieve the same with a single array that's long enough.

Roger.