br2684 testing needed for packet loss and performance

PATCH In review Karl Hiramoto 4 participants 14 messages Aug 28, 2009 – Sep 16, 2009

Subsystems (4)

Networking Network drivers Kbuild ATM

lore

Share & tools

lore

Karl Hiramoto

author

2009-08-28 10:39 UTC #0 lore raw

PATCH 1/1 br2684 testing needed for packet loss and performance

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 <[email protected]>

net/atm/br2684.c +33 ~~-10~~

@@ -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);

Signed-off-by Karl Hiramoto <[email protected]>

net/atm/br2684.c renamed from usr/src/linux-2.6.30.5/net/atm/br2684.c +28 ~~-10~~

@@ -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;

Chas Williams (CONTRACTOR)

2009-08-28 12:26 UTC #1 lore raw

Quoted 2 lines · In message <[email protected]>,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.

Quoted 5 lines

net/atm/br2684.c +5 -0

+/* 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;

net/atm/br2684.c static struct net_device *br2684_find_de · line 160

Quoted 10 lines

net/atm/br2684.c +10 -0

+
+	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.

net/atm/br2684.c static int br2684_xmit_vcc(struct sk_buf · line 235

Quoted 10 lines

net/atm/br2684.c +10 -0

+
+	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.

Karl Hiramoto

author

2009-08-29 10:24 UTC #2 lore raw

Quoted 7 lines · Chas Williams (CONTRACTOR)

Quoted 2 lines · In message <[email protected]>,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?

Quoted 5 lines

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

Karl Hiramoto

author

2009-08-29 11:27 UTC #3 lore raw

PATCH 1/1 atm/br2684: netif_stop_queue() when atm device busy and netif_wake_queue() when we can send packets again.

This patch removes the call to dev_kfree_skb() when the atm device is busy. Calling dev_kfree_skb() causes heavy packet loss then the device is under heavy load, the more correct behavior should be to stop the upper layers, then when the lower device can queue packets again wake the upper layers.

Signed-off-by Karl Hiramoto <[email protected]>

---
 net/atm/br2684.c |   37 +++++++++++++++++++++++++++----------
 1 files changed, 27 insertions(+), 10 deletions(-)

net/atm/br2684.c +27 ~~-10~~

index 2912665..5c42225 100644
@@ -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_dev(const struct br2684_if_spec *s)
 	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,19 @@ static int br2684_xmit_vcc(struct sk_buff *skb, struct net_device *dev,
 
 	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);
+		/*check for race with br2684_pop*/
+		if (atm_may_send(atmvcc, 0))
+			netif_start_queue(brvcc->device);
+	}
+
 	return 1;
 }

@@ -503,8 +518,10 @@ static int br2684_regvcc(struct atm_vcc *atmvcc, void __user * arg)
 	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;
 
 	__skb_queue_head_init(&queue);
 	rq = &sk_atm(atmvcc)->sk_receive_queue;

Signature · 1 line

1.6.3.3

Chas Williams (CONTRACTOR)

2009-08-31 14:29 UTC #4 lore raw

PATCH 1/1 atm/br2684: netif_stop_queue() when atm device busy and netif_wake_queue() when we can send packets again.

Signed-off-by Karl Hiramoto <[email protected]>

Signed-off-by Chas Williams <[email protected]>

---
 net/atm/br2684.c |   37 +++++++++++++++++++++++++++----------
 1 files changed, 27 insertions(+), 10 deletions(-)

net/atm/br2684.c +1 -1

index 2912665..5c42225 100644
@@ -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_dev(const struct br

2684_if_spec *s)
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,19 @@ static int br2684_xmit_vcc(struct sk_buff *skb, struct n et_device *dev,

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 highe
r
- * 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);
+ /*check for race with br2684_pop*/
+ if (atm_may_send(atmvcc, 0))
+ netif_start_queue(brvcc->device);
+ }
+
return 1;
}

@@ -503,8 +518,10 @@ static int br2684_regvcc(struct atm_vcc *atmvcc, void __u ser * arg)
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;

__skb_queue_head_init(&queue);
rq = &sk_atm(atmvcc)->sk_receive_queue;

David Miller

maintainer Applied

2009-09-03 06:26 UTC #5 lore raw

Re: [PATCH] atm/br2684: netif_stop_queue() when atm device busy and netif_wake_queue() when we can send packets again.

From: "Chas Williams (CONTRACTOR)" <[email protected]>
Date: Mon, 31 Aug 2009 10:29:47 -0400

Quoted 7 lines

This patch removes the call to dev_kfree_skb() when the atm device is busy. Calling dev_kfree_skb() causes heavy packet loss then the device is under heavy load, the more correct behavior should be to stop the upper layers, then when the lower device can queue packets again wake the upper layers.
Signed-off-by Karl Hiramoto <[email protected]>
Signed-off-by Chas Williams <[email protected]>

Applied to net-next-2.6, but Chas your email client corrupted the patch by breaking up long lines:

Review static struct net_device *br2684_find_dev(const struct br · line 142

Quoted 2 lines

(quoted diff) +0 -0

@@ -142,6 +142,22 @@ static struct net_device *br2684_find_dev(const struct br
 2684_if_spec *s)

Like that.

Chas Williams (CONTRACTOR)

2009-09-03 13:44 UTC #6 lore raw

Re: [PATCH] atm/br2684: netif_stop_queue() when atm device busy and netif_wake_queue() when we can send packets again.

Quoted 2 lines · In message <[email protected]>,David Miller writes:

Applied to net-next-2.6, but Chas your email client corrupted the patch by breaking up long lines:

grr.... i will fix it (again). thanks.

Philip A. Prindeville

2009-09-10 19:49 UTC #7 lore raw

Re: [Linux-ATM-General] [PATCH] atm/br2684: netif_stop_queue() when atm device busy and netif_wake_queue() when we can send packets again.

In reply to Karl Hiramoto

I had noticed that with my Solos card, and using Qwest 7062/896kb/s service that I was typically only getting ~400kb/s upstream, so I thought that delayed transmitter restarts might be the culprit and decided to try out this patch.

I'm running 2.6.27.26, and I modified the patch as below.

Has anyone confirmed this patch (Karl's) against 2.6.27?

Thanks,

-Philip

Quoted 77 lines · Karl Hiramoto

Signed-off-by Karl Hiramoto <[email protected]>

---
 net/atm/br2684.c |   37 +++++++++++++++++++++++++++----------
 1 files changed, 27 insertions(+), 10 deletions(-)

net/atm/br2684.c +25 ~~-10~~

index 2912665..5c42225 100644
@@ -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_dev(const struct br2684_if_spec *s)
 	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,19 @@ static int br2684_xmit_vcc(struct sk_buff *skb, struct net_device *dev,
 
 	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);
+		/*check for race with br2684_pop*/
+		if (atm_may_send(atmvcc, 0))
+			netif_start_queue(brvcc->device);
+	}
+
 	return 1;
 }

net/atm/br2684.c static int br2684_regvcc(struct atm_vcc *atmvcc, void __user * arg) · line 527

Quoted 10 lines

net/atm/br2684.c +2 -0

@@ -503,8 +518,10 @@ static int br2684_regvcc(struct atm_vcc *atmvcc, void __user * arg)
 	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;
 
 	__skb_queue_head_init(&queue);
 	rq = &sk_atm(atmvcc)->sk_receive_queue;

net/atm/br2684.c renamed from net/atm/br2684.c.orig +27 ~~-10~~

@@ -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,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,19 @@ 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);
+		/*check for race with br2684_pop*/
+		if (atm_may_send(atmvcc, 0))
+			netif_start_queue(brvcc->device);
+	}
+
 	return 1;
 }

@@ -509,8 +524,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;

Karl Hiramoto

author

2009-09-10 21:30 UTC #8 lore raw

Re: [Linux-ATM-General] [PATCH] atm/br2684: netif_stop_queue() when atm device busy and netif_wake_queue() when we can send packets again.

Quoted 5 lines · Philip A. Prindeville

I had noticed that with my Solos card, and using Qwest 7062/896kb/s service that I was typically only getting ~400kb/s upstream, so I thought that delayed transmitter restarts might be the culprit and decided to try out this patch.

Quoted 7 lines

I'm running 2.6.27.26, and I modified the patch as below.
Has anyone confirmed this patch (Karl's) against 2.6.27?
Thanks,
-Philip

I'd be interested in hearing comparisons with/without the patch. What i have is no upstream packet loss with this patch, however slightly lower total throughput. I think because the upper networking layers take time to restart the packet flow. I'm not really sure if or how many packets to upper layers buffer. I haven't had time to debug it further.

I don't have a solos card, but you may have to tweak the solos driver sk_sndbuf value.

--
Karl

David Miller

maintainer

2009-09-11 18:48 UTC #9 lore raw

Re: [Linux-ATM-General] [PATCH] atm/br2684: netif_stop_queue() when atm device busy and netif_wake_queue() when we can send packets again.

From: Karl Hiramoto <[email protected]>
Date: Thu, 10 Sep 2009 23:30:44 +0200

Quoted 1 line

I'm not really sure if or how many packets to upper layers buffer.

This is determined by ->tx_queue_len, so whatever value is being set for ATM network devices is what the core will use for backlog limiting while the device's TX queue is stopped.

Karl Hiramoto

author

2009-09-15 13:44 UTC #10 lore raw

Re: [Linux-ATM-General] [PATCH] atm/br2684: netif_stop_queue() when atm device busy and netif_wake_queue() when we can send packets again.

Quoted 10 lines · David Miller

From: Karl Hiramoto <[email protected]>
Date: Thu, 10 Sep 2009 23:30:44 +0200
Quoted 2 lines
I'm not really sure if or how many packets to upper layers buffer.
This is determined by ->tx_queue_len, so whatever value is being set for ATM network devices is what the core will use for backlog limiting while the device's TX queue is stopped.

I tried varying tx_queue_len by 10, 100, and 1000x, but it didn't seem to help much. Whenever the atm dev called netif_wake_queue() it seems like the driver still starves for packets and still takes time to get going again.

It seem like when the driver calls netif_wake_queue() it's TX hardware queue is nearly full, but it has space to accept new packets. The TX hardware queue has time to empty, devices starves for packets(goes idle), then finally a packet comes in from the upper networking layers. I'm not really sure at the moment where the problem lies to my maximum throughput dropping.

I did try changing sk_sndbuf to 256K but that didn't seem to help either.

--
Karl

Karl Hiramoto

author

2009-09-15 14:57 UTC #11 lore raw

Re: [Linux-ATM-General] [PATCH] atm/br2684: netif_stop_queue() when atm device busy and netif_wake_queue() when we can send packets again.

Quoted 14 lines · Karl Hiramoto

Quoted 5 lines · David Miller
From: Karl Hiramoto <[email protected]>
Date: Thu, 10 Sep 2009 23:30:44 +0200
Quoted 7 lines
Quoted 3 lines
I'm not really sure if or how many packets to upper layers buffer.
This is determined by ->tx_queue_len, so whatever value is being set for ATM network devices is what the core will use for backlog limiting while the device's TX queue is stopped.

Quoted 8 lines

I tried varying tx_queue_len by 10, 100, and 1000x, but it didn't seem to help much. Whenever the atm dev called netif_wake_queue() it seems like the driver still starves for packets and still takes time to get going again.
It seem like when the driver calls netif_wake_queue() it's TX hardware queue is nearly full, but it has space to accept new packets. The TX hardware queue has time to empty, devices starves for packets(goes idle), then finally a packet comes in from the upper networking layers. I'm not really sure at the moment where the problem lies to my maximum throughput dropping.
I did try changing sk_sndbuf to 256K but that didn't seem to help either.
--

Actually i think i spoke too soon, tuning TCP parameters, txqueuelen on all machines the server, router and client it seems my performance came back.

--
Karl

Philip A. Prindeville

2009-09-16 18:05 UTC #12 lore raw

Re: [Linux-ATM-General] [PATCH] atm/br2684: netif_stop_queue() when atm device busy and netif_wake_queue() when we can send packets again.

Quoted 35 lines · Karl Hiramoto

Quoted 33 lines · Karl Hiramoto
Quoted 6 lines · David Miller
From: Karl Hiramoto <[email protected]>
Date: Thu, 10 Sep 2009 23:30:44 +0200
Quoted 9 lines
Quoted 4 lines
I'm not really sure if or how many packets to upper layers buffer.
This is determined by ->tx_queue_len, so whatever value is being set for ATM network devices is what the core will use for backlog limiting while the device's TX queue is stopped.
I tried varying tx_queue_len by 10, 100, and 1000x, but it didn't seem to help much. Whenever the atm dev called netif_wake_queue() it seems like the driver still starves for packets and still takes time to get going again.
It seem like when the driver calls netif_wake_queue() it's TX hardware queue is nearly full, but it has space to accept new packets. The TX hardware queue has time to empty, devices starves for packets(goes idle), then finally a packet comes in from the upper networking
layers. I'm not really sure at the moment where the problem lies to my
maximum throughput dropping.
I did try changing sk_sndbuf to 256K but that didn't seem to help either.

Quoted 9 lines

Quoted 2 lines
--
Actually i think i spoke too soon, tuning TCP parameters, txqueuelen on all machines the server, router and client it seems my performance came back.
--
Karl

So what size are you currently using?

Out-of-the-box build, 2.6.27.29 seems to set it to 1000.

-Philip

Philip A. Prindeville

2009-09-11 19:58 UTC #13 lore raw

Re: [Linux-ATM-General] [PATCH] atm/br2684: netif_stop_queue() when atm device busy and netif_wake_queue() when we can send packets again.

In reply to Karl Hiramoto

Quoted 20 lines · Karl Hiramoto

Quoted 5 lines · Philip A. Prindeville
I had noticed that with my Solos card, and using Qwest 7062/896kb/s service that I was typically only getting ~400kb/s upstream, so I thought that delayed transmitter restarts might be the culprit and decided to try out this patch.
Quoted 7 lines
I'm running 2.6.27.26, and I modified the patch as below.
Has anyone confirmed this patch (Karl's) against 2.6.27?
Thanks,
-Philip
I'd be interested in hearing comparisons with/without the patch. What
i have is no upstream packet loss with this patch, however slightly lower total throughput. I think because the upper networking layers
take time to restart the packet flow. I'm not really sure if or how
many packets to upper layers buffer. I haven't had time to debug it further.

Quoted 5 lines

I don't have a solos card, but you may have to tweak the solos driver sk_sndbuf value.
Signature · 1 line
Karl

I'm running with the patch right now... Can't tell if latency has been affected.

Seems to be working a bit better, but I've not done any formalized testing.

-Philip