Re: [PATCH] xen network backend driver

[Ben Hutchings <bhutchings@solarflare.com>]

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.