[PATCH 3/4] Support accelerated network plugin modules

[PATCH 3/4] Support accelerated network plugin modules

[Kieran Mansley]

[-- Attachment #1: Type: text/plain, Size: 222 bytes --]

Add hooks in the netfront driver to allow an accelerated plugin module
to attach and provide a fast path for network traffic in the presence of
a virtualisable NIC.

Signed-off-by: Kieran Mansley <kmansley@solarflare.com>

[-- Attachment #2: frontend_accel --]
[-- Type: text/plain, Size: 26496 bytes --]

Frontend net driver acceleration

diff -r 325afaed01ff linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c
--- a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Tue Apr 17 09:07:31 2007 +0100
+++ b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Tue Apr 17 09:13:05 2007 +0100
@@ -3,6 +3,7 @@
  *
  * Copyright (c) 2002-2005, K A Fraser
  * Copyright (c) 2005, XenSource Ltd
+ * Copyright (C) 2007 Solarflare Communications, Inc.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License version 2
@@ -67,6 +68,8 @@
 #include <xen/platform-compat.h>
 #endif
 
+#include "netfront.h"
+
 /*
  * Mutually-exclusive module options to select receive data path:
  *  rx_copy : Packets are copied by network backend into local memory
@@ -137,57 +140,6 @@ static inline int netif_needs_gso(struct
 
 #define GRANT_INVALID_REF	0
 
-#define NET_TX_RING_SIZE __RING_SIZE((struct netif_tx_sring *)0, PAGE_SIZE)
-#define NET_RX_RING_SIZE __RING_SIZE((struct netif_rx_sring *)0, PAGE_SIZE)
-
-struct netfront_info {
-	struct list_head list;
-	struct net_device *netdev;
-
-	struct net_device_stats stats;
-
-	struct netif_tx_front_ring tx;
-	struct netif_rx_front_ring rx;
-
-	spinlock_t   tx_lock;
-	spinlock_t   rx_lock;
-
-	unsigned int irq;
-	unsigned int copying_receiver;
-	unsigned int carrier;
-
-	/* Receive-ring batched refills. */
-#define RX_MIN_TARGET 8
-#define RX_DFL_MIN_TARGET 64
-#define RX_MAX_TARGET min_t(int, NET_RX_RING_SIZE, 256)
-	unsigned rx_min_target, rx_max_target, rx_target;
-	struct sk_buff_head rx_batch;
-
-	struct timer_list rx_refill_timer;
-
-	/*
-	 * {tx,rx}_skbs store outstanding skbuffs. The first entry in tx_skbs
-	 * is an index into a chain of free entries.
-	 */
-	struct sk_buff *tx_skbs[NET_TX_RING_SIZE+1];
-	struct sk_buff *rx_skbs[NET_RX_RING_SIZE];
-
-#define TX_MAX_TARGET min_t(int, NET_RX_RING_SIZE, 256)
-	grant_ref_t gref_tx_head;
-	grant_ref_t grant_tx_ref[NET_TX_RING_SIZE + 1];
-	grant_ref_t gref_rx_head;
-	grant_ref_t grant_rx_ref[NET_RX_RING_SIZE];
-
-	struct xenbus_device *xbdev;
-	int tx_ring_ref;
-	int rx_ring_ref;
-	u8 mac[ETH_ALEN];
-
-	unsigned long rx_pfn_array[NET_RX_RING_SIZE];
-	struct multicall_entry rx_mcl[NET_RX_RING_SIZE+1];
-	struct mmu_update rx_mmu[NET_RX_RING_SIZE];
-};
-
 struct netfront_rx_info {
 	struct netif_rx_response rx;
 	struct netif_extra_info extras[XEN_NETIF_EXTRA_TYPE_MAX - 1];
@@ -271,6 +223,275 @@ static void xennet_sysfs_delif(struct ne
 #define xennet_sysfs_delif(dev) do { } while(0)
 #endif
 
+static struct netfront_accelerator *accelerators = NULL;
+static spinlock_t accelerators_lock;
+
+static int match_accelerator(const char *frontend, 
+                             struct netfront_accelerator *accelerator)
+{
+        return strcmp(frontend, accelerator->frontend) == 0;
+}
+
+
+static void add_accelerator_vif(struct netfront_accelerator *accelerator,
+                                struct netfront_info *np,
+                                struct xenbus_device *dev)
+{
+        unsigned flags;
+        spin_lock_irqsave(&np->accelerator_lock, flags);
+        np->accelerator = accelerator;
+        np->accel_vif_state.np = np;
+        np->accel_vif_state.dev = dev;
+        np->accel_vif_state.hooks = accelerator->hooks;
+        np->accel_vif_state.next = accelerator->vif_states;
+        accelerator->vif_states = &np->accel_vif_state;
+        spin_unlock_irqrestore(&np->accelerator_lock, flags);
+}
+
+
+static int init_accelerator(struct netfront_info *np, struct xenbus_device *dev, 
+                            const char *frontend)
+{
+        struct netfront_accelerator *accelerator = 
+                kmalloc(sizeof(struct netfront_accelerator), GFP_KERNEL);
+
+        if(!accelerator){
+                DPRINTK("%s: no memory for accelerator", __FUNCTION__);
+                return -ENOMEM;
+        }
+
+        accelerator->frontend = kmalloc(strlen(frontend), GFP_KERNEL);
+        if(!accelerator->frontend){
+                DPRINTK("%s: no memory for accelerator", __FUNCTION__);
+                kfree(accelerator);
+                return -ENOMEM;
+        }
+        strcpy(accelerator->frontend, frontend);
+
+        accelerator->vif_states = NULL;
+        accelerator->hooks = NULL;
+
+        accelerator->next = accelerators;
+        
+        accelerators = accelerator;
+
+        if(np){
+                add_accelerator_vif(accelerator, np, dev);
+        }
+
+        return 0;
+}                                        
+
+
+static int netfront_load_accelerator(struct netfront_info *np, 
+                                     struct xenbus_device *dev, 
+                                     const char *frontend)
+{
+        struct netfront_accelerator *accelerator = accelerators;
+        int rc;
+        unsigned flags;
+
+        spin_lock_irqsave(&accelerators_lock, flags);
+
+        /* Look at list of loaded accelerators to see if the requested
+           one is already there */
+        while(accelerator != NULL){
+                if(match_accelerator(frontend, accelerator)){
+                        /* Already know about it, already loaded, but
+                           these details weren't known at the time */
+                        if(accelerator->hooks == NULL)
+                                DPRINTK("%s: no hooks set", __FUNCTION__);
+                        else
+                                accelerator->hooks->new_device(np->netdev, dev);
+                        /* Tell accelerator about this frontend device */
+                        add_accelerator_vif(accelerator, np, dev);
+                        spin_unlock_irqrestore(&accelerators_lock, flags);
+                        return 0;
+                }
+
+                accelerator = accelerator->next;
+        }
+
+        /* Couldn't find it, so create a new one and load the module */
+        if((rc = init_accelerator(np, dev, frontend)) < 0) {
+                spin_unlock_irqrestore(&accelerators_lock, flags);
+                return rc;
+        }
+
+        spin_unlock_irqrestore(&accelerators_lock, flags);
+
+        DPRINTK("%s: loading module %s\n", __FUNCTION__, frontend);
+
+        /* load acceleration module */
+        request_module("%s", frontend);
+
+        /* Module should now call netfront_accelerator_loaded() once
+           it's up and running, and we can continue from there */
+
+        return 0;
+}
+
+
+static void accelerator_set_hooks(struct netfront_accelerator *accelerator,
+                                  struct netfront_accel_hooks *hooks)
+{
+        struct netfront_accel_vif_state *accel_vif_state;
+        unsigned flags;
+
+        DPRINTK("%s: %p %p\n", __FUNCTION__, accelerator, hooks);
+
+        accelerator->hooks = hooks;
+
+        accel_vif_state = accelerator->vif_states;
+        while(accel_vif_state != NULL) {
+                struct netfront_info *np = accel_vif_state->np;
+                hooks->new_device(np->netdev, accel_vif_state->dev);
+                spin_lock_irqsave(&np->accelerator_lock, flags);
+                accel_vif_state->hooks = hooks;
+                spin_unlock_irqrestore(&np->accelerator_lock, flags);
+
+                accel_vif_state = accel_vif_state->next;
+        }
+}
+
+
+/* Called by the accelerator once it's ready for action */
+int netfront_accelerator_loaded(const char *frontend, 
+                                struct netfront_accel_hooks *hooks)
+{
+        /* Tell accelerator about the frontend device */
+        struct netfront_accelerator *accelerator = accelerators;
+        unsigned flags;
+
+        spin_lock_irqsave(&accelerators_lock, flags);
+
+        /* Look through list of accelerators to see if it has already
+           been requested */
+        while(accelerator != NULL){
+                if(match_accelerator(frontend, accelerator)){
+                        accelerator_set_hooks(accelerator, hooks);
+                        spin_unlock_irqrestore(&accelerators_lock, flags);
+                        return 0;
+                }
+
+                accelerator = accelerator->next;
+        }
+
+        /* If it wasn't in the list, add it now so that when it is
+           requested the caller will find it */
+        if(accelerator == NULL){
+                DPRINTK("%s: Couldn't find matching accelerator (%s)\n",
+                        __FUNCTION__, frontend);
+                init_accelerator(NULL, NULL, frontend);
+        }
+
+        spin_unlock_irqrestore(&accelerators_lock, flags);
+
+        return 0;
+}
+EXPORT_SYMBOL_GPL(netfront_accelerator_loaded);
+
+
+void accelerator_disconnect_vif(struct netfront_accel_vif_state *vif_state)
+{
+        struct netfront_info *np = vif_state->np;
+        unsigned flags;
+
+        if(np) {
+                /* Spin lock doesn't protect poll, so
+                   make sure there's none of those
+                   going on */
+                netif_poll_disable(np->netdev);
+                /* Likewise for xmit */
+                netif_tx_disable(np->netdev);
+                
+                spin_lock_irqsave(&np->accelerator_lock, flags);
+                np->accel_vif_state.hooks = NULL;
+                spin_unlock_irqrestore(&np->accelerator_lock, flags);
+                
+                netif_wake_queue(np->netdev);
+                netif_poll_enable(np->netdev);
+        }
+       
+}
+
+
+void netfront_accelerator_unloaded(const char *frontend)
+{
+        /* Tell accelerator about the frontend device */
+        struct netfront_accelerator *accelerator = accelerators;
+        struct netfront_accelerator *prev = NULL;
+        unsigned flags;
+
+        spin_lock_irqsave(&accelerators_lock, flags);
+
+        while(accelerator != NULL){
+                if(match_accelerator(frontend, accelerator)){
+                        struct netfront_accel_vif_state *vif_state 
+                                = accelerator->vif_states;
+
+                        accelerator->hooks = NULL;
+
+                        spin_unlock_irqrestore(&accelerators_lock, flags);
+                                
+                        while(vif_state != NULL){
+                                accelerator_disconnect_vif(vif_state);
+                                vif_state = vif_state->next;
+                        }
+                        return;
+                }
+
+                prev = accelerator;
+        }
+        spin_unlock_irqrestore(&accelerators_lock, flags);
+}
+EXPORT_SYMBOL_GPL(netfront_accelerator_unloaded);
+
+
+#define NETFRONT_CALL_ACCELERATOR_HOOK(_np, _hook, _args...)            \
+        do {                                                            \
+                if((_np)->accelerator && (_np)->accel_vif_state.hooks)  \
+                        (_np)->accel_vif_state.hooks->_hook(_args);     \
+        } while(0)
+
+
+#define NETFRONT_LOCK_AND_CALL_ACCELERATOR_HOOK(_np, _hook, _args...)   \
+        do {                                                            \
+                unsigned _flags;                                        \
+                spin_lock_irqsave(&(_np)->accelerator_lock, _flags);    \
+                if((_np)->accelerator && (_np)->accel_vif_state.hooks)  \
+                        (_np)->accel_vif_state.hooks->_hook(_args);     \
+                spin_unlock_irqrestore(&(_np)->accelerator_lock, _flags); \
+        } while(0)
+
+
+int netfront_schedule_poll(struct net_device *dev)
+{
+        /* TODO do we need to protect this with any netfront locks? */
+        netif_rx_schedule(dev);
+        return 0;
+}
+EXPORT_SYMBOL_GPL(netfront_schedule_poll);
+
+
+int netfront_stop_queue(struct net_device *dev)
+{
+        netif_stop_queue(dev);
+        return 0;
+}
+EXPORT_SYMBOL_GPL(netfront_stop_queue);
+
+
+int netfront_wake_queue(struct net_device *dev)
+{
+        netif_wake_queue(dev);
+        return 0;
+}
+EXPORT_SYMBOL_GPL(netfront_wake_queue);
+
+
+
 static inline int xennet_can_sg(struct net_device *dev)
 {
 	return dev->features & NETIF_F_SG;
@@ -327,6 +548,8 @@ static int __devexit netfront_remove(str
 
 	DPRINTK("%s\n", dev->nodename);
 
+        NETFRONT_LOCK_AND_CALL_ACCELERATOR_HOOK(info, remove, dev);
+
 	netif_disconnect_backend(info);
 
 	del_timer_sync(&info->rx_refill_timer);
@@ -351,6 +574,8 @@ static int netfront_resume(struct xenbus
 	struct netfront_info *info = dev->dev.driver_data;
 
 	DPRINTK("%s\n", dev->nodename);
+
+        NETFRONT_LOCK_AND_CALL_ACCELERATOR_HOOK(info, resume, dev);
 
 	netif_disconnect_backend(info);
 	return 0;
@@ -570,6 +795,9 @@ static void backend_changed(struct xenbu
 		xenbus_frontend_closed(dev);
 		break;
 	}
+
+        NETFRONT_LOCK_AND_CALL_ACCELERATOR_HOOK(np, backend_changed, 
+                                                dev, backend_state);
 }
 
 /** Send a packet on a net device to encourage switches to learn the
@@ -626,8 +854,10 @@ static int network_open(struct net_devic
 	if (netfront_carrier_ok(np)) {
 		network_alloc_rx_buffers(dev);
 		np->rx.sring->rsp_event = np->rx.rsp_cons + 1;
-		if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx))
+		if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)){
+                        NETFRONT_LOCK_AND_CALL_ACCELERATOR_HOOK(np, stop_napi_interrupts, dev);
 			netif_rx_schedule(dev);
+                }
 	}
 	spin_unlock_bh(&np->rx_lock);
 
@@ -695,6 +925,8 @@ static void rx_refill_timeout(unsigned l
 static void rx_refill_timeout(unsigned long data)
 {
 	struct net_device *dev = (struct net_device *)data;
+	struct netfront_info *np = netdev_priv(dev);
+        NETFRONT_LOCK_AND_CALL_ACCELERATOR_HOOK(np, stop_napi_interrupts, dev);
 	netif_rx_schedule(dev);
 }
 
@@ -934,6 +1166,10 @@ static int network_start_xmit(struct sk_
 	unsigned int offset = offset_in_page(data);
 	unsigned int len = skb_headlen(skb);
 
+        if(np->accelerator && np->accel_vif_state.hooks)     
+                if(np->accel_vif_state.hooks->start_xmit(skb, dev))
+                                return 0;
+
 	frags += (offset + len + PAGE_SIZE - 1) / PAGE_SIZE;
 	if (unlikely(frags > MAX_SKB_FRAGS + 1)) {
 		printk(KERN_ALERT "xennet: skb rides the rocket: %d frags\n",
@@ -1037,8 +1273,10 @@ static irqreturn_t netif_int(int irq, vo
 	if (likely(netfront_carrier_ok(np))) {
 		network_tx_buf_gc(dev);
 		/* Under tx_lock: protects access to rx shared-ring indexes. */
-		if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx))
+		if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)) {
+                        NETFRONT_LOCK_AND_CALL_ACCELERATOR_HOOK(np, stop_napi_interrupts, dev);
 			netif_rx_schedule(dev);
+                }
 	}
 
 	spin_unlock_irqrestore(&np->tx_lock, flags);
@@ -1298,7 +1536,7 @@ static int netif_poll(struct net_device 
 	struct netif_extra_info *extras = rinfo.extras;
 	RING_IDX i, rp;
 	struct multicall_entry *mcl;
-	int work_done, budget, more_to_do = 1;
+	int work_done, budget, more_to_do = 1, accel_more_to_do = 1;
 	struct sk_buff_head rxq;
 	struct sk_buff_head errq;
 	struct sk_buff_head tmpq;
@@ -1465,6 +1703,20 @@ err:
 
 	network_alloc_rx_buffers(dev);
 
+        if(work_done < budget) {
+                /* there's some spare capacity, try the accelerated path */
+                int accel_budget = budget - work_done;
+                int accel_budget_start = accel_budget;
+                if(np->accelerator && np->accel_vif_state.hooks
+                   && np->accel_vif_state.hooks->netdev_poll){
+                        accel_more_to_do = 
+                                np->accel_vif_state.hooks->netdev_poll(dev, &accel_budget);
+                        work_done += (accel_budget_start - accel_budget);
+                } else {
+                        accel_more_to_do = 0;
+                }
+        }
+
 	*pbudget   -= work_done;
 	dev->quota -= work_done;
 
@@ -1472,15 +1724,26 @@ err:
 		local_irq_save(flags);
 
 		RING_FINAL_CHECK_FOR_RESPONSES(&np->rx, more_to_do);
-		if (!more_to_do)
+
+		if (!more_to_do && !accel_more_to_do) {
+                        /* Slow path has nothing more to do, see if
+                           fast path is likewise */
+                        if(np->accelerator && np->accel_vif_state.hooks
+                           && np->accel_vif_state.hooks->start_napi_interrupts) {
+                                accel_more_to_do = np->accel_vif_state.hooks->start_napi_interrupts(dev);
+                        }
+                }
+
+		if (!more_to_do && !accel_more_to_do) {
 			__netif_rx_complete(dev);
+                }
 
 		local_irq_restore(flags);
 	}
 
 	spin_unlock(&np->rx_lock);
-
-	return more_to_do;
+	
+        return more_to_do | accel_more_to_do;
 }
 
 static void netif_release_tx_bufs(struct netfront_info *np)
@@ -1680,7 +1943,9 @@ static int network_connect(struct net_de
 	struct sk_buff *skb;
 	grant_ref_t ref;
 	netif_rx_request_t *req;
-	unsigned int feature_rx_copy, feature_rx_flip;
+	unsigned int feature_rx_copy, feature_rx_flip, feature_accel;
+        char *accel_frontend;
+        int accel_len;
 
 	err = xenbus_scanf(XBT_NIL, np->xbdev->otherend,
 			   "feature-rx-copy", "%u", &feature_rx_copy);
@@ -1691,6 +1956,13 @@ static int network_connect(struct net_de
 	if (err != 1)
 		feature_rx_flip = 1;
 
+        feature_accel = 1;
+        accel_frontend = xenbus_read(XBT_NIL, np->xbdev->otherend, 
+                                     "accel", &accel_len);
+        if(IS_ERR(accel_frontend)){
+                feature_accel = 0;
+        }
+
 	/*
 	 * Copy packets on receive path if:
 	 *  (a) This was requested by user, and the backend supports it; or
@@ -1702,6 +1974,11 @@ static int network_connect(struct net_de
 	err = talk_to_backend(np->xbdev, np);
 	if (err)
 		return err;
+
+        if(feature_accel){
+                netfront_load_accelerator(np, np->xbdev, accel_frontend);
+                kfree(accel_frontend);
+        }
 
 	xennet_set_features(dev);
 
@@ -1948,6 +2225,9 @@ static struct net_device * __devinit cre
 
 	spin_lock_init(&np->tx_lock);
 	spin_lock_init(&np->rx_lock);
+	spin_lock_init(&np->accelerator_lock);
+        np->accel_vif_state.hooks = NULL;
+        np->accel_vif_state.next = NULL;
 
 	skb_queue_head_init(&np->rx_batch);
 	np->rx_target     = RX_DFL_MIN_TARGET;
@@ -2103,6 +2383,8 @@ static int __init netif_init(void)
 	if (is_initial_xendomain())
 		return 0;
 
+        spin_lock_init(&accelerators_lock);
+
 	IPRINTK("Initialising virtual ethernet driver.\n");
 
 	(void)register_inetaddr_notifier(&notifier_inetdev);
diff -r 325afaed01ff linux-2.6-xen-sparse/drivers/xen/netfront/netfront.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.h	Tue Apr 17 09:12:43 2007 +0100
@@ -0,0 +1,189 @@
+/******************************************************************************
+ * Virtual network driver for conversing with remote driver backends.
+ *
+ * Copyright (c) 2002-2005, K A Fraser
+ * Copyright (c) 2005, XenSource Ltd
+ * Copyright (C) 2007 Solarflare Communications, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef NETFRONT_H
+#define NETFRONT_H
+
+#include <xen/interface/io/netif.h>
+#include <linux/netdevice.h>
+#include <linux/skbuff.h>
+
+#define NET_TX_RING_SIZE __RING_SIZE((struct netif_tx_sring *)0, PAGE_SIZE)
+#define NET_RX_RING_SIZE __RING_SIZE((struct netif_rx_sring *)0, PAGE_SIZE)
+
+#include <xen/xenbus.h>
+/* Function pointer table for hooks into a network acceleration
+   plugin.  These are called at appropriate points from the netfront
+   driver */
+struct netfront_accel_hooks {
+        /* new_device: The plugin is asked to support a new network interface */
+        int (*new_device)(struct net_device *net_dev, struct xenbus_device *dev);
+        /* suspend, resume, remove: Equivalent to the normal xenbus_* callbacks */
+        int (*suspend)(struct xenbus_device *dev);
+        int (*resume)(struct xenbus_device *dev);
+        int (*remove)(struct xenbus_device *dev);
+        /* backend_changed: Callback from watch based on backend's
+           xenbus state changing */
+        void (*backend_changed)(struct xenbus_device *dev,
+                                enum xenbus_state backend_state);
+        /* The net_device is being polled, check the accelerated
+           hardware for any pending packets */
+        int (*netdev_poll)(struct net_device *dev, int *pbudget);
+        /* start_xmit: Used to give the accelerated plugin the option
+           of sending a packet.  Returns non-zero if has done so, or
+           zero to decline and force the packet onto normal send path */
+        int (*start_xmit)(struct sk_buff *skb, struct net_device *dev);
+        /* start/stop_napi_interrupts Used by netfront to indicate
+           when napi interrupts should be enabled or disabled */
+        int (*start_napi_interrupts)(struct net_device *dev);
+        void (*stop_napi_interrupts)(struct net_device *dev);
+};
+
+/* Per-netfront device state for the accelerator.  This is used to
+   allow efficient per-netfront device access to the accelerator hooks */
+struct netfront_accel_vif_state {
+        struct netfront_accel_vif_state *next;
+        struct xenbus_device *dev;
+        struct netfront_info *np;
+        struct netfront_accel_hooks *hooks;
+}; 
+
+/* Per-accelerator state stored in netfront.  These form a list that
+   is used to track which devices are accelerated by which plugins,
+   and what plugins are available/have been requested */
+struct netfront_accelerator {
+        /* ID of the accelerator */
+        int id;
+        /* String describing the accelerator.  Currently this is the
+           name of the accelerator module.  This is provided by the
+           backend accelerator through xenstore */
+        char *frontend;
+        /* The hooks into the accelerator plugin module */
+        struct netfront_accel_hooks *hooks;
+        /* List of per-netfront device state for each netfront device
+           that is using this accelerator */
+        struct netfront_accel_vif_state *vif_states;
+        /* Used to make a list */
+        struct netfront_accelerator *next;
+};
+
+
+struct netfront_info {
+	struct list_head list;
+	struct net_device *netdev;
+
+	struct net_device_stats stats;
+
+	struct netif_tx_front_ring tx;
+	struct netif_rx_front_ring rx;
+
+	spinlock_t   tx_lock;
+	spinlock_t   rx_lock;
+
+	unsigned int irq;
+	unsigned int copying_receiver;
+	unsigned int carrier;
+
+	/* Receive-ring batched refills. */
+#define RX_MIN_TARGET 8
+#define RX_DFL_MIN_TARGET 64
+#define RX_MAX_TARGET min_t(int, NET_RX_RING_SIZE, 256)
+	unsigned rx_min_target, rx_max_target, rx_target;
+	struct sk_buff_head rx_batch;
+
+	struct timer_list rx_refill_timer;
+
+	/*
+	 * {tx,rx}_skbs store outstanding skbuffs. The first entry in tx_skbs
+	 * is an index into a chain of free entries.
+	 */
+	struct sk_buff *tx_skbs[NET_TX_RING_SIZE+1];
+	struct sk_buff *rx_skbs[NET_RX_RING_SIZE];
+
+#define TX_MAX_TARGET min_t(int, NET_RX_RING_SIZE, 256)
+	grant_ref_t gref_tx_head;
+	grant_ref_t grant_tx_ref[NET_TX_RING_SIZE + 1];
+	grant_ref_t gref_rx_head;
+	grant_ref_t grant_rx_ref[NET_RX_RING_SIZE];
+
+	struct xenbus_device *xbdev;
+	int tx_ring_ref;
+	int rx_ring_ref;
+	u8 mac[ETH_ALEN];
+
+	unsigned long rx_pfn_array[NET_RX_RING_SIZE];
+	struct multicall_entry rx_mcl[NET_RX_RING_SIZE+1];
+	struct mmu_update rx_mmu[NET_RX_RING_SIZE];
+
+        /* Private pointer to state internal to accelerator module */
+        void *accel_priv;
+        /* The (list of) accelerator(s) used by this netfront device */
+        struct netfront_accelerator *accelerator;
+        /* The accelerator state for this netfront device */
+        struct netfront_accel_vif_state accel_vif_state;
+        /* Lock controlling access to the accelerator state, in
+           particular, np->accelerator and np->accel_vif_state.hooks */
+        spinlock_t accelerator_lock;
+};
+
+
+/* Called by an accelerator plugin module when it has loaded.
+ *
+ * frontend: the string describing the accelerator, currently the module name 
+ * hooks: the hooks for netfront to use to call into the accelerator
+ */
+extern int netfront_accelerator_loaded(const char *frontend, 
+                                       struct netfront_accel_hooks *hooks);
+
+/* Called by an accelerator plugin module when it is about to unload.
+ *
+ *  frontend: the string describing the accelerator.  Must match the
+ *  one passed to netfront_accelerator_loaded()
+ */ 
+extern void netfront_accelerator_unloaded(const char *frontend);
+
+/*  
+ * Request that a poll be scheduled by netfront on behalf of the
+ * accelerator */
+extern int netfront_schedule_poll(struct net_device *dev);
+
+/*  
+ * Request that the net_device tx queue should be stopped by netfront
+ * on behalf of the accelerator */
+extern int netfront_stop_queue(struct net_device *dev);
+
+/*  
+ * Request that the net_device tx queue should be started by netfront
+ * on behalf of the accelerator */
+extern int netfront_wake_queue(struct net_device *dev);
+
+#endif /* NETFRONT_H */

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 3/4] Support accelerated network plugin modules

[Muli Ben-Yehuda]

On Tue, May 08, 2007 at 10:55:09AM +0100, Kieran Mansley wrote:
> Add hooks in the netfront driver to allow an accelerated plugin module
> to attach and provide a fast path for network traffic in the presence of
> a virtualisable NIC.
> 
> Signed-off-by: Kieran Mansley <kmansley@solarflare.com>

> Frontend net driver acceleration
> 
> diff -r 325afaed01ff linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c
> --- a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Tue Apr 17 09:07:31 2007 +0100
> +++ b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Tue Apr 17 09:13:05 2007 +0100
> @@ -3,6 +3,7 @@
>   *
>   * Copyright (c) 2002-2005, K A Fraser
>   * Copyright (c) 2005, XenSource Ltd
> + * Copyright (C) 2007 Solarflare Communications, Inc.
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License version 2
> @@ -67,6 +68,8 @@
>  #include <xen/platform-compat.h>
>  #endif
>  
> +#include "netfront.h"
> +
>  /*
>   * Mutually-exclusive module options to select receive data path:
>   *  rx_copy : Packets are copied by network backend into local memory
> @@ -137,57 +140,6 @@ static inline int netif_needs_gso(struct
>  
>  #define GRANT_INVALID_REF	0
>  
> -#define NET_TX_RING_SIZE __RING_SIZE((struct netif_tx_sring *)0, PAGE_SIZE)
> -#define NET_RX_RING_SIZE __RING_SIZE((struct netif_rx_sring *)0, PAGE_SIZE)
> -
> -struct netfront_info {
> -	struct list_head list;
> -	struct net_device *netdev;
> -
> -	struct net_device_stats stats;
> -
> -	struct netif_tx_front_ring tx;
> -	struct netif_rx_front_ring rx;
> -
> -	spinlock_t   tx_lock;
> -	spinlock_t   rx_lock;
> -
> -	unsigned int irq;
> -	unsigned int copying_receiver;
> -	unsigned int carrier;
> -
> -	/* Receive-ring batched refills. */
> -#define RX_MIN_TARGET 8
> -#define RX_DFL_MIN_TARGET 64
> -#define RX_MAX_TARGET min_t(int, NET_RX_RING_SIZE, 256)
> -	unsigned rx_min_target, rx_max_target, rx_target;
> -	struct sk_buff_head rx_batch;
> -
> -	struct timer_list rx_refill_timer;
> -
> -	/*
> -	 * {tx,rx}_skbs store outstanding skbuffs. The first entry in tx_skbs
> -	 * is an index into a chain of free entries.
> -	 */
> -	struct sk_buff *tx_skbs[NET_TX_RING_SIZE+1];
> -	struct sk_buff *rx_skbs[NET_RX_RING_SIZE];
> -
> -#define TX_MAX_TARGET min_t(int, NET_RX_RING_SIZE, 256)
> -	grant_ref_t gref_tx_head;
> -	grant_ref_t grant_tx_ref[NET_TX_RING_SIZE + 1];
> -	grant_ref_t gref_rx_head;
> -	grant_ref_t grant_rx_ref[NET_RX_RING_SIZE];
> -
> -	struct xenbus_device *xbdev;
> -	int tx_ring_ref;
> -	int rx_ring_ref;
> -	u8 mac[ETH_ALEN];
> -
> -	unsigned long rx_pfn_array[NET_RX_RING_SIZE];
> -	struct multicall_entry rx_mcl[NET_RX_RING_SIZE+1];
> -	struct mmu_update rx_mmu[NET_RX_RING_SIZE];
> -};
> -
>  struct netfront_rx_info {
>  	struct netif_rx_response rx;
>  	struct netif_extra_info extras[XEN_NETIF_EXTRA_TYPE_MAX - 1];
> @@ -271,6 +223,275 @@ static void xennet_sysfs_delif(struct ne
>  #define xennet_sysfs_delif(dev) do { } while(0)
>  #endif
>  
> +static struct netfront_accelerator *accelerators = NULL;
> +static spinlock_t accelerators_lock;
> +
> +static int match_accelerator(const char *frontend, 
> +                             struct netfront_accelerator *accelerator)
> +{
> +        return strcmp(frontend, accelerator->frontend) == 0;
> +}
> +
> +
> +static void add_accelerator_vif(struct netfront_accelerator *accelerator,
> +                                struct netfront_info *np,
> +                                struct xenbus_device *dev)
> +{
> +        unsigned flags;
> +        spin_lock_irqsave(&np->accelerator_lock, flags);
> +        np->accelerator = accelerator;
> +        np->accel_vif_state.np = np;
> +        np->accel_vif_state.dev = dev;
> +        np->accel_vif_state.hooks = accelerator->hooks;
> +        np->accel_vif_state.next = accelerator->vif_states;
> +        accelerator->vif_states = &np->accel_vif_state;
> +        spin_unlock_irqrestore(&np->accelerator_lock, flags);
> +}

Is there a reason not to use standard Linux list.h here rather than
implementinng your own linked list?

> +static int init_accelerator(struct netfront_info *np, struct xenbus_device *dev, 
> +                            const char *frontend)
> +{
> +        struct netfront_accelerator *accelerator = 
> +                kmalloc(sizeof(struct netfront_accelerator), GFP_KERNEL);
> +
> +        if(!accelerator){
> +                DPRINTK("%s: no memory for accelerator", __FUNCTION__);
> +                return -ENOMEM;
> +        }
> +
> +        accelerator->frontend = kmalloc(strlen(frontend), GFP_KERNEL);
> +        if(!accelerator->frontend){
> +                DPRINTK("%s: no memory for accelerator", __FUNCTION__);
> +                kfree(accelerator);
> +                return -ENOMEM;
> +        }
> +        strcpy(accelerator->frontend, frontend);

You're not mallocing enough space for the final NULL. Use strlcpy?

> +
> +        accelerator->vif_states = NULL;
> +        accelerator->hooks = NULL;
> +
> +        accelerator->next = accelerators;
> +        
> +        accelerators = accelerator;

Shouldn't the list be protected here? also, better naming would be
appreciated - it's hard to differentate between 'accelerator' and
'accelerators' in first glance.

> +
> +        if(np){
> +                add_accelerator_vif(accelerator, np, dev);
> +        }

Coding style - no braces for a single line

> +        return 0;
> +}                                        
> +
> +
> +static int netfront_load_accelerator(struct netfront_info *np, 
> +                                     struct xenbus_device *dev, 
> +                                     const char *frontend)
> +{
> +        struct netfront_accelerator *accelerator = accelerators;
> +        int rc;
> +        unsigned flags;
> +
> +        spin_lock_irqsave(&accelerators_lock, flags);
> +
> +        /* Look at list of loaded accelerators to see if the requested
> +           one is already there */
> +        while(accelerator != NULL){
> +                if(match_accelerator(frontend, accelerator)){
> +                        /* Already know about it, already loaded, but
> +                           these details weren't known at the time */
> +                        if(accelerator->hooks == NULL)
> +                                DPRINTK("%s: no hooks set", __FUNCTION__);
> +                        else
> +
>                                  accelerator->hooks->new_device(np->accelerator->hooks->new_device(np->netdev, dev);

It would be better to call the hoook without holding the lock.

> +                        /* Tell accelerator about this frontend device */
> +                        add_accelerator_vif(accelerator, np, dev);
> +                        spin_unlock_irqrestore(&accelerators_lock, flags);
> +                        return 0;
> +                }
> +
> +                accelerator = accelerator->next;
> +        }
> +
> +        /* Couldn't find it, so create a new one and load the module */
> +        if((rc = init_accelerator(np, dev, frontend)) < 0) {
> +                spin_unlock_irqrestore(&accelerators_lock, flags);
> +                return rc;
> +        }

coding style - 'if ((', seperate assignment and test.
> +
> +        spin_unlock_irqrestore(&accelerators_lock, flags);
> +
> +        DPRINTK("%s: loading module %s\n", __FUNCTION__, frontend);
> +
> +        /* load acceleration module */
> +        request_module("%s", frontend);
> +
> +        /* Module should now call netfront_accelerator_loaded() once
> +           it's up and running, and we can continue from there */
> +
> +        return 0;
> +}
> +
> +
> +static void accelerator_set_hooks(struct netfront_accelerator *accelerator,
> +                                  struct netfront_accel_hooks *hooks)
> +{
> +        struct netfront_accel_vif_state *accel_vif_state;
> +        unsigned flags;
> +
> +        DPRINTK("%s: %p %p\n", __FUNCTION__, accelerator, hooks);
> +
> +        accelerator->hooks = hooks;
> +
> +        accel_vif_state = accelerator->vif_states;
> +        while(accel_vif_state != NULL) {
> +                struct netfront_info *np = accel_vif_state->np;
> +                hooks->new_device(np->netdev, accel_vif_state->dev);
> +                spin_lock_irqsave(&np->accelerator_lock, flags);

This is called with the accelerators_lock held (from
netfront_accelerator_loaded) - is the lock ordering guaranteed?

> +                accel_vif_state->hooks = hooks;
> +                spin_unlock_irqrestore(&np->accelerator_lock, flags);
> +
> +                accel_vif_state = accel_vif_state->next;
> +        }
> +}
> +
> +
> +/* Called by the accelerator once it's ready for action */
> +int netfront_accelerator_loaded(const char *frontend, 
> +                                struct netfront_accel_hooks *hooks)
> +{
> +        /* Tell accelerator about the frontend device */
> +        struct netfront_accelerator *accelerator = accelerators;
> +        unsigned flags;
> +
> +        spin_lock_irqsave(&accelerators_lock, flags);
> +
> +        /* Look through list of accelerators to see if it has already
> +           been requested */
> +        while(accelerator != NULL){
> +                if(match_accelerator(frontend, accelerator)){
> +                        accelerator_set_hooks(accelerator, hooks);
> +                        spin_unlock_irqrestore(&accelerators_lock, flags);
> +                        return 0;
> +                }
> +
> +                accelerator = accelerator->next;
> +        }
> +
> +        /* If it wasn't in the list, add it now so that when it is
> +           requested the caller will find it */
> +        if(accelerator == NULL){
> +                DPRINTK("%s: Couldn't find matching accelerator (%s)\n",
> +                        __FUNCTION__, frontend);
> +                init_accelerator(NULL, NULL, frontend);
> +        }
> +
> +        spin_unlock_irqrestore(&accelerators_lock, flags);
> +
> +        return 0;
> +}
> +EXPORT_SYMBOL_GPL(netfront_accelerator_loaded);
> +
> +
> +void accelerator_disconnect_vif(struct netfront_accel_vif_state *vif_state)
> +{
> +        struct netfront_info *np = vif_state->np;
> +        unsigned flags;
> +
> +        if(np) {
> +                /* Spin lock doesn't protect poll, so
> +                   make sure there's none of those
> +                   going on */
> +                netif_poll_disable(np->netdev);
> +                /* Likewise for xmit */
> +                netif_tx_disable(np->netdev);
> +                
> +                spin_lock_irqsave(&np->accelerator_lock, flags);
> +                np->accel_vif_state.hooks = NULL;
> +                spin_unlock_irqrestore(&np->accelerator_lock, flags);
> +                
> +                netif_wake_queue(np->netdev);
> +                netif_poll_enable(np->netdev);
> +        }
> +       
> +}
> +
> +
> +void netfront_accelerator_unloaded(const char *frontend)
> +{
> +        /* Tell accelerator about the frontend device */
> +        struct netfront_accelerator *accelerator = accelerators;
> +        struct netfront_accelerator *prev = NULL;
> +        unsigned flags;
> +
> +        spin_lock_irqsave(&accelerators_lock, flags);
> +
> +        while(accelerator != NULL){
> +                if(match_accelerator(frontend, accelerator)){
> +                        struct netfront_accel_vif_state *vif_state 
> +                                = accelerator->vif_states;
> +
> +                        accelerator->hooks = NULL;
> +
> +                        spin_unlock_irqrestore(&accelerators_lock, flags);
> +                                
> +                        while(vif_state != NULL){
> +                                accelerator_disconnect_vif(vif_state);
> +                                vif_state = vif_state->next;
> +                        }
> +                        return;
> +                }
> +
> +                prev = accelerator;

accelerator is never incremented here??? if accelerator is not NULL
and match_accelerator fails, we'll go into an infinite loop. I
strongly recommend you use the standard list macros
(list_for_each...).

> +        }
> +        spin_unlock_irqrestore(&accelerators_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(netfront_accelerator_unloaded);
> +
> +
> +#define NETFRONT_CALL_ACCELERATOR_HOOK(_np, _hook, _args...)            \
> +        do {                                                            \
> +                if((_np)->accelerator && (_np)->accel_vif_state.hooks)  \
> +                        (_np)->accel_vif_state.hooks->_hook(_args);     \
> +        } while(0)
> +
> +
> +#define NETFRONT_LOCK_AND_CALL_ACCELERATOR_HOOK(_np, _hook, _args...)   \
> +        do {                                                            \
> +                unsigned _flags;                                        \
> +                spin_lock_irqsave(&(_np)->accelerator_lock, _flags);    \
> +                if((_np)->accelerator && (_np)->accel_vif_state.hooks)  \
> +                        (_np)->accel_vif_state.hooks->_hook(_args);     \
> +                spin_unlock_irqrestore(&(_np)->accelerator_lock, _flags); \
> +        } while(0)

Please get rid of these macros - it's not exactly a lot of code to
duplicate and it makes it obvious what's going on.

> +
> +
> +int netfront_schedule_poll(struct net_device *dev)
> +{
> +        /* TODO do we need to protect this with any netfront locks? */
> +        netif_rx_schedule(dev);
> +        return 0;
> +}
> +EXPORT_SYMBOL_GPL(netfront_schedule_poll);
> +
> +
> +int netfront_stop_queue(struct net_device *dev)
> +{
> +        netif_stop_queue(dev);
> +        return 0;
> +}
> +EXPORT_SYMBOL_GPL(netfront_stop_queue);
> +
> +
> +int netfront_wake_queue(struct net_device *dev)
> +{
> +        netif_wake_queue(dev);
> +        return 0;
> +}
> +EXPORT_SYMBOL_GPL(netfront_wake_queue);

These look like pointless wrapper *if* we don't need to do antyhing
netfront specific in them.

I'll review the rest once you repost the patches.

Cheers,
Muli

Re: [PATCH 3/4] Support accelerated network plugin modules

[Kieran Mansley]

On Wed, 2007-05-09 at 14:25 +0300, Muli Ben-Yehuda wrote:
> On Tue, May 08, 2007 at 10:55:09AM +0100, Kieran Mansley wrote:
> > +#define NETFRONT_CALL_ACCELERATOR_HOOK(_np, _hook, _args...)
\
> > +        do {
\
> > +                if((_np)->accelerator && (_np)-
>accel_vif_state.hooks)  \
> > +                        (_np)->accel_vif_state.hooks->_hook(_args);
\
> > +        } while(0)
> > +
> > +
> > +#define NETFRONT_LOCK_AND_CALL_ACCELERATOR_HOOK(_np, _hook,
_args...)   \
> > +        do {
\
> > +                unsigned _flags;
\
> > +                spin_lock_irqsave(&(_np)->accelerator_lock,
_flags);    \
> > +                if((_np)->accelerator && (_np)-
>accel_vif_state.hooks)  \
> > +                        (_np)->accel_vif_state.hooks->_hook(_args);
\
> > +                spin_unlock_irqrestore(&(_np)->accelerator_lock,
_flags); \
> > +        } while(0)
> 
> Please get rid of these macros - it's not exactly a lot of code to
> duplicate and it makes it obvious what's going on.

The first macro I'm happy to get rid of - I noticed after Keir commented
on the use of caps in the name that it's no longer used.  The second I
think is enough code that it would unnecessarily clutter the existing
functions.  For this reason I'd rather leave it in (with a lower-case
name). 

> 
> I'll review the rest once you repost the patches.

Thanks for taking the time to look.  I'll integrate the rest of your
suggestions into the patches and repost.

Kieran


O

Re: [PATCH 3/4] Support accelerated network plugin modules

[Muli Ben-Yehuda]

On Wed, May 09, 2007 at 12:58:06PM +0100, Kieran Mansley wrote:

> > > +#define NETFRONT_LOCK_AND_CALL_ACCELERATOR_HOOK(_np, _hook,
> _args...)   \
> > > +        do {
> \
> > > +                unsigned _flags;
> \
> > > +                spin_lock_irqsave(&(_np)->accelerator_lock,
> _flags);    \
> > > +                if((_np)->accelerator && (_np)-
> >accel_vif_state.hooks)  \
> > > +                        (_np)->accel_vif_state.hooks->_hook(_args);
> \
> > > +                spin_unlock_irqrestore(&(_np)->accelerator_lock,
> _flags); \
> > > +        } while(0)
> > 
> > Please get rid of these macros - it's not exactly a lot of code to
> > duplicate and it makes it obvious what's going on.
> 
> The first macro I'm happy to get rid of - I noticed after Keir
> commented on the use of caps in the name that it's no longer used.
> The second I think is enough code that it would unnecessarily
> clutter the existing functions.  For this reason I'd rather leave it
> in (with a lower-case name).

It's a matter of taste, but I'd prefer it if it was obvious when
looking at the code that the hook is being called with a spinlock
held. That's actually another thing - why must every hook be called
with the spinlock held? if it's to protect the accelerator from going
away, what's actually needed is a ref count (struct kref) on the
accelerator.

Cheers,
Muli

Re: [PATCH 3/4] Support accelerated network plugin modules

[Keir Fraser]

On 9/5/07 13:17, "Muli Ben-Yehuda" <muli@il.ibm.com> wrote:

>> The first macro I'm happy to get rid of - I noticed after Keir
>> commented on the use of caps in the name that it's no longer used.
>> The second I think is enough code that it would unnecessarily
>> clutter the existing functions.  For this reason I'd rather leave it
>> in (with a lower-case name).
> 
> It's a matter of taste, but I'd prefer it if it was obvious when
> looking at the code that the hook is being called with a spinlock
> held. That's actually another thing - why must every hook be called
> with the spinlock held? if it's to protect the accelerator from going
> away, what's actually needed is a ref count (struct kref) on the
> accelerator.

I agree the lock should go. Removing the accelerator from under the feet of
an active vif just doesn't seem a sane action to support. And it should be
possible to support atomic-enough addition of an accelerator without need
for heavyweight locking. We don't want another lock-with-irqs-off on our
netfront data paths.

 -- Keir

Re: [PATCH 3/4] Support accelerated network plugin modules

[Kieran Mansley]

On Wed, 2007-05-09 at 14:09 +0100, Keir Fraser wrote:
> > It's a matter of taste, but I'd prefer it if it was obvious when
> > looking at the code that the hook is being called with a spinlock
> > held. That's actually another thing - why must every hook be called
> > with the spinlock held? if it's to protect the accelerator from going
> > away, what's actually needed is a ref count (struct kref) on the
> > accelerator.
> 
> I agree the lock should go. Removing the accelerator from under the feet of
> an active vif just doesn't seem a sane action to support.

It's been quite useful for me to be able to do so, and not supporting it
is in some ways more effort than supporting it.

>  And it should be
> possible to support atomic-enough addition of an accelerator without need
> for heavyweight locking. We don't want another lock-with-irqs-off on our
> netfront data paths.

I'm happy to replace that spinlock with a kref as suggested by Muli, and
this should then make that macro redundant too.  However, I'd been
careful to avoid spinlocks on the data path (it uses netif_poll_disable
() and netif_tx_disable() to protect those when removing) so this won't
deliver a benefit there as you might expect it to. 

Kieran