From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Bennieston Subject: Re: [PATCH RFC 2/4] xen-netback: Add support for multiple queues Date: Thu, 16 Jan 2014 10:04:49 +0000 Message-ID: <52D7AEC1.2070302@citrix.com> References: <1389803004-31812-1-git-send-email-andrew.bennieston@citrix.com> <1389803004-31812-3-git-send-email-andrew.bennieston@citrix.com> <20140116001807.GH5331@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1W3jok-0007iu-2H for xen-devel@lists.xenproject.org; Thu, 16 Jan 2014 10:04:54 +0000 In-Reply-To: <20140116001807.GH5331@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: xen-devel@lists.xenproject.org, paul.durrant@citrix.com, ian.campbell@citrix.com List-Id: xen-devel@lists.xenproject.org On 16/01/14 00:18, Wei Liu wrote: > On Wed, Jan 15, 2014 at 04:23:22PM +0000, Andrew J. Bennieston wrote: > [...] >> --- 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); > > Indentation. > > >> 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 looks a bit arbitrary. I guess it is better to set the default > value to number of CPUs in Dom0? > It is quite arbitrary. A default of the number of dom0 CPUs makes sense. I'll change it. >> /* >> * 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 >> +#include >> >> 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. >> + */ >> + err = xenbus_printf(XBT_NIL, dev->nodename, >> + "multi-queue-max-queues", "%u", xenvif_max_queues); > > Should prefix this with "feature-". > This isn't a feature flag, it is a parameter specifying the maximum number of queues the backend will allow. It implies feature-multi-queue, which is not written. Way back in 2013 I posted an RFC for the XenStore keys to negotiate this and there was some disagreement on how to approach this particular bit... One argument went that having a feature-multi-queue was redundant if you were also writing multi-queue-max-queues (regardless of the value that was specified). Another argument was that having feature-multi-queue allowed you to grep the xenstore-ls output for all supported features. I decided to take this approach, but I'm not committed to it. Prefixing this with feature- probably isn't right, though; the feature-* keys all appear to be boolean flags indicating whether a feature is supported; this communicates a further value. A simple change would be to add the (redundant) feature-multi-queue = 1 flag... >> + 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 */ > > You also need to check whether it gets back something larger than > permitted -- guest can be buggy or malicious. > Ah, yes. I missed that. >> >> 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. >> + */ > > I think even if the frontend only requests 1 queue you can still put it > under subdirectory? I don't have strong preference here though... There would still be some logic necessary to determine where to read these values for old frontends vs new frontends, so having the 1-queue case common to both made the most sense, to me. We'll always have to support new-backend/old-frontend and old-backend/new-frontend combinations so both will (always) have to check and do the appropriate thing here, whatever choice is made. > > After the protocol is settled it needs to be documented in netif.h. > Indeed. I'll wait a couple of iterations of these patches so the protocol can stabilise before doing that. >> + if (queue->vif->num_queues == 1) >> + xspath = (char *)dev->otherend; >> + else { >> + xspathsize = strlen(dev->otherend) + 10; > > Please avoid using magic number. > >> + xspath = kzalloc(xspathsize, GFP_KERNEL); >> + if (!xspath) { >> + xenbus_dev_fatal(dev, -ENOMEM, >> + "reading ring references"); > > "ring reference"? > Hmm, I probably need to reword that. Andrew. >> + return -ENOMEM; >> + } >> + snprintf(xspath, xspathsize, "%s/queue-%u", dev->otherend, >> + queue->number); > > Indentation. > > Wei. >