diff mbox

br2684 testing needed for packet loss and performance

Message ID 4A97B3A9.6040103@hiramoto.org
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Karl Hiramoto Aug. 28, 2009, 10:38 a.m. UTC 
Hi,

Anyone care to test or comment on  these patches?  I've attached 
versions for 2.6.28 and 2.6.30.

Note I've done all my tests without any QOS settings.  I start my link 
by something like "br2684ctl -c 0 -e 0 -p 1 -b -a 8.32"

Without these patches, if you open one or many file transfers, then also 
do a ping you will notice heavy packet loss.  This packet loss is being 
caused by the call to dev_kfree_skb(skb);


With these patches my max throughput drops 20% to  40% or so on a 24 
Mbps ADSL2+  line, but no longer have packet loss.   On a 15Mbps (and 
slower) line,  I no longer see packet loss with these patches.



Thanks
--
Karl
Signed-off-by: Karl Hiramoto <karl@hiramoto.org>
Signed-off-by: Karl Hiramoto <karl@hiramoto.org>

--- /usr/src/linux-2.6.30.5/net/atm/br2684.c	2009-06-10 05:05:27.000000000 +0200
+++ linux-2.6.30.5/net/atm/br2684.c	2009-08-28 12:02:41.000000000 +0200
@@ -69,7 +69,7 @@ struct br2684_vcc {
 	struct net_device *device;
 	/* keep old push, pop functions for chaining */
 	void (*old_push) (struct atm_vcc * vcc, struct sk_buff * skb);
-	/* void (*old_pop)(struct atm_vcc *vcc, struct sk_buff *skb); */
+	void (*old_pop)(struct atm_vcc *vcc, struct sk_buff *skb);
 	enum br2684_encaps encaps;
 	struct list_head brvccs;
 #ifdef CONFIG_ATM_BR2684_IPFILTER
@@ -142,6 +142,22 @@ static struct net_device *br2684_find_de
 	return NULL;
 }
 
+/* chained vcc->pop function.  Check if we should wake the netif_queue */
+static void br2684_pop(struct atm_vcc *vcc, struct sk_buff *skb)
+{
+	struct br2684_vcc *brvcc = BR2684_VCC(vcc);
+	struct net_device *net_dev = skb->dev;
+
+	pr_debug("br2684_pop(vcc %p ; net_dev %p )\n", vcc, net_dev);
+	brvcc->old_pop(vcc, skb);
+
+	if (!net_dev)
+		return;
+
+ 	if (atm_may_send(vcc, 0) {
+		netif_wake_queue(net_dev);
+ 	}
+}
 /*
  * Send a packet out a particular vcc.  Not to useful right now, but paves
  * the way for multiple vcc's per itf.  Returns true if we can send,
@@ -200,20 +216,20 @@ static int br2684_xmit_vcc(struct sk_buf
 
 	ATM_SKB(skb)->vcc = atmvcc = brvcc->atmvcc;
 	pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, atmvcc, atmvcc->dev);
-	if (!atm_may_send(atmvcc, skb->truesize)) {
-		/*
-		 * We free this here for now, because we cannot know in a higher
-		 * layer whether the skb pointer it supplied wasn't freed yet.
-		 * Now, it always is.
-		 */
-		dev_kfree_skb(skb);
-		return 0;
-	}
 	atomic_add(skb->truesize, &sk_atm(atmvcc)->sk_wmem_alloc);
 	ATM_SKB(skb)->atm_options = atmvcc->atm_options;
 	dev->stats.tx_packets++;
 	dev->stats.tx_bytes += skb->len;
 	atmvcc->send(atmvcc, skb);
+	
+	if (!atm_may_send(atmvcc, 0)) {
+		netif_stop_queue(brvcc->device);
+		barrier();
+		/*check for race with br2684_pop*/
+		if (atm_may_send(atmvcc, 0))
+			netif_start_queue(brvcc->device);
+	}
+	
 	return 1;
 }
 
@@ -502,8 +518,10 @@ static int br2684_regvcc(struct atm_vcc 
 	atmvcc->user_back = brvcc;
 	brvcc->encaps = (enum br2684_encaps)be.encaps;
 	brvcc->old_push = atmvcc->push;
+	brvcc->old_pop = atmvcc->pop;
 	barrier();
 	atmvcc->push = br2684_push;
+	atmvcc->pop = br2684_pop;
 
 	rq = &sk_atm(atmvcc)->sk_receive_queue;

Comments

chas williams - CONTRACTOR Aug. 28, 2009, 12:25 p.m. UTC | #1 
In message <4A97B3A9.6040103@hiramoto.org>,Karl Hiramoto writes:
>Anyone care to test or comment on  these patches?  I've attached 
>versions for 2.6.28 and 2.6.30.

this needs to be against the net-2.6 git repository.  but the 2.6.30
would probably apply just fine.  except for the comments, below this
patch looks fine and makes sense.  a similar thing had to be done for
the lec.c interface.

>+/* chained vcc->pop function.  Check if we should wake the netif_queue */
>+static void br2684_pop(struct atm_vcc *vcc, struct sk_buff *skb)
>+{
>+	struct br2684_vcc *brvcc = BR2684_VCC(vcc);
>+	struct net_device *net_dev = brvcc->device;
>+
>+	pr_debug("br2684_pop(vcc %p ; net_dev %p )\n", vcc, net_dev);
>+	brvcc->old_pop(vcc, skb);
>+
>+	if (!net_dev)
>+		return;
>+
>+ 	if (atm_may_send(vcc, 0)) {
>+		netif_wake_queue(net_dev);
>+ 	}

you dont need the { } here.

>+
>+	if (!atm_may_send(atmvcc, 0)) {
>+  		netif_stop_queue(brvcc->device);
>+		barrier();
>+		/* check for race with br26864_pop*/
>+		if (atm_may_send(atmvcc, 0)) {
>+			netif_start_queue(brvcc->device);
>+		}
>+	}
>+

i dont think the barrier() is necessary.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karl Hiramoto Aug. 29, 2009, 10:24 a.m. UTC | #2 
Chas Williams (CONTRACTOR) wrote:
> In message <4A97B3A9.6040103@hiramoto.org>,Karl Hiramoto writes:
>   
>> Anyone care to test or comment on  these patches?  I've attached 
>> versions for 2.6.28 and 2.6.30.
>>     
>
> this needs to be against the net-2.6 git repository.  but the 2.6.30
> would probably apply just fine.  
Ok i will resend the patch against net-2.6  or should i do net-next-2.6?

Do i have to wait for a merge window?

> except for the comments, below this
> patch looks fine and makes sense.  a similar thing had to be done for
> the lec.c interface.
>
>   
Ok I'll clean up all these issues.

--
karl
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- linux-2.6.28.9.orig/net/atm/br2684.c	2009-03-23 22:55:52.000000000 +0100
+++ linux-2.6.28.9/net/atm/br2684.c	2009-08-28 12:00:43.000000000 +0200
@@ -69,7 +69,7 @@  struct br2684_vcc {
 	struct net_device *device;
 	/* keep old push, pop functions for chaining */
 	void (*old_push) (struct atm_vcc * vcc, struct sk_buff * skb);
-	/* void (*old_pop)(struct atm_vcc *vcc, struct sk_buff *skb); */
+	void (*old_pop)(struct atm_vcc *vcc, struct sk_buff *skb);
 	enum br2684_encaps encaps;
 	struct list_head brvccs;
 #ifdef CONFIG_ATM_BR2684_IPFILTER
@@ -143,6 +143,23 @@  static struct net_device *br2684_find_de
 	return NULL;
 }
 
+/* chained vcc->pop function.  Check if we should wake the netif_queue */
+static void br2684_pop(struct atm_vcc *vcc, struct sk_buff *skb)
+{
+	struct br2684_vcc *brvcc = BR2684_VCC(vcc);
+	struct net_device *net_dev = brvcc->device;
+
+	pr_debug("br2684_pop(vcc %p ; net_dev %p )\n", vcc, net_dev);
+	brvcc->old_pop(vcc, skb);
+
+	if (!net_dev)
+		return;
+
+ 	if (atm_may_send(vcc, 0)) {
+		netif_wake_queue(net_dev);
+ 	}
+}
+
 /*
  * Send a packet out a particular vcc.  Not to useful right now, but paves
  * the way for multiple vcc's per itf.  Returns true if we can send,
@@ -200,20 +217,22 @@  static int br2684_xmit_vcc(struct sk_buf
 
 	ATM_SKB(skb)->vcc = atmvcc = brvcc->atmvcc;
 	pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, atmvcc, atmvcc->dev);
-	if (!atm_may_send(atmvcc, skb->truesize)) {
-		/*
-		 * We free this here for now, because we cannot know in a higher
-		 * layer whether the skb pointer it supplied wasn't freed yet.
-		 * Now, it always is.
-		 */
-		dev_kfree_skb(skb);
-		return 0;
-	}
+
 	atomic_add(skb->truesize, &sk_atm(atmvcc)->sk_wmem_alloc);
 	ATM_SKB(skb)->atm_options = atmvcc->atm_options;
 	brdev->stats.tx_packets++;
 	brdev->stats.tx_bytes += skb->len;
 	atmvcc->send(atmvcc, skb);
+
+	if (!atm_may_send(atmvcc, 0)) {
+  		netif_stop_queue(brvcc->device);
+		barrier();
+		/* check for race with br26864_pop*/
+		if (atm_may_send(atmvcc, 0)) {
+			netif_start_queue(brvcc->device);
+		}
+	}
+
 	return 1;
 }
 
@@ -509,8 +528,10 @@  static int br2684_regvcc(struct atm_vcc 
 	atmvcc->user_back = brvcc;
 	brvcc->encaps = (enum br2684_encaps)be.encaps;
 	brvcc->old_push = atmvcc->push;
+	brvcc->old_pop = atmvcc->pop;
 	barrier();
 	atmvcc->push = br2684_push;
+	atmvcc->pop  = br2684_pop;
 
 	rq = &sk_atm(atmvcc)->sk_receive_queue;
 
@@ -621,6 +642,8 @@  static int br2684_create(void __user * a
 
 	write_lock_irq(&devs_lock);
 	brdev->payload = payload;
+	netif_start_queue(netdev);
+
 	brdev->number = list_empty(&br2684_devs) ? 1 :
 	    BRPRIV(list_entry_brdev(br2684_devs.prev))->number + 1;
 	list_add_tail(&brdev->br2684_devs, &br2684_devs);