Re: [PATCH RFC 4/4] xen-netfront: Add support for multiple queues

[Andrew Bennieston <andrew.bennieston@citrix.com>]

On 16/01/14 00:27, Wei Liu wrote:
> On Wed, Jan 15, 2014 at 04:23:24PM +0000, Andrew J. Bennieston wrote:
> [...]
>> +/* Module parameters */
>> +unsigned int xennet_max_queues = 16;
>> +module_param(xennet_max_queues, uint, 0644);
>> +
>
> This looks quite arbitrary as well.

Arbitrary, but less arbitrary than the netback parameter. I wanted to
pick a value that was large enough to accommodate most sensible
backend-provided limits, but not so large that it wasted resources. It
is necessary only because I have to call alloc_etherdev_mq() long before
I can read the maximum number of queues from XenStore. This value gets
used there, so I wanted it to be at least as big as anything sensible
that a backend might provide.

>>   static const struct ethtool_ops xennet_ethtool_ops;
>>
> [...]
>> +static int write_queue_xenstore_keys(struct netfront_queue *queue,
>> +			   struct xenbus_transaction *xbt, int write_hierarchical)
>> +{
>> +	/* Write the queue-specific keys into XenStore in the traditional
>> +	 * way for a single queue, or in a queue subkeys for multiple
>> +	 * queues.
>> +	 */
>> +	struct xenbus_device *dev = queue->info->xbdev;
>> +	int err;
>> +	const char *message;
>> +	char *path;
>> +	size_t pathsize;
>> +
>> +	/* Choose the correct place to write the keys */
>> +	if (write_hierarchical) {
>> +		pathsize = strlen(dev->nodename) + 10;
>> +		path = kzalloc(pathsize, GFP_KERNEL);
>> +		if (!path) {
>> +			err = -ENOMEM;
>> +			message = "writing ring references";
>
> This error message doesn't sound right.
>

I'll reword it.

>> +			goto error;
>> +		}
>> +		snprintf(path, pathsize, "%s/queue-%u",
>> +				dev->nodename, queue->number);
>> +	}
>> +	else
>> +		path = (char *)dev->nodename;
>
> Coding style. Should be surounded by {};
>

OK.

>> +
> [...]
>> @@ -1740,10 +1838,17 @@ static int talk_to_netback(struct xenbus_device *dev,
>>   	int err;
>>   	unsigned int feature_split_evtchn;
>>   	unsigned int i = 0;
>> +	unsigned int max_queues = 0;
>>   	struct netfront_queue *queue = NULL;
>>
>>   	info->netdev->irq = 0;
>>
>> +	/* Check if backend supports multiple queues */
>> +	err = xenbus_scanf(XBT_NIL, info->xbdev->otherend,
>> +			"multi-queue-max-queues", "%u", &max_queues);
>> +	if (err < 0)
>> +		max_queues = 1;
>> +
>
> Need to check if backend provide too big a number for frontend.

Yes, I'll add that check.

Andrew.

>
> Wei.
>