[PATCH 1/3] ifb: remove the useless debug stats

[PATCH 1/3] ifb: remove the useless debug stats

[Changli Gao]

These debug stats are not exported, and become useless.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 drivers/net/ifb.c |   19 -------------------
 1 file changed, 19 deletions(-)
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index ab9f675..4387525 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -42,16 +42,6 @@
 struct ifb_private {
 	struct tasklet_struct   ifb_tasklet;
 	int     tasklet_pending;
-	/* mostly debug stats leave in for now */
-	unsigned long   st_task_enter; /* tasklet entered */
-	unsigned long   st_txq_refl_try; /* transmit queue refill attempt */
-	unsigned long   st_rxq_enter; /* receive queue entered */
-	unsigned long   st_rx2tx_tran; /* receive to trasmit transfers */
-	unsigned long   st_rxq_notenter; /*receiveQ not entered, resched */
-	unsigned long   st_rx_frm_egr; /* received from egress path */
-	unsigned long   st_rx_frm_ing; /* received from ingress path */
-	unsigned long   st_rxq_check;
-	unsigned long   st_rxq_rsch;
 	struct sk_buff_head     rq;
 	struct sk_buff_head     tq;
 };
@@ -73,19 +63,14 @@ static void ri_tasklet(unsigned long dev)
 	struct sk_buff *skb;
 
 	txq = netdev_get_tx_queue(_dev, 0);
-	dp->st_task_enter++;
 	if ((skb = skb_peek(&dp->tq)) == NULL) {
-		dp->st_txq_refl_try++;
 		if (__netif_tx_trylock(txq)) {
-			dp->st_rxq_enter++;
 			while ((skb = skb_dequeue(&dp->rq)) != NULL) {
 				skb_queue_tail(&dp->tq, skb);
-				dp->st_rx2tx_tran++;
 			}
 			__netif_tx_unlock(txq);
 		} else {
 			/* reschedule */
-			dp->st_rxq_notenter++;
 			goto resched;
 		}
 	}
@@ -110,10 +95,8 @@ static void ri_tasklet(unsigned long dev)
 		skb->skb_iif = _dev->ifindex;
 
 		if (from & AT_EGRESS) {
-			dp->st_rx_frm_egr++;
 			dev_queue_xmit(skb);
 		} else if (from & AT_INGRESS) {
-			dp->st_rx_frm_ing++;
 			skb_pull(skb, skb->dev->hard_header_len);
 			netif_rx(skb);
 		} else
@@ -121,13 +104,11 @@ static void ri_tasklet(unsigned long dev)
 	}
 
 	if (__netif_tx_trylock(txq)) {
-		dp->st_rxq_check++;
 		if ((skb = skb_peek(&dp->rq)) == NULL) {
 			dp->tasklet_pending = 0;
 			if (netif_queue_stopped(_dev))
 				netif_wake_queue(_dev);
 		} else {
-			dp->st_rxq_rsch++;
 			__netif_tx_unlock(txq);
 			goto resched;
 		}

[PATCH 2/3] ifb: remove unused macro TX_TIMEOUT

[Changli Gao]

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 drivers/net/ifb.c |    2 --
 1 file changed, 2 deletions(-)
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 4387525..d1e362a 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -36,8 +36,6 @@
 #include <net/pkt_sched.h>
 #include <net/net_namespace.h>
 
-#define TX_TIMEOUT  (2*HZ)
-
 #define TX_Q_LIMIT    32
 struct ifb_private {
 	struct tasklet_struct   ifb_tasklet;

[PATCH 3/3] ifb: move tq from ifb_private

[Changli Gao]

tq is only used in ri_tasklet, so we move it from ifb_private to a
stack variable of ri_tasklet.

skb_queue_splice_tail_init() is used instead of the open coded and slow
one.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 drivers/net/ifb.c |   49 ++++++++++++-------------------------------------
 1 file changed, 12 insertions(+), 37 deletions(-)
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index d1e362a..cd6e90d 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -39,9 +39,7 @@
 #define TX_Q_LIMIT    32
 struct ifb_private {
 	struct tasklet_struct   ifb_tasklet;
-	int     tasklet_pending;
 	struct sk_buff_head     rq;
-	struct sk_buff_head     tq;
 };
 
 static int numifbs = 2;
@@ -53,27 +51,25 @@ static int ifb_close(struct net_device *dev);
 
 static void ri_tasklet(unsigned long dev)
 {
-
 	struct net_device *_dev = (struct net_device *)dev;
 	struct ifb_private *dp = netdev_priv(_dev);
 	struct net_device_stats *stats = &_dev->stats;
 	struct netdev_queue *txq;
 	struct sk_buff *skb;
+	struct sk_buff_head tq;
 
+	__skb_queue_head_init(&tq);
 	txq = netdev_get_tx_queue(_dev, 0);
-	if ((skb = skb_peek(&dp->tq)) == NULL) {
-		if (__netif_tx_trylock(txq)) {
-			while ((skb = skb_dequeue(&dp->rq)) != NULL) {
-				skb_queue_tail(&dp->tq, skb);
-			}
-			__netif_tx_unlock(txq);
-		} else {
-			/* reschedule */
-			goto resched;
-		}
+	if (!__netif_tx_trylock(txq)) {
+		tasklet_schedule(&dp->ifb_tasklet);
+		return;
 	}
+	skb_queue_splice_tail_init(&dp->rq, &tq);
+	if (netif_tx_queue_stopped(txq))
+		netif_tx_wake_queue(txq);
+	__netif_tx_unlock(txq);
 
-	while ((skb = skb_dequeue(&dp->tq)) != NULL) {
+	while ((skb = __skb_dequeue(&tq)) != NULL) {
 		u32 from = G_TC_FROM(skb->tc_verd);
 
 		skb->tc_verd = 0;
@@ -87,7 +83,7 @@ static void ri_tasklet(unsigned long dev)
 			rcu_read_unlock();
 			dev_kfree_skb(skb);
 			stats->tx_dropped++;
-			break;
+			continue;
 		}
 		rcu_read_unlock();
 		skb->skb_iif = _dev->ifindex;
@@ -100,23 +96,6 @@ static void ri_tasklet(unsigned long dev)
 		} else
 			BUG();
 	}
-
-	if (__netif_tx_trylock(txq)) {
-		if ((skb = skb_peek(&dp->rq)) == NULL) {
-			dp->tasklet_pending = 0;
-			if (netif_queue_stopped(_dev))
-				netif_wake_queue(_dev);
-		} else {
-			__netif_tx_unlock(txq);
-			goto resched;
-		}
-		__netif_tx_unlock(txq);
-	} else {
-resched:
-		dp->tasklet_pending = 1;
-		tasklet_schedule(&dp->ifb_tasklet);
-	}
-
 }
 
 static const struct net_device_ops ifb_netdev_ops = {
@@ -162,10 +141,8 @@ static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	skb_queue_tail(&dp->rq, skb);
-	if (!dp->tasklet_pending) {
-		dp->tasklet_pending = 1;
+	if (skb_queue_len(&dp->rq) == 1)
 		tasklet_schedule(&dp->ifb_tasklet);
-	}
 
 	return NETDEV_TX_OK;
 }
@@ -177,7 +154,6 @@ static int ifb_close(struct net_device *dev)
 	tasklet_kill(&dp->ifb_tasklet);
 	netif_stop_queue(dev);
 	skb_queue_purge(&dp->rq);
-	skb_queue_purge(&dp->tq);
 	return 0;
 }
 
@@ -187,7 +163,6 @@ static int ifb_open(struct net_device *dev)
 
 	tasklet_init(&dp->ifb_tasklet, ri_tasklet, (unsigned long)dev);
 	skb_queue_head_init(&dp->rq);
-	skb_queue_head_init(&dp->tq);
 	netif_start_queue(dev);
 
 	return 0;

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Jarek Poplawski]

Changli Gao wrote:
> tq is only used in ri_tasklet, so we move it from ifb_private to a
> stack variable of ri_tasklet.
> 
> skb_queue_splice_tail_init() is used instead of the open coded and slow
> one.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
>  drivers/net/ifb.c |   49 ++++++++++++-------------------------------------
>  1 file changed, 12 insertions(+), 37 deletions(-)
> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
> index d1e362a..cd6e90d 100644
> --- a/drivers/net/ifb.c
> +++ b/drivers/net/ifb.c
> @@ -39,9 +39,7 @@
>  #define TX_Q_LIMIT    32
>  struct ifb_private {
>  	struct tasklet_struct   ifb_tasklet;
> -	int     tasklet_pending;
>  	struct sk_buff_head     rq;
> -	struct sk_buff_head     tq;
>  };
>  
>  static int numifbs = 2;
> @@ -53,27 +51,25 @@ static int ifb_close(struct net_device *dev);
>  
>  static void ri_tasklet(unsigned long dev)
>  {
> -
>  	struct net_device *_dev = (struct net_device *)dev;
>  	struct ifb_private *dp = netdev_priv(_dev);
>  	struct net_device_stats *stats = &_dev->stats;
>  	struct netdev_queue *txq;
>  	struct sk_buff *skb;
> +	struct sk_buff_head tq;
>  
> +	__skb_queue_head_init(&tq);
>  	txq = netdev_get_tx_queue(_dev, 0);
> -	if ((skb = skb_peek(&dp->tq)) == NULL) {
> -		if (__netif_tx_trylock(txq)) {
> -			while ((skb = skb_dequeue(&dp->rq)) != NULL) {
> -				skb_queue_tail(&dp->tq, skb);
> -			}
> -			__netif_tx_unlock(txq);
> -		} else {
> -			/* reschedule */
> -			goto resched;
> -		}
> +	if (!__netif_tx_trylock(txq)) {
> +		tasklet_schedule(&dp->ifb_tasklet);
> +		return;
>  	}
> +	skb_queue_splice_tail_init(&dp->rq, &tq);
> +	if (netif_tx_queue_stopped(txq))
> +		netif_tx_wake_queue(txq);
> +	__netif_tx_unlock(txq);
>  
> -	while ((skb = skb_dequeue(&dp->tq)) != NULL) {
> +	while ((skb = __skb_dequeue(&tq)) != NULL) {
>  		u32 from = G_TC_FROM(skb->tc_verd);
>  
>  		skb->tc_verd = 0;
> @@ -87,7 +83,7 @@ static void ri_tasklet(unsigned long dev)
>  			rcu_read_unlock();
>  			dev_kfree_skb(skb);
>  			stats->tx_dropped++;
> -			break;
> +			continue;

IMHO this line is a bugfix and should be a separate patch (for stable).

The rest looks OK to me but you could probably skip locking of dp->rq
similarly to tq.

Jarek P.

>  		}
>  		rcu_read_unlock();
>  		skb->skb_iif = _dev->ifindex;
> @@ -100,23 +96,6 @@ static void ri_tasklet(unsigned long dev)
>  		} else
>  			BUG();
>  	}
> -
> -	if (__netif_tx_trylock(txq)) {
> -		if ((skb = skb_peek(&dp->rq)) == NULL) {
> -			dp->tasklet_pending = 0;
> -			if (netif_queue_stopped(_dev))
> -				netif_wake_queue(_dev);
> -		} else {
> -			__netif_tx_unlock(txq);
> -			goto resched;
> -		}
> -		__netif_tx_unlock(txq);
> -	} else {
> -resched:
> -		dp->tasklet_pending = 1;
> -		tasklet_schedule(&dp->ifb_tasklet);
> -	}
> -
>  }
>  
>  static const struct net_device_ops ifb_netdev_ops = {
> @@ -162,10 +141,8 @@ static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
>  	}
>  
>  	skb_queue_tail(&dp->rq, skb);
> -	if (!dp->tasklet_pending) {
> -		dp->tasklet_pending = 1;
> +	if (skb_queue_len(&dp->rq) == 1)
>  		tasklet_schedule(&dp->ifb_tasklet);
> -	}
>  
>  	return NETDEV_TX_OK;
>  }
> @@ -177,7 +154,6 @@ static int ifb_close(struct net_device *dev)
>  	tasklet_kill(&dp->ifb_tasklet);
>  	netif_stop_queue(dev);
>  	skb_queue_purge(&dp->rq);
> -	skb_queue_purge(&dp->tq);
>  	return 0;
>  }
>  
> @@ -187,7 +163,6 @@ static int ifb_open(struct net_device *dev)
>  
>  	tasklet_init(&dp->ifb_tasklet, ri_tasklet, (unsigned long)dev);
>  	skb_queue_head_init(&dp->rq);
> -	skb_queue_head_init(&dp->tq);
>  	netif_start_queue(dev);
>  
>  	return 0;

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Changli Gao]

On Sat, Dec 4, 2010 at 9:15 PM, Jarek Poplawski <jarkao2@gmail.com> wrote:
> Changli Gao wrote:
>> tq is only used in ri_tasklet, so we move it from ifb_private to a
>> stack variable of ri_tasklet.
>>
>> skb_queue_splice_tail_init() is used instead of the open coded and slow
>> one.
>>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>> ---
>>  drivers/net/ifb.c |   49 ++++++++++++-------------------------------------
>>  1 file changed, 12 insertions(+), 37 deletions(-)
>> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
>> index d1e362a..cd6e90d 100644
>> --- a/drivers/net/ifb.c
>> +++ b/drivers/net/ifb.c
>> @@ -39,9 +39,7 @@
>>  #define TX_Q_LIMIT    32
>>  struct ifb_private {
>>       struct tasklet_struct   ifb_tasklet;
>> -     int     tasklet_pending;
>>       struct sk_buff_head     rq;
>> -     struct sk_buff_head     tq;
>>  };
>>
>>  static int numifbs = 2;
>> @@ -53,27 +51,25 @@ static int ifb_close(struct net_device *dev);
>>
>>  static void ri_tasklet(unsigned long dev)
>>  {
>> -
>>       struct net_device *_dev = (struct net_device *)dev;
>>       struct ifb_private *dp = netdev_priv(_dev);
>>       struct net_device_stats *stats = &_dev->stats;
>>       struct netdev_queue *txq;
>>       struct sk_buff *skb;
>> +     struct sk_buff_head tq;
>>
>> +     __skb_queue_head_init(&tq);
>>       txq = netdev_get_tx_queue(_dev, 0);
>> -     if ((skb = skb_peek(&dp->tq)) == NULL) {
>> -             if (__netif_tx_trylock(txq)) {
>> -                     while ((skb = skb_dequeue(&dp->rq)) != NULL) {
>> -                             skb_queue_tail(&dp->tq, skb);
>> -                     }
>> -                     __netif_tx_unlock(txq);
>> -             } else {
>> -                     /* reschedule */
>> -                     goto resched;
>> -             }
>> +     if (!__netif_tx_trylock(txq)) {
>> +             tasklet_schedule(&dp->ifb_tasklet);
>> +             return;
>>       }
>> +     skb_queue_splice_tail_init(&dp->rq, &tq);
>> +     if (netif_tx_queue_stopped(txq))
>> +             netif_tx_wake_queue(txq);
>> +     __netif_tx_unlock(txq);
>>
>> -     while ((skb = skb_dequeue(&dp->tq)) != NULL) {
>> +     while ((skb = __skb_dequeue(&tq)) != NULL) {
>>               u32 from = G_TC_FROM(skb->tc_verd);
>>
>>               skb->tc_verd = 0;
>> @@ -87,7 +83,7 @@ static void ri_tasklet(unsigned long dev)
>>                       rcu_read_unlock();
>>                       dev_kfree_skb(skb);
>>                       stats->tx_dropped++;
>> -                     break;
>> +                     continue;
>
> IMHO this line is a bugfix and should be a separate patch (for stable).

It sounds reasonable.

>
> The rest looks OK to me but you could probably skip locking of dp->rq
> similarly to tq.
>

OK. Thanks.


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 1/3] ifb: remove the useless debug stats

[jamal]

On Sat, 2010-12-04 at 13:55 +0800, Changli Gao wrote:
> These debug stats are not exported, and become useless.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

Given the complexity of ifb, these stats have proved useful over
the years - I would typically ask someone to printk them etc.
If you want me to get me excited (in a good way) you could export these
via link XSTATS ;-> If you dont have time, then it is likely
we are stable enough we can kill them in which case, here you go:

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>

cheers,
jamal

Re: [PATCH 2/3] ifb: remove unused macro TX_TIMEOUT

[jamal]

On Sat, 2010-12-04 at 13:55 +0800, Changli Gao wrote:
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>

cheers,
jamal

Re: [PATCH 3/3] ifb: move tq from ifb_private

[jamal]

On Sat, 2010-12-04 at 13:55 +0800, Changli Gao wrote:
> tq is only used in ri_tasklet, so we move it from ifb_private to a
> stack variable of ri_tasklet.
> 
> skb_queue_splice_tail_init() is used instead of the open coded and slow
> one.

I like the splice idea but this patch makes me twitch
a little. What test setup did you use to check it?

cheers,
jamal

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Changli Gao]

On Sat, Dec 4, 2010 at 10:18 PM, jamal <hadi@cyberus.ca> wrote:
>
> I like the splice idea but this patch makes me twitch
> a little. What test setup did you use to check it?
>

Just a simple test:

tc qdisc add dev eth0 ingress
tc filter add dev eth0 parent ffff: protocol ip basic action mirred
egress redirect dev ifb0

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 3/3] ifb: move tq from ifb_private

[jamal]

On Sat, 2010-12-04 at 14:15 +0100, Jarek Poplawski wrote:

> > @@ -87,7 +83,7 @@ static void ri_tasklet(unsigned long dev)
> >  			rcu_read_unlock();
> >  			dev_kfree_skb(skb);
> >  			stats->tx_dropped++;
> > -			break;
> > +			continue;
> 
> IMHO this line is a bugfix and should be a separate patch (for stable).

Should be separate, yes.
Bug? no. 
Improvement, maybe ;->
The idea was to defer processing at the first error. Changli
is changing it to continue despite the error.
The initial goal was to yield whenever possible since we dont maintain
a lot of state.

cheers,
jamal

Re: [PATCH 3/3] ifb: move tq from ifb_private

[jamal]

On Sat, 2010-12-04 at 09:18 -0500, jamal wrote:

> I like the splice idea but this patch makes me twitch
> a little. What test setup did you use to check it?

Ok, here's one thing you changed which is important. We do:

 -->XXX-->rq-->tq-->XXX-->

rq is controlled by queue limit. 
We only load rq to tq if all of tq is empty. If it is not
we dont move things over. Essentially this is a flow
control scheme. We dont want many sources to be overwhelming
us with packets and every time we grab a txqlen number of packets.
For this reason:
I would be comfortable if all you did was to add the splice
after you skb_peek() - i think that would be a good improvement
which is not bound to break anything else.

cheers,
jamal

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Changli Gao]

On Sat, Dec 4, 2010 at 10:28 PM, jamal <hadi@cyberus.ca> wrote:
> On Sat, 2010-12-04 at 14:15 +0100, Jarek Poplawski wrote:
>
>> > @@ -87,7 +83,7 @@ static void ri_tasklet(unsigned long dev)
>> >                     rcu_read_unlock();
>> >                     dev_kfree_skb(skb);
>> >                     stats->tx_dropped++;
>> > -                   break;
>> > +                   continue;
>>
>> IMHO this line is a bugfix and should be a separate patch (for stable).
>
> Should be separate, yes.
> Bug? no.

If we breaks the loop when there are still skbs in tq and no skb in
rq, the skbs will be left in txq until new skbs are enqueued into rq.
In rare cases, no new skb is queued, then these skbs will stay in rq
forever.

-       if (__netif_tx_trylock(txq)) {
-               if ((skb = skb_peek(&dp->rq)) == NULL) {
-                       dp->tasklet_pending = 0;
-                       if (netif_queue_stopped(_dev))
-                               netif_wake_queue(_dev);
-               } else {
-                       __netif_tx_unlock(txq);
-                       goto resched;
-               }
-               __netif_tx_unlock(txq);
-       } else {
-resched:
-               dp->tasklet_pending = 1;
-               tasklet_schedule(&dp->ifb_tasklet);
-       }
-


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 3/3] ifb: move tq from ifb_private

[jamal]

On Sat, 2010-12-04 at 09:28 -0500, jamal wrote:

> The idea was to defer processing at the first error. Changli
> is changing it to continue despite the error.
> The initial goal was to yield whenever possible since we dont maintain
> a lot of state.

Changli:
Other points we could defer processing is in case of packets
being dropped by dev_queue_xmit and netif_rx

cheers,
jamal

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Changli Gao]

On Sat, Dec 4, 2010 at 10:42 PM, jamal <hadi@cyberus.ca> wrote:
> On Sat, 2010-12-04 at 09:18 -0500, jamal wrote:
>
>> I like the splice idea but this patch makes me twitch
>> a little. What test setup did you use to check it?
>
> Ok, here's one thing you changed which is important. We do:
>
>  -->XXX-->rq-->tq-->XXX-->
>
> rq is controlled by queue limit.
> We only load rq to tq if all of tq is empty. If it is not
> we dont move things over. Essentially this is a flow
> control scheme. We dont want many sources to be overwhelming
> us with packets and every time we grab a txqlen number of packets.
> For this reason:
> I would be comfortable if all you did was to add the splice
> after you skb_peek() - i think that would be a good improvement
> which is not bound to break anything else.
>

Maybe you misread my patch. tq is a stack variable in ri_tasklet, and
initialized all the time. ri_tasklet() won't exits until tq is
empty().


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 3/3] ifb: move tq from ifb_private

[jamal]

On Sat, 2010-12-04 at 22:45 +0800, Changli Gao wrote:

> 
> If we breaks the loop when there are still skbs in tq and no skb in
> rq, the skbs will be left in txq until new skbs are enqueued into rq.
> In rare cases, no new skb is queued, then these skbs will stay in rq
> forever.

So should we goto resched? 

cheers,
jamal

Re: [PATCH 3/3] ifb: move tq from ifb_private

[jamal]

On Sat, 2010-12-04 at 22:50 +0800, Changli Gao wrote:

> Maybe you misread my patch. tq is a stack variable in ri_tasklet, and
> initialized all the time. ri_tasklet() won't exits until tq is
> empty().

in your patch is a variable on the stack.
What i am saying is you should defer processing when there is an
error (note the two other spots i mentioned). 
This means you may leave dp->tq non-empty and therefore it
needs to be saved somewhere as it is before your patch.

cheers,
jamal

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Changli Gao]

On Sat, Dec 4, 2010 at 10:55 PM, jamal <hadi@cyberus.ca> wrote:
> On Sat, 2010-12-04 at 22:45 +0800, Changli Gao wrote:
>
>>
>> If we breaks the loop when there are still skbs in tq and no skb in
>> rq, the skbs will be left in txq until new skbs are enqueued into rq.
>> In rare cases, no new skb is queued, then these skbs will stay in rq
>> forever.
>
> So should we goto resched?
>

Only if we can't lock the txq or rq isn't empty, we goto resched. So
it is a bug.


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Changli Gao]

On Sat, Dec 4, 2010 at 10:59 PM, jamal <hadi@cyberus.ca> wrote:
> On Sat, 2010-12-04 at 22:50 +0800, Changli Gao wrote:
>
>> Maybe you misread my patch. tq is a stack variable in ri_tasklet, and
>> initialized all the time. ri_tasklet() won't exits until tq is
>> empty().
>
> in your patch is a variable on the stack.
> What i am saying is you should defer processing when there is an
> error (note the two other spots i mentioned).
> This means you may leave dp->tq non-empty and therefore it
> needs to be saved somewhere as it is before your patch.
>

I know what you concern now. Thanks. I'll keep the old behavior in v2.


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 3/3] ifb: move tq from ifb_private

[jamal]

On Sat, 2010-12-04 at 23:01 +0800, Changli Gao wrote:
> On Sat, Dec 4, 2010 at 10:55 PM, jamal <hadi@cyberus.ca> wrote:
> > On Sat, 2010-12-04 at 22:45 +0800, Changli Gao wrote:
> >
> >>
> >> If we breaks the loop when there are still skbs in tq and no skb in
> >> rq, the skbs will be left in txq until new skbs are enqueued into rq.
> >> In rare cases, no new skb is queued, then these skbs will stay in rq
> >> forever.
> >
> > So should we goto resched?
> >
> 
> Only if we can't lock the txq or rq isn't empty, we goto resched. So
> it is a bug.

And to be explicit: Yes, meant to say there is a bug if we break out 
in the scenario you described above - the fix is to jump to resched.
Why do we need the lock?

cheers,
jamal

Re: [PATCH 3/3] ifb: move tq from ifb_private

[jamal]

On Sat, 2010-12-04 at 23:07 +0800, Changli Gao wrote:

> 
> I know what you concern now. Thanks. I'll keep the old behavior in v2.

Ok - thanks Changli.

cheers,
jamal

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Jarek Poplawski]

On Sat, Dec 04, 2010 at 09:48:47AM -0500, jamal wrote:
> On Sat, 2010-12-04 at 09:28 -0500, jamal wrote:
> 
> > The idea was to defer processing at the first error. Changli
> > is changing it to continue despite the error.
> > The initial goal was to yield whenever possible since we dont maintain
> > a lot of state.
> 
> Changli:
> Other points we could defer processing is in case of packets
> being dropped by dev_queue_xmit and netif_rx

Hmm... But we didn't care until now... ;-) Btw. is it really very
probable (and worth bothering) that this current error of NULL dev
get fixed before we purge this tq queue with deferral one by one?

Cheers,
Jarek P.

Re: [PATCH 3/3] ifb: move tq from ifb_private

[jamal]

On Sat, 2010-12-04 at 16:40 +0100, Jarek Poplawski wrote:

> Hmm... But we didn't care until now... ;-) 

Well, if Changli didnt post - there would be no discussion ;->
The message was lost in the translation somewhere; events such as
patches sometimes serve as good reminders.

> Btw. is it really very
> probable (and worth bothering) that this current error of NULL dev
> get fixed before we purge this tq queue with deferral one by one?

Indeed - this is working against something buggy. But it has happened
often in the past. And the likelihood of there being a few bad ones
in the train of packets when this occurs is high. But there are many
packets there that wont suffer this sympton - so the only fair scheme is
to check all. Note: a BUG() seems unreasonable and the deferring  serves
as a throttling scheme.
What do you have in mind?

cheers,
jamal

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Jarek Poplawski]

On Sat, Dec 04, 2010 at 11:08:01AM -0500, jamal wrote:
> On Sat, 2010-12-04 at 16:40 +0100, Jarek Poplawski wrote:
> 
> > Hmm... But we didn't care until now... ;-) 
> 
> Well, if Changli didnt post - there would be no discussion ;->
> The message was lost in the translation somewhere; events such as
> patches sometimes serve as good reminders.
> 
> > Btw. is it really very
> > probable (and worth bothering) that this current error of NULL dev
> > get fixed before we purge this tq queue with deferral one by one?
> 
> Indeed - this is working against something buggy. But it has happened
> often in the past. And the likelihood of there being a few bad ones
> in the train of packets when this occurs is high. But there are many
> packets there that wont suffer this sympton - so the only fair scheme is
> to check all. Note: a BUG() seems unreasonable and the deferring  serves
> as a throttling scheme.
> What do you have in mind?

I'm simply not convinced this kind of (fast) throttling can properly
fix any of the problems (what about other flows in the queue), while
Changli's patch makes this tasklet simpler and a bit faster.

Cheers,
Jarek P.

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Changli Gao]

On Sun, Dec 5, 2010 at 12:56 AM, Jarek Poplawski <jarkao2@gmail.com> wrote:
>
> I'm simply not convinced this kind of (fast) throttling can properly
> fix any of the problems (what about other flows in the queue), while
> Changli's patch makes this tasklet simpler and a bit faster.
>

The error case handled currently is the original netdev disappears but
the corresponding skbs are still in ifb.

I do also think checking the return value of netif_rx() and
dev_queue_xmit() can fix more 'problems'. :)

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Changli Gao]

On Sun, Dec 5, 2010 at 8:22 AM, Changli Gao <xiaosuo@gmail.com> wrote:
>
> The error case handled currently is the original netdev disappears but
> the corresponding skbs are still in ifb.
>
> I do also think checking the return value of netif_rx() and
> dev_queue_xmit() can fix more 'problems'. :)
>

I have posted the V2 and split the bug fix to a separate one.

BTW: My ultimate goal is making ifb a multi-queue NIC, and the number
of queues is equal to the number of the possible CPUs.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 3/3] ifb: move tq from ifb_private

[jamal]

On Sun, 2010-12-05 at 08:22 +0800, Changli Gao wrote:
> On Sun, Dec 5, 2010 at 12:56 AM, Jarek Poplawski <jarkao2@gmail.com> wrote:
> >
> > I'm simply not convinced this kind of (fast) throttling can properly
> > fix any of the problems (what about other flows in the queue), while
> > Changli's patch makes this tasklet simpler and a bit faster.
> >
> 
> The error case handled currently is the original netdev disappears but
> the corresponding skbs are still in ifb.
> 
> I do also think checking the return value of netif_rx() and
> dev_queue_xmit() can fix more 'problems'. :)


Please proceed with a patch for that as well..

cheers,
jamal

PS:- We forgot to thank Jarek for pointing out the bug.

Re: [PATCH 3/3] ifb: move tq from ifb_private

[jamal]

On Sun, 2010-12-05 at 09:13 +0800, Changli Gao wrote:

> BTW: My ultimate goal is making ifb a multi-queue NIC, and the number
> of queues is equal to the number of the possible CPUs.


My view is this is going to be tricky because:
- we use tasklets. When we  reschedule we can end up on a differrent
cpu. 
-I dont see any point in having a separate softIRQ 
- and if you do use other mechanisms it would require a lot more
testing since there are quiet a few use cases of ifb

cheers,
jamal

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Changli Gao]

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

On Sun, Dec 5, 2010 at 10:30 PM, jamal <hadi@cyberus.ca> wrote:
> On Sun, 2010-12-05 at 09:13 +0800, Changli Gao wrote:
>
>> BTW: My ultimate goal is making ifb a multi-queue NIC, and the number
>> of queues is equal to the number of the possible CPUs.
>
>
> My view is this is going to be tricky because:
> - we use tasklets. When we  reschedule we can end up on a differrent
> cpu.

The tasklets always been scheduled to the current CPU unless it has
been schedule already on the other CPU.

> -I dont see any point in having a separate softIRQ
> - and if you do use other mechanisms it would require a lot more
> testing since there are quiet a few use cases of ifb
>

I keep using tasklet. The attachment is the alpha version.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

[-- Attachment #2: ifb.c --]
[-- Type: text/x-csrc, Size: 7698 bytes --]

/* drivers/net/ifb.c:

	The purpose of this driver is to provide a device that allows
	for sharing of resources:

	1) qdiscs/policies that are per device as opposed to system wide.
	ifb allows for a device which can be redirected to thus providing
	an impression of sharing.

	2) Allows for queueing incoming traffic for shaping instead of
	dropping.

	The original concept is based on what is known as the IMQ
	driver initially written by Martin Devera, later rewritten
	by Patrick McHardy and then maintained by Andre Correa.

	You need the tc action  mirror or redirect to feed this device
	packets.

	This program is free software; you can redistribute it and/or
	modify it under the terms of the GNU General Public License
	as published by the Free Software Foundation; either version
	2 of the License, or (at your option) any later version.

	Authors:	Jamal Hadi Salim (2005)

*/


#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
#include <linux/init.h>
#include <linux/moduleparam.h>
#include <net/pkt_sched.h>
#include <net/net_namespace.h>

#define TX_Q_LIMIT    32
struct ifb_q_private {
	struct tasklet_struct	ifb_tasklet;
	struct sk_buff_head	rq;
	struct u64_stats_sync	syncp;
	u64			rx_packets;
	u64			rx_bytes;
	u64			rx_dropped;
};

struct ifb_private {
	struct ifb_q_private __percpu	*q;
};

static int numifbs = 2;

static void ri_tasklet(unsigned long _dev)
{
	struct net_device *dev = (struct net_device *)_dev;
	struct ifb_private *p = netdev_priv(dev);
	struct ifb_q_private *qp;
	struct netdev_queue *txq;
	struct sk_buff *skb;
	struct sk_buff_head tq;

	__skb_queue_head_init(&tq);
	txq = netdev_get_tx_queue(dev, raw_smp_processor_id());
	qp = per_cpu_ptr(p->q, raw_smp_processor_id());
	if (!__netif_tx_trylock(txq)) {
		tasklet_schedule(&qp->ifb_tasklet);
		return;
	}
	skb_queue_splice_tail_init(&qp->rq, &tq);
	if (netif_tx_queue_stopped(txq))
		netif_tx_wake_queue(txq);
	__netif_tx_unlock(txq);

	while ((skb = __skb_dequeue(&tq)) != NULL) {
		u32 from = G_TC_FROM(skb->tc_verd);

		skb->tc_verd = 0;
		skb->tc_verd = SET_TC_NCLS(skb->tc_verd);
		u64_stats_update_begin(&qp->syncp);
		txq->tx_packets++;
		txq->tx_bytes += skb->len;

		rcu_read_lock();
		skb->dev = dev_get_by_index_rcu(&init_net, skb->skb_iif);
		if (!skb->dev) {
			rcu_read_unlock();
			txq->tx_dropped++;
			u64_stats_update_end(&qp->syncp);
			dev_kfree_skb(skb);
			continue;
		}
		rcu_read_unlock();
		u64_stats_update_end(&qp->syncp);
		skb->skb_iif = dev->ifindex;

		if (from & AT_EGRESS) {
			dev_queue_xmit(skb);
		} else if (from & AT_INGRESS) {
			skb_pull(skb, skb->dev->hard_header_len);
			netif_rx(skb);
		} else
			BUG();
	}
}

static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
{
	struct ifb_private *p = netdev_priv(dev);
	struct ifb_q_private *qp = per_cpu_ptr(p->q,
					       skb_get_queue_mapping(skb));
	u32 from = G_TC_FROM(skb->tc_verd);

	u64_stats_update_begin(&qp->syncp);
	qp->rx_packets++;
	qp->rx_bytes += skb->len;

	if (!(from & (AT_INGRESS|AT_EGRESS)) || !skb->skb_iif) {
		qp->rx_dropped++;
		u64_stats_update_end(&qp->syncp);
		dev_kfree_skb(skb);
		return NETDEV_TX_OK;
	}
	u64_stats_update_end(&qp->syncp);

	__skb_queue_tail(&qp->rq, skb);
	if (skb_queue_len(&qp->rq) == 1)
		tasklet_schedule(&qp->ifb_tasklet);

	if (skb_queue_len(&qp->rq) >= dev->tx_queue_len)
		netif_stop_queue(dev);

	return NETDEV_TX_OK;
}

static int ifb_close(struct net_device *dev)
{
	struct ifb_private *p = netdev_priv(dev);
	struct ifb_q_private *qp;
	int cpu;

	for_each_possible_cpu(cpu) {
		qp = per_cpu_ptr(p->q, cpu);
		tasklet_kill(&qp->ifb_tasklet);
		netif_tx_stop_queue(netdev_get_tx_queue(dev, cpu));
		__skb_queue_purge(&qp->rq);
	}

	return 0;
}

static int ifb_open(struct net_device *dev)
{
	int cpu;

	for_each_possible_cpu(cpu)
		netif_tx_start_queue(netdev_get_tx_queue(dev, cpu));

	return 0;
}

static int ifb_init(struct net_device *dev)
{
	struct ifb_private *p = netdev_priv(dev);
	struct ifb_q_private *q;
	int cpu;

	p->q = alloc_percpu(struct ifb_q_private);
	if (!p->q)
		return -ENOMEM;
	for_each_possible_cpu(cpu) {
		q = per_cpu_ptr(p->q, cpu);
		tasklet_init(&q->ifb_tasklet, ri_tasklet, (unsigned long)dev);
		__skb_queue_head_init(&q->rq);
	}

	return 0;
}

static void ifb_uninit(struct net_device *dev)
{
	struct ifb_private *p = netdev_priv(dev);

	free_percpu(p->q);
}

static u16 ifb_select_queue(struct net_device *dev, struct sk_buff *skb)
{
	return smp_processor_id();
}

static struct rtnl_link_stats64 *ifb_get_stats64(struct net_device *dev,
		struct rtnl_link_stats64 *stats)
{
	struct ifb_private *p = netdev_priv(dev);
	struct ifb_q_private *q;
	struct netdev_queue *txq;
	int cpu;
	u64 rx_packets, rx_bytes, rx_dropped;
	u64 tx_packets, tx_bytes, tx_dropped;
	unsigned int start;

	for_each_possible_cpu(cpu) {
		q = per_cpu_ptr(p->q, cpu);
		txq = netdev_get_tx_queue(dev, cpu);
		do {
			start = u64_stats_fetch_begin_bh(&q->syncp);
			rx_packets = q->rx_packets;
			rx_bytes = q->rx_bytes;
			rx_dropped = q->rx_dropped;
			tx_packets = txq->tx_packets;
			tx_bytes = txq->tx_bytes;
			tx_dropped = txq->tx_dropped;
		} while (u64_stats_fetch_retry_bh(&q->syncp, start));
		stats->rx_packets += rx_packets;
		stats->rx_bytes += rx_bytes;
		stats->rx_dropped += rx_dropped;
		stats->tx_packets += tx_packets;
		stats->tx_bytes += tx_bytes;
		stats->tx_dropped += tx_dropped;
	}

	return stats;
}

static const struct net_device_ops ifb_netdev_ops = {
	.ndo_init		= ifb_init,
	.ndo_uninit		= ifb_uninit,
	.ndo_open		= ifb_open,
	.ndo_stop		= ifb_close,
	.ndo_start_xmit		= ifb_xmit,
	.ndo_validate_addr	= eth_validate_addr,
	.ndo_select_queue	= ifb_select_queue,
	.ndo_get_stats64	= ifb_get_stats64,
};

static void ifb_setup(struct net_device *dev)
{
	/* Initialize the device structure. */
	dev->destructor = free_netdev;
	dev->netdev_ops = &ifb_netdev_ops;

	/* Fill in device structure with ethernet-generic values. */
	ether_setup(dev);
	dev->tx_queue_len = TX_Q_LIMIT;

	dev->flags |= IFF_NOARP;
	dev->flags &= ~IFF_MULTICAST;
	dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
	random_ether_addr(dev->dev_addr);
}

static int ifb_validate(struct nlattr *tb[], struct nlattr *data[])
{
	if (tb[IFLA_ADDRESS]) {
		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)
			return -EINVAL;
		if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS])))
			return -EADDRNOTAVAIL;
	}

	return 0;
}

static struct rtnl_link_ops ifb_link_ops __read_mostly = {
	.kind		= "ifb",
	.priv_size	= sizeof(struct ifb_private),
	.setup		= ifb_setup,
	.validate	= ifb_validate,
};

/* Number of ifb devices to be set up by this module. */
module_param(numifbs, int, 0);
MODULE_PARM_DESC(numifbs, "Number of ifb devices");

static int __init ifb_init_one(int index)
{
	struct net_device *dev_ifb;
	int err;

	dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private), "ifb%d",
				  ifb_setup, num_possible_cpus());

	if (!dev_ifb)
		return -ENOMEM;

	err = dev_alloc_name(dev_ifb, dev_ifb->name);
	if (err < 0)
		goto err;

	dev_ifb->rtnl_link_ops = &ifb_link_ops;
	err = register_netdevice(dev_ifb);
	if (err < 0)
		goto err;

	return 0;

err:
	free_netdev(dev_ifb);
	return err;
}

static int __init ifb_init_module(void)
{
	int i, err;

	rtnl_lock();
	err = __rtnl_link_register(&ifb_link_ops);

	for (i = 0; i < numifbs && !err; i++)
		err = ifb_init_one(i);
	if (err)
		__rtnl_link_unregister(&ifb_link_ops);
	rtnl_unlock();

	return err;
}

static void __exit ifb_cleanup_module(void)
{
	rtnl_link_unregister(&ifb_link_ops);
}

module_init(ifb_init_module);
module_exit(ifb_cleanup_module);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Jamal Hadi Salim");
MODULE_ALIAS_RTNL_LINK("ifb");

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Eric Dumazet]

Le dimanche 05 décembre 2010 à 22:40 +0800, Changli Gao a écrit :
> On Sun, Dec 5, 2010 at 10:30 PM, jamal <hadi@cyberus.ca> wrote:
> > On Sun, 2010-12-05 at 09:13 +0800, Changli Gao wrote:
> >
> >> BTW: My ultimate goal is making ifb a multi-queue NIC, and the number
> >> of queues is equal to the number of the possible CPUs.
> >
> >
> > My view is this is going to be tricky because:
> > - we use tasklets. When we  reschedule we can end up on a differrent
> > cpu.
> 
> The tasklets always been scheduled to the current CPU unless it has
> been schedule already on the other CPU.
> 
> > -I dont see any point in having a separate softIRQ
> > - and if you do use other mechanisms it would require a lot more
> > testing since there are quiet a few use cases of ifb
> >
> 
> I keep using tasklet. The attachment is the alpha version.
> 

        for_each_possible_cpu(cpu) {
                q = per_cpu_ptr(p->q, cpu);
...

        dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private), "ifb%d",
                                  ifb_setup, num_possible_cpus());

This is a very usual error.

You can have machines with 2 possibles cpus, numbered 0 and 8

Therere, you must use nr_cpu_ids here instead of num_possible_cpus()

Re: [PATCH 3/3] ifb: move tq from ifb_private

[jamal]

On Sun, 2010-12-05 at 22:40 +0800, Changli Gao wrote:
> On Sun, Dec 5, 2010 at 10:30 PM, jamal <hadi@cyberus.ca> wrote:

> > - we use tasklets. When we  reschedule we can end up on a differrent
> > cpu.
> 
> The tasklets always been scheduled to the current CPU unless it has
> been schedule already on the other CPU.

I dont think this can be guaranteed - So the potential of packet 
reordering exists.

> I keep using tasklet. The attachment is the alpha version.

>From quick glance I dont see any technicalities - just the 
reordering concern.

cheers,
jamal

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Changli Gao]

On Sun, Dec 5, 2010 at 11:13 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le dimanche 05 décembre 2010 à 22:40 +0800, Changli Gao a écrit :
>> On Sun, Dec 5, 2010 at 10:30 PM, jamal <hadi@cyberus.ca> wrote:
>> > On Sun, 2010-12-05 at 09:13 +0800, Changli Gao wrote:
>> >
>> >> BTW: My ultimate goal is making ifb a multi-queue NIC, and the number
>> >> of queues is equal to the number of the possible CPUs.
>> >
>> >
>> > My view is this is going to be tricky because:
>> > - we use tasklets. When we  reschedule we can end up on a differrent
>> > cpu.
>>
>> The tasklets always been scheduled to the current CPU unless it has
>> been schedule already on the other CPU.
>>
>> > -I dont see any point in having a separate softIRQ
>> > - and if you do use other mechanisms it would require a lot more
>> > testing since there are quiet a few use cases of ifb
>> >
>>
>> I keep using tasklet. The attachment is the alpha version.
>>
>
>        for_each_possible_cpu(cpu) {
>                q = per_cpu_ptr(p->q, cpu);
> ...
>
>        dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private), "ifb%d",
>                                  ifb_setup, num_possible_cpus());
>
> This is a very usual error.
>
> You can have machines with 2 possibles cpus, numbered 0 and 8
>
> Therere, you must use nr_cpu_ids here instead of num_possible_cpus()
>

Thanks.


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Changli Gao]

On Sun, Dec 5, 2010 at 11:13 PM, jamal <hadi@cyberus.ca> wrote:
> On Sun, 2010-12-05 at 22:40 +0800, Changli Gao wrote:
>> On Sun, Dec 5, 2010 at 10:30 PM, jamal <hadi@cyberus.ca> wrote:
>
>> > - we use tasklets. When we  reschedule we can end up on a differrent
>> > cpu.
>>
>> The tasklets always been scheduled to the current CPU unless it has
>> been schedule already on the other CPU.
>
> I dont think this can be guaranteed - So the potential of packet
> reordering exists.
>

The current implementation can guarantee it. However, I can't find
documentation about this 'feature', but I think this behavior should
not be changed in future since maybe some call sites relay on it.


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 3/3] ifb: move tq from ifb_private

[jamal]

On Sun, 2010-12-05 at 23:22 +0800, Changli Gao wrote:

> The current implementation can guarantee it. However, I can't find
> documentation about this 'feature', but I think this behavior should
> not be changed in future since maybe some call sites relay on it.

When you think you are in good shape - please add some debug hooks
so we can verify this especially under a huge traffic input and
I will test it (thats what end of year holidays are for).

cheers,
jamal

Re: [PATCH 3/3] ifb: move tq from ifb_private

[Jarek Poplawski]

On Sun, Dec 05, 2010 at 09:27:11AM -0500, jamal wrote:
...
> PS:- We forgot to thank Jarek for pointing out the bug.

Not at all! I only pointed out that Changli didn't point out ;-)
All credits go to him and you for pointing out the appropriate fix.

Cheers,
Jarek P.