Xen devel - Feb 2010 - [GIT] netback fixes from XCP kernel tree

The following changes since commit c6f55dd4eb6ec8bdcfe221210a84907c496f6a75:
  Jeremy Fitzhardinge (1):
        xen/netback: include linux/sched.h for TASK_* definitions

are available in the git repository at:

  git://xenbits.xensource.com/people/ianc/linux-2.6.git for-jeremy/netback

Ian Campbell (5):
      xen: netback: remove unused xen_network_done code
      xen: netback: factor disconnect from backend into new function.
      xen: netback: wait for hotplug scripts to complete before signalling
connected to frontend
      xen/netback: Always pull through PKT_PROT_LEN bytes into the linear part
of an skb.
      xen/netback: Allow setting of large MTU before rings have connected.

Steven Smith (1):
      xen/netback: try to pull a minimum of 72 bytes into the skb data area

 drivers/xen/netback/common.h    |    2 +
 drivers/xen/netback/interface.c |    6 +++-
 drivers/xen/netback/netback.c   |   42 +++++++----------------
 drivers/xen/netback/xenbus.c    |   69 ++++++++++++++++++++++++++++++++++----
 4 files changed, 81 insertions(+), 38 deletions(-)



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

It has been disabled effectively forever.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/netback/netback.c |   23 -----------------------
 1 files changed, 0 insertions(+), 23 deletions(-)

diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
index d88103a..7e1dfd1 100644
--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -343,25 +343,6 @@ int netif_be_start_xmit(struct sk_buff *skb, struct
net_device *dev)
 	return 0;
 }
 
-#if 0
-static void xen_network_done_notify(void)
-{
-	static struct net_device *eth0_dev = NULL;
-	if (unlikely(eth0_dev == NULL))
-		eth0_dev = __dev_get_by_name("eth0");
-	netif_rx_schedule(eth0_dev);
-}
-/*
- * Add following to poll() function in NAPI driver (Tigon3 is example):
- *  if ( xen_network_done() )
- *      tg3_enable_ints(tp);
- */
-int xen_network_done(void)
-{
-	return skb_queue_empty(&rx_queue);
-}
-#endif
-
 struct netrx_pending_operations {
 	unsigned trans_prod, trans_cons;
 	unsigned mmu_prod, mmu_mcl;
@@ -676,10 +657,6 @@ static void net_rx_action(unsigned long unused)
 	/* More work to do? */
 	if (!skb_queue_empty(&rx_queue) && !timer_pending(&net_timer))
 		tasklet_schedule(&net_rx_tasklet);
-#if 0
-	else
-		xen_network_done_notify();
-#endif
 }
 
 static void net_alarm(unsigned long unused)
-- 
1.5.6.5


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

Makes subsequent patches cleaner.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/netback/xenbus.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/xen/netback/xenbus.c b/drivers/xen/netback/xenbus.c
index 9022c99..b114aa4 100644
--- a/drivers/xen/netback/xenbus.c
+++ b/drivers/xen/netback/xenbus.c
@@ -213,6 +213,16 @@ static void backend_create_netif(struct backend_info *be)
 }
 
 
+static void disconnect_backend(struct xenbus_device *dev)
+{
+	struct backend_info *be = dev_get_drvdata(&dev->dev);
+
+	if (be->netif) {
+		netif_disconnect(be->netif);
+		be->netif = NULL;
+	}
+}
+
 /**
  * Callback received when the frontend''s state changes.
  */
@@ -246,11 +256,9 @@ static void frontend_changed(struct xenbus_device *dev,
 		break;
 
 	case XenbusStateClosing:
-		if (be->netif) {
+		if (be->netif)
 			kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
-			netif_disconnect(be->netif);
-			be->netif = NULL;
-		}
+		disconnect_backend(dev);
 		xenbus_switch_state(dev, XenbusStateClosing);
 		break;
 
-- 
1.5.6.5


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

Avoid the situation where the frontend is sending packets but the
domain 0 bridging (or whatever) is not yet configured (because the
hotplug scripts are too slow) and so packets get dropped.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Steven.Smith@citrix.com
---
 drivers/xen/netback/common.h |    2 +
 drivers/xen/netback/xenbus.c |   45 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/netback/common.h b/drivers/xen/netback/common.h
index ea617b9..51f97c0 100644
--- a/drivers/xen/netback/common.h
+++ b/drivers/xen/netback/common.h
@@ -147,6 +147,8 @@ struct backend_info {
 	struct xenbus_device *dev;
 	struct xen_netif *netif;
 	enum xenbus_state frontend_state;
+	struct xenbus_watch hotplug_status_watch;
+	int have_hotplug_status_watch:1;
 
 	/* State relating to the netback accelerator */
 	void *netback_accel_priv;
diff --git a/drivers/xen/netback/xenbus.c b/drivers/xen/netback/xenbus.c
index b114aa4..b89853a 100644
--- a/drivers/xen/netback/xenbus.c
+++ b/drivers/xen/netback/xenbus.c
@@ -32,6 +32,7 @@
 static int connect_rings(struct backend_info *);
 static void connect(struct backend_info *);
 static void backend_create_netif(struct backend_info *be);
+static void unregister_hotplug_status_watch(struct backend_info *be);
 
 static int netback_remove(struct xenbus_device *dev)
 {
@@ -39,8 +40,10 @@ static int netback_remove(struct xenbus_device *dev)
 
 	//netback_remove_accelerators(be, dev);
 
+	unregister_hotplug_status_watch(be);
 	if (be->netif) {
 		kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
+		xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
 		netif_disconnect(be->netif);
 		be->netif = NULL;
 	}
@@ -218,6 +221,7 @@ static void disconnect_backend(struct xenbus_device *dev)
 	struct backend_info *be = dev_get_drvdata(&dev->dev);
 
 	if (be->netif) {
+		xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
 		netif_disconnect(be->netif);
 		be->netif = NULL;
 	}
@@ -337,6 +341,36 @@ static int xen_net_read_mac(struct xenbus_device *dev, u8
mac[])
 	return 0;
 }
 
+static void unregister_hotplug_status_watch(struct backend_info *be)
+{
+	if (be->have_hotplug_status_watch) {
+		unregister_xenbus_watch(&be->hotplug_status_watch);
+		kfree(be->hotplug_status_watch.node);
+	}
+	be->have_hotplug_status_watch = 0;
+}
+
+static void hotplug_status_changed(struct xenbus_watch *watch,
+				   const char **vec,
+				   unsigned int vec_size)
+{
+	struct backend_info *be = container_of(watch,
+					       struct backend_info,
+					       hotplug_status_watch);
+	char *str;
+	unsigned int len;
+
+	str = xenbus_read(XBT_NIL, be->dev->nodename,
"hotplug-status", &len);
+	if (IS_ERR(str))
+		return;
+	if (len == sizeof("connected")-1 && !memcmp(str,
"connected", len)) {
+		xenbus_switch_state(be->dev, XenbusStateConnected);
+		/* Not interested in this watch anymore. */
+		unregister_hotplug_status_watch(be);
+	}
+	kfree(str);
+}
+
 static void connect(struct backend_info *be)
 {
 	int err;
@@ -356,7 +390,16 @@ static void connect(struct backend_info *be)
 			  &be->netif->credit_usec);
 	be->netif->remaining_credit = be->netif->credit_bytes;
 
-	xenbus_switch_state(dev, XenbusStateConnected);
+	unregister_hotplug_status_watch(be);
+	err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch,
+				   hotplug_status_changed,
+				   "%s/%s", dev->nodename, "hotplug-status");
+	if (err) {
+		/* Switch now, since we can''t do a watch. */
+		xenbus_switch_state(dev, XenbusStateConnected);
+	} else {
+		be->have_hotplug_status_watch = 1;
+	}
 
 	netif_wake_queue(be->netif->dev);
 }
-- 
1.5.6.5


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

Previously PKT_PROT_LEN would only have an effect on the first fragment.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/netback/netback.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
index 7e1dfd1..e668704 100644
--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -1346,6 +1346,16 @@ static void net_tx_submit(void)
 
 		netbk_fill_frags(skb);
 
+		/*
+		 * If the initial fragment was < PKT_PROT_LEN then
+		 * pull through some bytes from the other fragments to
+		 * increase the linear region to PKT_PROT_LEN bytes.
+		 */
+		if (skb_headlen(skb) < PKT_PROT_LEN && skb_is_nonlinear(skb)) {
+			int target = min_t(int, skb->len, PKT_PROT_LEN);
+			__pskb_pull_tail(skb, target - skb_headlen(skb));
+		}
+
 		skb->dev      = netif->dev;
 		skb->protocol = eth_type_trans(skb, skb->dev);
 
-- 
1.5.6.5


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

From: Steven Smith <ssmith@xensource.com>

---
 drivers/xen/netback/netback.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
index e668704..0bc6398 100644
--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -116,13 +116,10 @@ static inline int netif_page_index(struct page *pg)
 /*
  * This is the amount of packet we copy rather than map, so that the
  * guest can''t fiddle with the contents of the headers while we do
- * packet processing on them (netfilter, routing, etc).  This could
- * probably do with being larger, since 1) 64-bytes isn''t necessarily
- * long enough to cover a full christmas-tree ip+tcp header, let alone
- * packet contents, and 2) the data is probably in cache anyway
- * (though perhaps some other cpu''s cache).
+ * packet processing on them (netfilter, routing, etc). 72 is enough
+ * to cover TCP+IP headers including options.
  */
-#define PKT_PROT_LEN 64
+#define PKT_PROT_LEN 72
 
 static struct pending_tx_info {
 	struct xen_netif_tx_request req;
-- 
1.5.6.5


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

Something odd happened with git-formatpatch here, there was a commit
message originally:

        xen/netback: try to pull a minimum of 72 bytes into the skb data area
        when receiving a packet into netback.  The previous number, 64, tended
        to place a fragment boundary in the middle of the TCP header options
        and led to unnecessary fragmentation in Windows <-> Windows
        networking.

Ian.

On Tue, 2010-02-23 at 16:47 +0000, Ian Campbell wrote:
> From: Steven Smith <ssmith@xensource.com>
> 
> ---
>  drivers/xen/netback/netback.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
> index e668704..0bc6398 100644
> --- a/drivers/xen/netback/netback.c
> +++ b/drivers/xen/netback/netback.c
> @@ -116,13 +116,10 @@ static inline int netif_page_index(struct page *pg)
>  /*
>   * This is the amount of packet we copy rather than map, so that the
>   * guest can''t fiddle with the contents of the headers while we
do
> - * packet processing on them (netfilter, routing, etc).  This could
> - * probably do with being larger, since 1) 64-bytes isn''t
necessarily
> - * long enough to cover a full christmas-tree ip+tcp header, let alone
> - * packet contents, and 2) the data is probably in cache anyway
> - * (though perhaps some other cpu''s cache).
> + * packet processing on them (netfilter, routing, etc). 72 is enough
> + * to cover TCP+IP headers including options.
>   */
> -#define PKT_PROT_LEN 64
> +#define PKT_PROT_LEN 72
>  
>  static struct pending_tx_info {
>  	struct xen_netif_tx_request req;


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

Could you point out what problem this addresses?

Thanks, Jan
>>> Ian Campbell <ian.campbell@citrix.com> 23.02.10 17:47
>>>
Previously PKT_PROT_LEN would only have an effect on the first fragment.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/netback/netback.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
index 7e1dfd1..e668704 100644
--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -1346,6 +1346,16 @@ static void net_tx_submit(void)
 
 		netbk_fill_frags(skb);
 
+		/*
+		 * If the initial fragment was < PKT_PROT_LEN then
+		 * pull through some bytes from the other fragments to
+		 * increase the linear region to PKT_PROT_LEN bytes.
+		 */
+		if (skb_headlen(skb) < PKT_PROT_LEN && skb_is_nonlinear(skb)) {
+			int target = min_t(int, skb->len, PKT_PROT_LEN);
+			__pskb_pull_tail(skb, target - skb_headlen(skb));
+		}
+
 		skb->dev      = netif->dev;
 		skb->protocol = eth_type_trans(skb, skb->dev);
 
-- 
1.5.6.5


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


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

On Wed, 2010-02-24 at 08:28 +0000, Jan Beulich wrote: 
> Could you point out what problem this addresses?
It ensures that at least the TCP/IP headers will be pulled into the
linear part of the SKB. At least skb_checksum_setup relies on this and I
think it is a more generic assumption in at least some parts of the
network stack as well. The next patch increases PKT_PROT_LEN to include
the TCP options as well since we have observed cases where Windows
guests with PV drivers can generate a frame with a split at the point.

In the common case the first fragment should already contain
PKT_PROT_LEN bytes so I don''t think it will trigger often.

Ian.
> 
> Thanks, Jan
> 
> >>> Ian Campbell <ian.campbell@citrix.com> 23.02.10 17:47
>>>
> Previously PKT_PROT_LEN would only have an effect on the first fragment.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  drivers/xen/netback/netback.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
> index 7e1dfd1..e668704 100644
> --- a/drivers/xen/netback/netback.c
> +++ b/drivers/xen/netback/netback.c
> @@ -1346,6 +1346,16 @@ static void net_tx_submit(void)
>  
>  		netbk_fill_frags(skb);
>  
> +		/*
> +		 * If the initial fragment was < PKT_PROT_LEN then
> +		 * pull through some bytes from the other fragments to
> +		 * increase the linear region to PKT_PROT_LEN bytes.
> +		 */
> +		if (skb_headlen(skb) < PKT_PROT_LEN && skb_is_nonlinear(skb))
{
> +			int target = min_t(int, skb->len, PKT_PROT_LEN);
> +			__pskb_pull_tail(skb, target - skb_headlen(skb));
> +		}
> +
>  		skb->dev      = netif->dev;
>  		skb->protocol = eth_type_trans(skb, skb->dev);
>  


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

> 
> On Wed, 2010-02-24 at 08:28 +0000, Jan Beulich wrote:
> > Could you point out what problem this addresses?
> 
> It ensures that at least the TCP/IP headers will be pulled into the
> linear part of the SKB. At least skb_checksum_setup relies on this and
I
> think it is a more generic assumption in at least some parts of the
> network stack as well. The next patch increases PKT_PROT_LEN to
include
> the TCP options as well since we have observed cases where Windows
> guests with PV drivers can generate a frame with a split at the point.
> 
My PV drivers couldn''t always be relied on to do this, because Windows
couldn''t be relied on to do it either. I just coalesced the first
buffer
to some minimum value depending on the header type. If you are doing TCP
checksum offload then you have to tell Windows that you support IP
checksum offload too - Linux doesn''t support that so you have to lie to
Windows about it and calculate the IP checksum in the PV drivers, and if
you are doing that you have to have your own copy of the header anyway
so it turns out not to be a big deal.

James


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

On Wed, 2010-02-24 at 09:23 +0000, James Harper wrote:
> > 
> > On Wed, 2010-02-24 at 08:28 +0000, Jan Beulich wrote:
> > > Could you point out what problem this addresses?
> > 
> > It ensures that at least the TCP/IP headers will be pulled into the
> > linear part of the SKB. At least skb_checksum_setup relies on this and
> I
> > think it is a more generic assumption in at least some parts of the
> > network stack as well. The next patch increases PKT_PROT_LEN to
> include
> > the TCP options as well since we have observed cases where Windows
> > guests with PV drivers can generate a frame with a split at the point.
> > 
> 
> My PV drivers couldn''t always be relied on to do this, because
Windows
> couldn''t be relied on to do it either. I just coalesced the first
buffer
> to some minimum value depending on the header type. If you are doing TCP
> checksum offload then you have to tell Windows that you support IP
> checksum offload too - Linux doesn''t support that so you have to
lie to
> Windows about it and calculate the IP checksum in the PV drivers, and if
> you are doing that you have to have your own copy of the header anyway
> so it turns out not to be a big deal.
Yes, it''s best if the guest takes care of this in the common case but
domain 0 can''t trust the guest so we need a backstop in netback too. If
the guest is doing the sane thing the backstop shouldn''t trigger.

Ian.


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