[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] Re: [PATCH] xen network backend driver
On Wed, 2011-01-19 at 15:01 +0000, Ian Campbell wrote: [...] > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig > index cbf0635..5b088f5 100644 > --- a/drivers/net/Kconfig > +++ b/drivers/net/Kconfig > @@ -2970,6 +2970,13 @@ config XEN_NETDEV_FRONTEND > if you are compiling a kernel for a Xen guest, you almost > certainly want to enable this. > > +config XEN_NETDEV_BACKEND > + tristate "Xen backend network device" > + depends on XEN_BACKEND > + help > + Implement the network backend driver, which passes packets > + from the guest domain's frontend drivers to the network. This is not an accurate description. The backend driver doesn't pass packets to 'the network' (I assume that means physical network); that's done by a bridge or by routing. [...] > diff --git a/drivers/net/xen-netback/Makefile > b/drivers/net/xen-netback/Makefile > new file mode 100644 > index 0000000..e346e81 > --- /dev/null > +++ b/drivers/net/xen-netback/Makefile > @@ -0,0 +1,3 @@ > +obj-$(CONFIG_XEN_NETDEV_BACKEND) := xen-netback.o > + > +xen-netback-y := netback.o xenbus.o interface.o > diff --git a/drivers/net/xen-netback/common.h > b/drivers/net/xen-netback/common.h > new file mode 100644 > index 0000000..2d55ed6 > --- /dev/null > +++ b/drivers/net/xen-netback/common.h > @@ -0,0 +1,250 @@ > +/****************************************************************************** > + * arch/xen/drivers/netif/backend/common.h Doesn't match the actual filename, but then why include the filename at all? [...] > +#ifndef __NETIF__BACKEND__COMMON_H__ > +#define __NETIF__BACKEND__COMMON_H__ Also doesn't match the filename. > + > +#define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__ > + > +#include <linux/version.h> Don't include <linux/version.h> in an in-tree driver. [...] > +void netif_disconnect(struct xen_netif *netif); > + > +void netif_set_features(struct xen_netif *netif); > +struct xen_netif *netif_alloc(struct device *parent, domid_t domid, > + unsigned int handle); [...] The 'netif_' prefix belongs to the networking core; you should use some other prefix for these. [...] > --- /dev/null > +++ b/drivers/net/xen-netback/interface.c [...] > +/* > + * Module parameter 'queue_length': > + * > + * Enables queuing in the network stack when a client has run out of receive > + * descriptors. > + */ > +static unsigned long netbk_queue_length = 32; > +module_param_named(queue_length, netbk_queue_length, ulong, 0644); This can be controlled through sysfs later, so it doesn't need to be a module parameter. [...] > +static int netbk_set_tx_csum(struct net_device *dev, u32 data) > +{ > + struct xen_netif *netif = netdev_priv(dev); > + if (data) { > + if (!netif->csum) > + return -ENOSYS; Should be -EOPNOTSUPP. Same in netbk_set_sg(), netbk_set_tso(). [...] > +struct xen_netif *netif_alloc(struct device *parent, domid_t domid, > + unsigned int handle) > +{ [...] > + /* > + * Initialise a dummy MAC address. We choose the numerically > + * largest non-broadcast address to prevent the address getting > + * stolen by an Ethernet bridge for STP purposes. > + * (FE:FF:FF:FF:FF:FF) > + */ > + memset(dev->dev_addr, 0xFF, ETH_ALEN); > + dev->dev_addr[0] &= ~0x01; I'm a bit dubious about this. [...] > --- /dev/null > +++ b/drivers/net/xen-netback/netback.c [...] > +static void net_tx_action(unsigned long data); > + > +static void net_rx_action(unsigned long data); The 'net_' prefix belongs to the networking core. [...] > +static int netif_get_page_ext(struct page *pg, > + unsigned int *_group, unsigned int *_idx) The '_' prefix is usually meant to distinguish lower-level functions or to avoid naming conflicts in macros. I don't see any justification for using it here. [...] > +static int MODPARM_netback_kthread; > +module_param_named(netback_kthread, MODPARM_netback_kthread, bool, 0); > +MODULE_PARM_DESC(netback_kthread, "Use kernel thread to replace tasklet"); > + > +/* > + * Netback bottom half handler. > + * dir indicates the data direction. > + * rx: 1, tx: 0. > + */ > +static inline void xen_netbk_bh_handler(struct xen_netbk *netbk, int dir) > +{ > + if (MODPARM_netback_kthread) > + wake_up(&netbk->kthread.netbk_action_wq); > + else if (dir) > + tasklet_schedule(&netbk->tasklet.net_rx_tasklet); > + else > + tasklet_schedule(&netbk->tasklet.net_tx_tasklet); > +} Ugh, please just use NAPI. [...] > +static struct sk_buff *netbk_copy_skb(struct sk_buff *skb) > +{ This should be implemented in net/core/skbuff.c as a variant of skb_copy() and pskb_copy(), sharing as much code as possible. [...] > +static int skb_checksum_setup(struct sk_buff *skb) The 'skb_' prefix belongs to the networking core. > +{ > + struct iphdr *iph; > + unsigned char *th; > + int err = -EPROTO; > + > + if (skb->protocol != htons(ETH_P_IP)) > + goto out; > + > + iph = (void *)skb->data; > + th = skb->data + 4 * iph->ihl; > + if (th >= skb_tail_pointer(skb)) > + goto out; > + > + skb->csum_start = th - skb->head; > + switch (iph->protocol) { > + case IPPROTO_TCP: > + skb->csum_offset = offsetof(struct tcphdr, check); > + break; > + case IPPROTO_UDP: > + skb->csum_offset = offsetof(struct udphdr, check); > + break; > + default: > + if (net_ratelimit()) > + printk(KERN_ERR "Attempting to checksum a non-" > + "TCP/UDP packet, dropping a protocol" > + " %d packet", iph->protocol); This is missing a newline, and missing any indicator of what generated it. Use pr_err() to cover the latter. [...] > +#ifdef NETBE_DEBUG_INTERRUPT > +static irqreturn_t netif_be_dbg(int irq, void *dev_id, struct pt_regs *regs) This wouldn't compile on anything after 2.6.18! Clearly no-one defines NETBE_DEBUG_INTERRUPT, and you can remove this code entirely. [...] > +module_init(netback_init); [...] No module_fini? Ben. -- Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |