[PATCH 1/5] ifb: remove function declarations

[PATCH 1/5] ifb: remove function declarations

[Changli Gao]

Restructure to remove function declarations.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 drivers/net/ifb.c |   51 +++++++++++++++++++++++----------------------------
 1 file changed, 23 insertions(+), 28 deletions(-)
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index bfa03db..0667a61 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -46,11 +46,6 @@ struct ifb_private {
 
 static int numifbs = 2;
 
-static void ri_tasklet(unsigned long dev);
-static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev);
-static int ifb_open(struct net_device *dev);
-static int ifb_close(struct net_device *dev);
-
 static void ri_tasklet(unsigned long dev)
 {
 
@@ -119,29 +114,6 @@ resched:
 
 }
 
-static const struct net_device_ops ifb_netdev_ops = {
-	.ndo_open	= ifb_open,
-	.ndo_stop	= ifb_close,
-	.ndo_start_xmit	= ifb_xmit,
-	.ndo_validate_addr = eth_validate_addr,
-};
-
-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 netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct ifb_private *dp = netdev_priv(dev);
@@ -193,6 +165,29 @@ static int ifb_open(struct net_device *dev)
 	return 0;
 }
 
+static const struct net_device_ops ifb_netdev_ops = {
+	.ndo_open		= ifb_open,
+	.ndo_stop		= ifb_close,
+	.ndo_start_xmit		= ifb_xmit,
+	.ndo_validate_addr	= eth_validate_addr,
+};
+
+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]) {

[PATCH 2/5] ifb: code cleanup

[Changli Gao]

Clean up code according to scripts/checkpatch.pl

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 drivers/net/ifb.c |   22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 0667a61..1628d01 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -15,14 +15,14 @@
 	by Patrick McHardy and then maintained by Andre Correa.
 
 	You need the tc action  mirror or redirect to feed this device
-       	packets.
+	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)
+	Authors:	Jamal Hadi Salim (2005)
 
 */
 
@@ -56,7 +56,8 @@ static void ri_tasklet(unsigned long dev)
 	struct sk_buff *skb;
 
 	txq = netdev_get_tx_queue(_dev, 0);
-	if ((skb = skb_peek(&dp->tq)) == NULL) {
+	skb = skb_peek(&dp->tq);
+	if (skb == NULL) {
 		if (__netif_tx_trylock(txq)) {
 			skb_queue_splice_tail_init(&dp->rq, &dp->tq);
 			__netif_tx_unlock(txq);
@@ -72,7 +73,7 @@ static void ri_tasklet(unsigned long dev)
 		skb->tc_verd = 0;
 		skb->tc_verd = SET_TC_NCLS(skb->tc_verd);
 		stats->tx_packets++;
-		stats->tx_bytes +=skb->len;
+		stats->tx_bytes += skb->len;
 
 		rcu_read_lock();
 		skb->dev = dev_get_by_index_rcu(&init_net, skb->skb_iif);
@@ -97,7 +98,8 @@ static void ri_tasklet(unsigned long dev)
 	}
 
 	if (__netif_tx_trylock(txq)) {
-		if ((skb = skb_peek(&dp->rq)) == NULL) {
+		skb = skb_peek(&dp->rq);
+		if (skb == NULL) {
 			dp->tasklet_pending = 0;
 			if (netif_queue_stopped(_dev))
 				netif_wake_queue(_dev);
@@ -121,7 +123,7 @@ static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
 	u32 from = G_TC_FROM(skb->tc_verd);
 
 	stats->rx_packets++;
-	stats->rx_bytes+=skb->len;
+	stats->rx_bytes += skb->len;
 
 	if (!(from & (AT_INGRESS|AT_EGRESS)) || !skb->skb_iif) {
 		dev_kfree_skb(skb);
@@ -129,9 +131,8 @@ static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
 		return NETDEV_TX_OK;
 	}
 
-	if (skb_queue_len(&dp->rq) >= dev->tx_queue_len) {
+	if (skb_queue_len(&dp->rq) >= dev->tx_queue_len)
 		netif_stop_queue(dev);
-	}
 
 	__skb_queue_tail(&dp->rq, skb);
 	if (!dp->tasklet_pending) {
@@ -150,6 +151,7 @@ static int ifb_close(struct net_device *dev)
 	netif_stop_queue(dev);
 	__skb_queue_purge(&dp->rq);
 	__skb_queue_purge(&dp->tq);
+
 	return 0;
 }
 
@@ -196,6 +198,7 @@ static int ifb_validate(struct nlattr *tb[], struct nlattr *data[])
 		if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS])))
 			return -EADDRNOTAVAIL;
 	}
+
 	return 0;
 }
 
@@ -215,8 +218,7 @@ static int __init ifb_init_one(int index)
 	struct net_device *dev_ifb;
 	int err;
 
-	dev_ifb = alloc_netdev(sizeof(struct ifb_private),
-				 "ifb%d", ifb_setup);
+	dev_ifb = alloc_netdev(sizeof(struct ifb_private), "ifb%d", ifb_setup);
 
 	if (!dev_ifb)
 		return -ENOMEM;

[PATCH 3/5] ifb: fix tx_queue_len overlimit

[Changli Gao]

We should check the length of rq after enqueuing, otherwise the length
of rq will be over tx_queue_len.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 drivers/net/ifb.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 1628d01..57c5cfb 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -131,15 +131,15 @@ static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
 		return NETDEV_TX_OK;
 	}
 
-	if (skb_queue_len(&dp->rq) >= dev->tx_queue_len)
-		netif_stop_queue(dev);
-
 	__skb_queue_tail(&dp->rq, skb);
 	if (!dp->tasklet_pending) {
 		dp->tasklet_pending = 1;
 		tasklet_schedule(&dp->ifb_tasklet);
 	}
 
+	if (skb_queue_len(&dp->rq) >= dev->tx_queue_len)
+		netif_stop_queue(dev);
+
 	return NETDEV_TX_OK;
 }
 

[PATCH 4/5] ifb: add multiqueue support

[Changli Gao]

Each ifb NIC has nr_cpu_ids rx queues and nr_cpu_ids queues. Packets
transmitted to ifb are enqueued to the corresponding per cpu tx queues,
and processed in the corresponding per cpu tasklet latter.

The stats are converted to the u64 ones.

tq is a stack variable now. It makes ifb_q_private smaller and tx queue
locked only once in ri_tasklet.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 drivers/net/ifb.c |  211 ++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 141 insertions(+), 70 deletions(-)
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 57c5cfb..16c767b 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -37,56 +37,63 @@
 #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 tasklet_struct   ifb_tasklet;
-	int     tasklet_pending;
-	struct sk_buff_head     rq;
-	struct sk_buff_head     tq;
+	struct ifb_q_private __percpu	*q;
 };
 
 static int numifbs = 2;
 
-static void ri_tasklet(unsigned long 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 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;
-
-	txq = netdev_get_tx_queue(_dev, 0);
-	skb = skb_peek(&dp->tq);
-	if (skb == NULL) {
-		if (__netif_tx_trylock(txq)) {
-			skb_queue_splice_tail_init(&dp->rq, &dp->tq);
-			__netif_tx_unlock(txq);
-		} else {
-			/* reschedule */
-			goto resched;
-		}
+	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(&dp->tq)) != NULL) {
+	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);
-		stats->tx_packets++;
-		stats->tx_bytes += skb->len;
+		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);
-			stats->tx_dropped++;
-			if (skb_queue_len(&dp->tq) != 0)
-				goto resched;
-			break;
+			continue;
 		}
 		rcu_read_unlock();
-		skb->skb_iif = _dev->ifindex;
+		u64_stats_update_end(&qp->syncp);
+		skb->skb_iif = dev->ifindex;
 
 		if (from & AT_EGRESS) {
 			dev_queue_xmit(skb);
@@ -96,48 +103,32 @@ static void ri_tasklet(unsigned long dev)
 		} else
 			BUG();
 	}
-
-	if (__netif_tx_trylock(txq)) {
-		skb = skb_peek(&dp->rq);
-		if (skb == 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 netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
 {
-	struct ifb_private *dp = netdev_priv(dev);
-	struct net_device_stats *stats = &dev->stats;
+	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);
 
-	stats->rx_packets++;
-	stats->rx_bytes += skb->len;
+	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);
-		stats->rx_dropped++;
 		return NETDEV_TX_OK;
 	}
+	u64_stats_update_end(&qp->syncp);
 
-	__skb_queue_tail(&dp->rq, skb);
-	if (!dp->tasklet_pending) {
-		dp->tasklet_pending = 1;
-		tasklet_schedule(&dp->ifb_tasklet);
-	}
+	__skb_queue_tail(&qp->rq, skb);
+	if (skb_queue_len(&qp->rq) == 1)
+		tasklet_schedule(&qp->ifb_tasklet);
 
-	if (skb_queue_len(&dp->rq) >= dev->tx_queue_len)
+	if (skb_queue_len(&qp->rq) >= dev->tx_queue_len)
 		netif_stop_queue(dev);
 
 	return NETDEV_TX_OK;
@@ -145,33 +136,103 @@ static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
 
 static int ifb_close(struct net_device *dev)
 {
-	struct ifb_private *dp = netdev_priv(dev);
-
-	tasklet_kill(&dp->ifb_tasklet);
-	netif_stop_queue(dev);
-	__skb_queue_purge(&dp->rq);
-	__skb_queue_purge(&dp->tq);
+	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)
 {
-	struct ifb_private *dp = netdev_priv(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;
 
-	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);
+	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)
@@ -202,11 +263,21 @@ static int ifb_validate(struct nlattr *tb[], struct nlattr *data[])
 	return 0;
 }
 
+static int ifb_get_tx_queues(struct net *net, struct nlattr *tb[],
+			     unsigned int *tx_queues,
+			     unsigned int *real_tx_queues)
+{
+	*real_tx_queues = *tx_queues = nr_cpu_ids;
+
+	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,
+	.get_tx_queues	= ifb_get_tx_queues,
 };
 
 /* Number of ifb devices to be set up by this module. */
@@ -218,8 +289,8 @@ static int __init ifb_init_one(int index)
 	struct net_device *dev_ifb;
 	int err;
 
-	dev_ifb = alloc_netdev(sizeof(struct ifb_private), "ifb%d", ifb_setup);
-
+	dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private), "ifb%d",
+				  ifb_setup, nr_cpu_ids);
 	if (!dev_ifb)
 		return -ENOMEM;
 

[PATCH 5/5] net: add skb.old_queue_mapping

[Changli Gao]

For the skbs returned from ifb, we should use the queue_mapping
saved before ifb.

We save old queue_mapping in old_queue_mapping just before calling 
dev_queue_xmit, and restore the old_queue_mapping to queue_mapping
just before reinjecting the skb.

dev_pick_tx() use the current queue_mapping for the skbs reinjected
by ifb.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 drivers/net/ifb.c      |    1 +
 include/linux/skbuff.h |    3 +++
 net/core/dev.c         |    5 +++++
 net/sched/act_mirred.c |    1 +
 4 files changed, 10 insertions(+)
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 16c767b..481e4b1 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -95,6 +95,7 @@ static void ri_tasklet(unsigned long _dev)
 		u64_stats_update_end(&qp->syncp);
 		skb->skb_iif = dev->ifindex;
 
+		skb->queue_mapping = skb->old_queue_mapping;
 		if (from & AT_EGRESS) {
 			dev_queue_xmit(skb);
 		} else if (from & AT_INGRESS) {
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 19f37a6..2ce2a96 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -403,6 +403,9 @@ struct sk_buff {
 	};
 
 	__u16			vlan_tci;
+#ifdef CONFIG_NET_CLS_ACT
+	__u16			old_queue_mapping;
+#endif
 
 	sk_buff_data_t		transport_header;
 	sk_buff_data_t		network_header;
diff --git a/net/core/dev.c b/net/core/dev.c
index d28b3a0..8e97cfd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2190,6 +2190,11 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev,
 	int queue_index;
 	const struct net_device_ops *ops = dev->netdev_ops;
 
+#ifdef CONFIG_NET_CLS_ACT
+	if (skb->tc_verd & TC_NCLS)
+		queue_index = skb_get_queue_mapping(skb);
+	else
+#endif
 	if (dev->real_num_tx_queues == 1)
 		queue_index = 0;
 	else if (ops->ndo_select_queue) {
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 0c311be..853eb30 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -197,6 +197,7 @@ static int tcf_mirred(struct sk_buff *skb, struct tc_action *a,
 
 	skb2->skb_iif = skb->dev->ifindex;
 	skb2->dev = dev;
+	skb2->old_queue_mapping = skb->queue_mapping;
 	dev_queue_xmit(skb2);
 	err = 0;
 

Re: [PATCH 4/5] ifb: add multiqueue support

[Eric Dumazet]

Le lundi 13 décembre 2010 à 22:43 +0800, Changli Gao a écrit :
> Each ifb NIC has nr_cpu_ids rx queues and nr_cpu_ids queues. Packets
> transmitted to ifb are enqueued to the corresponding per cpu tx queues,
> and processed in the corresponding per cpu tasklet latter.
> 
> The stats are converted to the u64 ones.
> 
> tq is a stack variable now. It makes ifb_q_private smaller and tx queue
> locked only once in ri_tasklet.
> 

Might be good to say the tx_queue_len is multiplied by number of online
cpus ;)

> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
>  drivers/net/ifb.c |  211 ++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 141 insertions(+), 70 deletions(-)
> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
> index 57c5cfb..16c767b 100644
> --- a/drivers/net/ifb.c
> +++ b/drivers/net/ifb.c
> @@ -37,56 +37,63 @@
>  #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 tasklet_struct   ifb_tasklet;
> -	int     tasklet_pending;
> -	struct sk_buff_head     rq;
> -	struct sk_buff_head     tq;
> +	struct ifb_q_private __percpu	*q;

You probably could use dev->ml_priv (lstats/tstats/dstats)
so that ifb_private just disapears (we save a full cache line)

>  };
>  
>  static int numifbs = 2;
>  
> -static void ri_tasklet(unsigned long 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 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;
> -
> -	txq = netdev_get_tx_queue(_dev, 0);
> -	skb = skb_peek(&dp->tq);
> -	if (skb == NULL) {
> -		if (__netif_tx_trylock(txq)) {
> -			skb_queue_splice_tail_init(&dp->rq, &dp->tq);
> -			__netif_tx_unlock(txq);
> -		} else {
> -			/* reschedule */
> -			goto resched;
> -		}
> +	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());

	qp = this_cpu_ptr(dev->ifb_qp); is faster

> +	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(&dp->tq)) != NULL) {
> +	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);
> -		stats->tx_packets++;
> -		stats->tx_bytes += skb->len;
> +		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);
> -			stats->tx_dropped++;
> -			if (skb_queue_len(&dp->tq) != 0)
> -				goto resched;
> -			break;
> +			continue;
>  		}
>  		rcu_read_unlock();
> -		skb->skb_iif = _dev->ifindex;
> +		u64_stats_update_end(&qp->syncp);
> +		skb->skb_iif = dev->ifindex;

Why is this necessary ? shouldnt skb->skb_iif already be dev->ifindex ?

>  
>  		if (from & AT_EGRESS) {
>  			dev_queue_xmit(skb);
> @@ -96,48 +103,32 @@ static void ri_tasklet(unsigned long dev)
>  		} else
>  			BUG();
>  	}
> -
> -	if (__netif_tx_trylock(txq)) {
> -		skb = skb_peek(&dp->rq);
> -		if (skb == 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 netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
>  {
> -	struct ifb_private *dp = netdev_priv(dev);
> -	struct net_device_stats *stats = &dev->stats;
> +	struct ifb_private *p = netdev_priv(dev);
> +	struct ifb_q_private *qp = per_cpu_ptr(p->q,
> +					       skb_get_queue_mapping(skb));

Would be good to add a 

WARN_ON(skb_get_queue_mapping(skb) != smp_processor_id());


>  	u32 from = G_TC_FROM(skb->tc_verd);
>  
> -	stats->rx_packets++;
> -	stats->rx_bytes += skb->len;
> +	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);
> -		stats->rx_dropped++;
>  		return NETDEV_TX_OK;
>  	}
> +	u64_stats_update_end(&qp->syncp);
>  
> -	__skb_queue_tail(&dp->rq, skb);
> -	if (!dp->tasklet_pending) {
> -		dp->tasklet_pending = 1;
> -		tasklet_schedule(&dp->ifb_tasklet);
> -	}
> +	__skb_queue_tail(&qp->rq, skb);
> +	if (skb_queue_len(&qp->rq) == 1)
> +		tasklet_schedule(&qp->ifb_tasklet);
>  
> -	if (skb_queue_len(&dp->rq) >= dev->tx_queue_len)
> +	if (skb_queue_len(&qp->rq) >= dev->tx_queue_len)

This seems wrong...
You need to change to netif_tx_stop_queue(txq)

>  		netif_stop_queue(dev);
>  
>  	return NETDEV_TX_OK;
> @@ -145,33 +136,103 @@ static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  static int ifb_close(struct net_device *dev)
>  {
> -	struct ifb_private *dp = netdev_priv(dev);
> -
> -	tasklet_kill(&dp->ifb_tasklet);
> -	netif_stop_queue(dev);
> -	__skb_queue_purge(&dp->rq);
> -	__skb_queue_purge(&dp->tq);
> +	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)
>  {
> -	struct ifb_private *dp = netdev_priv(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;
>  
> -	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);
> +	p->q = alloc_percpu(struct ifb_q_private);
> +	if (!p->q)
> +		return -ENOMEM;

Hmm, maybe  use netdev_queue_numa_node_write() somewhere, so that
qdisc_alloc() can use NUMA affinities.

> +	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)
> @@ -202,11 +263,21 @@ static int ifb_validate(struct nlattr *tb[], struct nlattr *data[])
>  	return 0;
>  }
>  
> +static int ifb_get_tx_queues(struct net *net, struct nlattr *tb[],
> +			     unsigned int *tx_queues,
> +			     unsigned int *real_tx_queues)
> +{
> +	*real_tx_queues = *tx_queues = nr_cpu_ids;
> +
> +	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,
> +	.get_tx_queues	= ifb_get_tx_queues,
>  };
>  
>  /* Number of ifb devices to be set up by this module. */
> @@ -218,8 +289,8 @@ static int __init ifb_init_one(int index)
>  	struct net_device *dev_ifb;
>  	int err;
>  
> -	dev_ifb = alloc_netdev(sizeof(struct ifb_private), "ifb%d", ifb_setup);
> -
> +	dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private), "ifb%d",
> +				  ifb_setup, nr_cpu_ids);
>  	if (!dev_ifb)
>  		return -ENOMEM;
>  

Re: [PATCH 5/5] net: add skb.old_queue_mapping

[Eric Dumazet]

Le lundi 13 décembre 2010 à 22:43 +0800, Changli Gao a écrit :
> For the skbs returned from ifb, we should use the queue_mapping
> saved before ifb.
> 
> We save old queue_mapping in old_queue_mapping just before calling 
> dev_queue_xmit, and restore the old_queue_mapping to queue_mapping
> just before reinjecting the skb.
> 
> dev_pick_tx() use the current queue_mapping for the skbs reinjected
> by ifb.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
>  drivers/net/ifb.c      |    1 +
>  include/linux/skbuff.h |    3 +++
>  net/core/dev.c         |    5 +++++
>  net/sched/act_mirred.c |    1 +
>  4 files changed, 10 insertions(+)
> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
> index 16c767b..481e4b1 100644
> --- a/drivers/net/ifb.c
> +++ b/drivers/net/ifb.c
> @@ -95,6 +95,7 @@ static void ri_tasklet(unsigned long _dev)
>  		u64_stats_update_end(&qp->syncp);
>  		skb->skb_iif = dev->ifindex;
>  
> +		skb->queue_mapping = skb->old_queue_mapping;
>  		if (from & AT_EGRESS) {
>  			dev_queue_xmit(skb);
>  		} else if (from & AT_INGRESS) {
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 19f37a6..2ce2a96 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -403,6 +403,9 @@ struct sk_buff {
>  	};
>  
>  	__u16			vlan_tci;
> +#ifdef CONFIG_NET_CLS_ACT
> +	__u16			old_queue_mapping;
> +#endif
>  

Are you sure we need this field here ? Why not using cb[] for example ?


>  	sk_buff_data_t		transport_header;
>  	sk_buff_data_t		network_header;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d28b3a0..8e97cfd 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2190,6 +2190,11 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev,
>  	int queue_index;
>  	const struct net_device_ops *ops = dev->netdev_ops;
>  
> +#ifdef CONFIG_NET_CLS_ACT
> +	if (skb->tc_verd & TC_NCLS)
> +		queue_index = skb_get_queue_mapping(skb);
> +	else
> +#endif
>  	if (dev->real_num_tx_queues == 1)
>  		queue_index = 0;
>  	else if (ops->ndo_select_queue) {
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 0c311be..853eb30 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -197,6 +197,7 @@ static int tcf_mirred(struct sk_buff *skb, struct tc_action *a,
>  
>  	skb2->skb_iif = skb->dev->ifindex;
>  	skb2->dev = dev;
> +	skb2->old_queue_mapping = skb->queue_mapping;
>  	dev_queue_xmit(skb2);
>  	err = 0;
>  

Re: [PATCH 4/5] ifb: add multiqueue support

[Eric Dumazet]

Le lundi 13 décembre 2010 à 22:43 +0800, Changli Gao a écrit :
> Each ifb NIC has nr_cpu_ids rx queues and nr_cpu_ids queues. Packets
> transmitted to ifb are enqueued to the corresponding per cpu tx queues,
> and processed in the corresponding per cpu tasklet latter.
> 
> The stats are converted to the u64 ones.
> 
> tq is a stack variable now. It makes ifb_q_private smaller and tx queue
> locked only once in ri_tasklet.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
>  drivers/net/ifb.c |  211 ++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 141 insertions(+), 70 deletions(-)
> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
> index 57c5cfb..16c767b 100644
> --- a/drivers/net/ifb.c
> +++ b/drivers/net/ifb.c
> @@ -37,56 +37,63 @@
>  #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 tasklet_struct   ifb_tasklet;
> -	int     tasklet_pending;
> -	struct sk_buff_head     rq;
> -	struct sk_buff_head     tq;
> +	struct ifb_q_private __percpu	*q;
>  };
>  
>  static int numifbs = 2;
>  
> -static void ri_tasklet(unsigned long 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 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;
> -
> -	txq = netdev_get_tx_queue(_dev, 0);
> -	skb = skb_peek(&dp->tq);
> -	if (skb == NULL) {
> -		if (__netif_tx_trylock(txq)) {
> -			skb_queue_splice_tail_init(&dp->rq, &dp->tq);
> -			__netif_tx_unlock(txq);
> -		} else {
> -			/* reschedule */
> -			goto resched;
> -		}
> +	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());


Hmm, this wont work with CPU HOTPLUG.

When we put a cpu offline, we can transfert tasklets from this cpu to
another 'online cpu' 

To solve this, you need that ri_tasklet() not use a "device pointer"
parameter but a pointer to 'cpu' private data, since it can be different
than the data of the current cpu.

static void ri_tasklet(unsigned long arg)
{
struct ifb_q_private *qp = (struct ifb_q_private *)arg;

	...
}

Re: [PATCH 5/5] net: add skb.old_queue_mapping

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 13 Dec 2010 17:56:52 +0100

> Le lundi 13 décembre 2010 à 22:43 +0800, Changli Gao a écrit :
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 19f37a6..2ce2a96 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -403,6 +403,9 @@ struct sk_buff {
>>  	};
>>  
>>  	__u16			vlan_tci;
>> +#ifdef CONFIG_NET_CLS_ACT
>> +	__u16			old_queue_mapping;
>> +#endif
>>  
> 
> Are you sure we need this field here ? Why not using cb[] for example ?

Agreed, we should be removing sk_buff members not adding new ones.

We should especially not be adding new members to this critical
structure for more obscure facilities like ifb.

Re: [PATCH 4/5] ifb: add multiqueue support

[Changli Gao]

On Tue, Dec 14, 2010 at 12:26 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 13 décembre 2010 à 22:43 +0800, Changli Gao a écrit :
>> Each ifb NIC has nr_cpu_ids rx queues and nr_cpu_ids queues. Packets
>> transmitted to ifb are enqueued to the corresponding per cpu tx queues,
>> and processed in the corresponding per cpu tasklet latter.
>>
>> The stats are converted to the u64 ones.
>>
>> tq is a stack variable now. It makes ifb_q_private smaller and tx queue
>> locked only once in ri_tasklet.
>>
>
> Might be good to say the tx_queue_len is multiplied by number of online
> cpus ;)

Thanks for completion.

>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>> ---
>>  drivers/net/ifb.c |  211 ++++++++++++++++++++++++++++++++++++------------------
>>  1 file changed, 141 insertions(+), 70 deletions(-)
>> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
>> index 57c5cfb..16c767b 100644
>> --- a/drivers/net/ifb.c
>> +++ b/drivers/net/ifb.c
>> @@ -37,56 +37,63 @@
>>  #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 tasklet_struct   ifb_tasklet;
>> -     int     tasklet_pending;
>> -     struct sk_buff_head     rq;
>> -     struct sk_buff_head     tq;
>> +     struct ifb_q_private __percpu   *q;
>
> You probably could use dev->ml_priv (lstats/tstats/dstats)
> so that ifb_private just disapears (we save a full cache line)

Good idea. Thanks.

>
>>  };
>>
>>  static int numifbs = 2;
>>
>> -static void ri_tasklet(unsigned long 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 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;
>> -
>> -     txq = netdev_get_tx_queue(_dev, 0);
>> -     skb = skb_peek(&dp->tq);
>> -     if (skb == NULL) {
>> -             if (__netif_tx_trylock(txq)) {
>> -                     skb_queue_splice_tail_init(&dp->rq, &dp->tq);
>> -                     __netif_tx_unlock(txq);
>> -             } else {
>> -                     /* reschedule */
>> -                     goto resched;
>> -             }
>> +     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());
>
>        qp = this_cpu_ptr(dev->ifb_qp); is faster

and less, thanks. :)

>
>> +     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(&dp->tq)) != NULL) {
>> +     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);
>> -             stats->tx_packets++;
>> -             stats->tx_bytes += skb->len;
>> +             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);
>> -                     stats->tx_dropped++;
>> -                     if (skb_queue_len(&dp->tq) != 0)
>> -                             goto resched;
>> -                     break;
>> +                     continue;
>>               }
>>               rcu_read_unlock();
>> -             skb->skb_iif = _dev->ifindex;
>> +             u64_stats_update_end(&qp->syncp);
>> +             skb->skb_iif = dev->ifindex;
>
> Why is this necessary ? shouldnt skb->skb_iif already be dev->ifindex ?

No. In fact, act_mirred use skb_iff to save the original dev.

        skb2->skb_iif = skb->dev->ifindex;
        skb2->dev = dev;

>
>>
>>               if (from & AT_EGRESS) {
>>                       dev_queue_xmit(skb);
>> @@ -96,48 +103,32 @@ static void ri_tasklet(unsigned long dev)
>>               } else
>>                       BUG();
>>       }
>> -
>> -     if (__netif_tx_trylock(txq)) {
>> -             skb = skb_peek(&dp->rq);
>> -             if (skb == 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 netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
>>  {
>> -     struct ifb_private *dp = netdev_priv(dev);
>> -     struct net_device_stats *stats = &dev->stats;
>> +     struct ifb_private *p = netdev_priv(dev);
>> +     struct ifb_q_private *qp = per_cpu_ptr(p->q,
>> +                                            skb_get_queue_mapping(skb));
>
> Would be good to add a
>
> WARN_ON(skb_get_queue_mapping(skb) != smp_processor_id());

I think it maybe what Jamal wants too. Thanks.

>
>
>>       u32 from = G_TC_FROM(skb->tc_verd);
>>
>> -     stats->rx_packets++;
>> -     stats->rx_bytes += skb->len;
>> +     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);
>> -             stats->rx_dropped++;
>>               return NETDEV_TX_OK;
>>       }
>> +     u64_stats_update_end(&qp->syncp);
>>
>> -     __skb_queue_tail(&dp->rq, skb);
>> -     if (!dp->tasklet_pending) {
>> -             dp->tasklet_pending = 1;
>> -             tasklet_schedule(&dp->ifb_tasklet);
>> -     }
>> +     __skb_queue_tail(&qp->rq, skb);
>> +     if (skb_queue_len(&qp->rq) == 1)
>> +             tasklet_schedule(&qp->ifb_tasklet);
>>
>> -     if (skb_queue_len(&dp->rq) >= dev->tx_queue_len)
>> +     if (skb_queue_len(&qp->rq) >= dev->tx_queue_len)
>
> This seems wrong...
> You need to change to netif_tx_stop_queue(txq)

Thanks for catching this. I missed it.

>
>>               netif_stop_queue(dev);
>>
>>       return NETDEV_TX_OK;
>> @@ -145,33 +136,103 @@ static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
>>
>>  static int ifb_close(struct net_device *dev)
>>  {
>> -     struct ifb_private *dp = netdev_priv(dev);
>> -
>> -     tasklet_kill(&dp->ifb_tasklet);
>> -     netif_stop_queue(dev);
>> -     __skb_queue_purge(&dp->rq);
>> -     __skb_queue_purge(&dp->tq);
>> +     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)
>>  {
>> -     struct ifb_private *dp = netdev_priv(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;
>>
>> -     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);
>> +     p->q = alloc_percpu(struct ifb_q_private);
>> +     if (!p->q)
>> +             return -ENOMEM;
>
> Hmm, maybe  use netdev_queue_numa_node_write() somewhere, so that
> qdisc_alloc() can use NUMA affinities.

I'll add it, thanks.

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

Re: [PATCH 4/5] ifb: add multiqueue support

[Changli Gao]

On Tue, Dec 14, 2010 at 1:05 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> Hmm, this wont work with CPU HOTPLUG.
>
> When we put a cpu offline, we can transfert tasklets from this cpu to
> another 'online cpu'
>
> To solve this, you need that ri_tasklet() not use a "device pointer"
> parameter but a pointer to 'cpu' private data, since it can be different
> than the data of the current cpu.
>
> static void ri_tasklet(unsigned long arg)
> {
> struct ifb_q_private *qp = (struct ifb_q_private *)arg;
>
>        ...
> }
>

You are right, and it was my original way. Thanks.


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

Re: [PATCH 5/5] net: add skb.old_queue_mapping

[Changli Gao]

On Tue, Dec 14, 2010 at 12:56 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 13 décembre 2010 à 22:43 +0800, Changli Gao a écrit :
>> For the skbs returned from ifb, we should use the queue_mapping
>> saved before ifb.
>>
>> We save old queue_mapping in old_queue_mapping just before calling
>> dev_queue_xmit, and restore the old_queue_mapping to queue_mapping
>> just before reinjecting the skb.
>>
>> dev_pick_tx() use the current queue_mapping for the skbs reinjected
>> by ifb.
>>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>> ---
>>  drivers/net/ifb.c      |    1 +
>>  include/linux/skbuff.h |    3 +++
>>  net/core/dev.c         |    5 +++++
>>  net/sched/act_mirred.c |    1 +
>>  4 files changed, 10 insertions(+)
>> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
>> index 16c767b..481e4b1 100644
>> --- a/drivers/net/ifb.c
>> +++ b/drivers/net/ifb.c
>> @@ -95,6 +95,7 @@ static void ri_tasklet(unsigned long _dev)
>>               u64_stats_update_end(&qp->syncp);
>>               skb->skb_iif = dev->ifindex;
>>
>> +             skb->queue_mapping = skb->old_queue_mapping;
>>               if (from & AT_EGRESS) {
>>                       dev_queue_xmit(skb);
>>               } else if (from & AT_INGRESS) {
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 19f37a6..2ce2a96 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -403,6 +403,9 @@ struct sk_buff {
>>       };
>>
>>       __u16                   vlan_tci;
>> +#ifdef CONFIG_NET_CLS_ACT
>> +     __u16                   old_queue_mapping;
>> +#endif
>>
>
> Are you sure we need this field here ? Why not using cb[] for example ?
>

skb->cb can only be used in the same layer, so we can't use it to save
a value crossing layers. A quick grep finds sch_netem just uses
skb->cb. Maybe we can put all the skb->cb in the qdisc layer and
old_queue_mapping in a structure as a new skb->cb. Is it OK? Thanks.


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

Re: [PATCH 5/5] net: add skb.old_queue_mapping

[Changli Gao]

On Tue, Dec 14, 2010 at 7:58 AM, Changli Gao <xiaosuo@gmail.com> wrote:
>
> skb->cb can only be used in the same layer, so we can't use it to save
> a value crossing layers. A quick grep finds sch_netem just uses
> skb->cb. Maybe we can put all the skb->cb in the qdisc layer and
> old_queue_mapping in a structure as a new skb->cb. Is it OK? Thanks.
>

I can't do that. Below the qdisc layer, there is another user of
skb->cb GSO. Though we can set the GSO features of ifb to skip
gso_segments(), it is too tricky and weak.

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

Re: [PATCH 1/5] ifb: remove function declarations

[jamal]

Sorry - I am very distracted with work but will get back to you
on these patches. If i can respond to from a quick glance such
as this one, I will do now.

On Mon, 2010-12-13 at 22:43 +0800, Changli Gao wrote:
> Restructure to remove function declarations.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

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

cheers,
jamal

Re: [PATCH 2/5] ifb: code cleanup

[jamal]

On Mon, 2010-12-13 at 22:43 +0800, Changli Gao wrote:
> Clean up code according to scripts/checkpatch.pl
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

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

cheers,
jamal

Re: [PATCH 3/5] ifb: fix tx_queue_len overlimit

[jamal]

On Mon, 2010-12-13 at 22:43 +0800, Changli Gao wrote:
> We should check the length of rq after enqueuing, otherwise the length
> of rq will be over tx_queue_len.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

Not sure about this one. The check is also for ==, so we are going
to stop before we go over tx_queue_len. What am i missing?

cheers,
jamal

Re: [PATCH 4/5] ifb: add multiqueue support

[jamal]

On Mon, 2010-12-13 at 22:43 +0800, Changli Gao wrote:
> Each ifb NIC has nr_cpu_ids rx queues and nr_cpu_ids queues. Packets
> transmitted to ifb are enqueued to the corresponding per cpu tx queues,
> and processed in the corresponding per cpu tasklet latter.
> 
> The stats are converted to the u64 ones.
> 
> tq is a stack variable now. It makes ifb_q_private smaller and tx queue
> locked only once in ri_tasklet.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

I will look closely at this one later and catchup with the discussion
you had with Eric.

cheers,
jamal

Re: [PATCH 3/5] ifb: fix tx_queue_len overlimit

[Changli Gao]

On Wed, Dec 15, 2010 at 8:33 PM, jamal <hadi@cyberus.ca> wrote:
> On Mon, 2010-12-13 at 22:43 +0800, Changli Gao wrote:
>> We should check the length of rq after enqueuing, otherwise the length
>> of rq will be over tx_queue_len.
>>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>
> Not sure about this one. The check is also for ==, so we are going
> to stop before we go over tx_queue_len. What am i missing?
>

But then, we put this skb into rq in any way, and the length of rq
becomes tx_queue_len + 1.



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

Re: [PATCH 5/5] net: add skb.old_queue_mapping

[jamal]

On Mon, 2010-12-13 at 22:43 +0800, Changli Gao wrote:
> For the skbs returned from ifb, we should use the queue_mapping
> saved before ifb.
> 
> We save old queue_mapping in old_queue_mapping just before calling 
> dev_queue_xmit, and restore the old_queue_mapping to queue_mapping
> just before reinjecting the skb.
> 
> dev_pick_tx() use the current queue_mapping for the skbs reinjected
> by ifb.

You are hard-coding policy here, no? ifb can do a lot of funky things
which change the nature of the flow. I can shape, i can edit packets
etc. Why is the queue mapping any different?

cheers,
jamal

Re: [PATCH 3/5] ifb: fix tx_queue_len overlimit

[jamal]

On Wed, 2010-12-15 at 20:36 +0800, Changli Gao wrote:

> 
> But then, we put this skb into rq in any way, and the length of rq
> becomes tx_queue_len + 1.

;->

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


cheers,
jamal

Re: [PATCH 5/5] net: add skb.old_queue_mapping

[Changli Gao]

On Wed, Dec 15, 2010 at 8:40 PM, jamal <hadi@cyberus.ca> wrote:
> On Mon, 2010-12-13 at 22:43 +0800, Changli Gao wrote:
>>
>> dev_pick_tx() use the current queue_mapping for the skbs reinjected
>> by ifb.
>
> You are hard-coding policy here, no? ifb can do a lot of funky things
> which change the nature of the flow. I can shape, i can edit packets
> etc. Why is the queue mapping any different?
>

It seems reasonable. Thanks. I'll remove this hard-coding policy.

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