[PATCH] Re-define PKT_PROT_LEN to be bigger.

[PATCH] Re-define PKT_PROT_LEN to be bigger.

[Paul Durrant]

Re-define PKT_PROT_LEN to be big enough to handle maximal IPv4 and TCP options and phrase
the definition so that it's reasonably obvious that's what it's for.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
 drivers/xen/netback/netback.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
index c448675..1a4a20e 100644
--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -128,7 +128,7 @@ static inline int netif_get_page_ext(struct page *pg, unsigned int *_group, unsi
  * packet processing on them (netfilter, routing, etc). 72 is enough
  * to cover TCP+IP headers including options.
  */
-#define PKT_PROT_LEN 72
+#define PKT_PROT_LEN    (ETH_HLEN + 4 + (15 * 4) + (15 * 4))
 
 static inline pending_ring_idx_t pending_index(unsigned i)
 {
-- 
1.5.6.5

[PATCH] Don't count packets we don't actually receive.

[Paul Durrant]

Make sure we only bump rx_packets when we're definitely going to call netif_rx_ni().

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
 drivers/xen/netback/netback.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
index 1a4a20e..066d140 100644
--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -1519,9 +1519,6 @@ static void net_tx_submit(struct xen_netbk *netbk)
 		skb->dev      = netif->dev;
 		skb->protocol = eth_type_trans(skb, skb->dev);
 
-		netif->stats.rx_bytes += skb->len;
-		netif->stats.rx_packets++;
-
 		if (skb->ip_summed == CHECKSUM_PARTIAL) {
 			if (skb_checksum_setup(skb)) {
 				DPRINTK("Can't setup checksum in net_tx_action\n");
@@ -1537,6 +1534,9 @@ static void net_tx_submit(struct xen_netbk *netbk)
 			continue;
 		}
 
+		netif->stats.rx_bytes += skb->len;
+		netif->stats.rx_packets++;
+
 		netif_rx_ni(skb);
 		netif->dev->last_rx = jiffies;
 	}
-- 
1.5.6.5

[PATCH] Remove the 500ms timeout to restart the netif queue.

[Paul Durrant]

It is generally unhelpful as it results in a massive tail-drop should a guest become
unresponsive for a relatively short period of time and no back-pressure (other than
that caused by a higher layer protocol) is applied to the sender.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
 drivers/xen/netback/netback.c |   20 +-------------------
 1 files changed, 1 insertions(+), 19 deletions(-)

diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
index 066d140..87a2cd4 100644
--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -271,13 +271,6 @@ static inline int netbk_queue_full(struct xen_netif *netif)
 	       ((netif->rx.rsp_prod_pvt + NET_RX_RING_SIZE - peek) < needed);
 }
 
-static void tx_queue_callback(unsigned long data)
-{
-	struct xen_netif *netif = (struct xen_netif *)data;
-	if (netif_schedulable(netif))
-		netif_wake_queue(netif->dev);
-}
-
 /* Figure out how many ring slots we're going to need to send @skb to
    the guest. */
 static unsigned count_skb_slots(struct sk_buff *skb, struct xen_netif *netif)
@@ -360,19 +353,8 @@ int netif_be_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		netif->rx.sring->req_event = netif->rx_req_cons_peek +
 			netbk_max_required_rx_slots(netif);
 		mb(); /* request notification /then/ check & stop the queue */
-		if (netbk_queue_full(netif)) {
+		if (netbk_queue_full(netif))
 			netif_stop_queue(dev);
-			/*
-			 * Schedule 500ms timeout to restart the queue, thus
-			 * ensuring that an inactive queue will be drained.
-			 * Packets will be immediately be dropped until more
-			 * receive buffers become available (see
-			 * netbk_queue_full() check above).
-			 */
-			netif->tx_queue_timeout.data = (unsigned long)netif;
-			netif->tx_queue_timeout.function = tx_queue_callback;
-			mod_timer(&netif->tx_queue_timeout, jiffies + HZ/2);
-		}
 	}
 	skb_queue_tail(&netbk->rx_queue, skb);
 
-- 
1.5.6.5

[PATCH] Add a missing test to tx_work_todo.

[Paul Durrant]

Adda test so that, when netback is using worker threads, net_tx_action()
gets called in a timely manner when the pending_inuse list is populated.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
 drivers/xen/netback/netback.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
index 87a2cd4..eca61a9 100644
--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -1712,6 +1712,10 @@ static inline int tx_work_todo(struct xen_netbk *netbk)
 	if (netbk->dealloc_cons != netbk->dealloc_prod)
 		return 1;
 
+	if (netbk_copy_skb_mode == NETBK_DELAYED_COPY_SKB &&
+	    !list_empty(&netbk->pending_inuse_head))
+		return 1;
+
 	if (((nr_pending_reqs(netbk) + MAX_SKB_FRAGS) < MAX_PENDING_REQS) &&
 			!list_empty(&netbk->net_schedule_list))
 		return 1;
-- 
1.5.6.5

[PATCH] Re-factor net_tx_action_dealloc() slightly.

[Paul Durrant]

There is no need for processing of the pending_inuse list to be within the dealloc_prod/cons
loop.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
 drivers/xen/netback/netback.c |   26 ++++++++++++++------------
 1 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
index eca61a9..25adbf4 100644
--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -913,11 +913,20 @@ static inline void net_tx_action_dealloc(struct xen_netbk *netbk)
 			gop++;
 		}
 
-		if (netbk_copy_skb_mode != NETBK_DELAYED_COPY_SKB ||
-		    list_empty(&netbk->pending_inuse_head))
-			break;
+	} while (dp != netbk->dealloc_prod);
+
+	netbk->dealloc_cons = dc;
 
-		/* Copy any entries that have been pending for too long. */
+	ret = HYPERVISOR_grant_table_op(
+		GNTTABOP_unmap_grant_ref, netbk->tx_unmap_ops,
+		gop - netbk->tx_unmap_ops);
+	BUG_ON(ret);
+
+	/*
+	 * Copy any entries that have been pending for too long
+	 */
+	if (netbk_copy_skb_mode == NETBK_DELAYED_COPY_SKB &&
+	    !list_empty(&netbk->pending_inuse_head)) {
 		list_for_each_entry_safe(inuse, n,
 				&netbk->pending_inuse_head, list) {
 			struct pending_tx_info *pending_tx_info;
@@ -943,14 +952,7 @@ static inline void net_tx_action_dealloc(struct xen_netbk *netbk)
 
 			break;
 		}
-	} while (dp != netbk->dealloc_prod);
-
-	netbk->dealloc_cons = dc;
-
-	ret = HYPERVISOR_grant_table_op(
-		GNTTABOP_unmap_grant_ref, netbk->tx_unmap_ops,
-		gop - netbk->tx_unmap_ops);
-	BUG_ON(ret);
+	}
 
 	list_for_each_entry_safe(inuse, n, &list, list) {
 		struct pending_tx_info *pending_tx_info;
-- 
1.5.6.5

Re: [PATCH] Remove the 500ms timeout to restart the netif queue.

[Konrad Rzeszutek Wilk]

On Tue, Dec 14, 2010 at 08:35:20PM +0000, Paul Durrant wrote:
> It is generally unhelpful as it results in a massive tail-drop should a guest become
> unresponsive for a relatively short period of time and no back-pressure (other than
> that caused by a higher layer protocol) is applied to the sender.

The patch just removes the timeout, so the queue will be considered unavailable..
So will the queue be restarted via the net_rx_action logic?

> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
>  drivers/xen/netback/netback.c |   20 +-------------------
>  1 files changed, 1 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
> index 066d140..87a2cd4 100644
> --- a/drivers/xen/netback/netback.c
> +++ b/drivers/xen/netback/netback.c
> @@ -271,13 +271,6 @@ static inline int netbk_queue_full(struct xen_netif *netif)
>  	       ((netif->rx.rsp_prod_pvt + NET_RX_RING_SIZE - peek) < needed);
>  }
>  
> -static void tx_queue_callback(unsigned long data)
> -{
> -	struct xen_netif *netif = (struct xen_netif *)data;
> -	if (netif_schedulable(netif))
> -		netif_wake_queue(netif->dev);
> -}
> -
>  /* Figure out how many ring slots we're going to need to send @skb to
>     the guest. */
>  static unsigned count_skb_slots(struct sk_buff *skb, struct xen_netif *netif)
> @@ -360,19 +353,8 @@ int netif_be_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  		netif->rx.sring->req_event = netif->rx_req_cons_peek +
>  			netbk_max_required_rx_slots(netif);
>  		mb(); /* request notification /then/ check & stop the queue */
> -		if (netbk_queue_full(netif)) {
> +		if (netbk_queue_full(netif))
>  			netif_stop_queue(dev);
> -			/*
> -			 * Schedule 500ms timeout to restart the queue, thus
> -			 * ensuring that an inactive queue will be drained.
> -			 * Packets will be immediately be dropped until more
> -			 * receive buffers become available (see
> -			 * netbk_queue_full() check above).
> -			 */
> -			netif->tx_queue_timeout.data = (unsigned long)netif;
> -			netif->tx_queue_timeout.function = tx_queue_callback;
> -			mod_timer(&netif->tx_queue_timeout, jiffies + HZ/2);
> -		}
>  	}
>  	skb_queue_tail(&netbk->rx_queue, skb);
>  
> -- 
> 1.5.6.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: [PATCH] Remove the 500ms timeout to restart the netif queue.

[Paul Durrant]

Yes, the code to restart the queue when space becomes available in the shared ring is already there. Removal of the timeout simply means that the queue will not be restarted until that code runs.

  Paul

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: 15 December 2010 15:59
> To: Paul Durrant
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] Remove the 500ms timeout to restart
> the netif queue.
> 
> On Tue, Dec 14, 2010 at 08:35:20PM +0000, Paul Durrant wrote:
> > It is generally unhelpful as it results in a massive tail-drop
> should a guest become
> > unresponsive for a relatively short period of time and no back-
> pressure (other than
> > that caused by a higher layer protocol) is applied to the sender.
> 
> The patch just removes the timeout, so the queue will be considered
> unavailable..
> So will the queue be restarted via the net_rx_action logic?
> 
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> >  drivers/xen/netback/netback.c |   20 +-------------------
> >  1 files changed, 1 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/xen/netback/netback.c
> b/drivers/xen/netback/netback.c
> > index 066d140..87a2cd4 100644
> > --- a/drivers/xen/netback/netback.c
> > +++ b/drivers/xen/netback/netback.c
> > @@ -271,13 +271,6 @@ static inline int netbk_queue_full(struct
> xen_netif *netif)
> >  	       ((netif->rx.rsp_prod_pvt + NET_RX_RING_SIZE - peek) <
> needed);
> >  }
> >
> > -static void tx_queue_callback(unsigned long data)
> > -{
> > -	struct xen_netif *netif = (struct xen_netif *)data;
> > -	if (netif_schedulable(netif))
> > -		netif_wake_queue(netif->dev);
> > -}
> > -
> >  /* Figure out how many ring slots we're going to need to send
> @skb to
> >     the guest. */
> >  static unsigned count_skb_slots(struct sk_buff *skb, struct
> xen_netif *netif)
> > @@ -360,19 +353,8 @@ int netif_be_start_xmit(struct sk_buff *skb,
> struct net_device *dev)
> >  		netif->rx.sring->req_event = netif->rx_req_cons_peek +
> >  			netbk_max_required_rx_slots(netif);
> >  		mb(); /* request notification /then/ check & stop the
> queue */
> > -		if (netbk_queue_full(netif)) {
> > +		if (netbk_queue_full(netif))
> >  			netif_stop_queue(dev);
> > -			/*
> > -			 * Schedule 500ms timeout to restart the queue,
> thus
> > -			 * ensuring that an inactive queue will be
> drained.
> > -			 * Packets will be immediately be dropped until
> more
> > -			 * receive buffers become available (see
> > -			 * netbk_queue_full() check above).
> > -			 */
> > -			netif->tx_queue_timeout.data = (unsigned
> long)netif;
> > -			netif->tx_queue_timeout.function =
> tx_queue_callback;
> > -			mod_timer(&netif->tx_queue_timeout, jiffies +
> HZ/2);
> > -		}
> >  	}
> >  	skb_queue_tail(&netbk->rx_queue, skb);
> >
> > --
> > 1.5.6.5
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel

Re: [PATCH] Re-define PKT_PROT_LEN to be bigger.

[Jeremy Fitzhardinge]

On 12/14/2010 12:35 PM, Paul Durrant wrote:
> Re-define PKT_PROT_LEN to be big enough to handle maximal IPv4 and TCP options and phrase
> the definition so that it's reasonably obvious that's what it's for.

Which kernel are these for?

Thanks,
    J

> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
>  drivers/xen/netback/netback.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
> index c448675..1a4a20e 100644
> --- a/drivers/xen/netback/netback.c
> +++ b/drivers/xen/netback/netback.c
> @@ -128,7 +128,7 @@ static inline int netif_get_page_ext(struct page *pg, unsigned int *_group, unsi
>   * packet processing on them (netfilter, routing, etc). 72 is enough
>   * to cover TCP+IP headers including options.
>   */
> -#define PKT_PROT_LEN 72
> +#define PKT_PROT_LEN    (ETH_HLEN + 4 + (15 * 4) + (15 * 4))
>  
>  static inline pending_ring_idx_t pending_index(unsigned i)
>  {

RE: [PATCH] Re-define PKT_PROT_LEN to be bigger.

[Paul Durrant]

Sorry, should have said... These are patches against the pvops xen/stable-2.6.32.x branch.

  Cheers,

    Paul

> -----Original Message-----
> From: Jeremy Fitzhardinge [mailto:jeremy@goop.org]
> Sent: 14 December 2010 22:17
> To: Paul Durrant
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] Re-define PKT_PROT_LEN to be
> bigger.
> 
> On 12/14/2010 12:35 PM, Paul Durrant wrote:
> > Re-define PKT_PROT_LEN to be big enough to handle maximal IPv4 and
> TCP options and phrase
> > the definition so that it's reasonably obvious that's what it's
> for.
> 
> Which kernel are these for?
> 
> Thanks,
>     J
> 
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> >  drivers/xen/netback/netback.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/xen/netback/netback.c
> b/drivers/xen/netback/netback.c
> > index c448675..1a4a20e 100644
> > --- a/drivers/xen/netback/netback.c
> > +++ b/drivers/xen/netback/netback.c
> > @@ -128,7 +128,7 @@ static inline int netif_get_page_ext(struct
> page *pg, unsigned int *_group, unsi
> >   * packet processing on them (netfilter, routing, etc). 72 is
> enough
> >   * to cover TCP+IP headers including options.
> >   */
> > -#define PKT_PROT_LEN 72
> > +#define PKT_PROT_LEN    (ETH_HLEN + 4 + (15 * 4) + (15 * 4))
> >
> >  static inline pending_ring_idx_t pending_index(unsigned i)
> >  {

Re: [PATCH] Re-define PKT_PROT_LEN to be bigger.

[Ian Campbell]

On Tue, 2010-12-14 at 20:35 +0000, Paul Durrant wrote:
> Re-define PKT_PROT_LEN to be big enough to handle maximal IPv4 and TCP options and phrase
> the definition so that it's reasonably obvious that's what it's for.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
>  drivers/xen/netback/netback.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
> index c448675..1a4a20e 100644
> --- a/drivers/xen/netback/netback.c
> +++ b/drivers/xen/netback/netback.c
> @@ -128,7 +128,7 @@ static inline int netif_get_page_ext(struct page *pg, unsigned int *_group, unsi
>   * packet processing on them (netfilter, routing, etc). 72 is enough

Missed this comment.

>   * to cover TCP+IP headers including options.
>   */
> -#define PKT_PROT_LEN 72
> +#define PKT_PROT_LEN    (ETH_HLEN + 4 + (15 * 4) + (15 * 4))

Clearer as:
#define ETH_HLEN_VLAN_HLEN + \
	MAX_IPOPTLEN + sizeof(struct iphdr) + \
	MAX_TCP_OPTION_SPACE + sizeof(struct tcphdr)
?

Re: [PATCH] Re-define PKT_PROT_LEN to be bigger.

[Ian Campbell]

I applied these 5 patches (replacing this first one with the repost from
<1292406492-20516-1-git-send-email-paul.durrant@citrix.com>) to my WIP
branch for upstreaming. Not sure if they also want to go into the
next-2.6.32 branch.

Ian.

On Tue, 2010-12-14 at 20:35 +0000, Paul Durrant wrote:
> Re-define PKT_PROT_LEN to be big enough to handle maximal IPv4 and TCP options and phrase
> the definition so that it's reasonably obvious that's what it's for.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
>  drivers/xen/netback/netback.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
> index c448675..1a4a20e 100644
> --- a/drivers/xen/netback/netback.c
> +++ b/drivers/xen/netback/netback.c
> @@ -128,7 +128,7 @@ static inline int netif_get_page_ext(struct page *pg, unsigned int *_group, unsi
>   * packet processing on them (netfilter, routing, etc). 72 is enough
>   * to cover TCP+IP headers including options.
>   */
> -#define PKT_PROT_LEN 72
> +#define PKT_PROT_LEN    (ETH_HLEN + 4 + (15 * 4) + (15 * 4))
>  
>  static inline pending_ring_idx_t pending_index(unsigned i)
>  {

[PATCH] Re-define PKT_PROT_LEN to be bigger.

[Paul Durrant]

Re-define PKT_PROT_LEN to be big enough to handle maximal IPv4 and TCP options and phrase
the definition so that it's reasonably obvious that's what it's for.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
 drivers/xen/netback/netback.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
index c448675..368daa6 100644
--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -36,9 +36,11 @@
 
 #include "common.h"
 
-#include <linux/tcp.h>
-#include <linux/udp.h>
 #include <linux/kthread.h>
+#include <linux/if_vlan.h>
+#include <linux/udp.h>
+
+#include <net/tcp.h>
 
 #include <xen/balloon.h>
 #include <xen/events.h>
@@ -125,10 +127,12 @@ static inline int netif_get_page_ext(struct page *pg, unsigned int *_group, unsi
 /*
  * 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). 72 is enough
- * to cover TCP+IP headers including options.
+ * packet processing on them (netfilter, routing, etc).
  */
-#define PKT_PROT_LEN 72
+#define PKT_PROT_LEN    (ETH_HLEN + \
+			 VLAN_HLEN + \
+			 sizeof(struct iphdr) + MAX_IPOPTLEN + \
+			 sizeof(struct tcphdr) + MAX_TCP_OPTION_SPACE)
 
 static inline pending_ring_idx_t pending_index(unsigned i)
 {
-- 
1.5.6.5