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

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

On 16/01/14 10:28, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew J. Bennieston [mailto:andrew.bennieston@citrix.com]
>> Sent: 15 January 2014 16:23
>> To: xen-devel@lists.xenproject.org
>> Cc: Ian Campbell; Wei Liu; Paul Durrant; Andrew Bennieston
>> Subject: [PATCH RFC 2/4] xen-netback: Add support for multiple queues
>>
>> From: "Andrew J. Bennieston" <andrew.bennieston@citrix.com>
>>
>> Builds on the refactoring of the previous patch to implement multiple
>> queues between xen-netfront and xen-netback.
>>
>> Writes the maximum supported number of queues into XenStore, and reads
>> the values written by the frontend to determine how many queues to use.
>>
>> Ring references and event channels are read from XenStore on a per-queue
>> basis and rings are connected accordingly.
>>
>> Signed-off-by: Andrew J. Bennieston <andrew.bennieston@citrix.com>
>> ---
>>   drivers/net/xen-netback/common.h    |    2 +
>>   drivers/net/xen-netback/interface.c |    8 +++-
>>   drivers/net/xen-netback/netback.c   |    3 ++
>>   drivers/net/xen-netback/xenbus.c    |   70
>> ++++++++++++++++++++++++++++++-----
>>   4 files changed, 72 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-
>> netback/common.h
>> index 54d2eeb..97efd09 100644
>> --- a/drivers/net/xen-netback/common.h
>> +++ b/drivers/net/xen-netback/common.h
>> @@ -254,4 +254,6 @@ void xenvif_carrier_on(struct xenvif *vif);
>>
>>   extern bool separate_tx_rx_irq;
>>
>> +extern unsigned int xenvif_max_queues;
>> +
>>   #endif /* __XEN_NETBACK__COMMON_H__ */
>> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-
>> netback/interface.c
>> index 0113324..0234ff0 100644
>> --- a/drivers/net/xen-netback/interface.c
>> +++ b/drivers/net/xen-netback/interface.c
>> @@ -355,7 +355,13 @@ struct xenvif *xenvif_alloc(struct device *parent,
>> domid_t domid,
>>   	char name[IFNAMSIZ] = {};
>>
>>   	snprintf(name, IFNAMSIZ - 1, "vif%u.%u", domid, handle);
>> -	dev = alloc_netdev_mq(sizeof(struct xenvif), name, ether_setup, 1);
>> +	/*
>> +	 * Allocate a netdev with the max. supported number of queues.
>> +	 * When the guest selects the desired number, it will be updated
>> +	 * via netif_set_real_num_tx_queues().
>> +	 */
>> +	dev = alloc_netdev_mq(sizeof(struct xenvif), name, ether_setup,
>> +			xenvif_max_queues);
>>   	if (dev == NULL) {
>>   		pr_warn("Could not allocate netdev for %s\n", name);
>>   		return ERR_PTR(-ENOMEM);
>> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-
>> netback/netback.c
>> index 586e741..5d717d7 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -55,6 +55,9 @@
>>   bool separate_tx_rx_irq = 1;
>>   module_param(separate_tx_rx_irq, bool, 0644);
>>
>> +unsigned int xenvif_max_queues = 4;
>> +module_param(xenvif_max_queues, uint, 0644);
>> +
>>   /*
>>    * This is the maximum slots a skb can have. If a guest sends a skb
>>    * which exceeds this limit it is considered malicious.
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-
>> netback/xenbus.c
>> index c3332e2..ce7ca9a 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -21,6 +21,7 @@
>>
>>   #include "common.h"
>>   #include <linux/vmalloc.h>
>> +#include <linux/rtnetlink.h>
>>
>>   struct backend_info {
>>   	struct xenbus_device *dev;
>> @@ -160,6 +161,14 @@ static int netback_probe(struct xenbus_device *dev,
>>   	if (err)
>>   		pr_debug("Error writing feature-split-event-channels\n");
>>
>> +	/*
>> +	 * Multi-queue support: This is an optional feature.
>> +	 */
>
> Comment style. Did you run checkpatch?
>
>> +	err = xenbus_printf(XBT_NIL, dev->nodename,
>> +			"multi-queue-max-queues", "%u",
>> xenvif_max_queues);
>> +	if (err)
>> +		pr_debug("Error writing multi-queue-max-queues\n");
>> +
>>   	err = xenbus_switch_state(dev, XenbusStateInitWait);
>>   	if (err)
>>   		goto fail;
>> @@ -491,6 +500,16 @@ static void connect(struct backend_info *be)
>>   	unsigned long credit_bytes, credit_usec;
>>   	unsigned int queue_index;
>>   	struct xenvif_queue *queue;
>> +	unsigned int requested_num_queues;
>> +
>> +	/* Check whether the frontend requested multiple queues
>> +	 * and read the number requested.
>> +	 */
>> +	err = xenbus_scanf(XBT_NIL, dev->otherend,
>> +			"multi-queue-num-queues",
>> +			"%u", &requested_num_queues);
>> +	if (err < 0)
>> +		requested_num_queues = 1; /* Fall back to single queue */
>>
>>   	err = xen_net_read_mac(dev, be->vif->fe_dev_addr);
>>   	if (err) {
>> @@ -501,9 +520,13 @@ static void connect(struct backend_info *be)
>>   	xen_net_read_rate(dev, &credit_bytes, &credit_usec);
>>   	read_xenbus_vif_flags(be);
>>
>> -	be->vif->num_queues = 1;
>> +	/* Use the number of queues requested by the frontend */
>> +	be->vif->num_queues = requested_num_queues;
>>   	be->vif->queues = vzalloc(be->vif->num_queues *
>>   			sizeof(struct xenvif_queue));
>> +	rtnl_lock();
>> +	netif_set_real_num_tx_queues(be->vif->dev, be->vif-
>>> num_queues);
>> +	rtnl_unlock();
>>
>>   	for (queue_index = 0; queue_index < be->vif->num_queues;
>> ++queue_index)
>>   	{
>> @@ -549,29 +572,51 @@ static int connect_rings(struct backend_info *be,
>> struct xenvif_queue *queue)
>>   	unsigned long tx_ring_ref, rx_ring_ref;
>>   	unsigned int tx_evtchn, rx_evtchn;
>>   	int err;
>> +	char *xspath = NULL;
>> +	size_t xspathsize;
>> +
>> +	/* If the frontend requested 1 queue, or we have fallen back
>> +	 * to single queue due to lack of frontend support for multi-
>> +	 * queue, expect the remaining XenStore keys in the toplevel
>> +	 * directory. Otherwise, expect them in a subdirectory called
>> +	 * queue-N.
>> +	 */
>> +	if (queue->vif->num_queues == 1)
>> +		xspath = (char *)dev->otherend;
>> +	else {
>> +		xspathsize = strlen(dev->otherend) + 10;
>
> Magic number.
>
>    Paul
>
Yes, I'll change this to a defined constant.

Andrew

>> +		xspath = kzalloc(xspathsize, GFP_KERNEL);
>> +		if (!xspath) {
>> +			xenbus_dev_fatal(dev, -ENOMEM,
>> +					"reading ring references");
>> +			return -ENOMEM;
>> +		}
>> +		snprintf(xspath, xspathsize, "%s/queue-%u", dev-
>>> otherend,
>> +				queue->number);
>> +	}
>>
>> -	err = xenbus_gather(XBT_NIL, dev->otherend,
>> +	err = xenbus_gather(XBT_NIL, xspath,
>>   			    "tx-ring-ref", "%lu", &tx_ring_ref,
>>   			    "rx-ring-ref", "%lu", &rx_ring_ref, NULL);
>>   	if (err) {
>>   		xenbus_dev_fatal(dev, err,
>>   				 "reading %s/ring-ref",
>> -				 dev->otherend);
>> -		return err;
>> +				 xspath);
>> +		goto err;
>>   	}
>>
>>   	/* Try split event channels first, then single event channel. */
>> -	err = xenbus_gather(XBT_NIL, dev->otherend,
>> +	err = xenbus_gather(XBT_NIL, xspath,
>>   			    "event-channel-tx", "%u", &tx_evtchn,
>>   			    "event-channel-rx", "%u", &rx_evtchn, NULL);
>>   	if (err < 0) {
>> -		err = xenbus_scanf(XBT_NIL, dev->otherend,
>> +		err = xenbus_scanf(XBT_NIL, xspath,
>>   				   "event-channel", "%u", &tx_evtchn);
>>   		if (err < 0) {
>>   			xenbus_dev_fatal(dev, err,
>>   					 "reading %s/event-channel(-tx/rx)",
>> -					 dev->otherend);
>> -			return err;
>> +					 xspath);
>> +			goto err;
>>   		}
>>   		rx_evtchn = tx_evtchn;
>>   	}
>> @@ -584,10 +629,15 @@ static int connect_rings(struct backend_info *be,
>> struct xenvif_queue *queue)
>>   				 "mapping shared-frames %lu/%lu port tx %u
>> rx %u",
>>   				 tx_ring_ref, rx_ring_ref,
>>   				 tx_evtchn, rx_evtchn);
>> -		return err;
>> +		goto err;
>>   	}
>>
>> -	return 0;
>> +	err = 0;
>> +err: /* Regular return falls through with err == 0 */
>> +	if (xspath != dev->otherend)
>> +		kfree(xspath);
>> +
>> +	return err;
>>   }
>>
>>   static int read_xenbus_vif_flags(struct backend_info *be)
>> --
>> 1.7.10.4
>