| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <53074490.8030806@citrix.com>
Date: Fri, 21 Feb 2014 12:20:32 +0000
From: Andrew Bennieston <andrew.bennieston@...rix.com>
To: Paul Durrant <Paul.Durrant@...rix.com>,
"xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>
CC: Ian Campbell <Ian.Campbell@...rix.com>,
Wei Liu <wei.liu2@...rix.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
David Vrabel <david.vrabel@...rix.com>
Subject: Re: [PATCH V4 net-next 2/5] xen-netback: Add support for multiple
queues
On 21/02/14 12:13, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew J. Bennieston [mailto:andrew.bennieston@...rix.com]
>> Sent: 17 February 2014 17:58
>> To: xen-devel@...ts.xenproject.org
>> Cc: Ian Campbell; Wei Liu; Paul Durrant; netdev@...r.kernel.org; David
>> Vrabel; Andrew Bennieston
>> Subject: [PATCH V4 net-next 2/5] xen-netback: Add support for multiple
>> queues
>>
>> From: "Andrew J. Bennieston" <andrew.bennieston@...rix.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@...rix.com>
>> ---
>> drivers/net/xen-netback/common.h | 2 +
>> drivers/net/xen-netback/interface.c | 7 +++-
>> drivers/net/xen-netback/netback.c | 8 ++++
>> drivers/net/xen-netback/xenbus.c | 76
>> ++++++++++++++++++++++++++++++-----
>> 4 files changed, 82 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-
>> netback/common.h
>> index 2550867..8180929 100644
>> --- a/drivers/net/xen-netback/common.h
>> +++ b/drivers/net/xen-netback/common.h
>> @@ -261,4 +261,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 daf93f6..bc7a82d 100644
>> --- a/drivers/net/xen-netback/interface.c
>> +++ b/drivers/net/xen-netback/interface.c
>> @@ -373,7 +373,12 @@ 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 46b2f5b..64d66a1 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -54,6 +54,11 @@
>> bool separate_tx_rx_irq = 1;
>> module_param(separate_tx_rx_irq, bool, 0644);
>>
>> +unsigned int xenvif_max_queues;
>> +module_param(xenvif_max_queues, uint, 0644);
>> +MODULE_PARM_DESC(xenvif_max_queues,
>> + "Maximum number of queues per virtual interface");
>> +
>> /*
>> * This is the maximum slots a skb can have. If a guest sends a skb
>> * which exceeds this limit it is considered malicious.
>> @@ -1585,6 +1590,9 @@ static int __init netback_init(void)
>> if (!xen_domain())
>> return -ENODEV;
>>
>> + /* Allow as many queues as there are CPUs, by default */
>> + xenvif_max_queues = num_online_cpus();
>> +
>> if (fatal_skb_slots < XEN_NETBK_LEGACY_SLOTS_MAX) {
>> pr_info("fatal_skb_slots too small (%d), bump it to
>> XEN_NETBK_LEGACY_SLOTS_MAX (%d)\n",
>> fatal_skb_slots, XEN_NETBK_LEGACY_SLOTS_MAX);
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-
>> netback/xenbus.c
>> index f23ea0a..d11f51e 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -20,6 +20,7 @@
>>
>> #include "common.h"
>> #include <linux/vmalloc.h>
>> +#include <linux/rtnetlink.h>
>>
>> struct backend_info {
>> struct xenbus_device *dev;
>> @@ -159,6 +160,12 @@ 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);
>> + if (err)
>> + pr_debug("Error writing multi-queue-max-queues\n");
>> +
>> err = xenbus_switch_state(dev, XenbusStateInitWait);
>> if (err)
>> goto fail;
>> @@ -490,6 +497,23 @@ 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 */
>> + } else if (requested_num_queues > xenvif_max_queues) {
>> + /* buggy or malicious guest */
>> + xenbus_dev_fatal(dev, err,
>> + "guest requested %u queues, exceeding the
>> maximum of %u.",
>> + requested_num_queues, xenvif_max_queues);
>> + return;
>> + }
>>
>> err = xen_net_read_mac(dev, be->vif->fe_dev_addr);
>> if (err) {
>> @@ -500,9 +524,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) {
>> queue = &be->vif->queues[queue_index];
>> @@ -547,29 +575,52 @@ 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;
>
> I don't think you need the NULL init here. xspath is set in both branches of the if statement below.
Indeed, but I prefer to initialise things sanely where possible. It
makes it easier to spot problems with later modifications of the code,
e.g. if one of those branches changed.
Andrew.
>
> Paul
>
>> + size_t xspathsize;
>> + const size_t xenstore_path_ext_size = 11; /* sufficient for "/queue-
>> NNN" */
>> +
>> + /* 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) +
>> xenstore_path_ext_size;
>> + 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->id);
>> + }
>>
>> - 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;
>> }
>> @@ -582,10 +633,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
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists