[PATCH] Fix byte stats on eql

[PATCH] Fix byte stats on eql

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

diff --git a/drivers/net/eql.c b/drivers/net/eql.c
index a59cf96..f2d3741 100644
--- a/drivers/net/eql.c
+++ b/drivers/net/eql.c
@@ -345,6 +345,7 @@ static netdev_tx_t eql_slave_xmit(struct sk_buff
*skb, struct net_device *dev)
 		slave->bytes_queued += skb->len;
 		dev_queue_xmit(skb);
 		dev->stats.tx_packets++;
+		dev->stats.tx_bytes += skb->len;
 	} else {
 		dev->stats.tx_dropped++;
 		dev_kfree_skb(skb);

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation



[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5818 bytes --]

Re: [PATCH] Fix byte stats on eql

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Sun, 25 Mar 2012 11:01:40 +0100

> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

Please use a driver/subsystem prefix in your Subject lines,
in this case "eql: Fix byte stats." would have been appropriate.

> @@ -345,6 +345,7 @@ static netdev_tx_t eql_slave_xmit(struct sk_buff
> *skb, struct net_device *dev)

Patch corrupted by your email client.

[PATCH] eql: Fix byte stats

[David Woodhouse]

---
Sorry, forgot to tell Evolution not to word-wrap last time. It's a
manual process — insert the patch from a text file, and set it to
'Preformatted'. I've also disabled the signature on this version; I'm
not entirely sure if the git-am bug with QP got fixed yet.

diff --git a/drivers/net/eql.c b/drivers/net/eql.c
index a59cf96..f2d3741 100644
--- a/drivers/net/eql.c
+++ b/drivers/net/eql.c
@@ -345,6 +345,7 @@ static netdev_tx_t eql_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 		slave->bytes_queued += skb->len;
 		dev_queue_xmit(skb);
 		dev->stats.tx_packets++;
+		dev->stats.tx_bytes += skb->len;
 	} else {
 		dev->stats.tx_dropped++;
 		dev_kfree_skb(skb);

-- 
dwmw2

Re: [PATCH] eql: Fix byte stats

[Eric Dumazet]

Le lundi 26 mars 2012 à 08:29 +0100, David Woodhouse a écrit :

Missing changelog and "Signed-off-by: ..."

> ---
> Sorry, forgot to tell Evolution not to word-wrap last time. It's a
> manual process — insert the patch from a text file, and set it to
> 'Preformatted'. I've also disabled the signature on this version; I'm
> not entirely sure if the git-am bug with QP got fixed yet.
> 

I use Evolution too, its not that bad :)

> diff --git a/drivers/net/eql.c b/drivers/net/eql.c
> index a59cf96..f2d3741 100644
> --- a/drivers/net/eql.c
> +++ b/drivers/net/eql.c
> @@ -345,6 +345,7 @@ static netdev_tx_t eql_slave_xmit(struct sk_buff *skb, struct net_device *dev)
>  		slave->bytes_queued += skb->len;
>  		dev_queue_xmit(skb);
>  		dev->stats.tx_packets++;
> +		dev->stats.tx_bytes += skb->len;
>  	} else {
>  		dev->stats.tx_dropped++;
>  		dev_kfree_skb(skb);
> 

That adds a bug unfortunately.

After dev_queue_xmit(skb) call, you cant safely dereference skb anymore.

Re: [PATCH] eql: Fix byte stats

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

On Tue, 2012-03-27 at 01:12 -0700, Eric Dumazet wrote:
> Le lundi 26 mars 2012 à 08:29 +0100, David Woodhouse a écrit :
> 
> Missing changelog and "Signed-off-by: ..."

The changelog was in the subject line; there's nothing more to say.
The lack of Signed-off-by is pure stupidity on my part. The baby ate my
brain. That's my excuse and I'm sticking to it.

> That adds a bug unfortunately.
> 
> After dev_queue_xmit(skb) call, you cant safely dereference skb anymore.

Thanks. I blame the baby for that one too. Once she's been taken
swimming and dropped off at nursery and I've had at least two more cups
of tea, I'll have another go :)

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5818 bytes --]

[PATCH] eql: dont rely on HZ=100

[Eric Dumazet]

Le mardi 27 mars 2012 à 09:17 +0100, David Woodhouse a écrit :

> Thanks. I blame the baby for that one too. Once she's been taken
> swimming and dropped off at nursery and I've had at least two more cups
> of tea, I'll have another go :)
> 

Looking at this driver, it seems it depends on HZ=100 ?

[PATCH] eql: dont rely on HZ=100

HZ is more likely to be 1000 these days.

timer handlers are run from softirq, no need to disable bh

skb priority 1 is TC_PRIO_FILLER

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 drivers/net/eql.c      |    7 ++++---
 include/linux/if_eql.h |    2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/eql.c b/drivers/net/eql.c
index a59cf96..f219d38 100644
--- a/drivers/net/eql.c
+++ b/drivers/net/eql.c
@@ -125,6 +125,7 @@
 #include <linux/if.h>
 #include <linux/if_arp.h>
 #include <linux/if_eql.h>
+#include <linux/pkt_sched.h>
 
 #include <asm/uaccess.h>
 
@@ -143,7 +144,7 @@ static void eql_timer(unsigned long param)
 	equalizer_t *eql = (equalizer_t *) param;
 	struct list_head *this, *tmp, *head;
 
-	spin_lock_bh(&eql->queue.lock);
+	spin_lock(&eql->queue.lock);
 	head = &eql->queue.all_slaves;
 	list_for_each_safe(this, tmp, head) {
 		slave_t *slave = list_entry(this, slave_t, list);
@@ -157,7 +158,7 @@ static void eql_timer(unsigned long param)
 		}
 
 	}
-	spin_unlock_bh(&eql->queue.lock);
+	spin_unlock(&eql->queue.lock);
 
 	eql->timer.expires = jiffies + EQL_DEFAULT_RESCHED_IVAL;
 	add_timer(&eql->timer);
@@ -341,7 +342,7 @@ static netdev_tx_t eql_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 		struct net_device *slave_dev = slave->dev;
 
 		skb->dev = slave_dev;
-		skb->priority = 1;
+		skb->priority = TC_PRIO_FILLER;
 		slave->bytes_queued += skb->len;
 		dev_queue_xmit(skb);
 		dev->stats.tx_packets++;
diff --git a/include/linux/if_eql.h b/include/linux/if_eql.h
index 79c4f26..18a5d02 100644
--- a/include/linux/if_eql.h
+++ b/include/linux/if_eql.h
@@ -22,7 +22,7 @@
 #define EQL_DEFAULT_SLAVE_PRIORITY 28800
 #define EQL_DEFAULT_MAX_SLAVES     4
 #define EQL_DEFAULT_MTU            576
-#define EQL_DEFAULT_RESCHED_IVAL   100
+#define EQL_DEFAULT_RESCHED_IVAL   HZ
 
 #define EQL_ENSLAVE     (SIOCDEVPRIVATE)
 #define EQL_EMANCIPATE  (SIOCDEVPRIVATE + 1)

Re: [PATCH] eql: dont rely on HZ=100

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 27 Mar 2012 01:52:00 -0700

> [PATCH] eql: dont rely on HZ=100
> 
> HZ is more likely to be 1000 these days.
> 
> timer handlers are run from softirq, no need to disable bh
> 
> skb priority 1 is TC_PRIO_FILLER
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied.

Strange we export this EQL_DEFAULT_RESCHED_IVAL to userspace, where
HZ will take on a different value I think, something akin to USER_HZ.

Anyways, that's a different unrelated problem to the bug you're
fixing.