Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1241352 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 8/9] xen/blkback: pseudo support for multi hardware  queues/rings Bob Liu <bob.liu@oracle.com> - 2015-10-07 13:00 +0200
    Re: [PATCH v3 8/9] xen/blkback: pseudo support for multi hardware  queues/rings Roger Pau Monné <roger.pau@citrix.com> - 2015-10-07 13:50 +0200

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

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.

>>  
>>  	/* Not ready to connect? */
>> -	if (!blkif->ring.irq || !blkif->vbd.bdev)
>> +	if (!blkif->rings || !blkif->rings[0].irq || !blkif->vbd.bdev)
>>  		return;
>>  
>>  	/* Already connected? */
>> @@ -113,19 +116,68 @@ static void xen_update_blkif_status(struct xen_blkif *blkif)
>>  	}
>>  	invalidate_inode_pages2(blkif->vbd.bdev->bd_inode->i_mapping);
>>  
>> -	blkif->ring.xenblkd = kthread_run(xen_blkif_schedule, &blkif->ring, "%s", name);
>> -	if (IS_ERR(blkif->ring.xenblkd)) {
>> -		err = PTR_ERR(blkif->ring.xenblkd);
>> -		blkif->ring.xenblkd = NULL;
>> -		xenbus_dev_error(blkif->be->dev, err, "start xenblkd");
>> -		return;
>> +	if (blkif->nr_rings == 1) {
>> +		blkif->rings[0].xenblkd = kthread_run(xen_blkif_schedule, &blkif->rings[0], "%s", name);
>> +		if (IS_ERR(blkif->rings[0].xenblkd)) {
>> +			err = PTR_ERR(blkif->rings[0].xenblkd);
>> +			blkif->rings[0].xenblkd = NULL;
>> +			xenbus_dev_error(blkif->be->dev, err, "start xenblkd");
>> +			return;
>> +		}
>> +	} else {
>> +		for (i = 0; i < blkif->nr_rings; i++) {
>> +			snprintf(per_ring_name, BLKBACK_NAME_LEN + 2, "%s-%d", name, i);
>> +			ring = &blkif->rings[i];
>> +			ring->xenblkd = kthread_run(xen_blkif_schedule, ring, "%s", per_ring_name);
>> +			if (IS_ERR(ring->xenblkd)) {
>> +				err = PTR_ERR(ring->xenblkd);
>> +				ring->xenblkd = NULL;
>> +				xenbus_dev_error(blkif->be->dev, err,
>> +						"start %s xenblkd", per_ring_name);
>> +				return;
>> +			}
>> +		}
>> +	}
>> +}
>> +
>> +static int xen_blkif_alloc_rings(struct xen_blkif *blkif)
>> +{
>> +	struct xen_blkif_ring *ring;
>> +	int r;
>> +
>> +	blkif->rings = kzalloc(blkif->nr_rings * sizeof(struct xen_blkif_ring), GFP_KERNEL);
>> +	if (!blkif->rings)
>> +		return -ENOMEM;
>> +
>> +	for (r = 0; r < blkif->nr_rings; r++) {
>> +		ring = &blkif->rings[r];
>> +
>> +		spin_lock_init(&ring->blk_ring_lock);
>> +		init_waitqueue_head(&ring->wq);
>> +		ring->st_print = jiffies;
>> +		ring->persistent_gnts.rb_node = NULL;
>> +		spin_lock_init(&ring->free_pages_lock);
>> +		INIT_LIST_HEAD(&ring->free_pages);
>> +		INIT_LIST_HEAD(&ring->persistent_purge_list);
>> +		ring->free_pages_num = 0;
>> +		atomic_set(&ring->persistent_gnt_in_use, 0);
>> +		atomic_set(&ring->inflight, 0);
>> +		INIT_WORK(&ring->persistent_purge_work, xen_blkbk_unmap_purged_grants);
>> +		INIT_LIST_HEAD(&ring->pending_free);
>> +
>> +		spin_lock_init(&ring->pending_free_lock);
>> +		init_waitqueue_head(&ring->pending_free_wq);
>> +		init_waitqueue_head(&ring->shutdown_wq);
> 
> I've already commented on the previous patch, but a bunch of this needs
> to be per-device rather than per-ring.
> 

Will update and all other comments.

Thanks
-Bob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1241407

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web