[PATCH 5/5 v2] net: add old_queue_mapping into skb->cb

[PATCH 5/5 v2] net: add old_queue_mapping into skb->cb

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

A new struct dev_skb_cb is added, and valid in qdisc and gso layer.
The original qdisc_skb_cb and DEV_GSO_CB are placed after dev_skb_cb.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
v2: save old_queue_mapping in skb->cb
 drivers/net/ifb.c         |    1 +
 include/linux/netdevice.h |   11 +++++++++++
 include/net/sch_generic.h |    6 ++++--
 net/core/dev.c            |   18 ++++++++++++++----
 net/sched/act_mirred.c    |    1 +
 net/sched/sch_netem.c     |    5 +++--
 6 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 918a38e..ff01795 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -92,6 +92,7 @@ static void ri_tasklet(unsigned long arg)
 		u64_stats_update_end(&qp->syncp);
 		skb->skb_iif = dev->ifindex;
 
+		skb->queue_mapping = dev_skb_cb(skb)->old_queue_mapping;
 		if (from & AT_EGRESS) {
 			dev_queue_xmit(skb);
 		} else if (from & AT_INGRESS) {
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d31bc3c..9820a48 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1295,6 +1295,17 @@ struct napi_gro_cb {
 
 #define NAPI_GRO_CB(skb) ((struct napi_gro_cb *)(skb)->cb)
 
+struct dev_skb_cb {
+	u16		old_queue_mapping;
+	unsigned long	data[];
+};
+
+static inline struct dev_skb_cb *dev_skb_cb(struct sk_buff *skb)
+{
+	BUILD_BUG_ON(sizeof(skb->cb) < sizeof(struct dev_skb_cb));
+	return (struct dev_skb_cb *)skb->cb;
+}
+
 struct packet_type {
 	__be16			type;	/* This is really htons(ether_type). */
 	struct net_device	*dev;	/* NULL is wildcarded here	     */
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index ea1f8a8..1089938 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -199,7 +199,7 @@ struct tcf_proto {
 
 struct qdisc_skb_cb {
 	unsigned int		pkt_len;
-	char			data[];
+	unsigned long		data[];
 };
 
 static inline int qdisc_qlen(struct Qdisc *q)
@@ -209,7 +209,9 @@ static inline int qdisc_qlen(struct Qdisc *q)
 
 static inline struct qdisc_skb_cb *qdisc_skb_cb(struct sk_buff *skb)
 {
-	return (struct qdisc_skb_cb *)skb->cb;
+	BUILD_BUG_ON(sizeof(skb->cb) < sizeof(struct dev_skb_cb) +
+				       sizeof(struct qdisc_skb_cb));
+	return (struct qdisc_skb_cb *)dev_skb_cb(skb)->data;
 }
 
 static inline spinlock_t *qdisc_lock(struct Qdisc *qdisc)
diff --git a/net/core/dev.c b/net/core/dev.c
index d28b3a0..11a5a13 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1904,7 +1904,12 @@ struct dev_gso_cb {
 	void (*destructor)(struct sk_buff *skb);
 };
 
-#define DEV_GSO_CB(skb) ((struct dev_gso_cb *)(skb)->cb)
+static inline struct dev_gso_cb *dev_gso_cb(struct sk_buff *skb)
+{
+	BUILD_BUG_ON(sizeof(skb->cb) < sizeof(struct dev_skb_cb) +
+				       sizeof(struct dev_gso_cb));
+	return (struct dev_gso_cb *)dev_skb_cb(skb)->data;
+}
 
 static void dev_gso_skb_destructor(struct sk_buff *skb)
 {
@@ -1918,7 +1923,7 @@ static void dev_gso_skb_destructor(struct sk_buff *skb)
 		kfree_skb(nskb);
 	} while (skb->next);
 
-	cb = DEV_GSO_CB(skb);
+	cb = dev_gso_cb(skb);
 	if (cb->destructor)
 		cb->destructor(skb);
 }
@@ -1947,7 +1952,7 @@ static int dev_gso_segment(struct sk_buff *skb)
 		return PTR_ERR(segs);
 
 	skb->next = segs;
-	DEV_GSO_CB(skb)->destructor = skb->destructor;
+	dev_gso_cb(skb)->destructor = skb->destructor;
 	skb->destructor = dev_gso_skb_destructor;
 
 	return 0;
@@ -2103,7 +2108,7 @@ gso:
 
 out_kfree_gso_skb:
 	if (likely(skb->next == NULL))
-		skb->destructor = DEV_GSO_CB(skb)->destructor;
+		skb->destructor = dev_gso_cb(skb)->destructor;
 out_kfree_skb:
 	kfree_skb(skb);
 out:
@@ -2190,6 +2195,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..419e82f 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;
+	dev_skb_cb(skb2)->old_queue_mapping = skb->queue_mapping;
 	dev_queue_xmit(skb2);
 	err = 0;
 
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index e5593c0..df4ff89 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -82,8 +82,9 @@ struct netem_skb_cb {
 
 static inline struct netem_skb_cb *netem_skb_cb(struct sk_buff *skb)
 {
-	BUILD_BUG_ON(sizeof(skb->cb) <
-		sizeof(struct qdisc_skb_cb) + sizeof(struct netem_skb_cb));
+	BUILD_BUG_ON(sizeof(skb->cb) < sizeof(struct dev_skb_cb) +
+				       sizeof(struct qdisc_skb_cb) +
+				       sizeof(struct netem_skb_cb));
 	return (struct netem_skb_cb *)qdisc_skb_cb(skb)->data;
 }
 

Re: [PATCH 5/5 v2] net: add old_queue_mapping into skb->cb

[Eric Dumazet]

Le mercredi 15 décembre 2010 à 13:23 +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.
> 
> A new struct dev_skb_cb is added, and valid in qdisc and gso layer.
> The original qdisc_skb_cb and DEV_GSO_CB are placed after dev_skb_cb.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

This is really ugly and error prone.

Could you just use a more normal way to express this ?


struct ifb_save_fields_cb {
	u16 queue_mapping;
};

struct napi_gro_cb {
	struct ifb_save_fields_cb ifb_cb; /* needed by ifb, must be first */
	...
}


struct qdisc_skb_cb {
	struct ifb_save_fields_cb ifb_cb; /* needed by ifb, must be first */
	unsigned int	pkt_len;
};

Re: [PATCH 5/5 v2] net: add old_queue_mapping into skb->cb

[Changli Gao]

On Wed, Dec 15, 2010 at 3:13 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> This is really ugly and error prone.
>
> Could you just use a more normal way to express this ?
>
>
> struct ifb_save_fields_cb {
>        u16 queue_mapping;
> };
>
> struct napi_gro_cb {
>        struct ifb_save_fields_cb ifb_cb; /* needed by ifb, must be first */
>        ...
> }
>
>
> struct qdisc_skb_cb {
>        struct ifb_save_fields_cb ifb_cb; /* needed by ifb, must be first */
>        unsigned int    pkt_len;
> };
>

It seems a better way. I'll update netem_skb_cb too. Thanks.

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

|PATCH net-next-2.6] ifb: use netif_receive_skb() instead of netif_rx()

[Eric Dumazet]

In ri_tasklet(), we run from softirq, so can directly handle packet
through netif_receive_skb() instead of netif_rx().
There is no risk of recursion.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 drivers/net/ifb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index bfa03db..a44901b 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -96,7 +96,7 @@ static void ri_tasklet(unsigned long dev)
 			dev_queue_xmit(skb);
 		} else if (from & AT_INGRESS) {
 			skb_pull(skb, skb->dev->hard_header_len);
-			netif_rx(skb);
+			netif_receive_skb(skb);
 		} else
 			BUG();
 	}

Re: |PATCH net-next-2.6] ifb: use netif_receive_skb() instead of netif_rx()

[Changli Gao]

On Wed, Dec 15, 2010 at 4:39 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> In ri_tasklet(), we run from softirq, so can directly handle packet
> through netif_receive_skb() instead of netif_rx().
> There is no risk of recursion.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---

Acked-by: Changli Gao <xiaosuo@gmail.com>


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

Re: |PATCH net-next-2.6] ifb: use netif_receive_skb() instead of netif_rx()

[jamal]

On Wed, 2010-12-15 at 09:39 +0100, Eric Dumazet wrote:
> In ri_tasklet(), we run from softirq, so can directly handle packet
> through netif_receive_skb() instead of netif_rx().
> There is no risk of recursion.

Eric, did you do at least a simple test on this one? 
It used to be problematic (I cant remember why or
what use case was problematic).

cheers,
jamal

Re: |PATCH net-next-2.6] ifb: use netif_receive_skb() instead of netif_rx()

[Eric Dumazet]

Le mercredi 15 décembre 2010 à 07:49 -0500, jamal a écrit :
> On Wed, 2010-12-15 at 09:39 +0100, Eric Dumazet wrote:
> > In ri_tasklet(), we run from softirq, so can directly handle packet
> > through netif_receive_skb() instead of netif_rx().
> > There is no risk of recursion.
> 
> Eric, did you do at least a simple test on this one? 
> It used to be problematic (I cant remember why or
> what use case was problematic).

Yes, I run SFQ / IFB right now on my dev machine, and found SFQ bugs by
the way ;)

Re: |PATCH net-next-2.6] ifb: use netif_receive_skb() instead of netif_rx()

[Stephen Hemminger]

On Wed, 15 Dec 2010 07:49:23 -0500
jamal <hadi@cyberus.ca> wrote:

> On Wed, 2010-12-15 at 09:39 +0100, Eric Dumazet wrote:
> > In ri_tasklet(), we run from softirq, so can directly handle packet
> > through netif_receive_skb() instead of netif_rx().
> > There is no risk of recursion.
> 
> Eric, did you do at least a simple test on this one? 
> It used to be problematic (I cant remember why or
> what use case was problematic).

Only risk is stack overflow.

Re: [PATCH 5/5 v2] net: add old_queue_mapping into skb->cb

[Stephen Hemminger]

On Wed, 15 Dec 2010 13:23:56 +0800
Changli Gao <xiaosuo@gmail.com> 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.
> 
> A new struct dev_skb_cb is added, and valid in qdisc and gso layer.
> The original qdisc_skb_cb and DEV_GSO_CB are placed after dev_skb_cb.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

What about a more general mechanism that lets a layer push
some amount of data onto the skb and then pop it off.
Kind of link notes to self, maybe even encode them as netlink
(except netlink messages have excess padding).

This would allow nesting, and avoid cb[] clobbering.
The existing usage of cb[] is only because there wasn't a better
solution.

-- 

Re: |PATCH net-next-2.6] ifb: use netif_receive_skb() instead of netif_rx()

[jamal]

On Wed, 2010-12-15 at 15:21 +0100, Eric Dumazet wrote:
> Le mercredi 15 décembre 2010 à 07:49 -0500, jamal a écrit :

> > Eric, did you do at least a simple test on this one? 
> > It used to be problematic (I cant remember why or
> > what use case was problematic).
> 
> Yes, I run SFQ / IFB right now on my dev machine, and found SFQ bugs by
> the way ;)

Ok, thanks;->

cheers,
jamal