Skip to contentSkip to conversation

act_mirred: cleanup and optimization

PATCH In review Changli Gao 3 participants 5 messages Nov 13, 2009 – Nov 15, 2009

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Changli Gao

author
#0 lore raw

PATCH 1/1 act_mirred: cleanup and optimization

act_mirred: cleanup and optimization.

cleanup and optimization.
1. don't let go back using goto.
2. move checking if eaction is valid in tcf_mirred_init().
3. don't call skb_act_clone() until it is necessary.
4. one exit of the critical context.
5. allow eaction is TCA_INGRESS_MIRROR & TCA_INGRESS_REDIR.

Signed-off-by Changli Gao <[email protected]>

----
net/sched/act_mirred.c | 121
++++++++++++++++++++++++++-----------------------

1 file changed, 65 insertions(+), 56 deletions(-)
net/sched/act_mirred.c +65 -56
index b9aaab4..1e8b042 100644
@@ -8,8 +8,6 @@
  *
  * Authors:	Jamal Hadi Salim (2002-4)
  *
- * TODO: Add ingress support (and socket redirect support)
- *
  */
 
 #include <linux/types.h>

@@ -65,52 +63,63 @@ static int tcf_mirred_init(struct nlattr *nla, struct nlattr *est,
 	struct tc_mirred *parm;
 	struct tcf_mirred *m;
 	struct tcf_common *pc;
-	struct net_device *dev = NULL;
-	int ret = 0, err;
-	int ok_push = 0;
+	struct net_device *dev;
+	int ret, ok_push = 0;
 
 	if (nla == NULL)
 		return -EINVAL;
-
-	err = nla_parse_nested(tb, TCA_MIRRED_MAX, nla, mirred_policy);
-	if (err < 0)
-		return err;
-
+	ret = nla_parse_nested(tb, TCA_MIRRED_MAX, nla, mirred_policy);
+	if (ret < 0)
+		return ret;
 	if (tb[TCA_MIRRED_PARMS] == NULL)
 		return -EINVAL;
 	parm = nla_data(tb[TCA_MIRRED_PARMS]);
-
+	switch (parm->eaction) {
+	case TCA_EGRESS_MIRROR:
+	case TCA_EGRESS_REDIR:
+	case TCA_INGRESS_MIRROR:
+	case TCA_INGRESS_REDIR:
+		break;
+	default:
+		return -EINVAL;
+	}
 	if (parm->ifindex) {
-		dev = __dev_get_by_index(&init_net, parm->ifindex);
+		dev = dev_get_by_index(&init_net, parm->ifindex);
 		if (dev == NULL)
 			return -ENODEV;
 		switch (dev->type) {
-			case ARPHRD_TUNNEL:
-			case ARPHRD_TUNNEL6:
-			case ARPHRD_SIT:
-			case ARPHRD_IPGRE:
-			case ARPHRD_VOID:
-			case ARPHRD_NONE:
-				ok_push = 0;
-				break;
-			default:
-				ok_push = 1;
-				break;
+		case ARPHRD_TUNNEL:
+		case ARPHRD_TUNNEL6:
+		case ARPHRD_SIT:
+		case ARPHRD_IPGRE:
+		case ARPHRD_VOID:
+		case ARPHRD_NONE:
+			ok_push = 0;
+			break;
+		default:
+			ok_push = 1;
+			break;
 		}
+	} else {
+		dev = NULL;
 	}
 
 	pc = tcf_hash_check(parm->index, a, bind, &mirred_hash_info);
 	if (!pc) {
-		if (!parm->ifindex)
+		if (dev == NULL)
 			return -EINVAL;
 		pc = tcf_hash_create(parm->index, est, a, sizeof(*m), bind,
 				     &mirred_idx_gen, &mirred_hash_info);
-		if (IS_ERR(pc))
-		    return PTR_ERR(pc);
+		if (IS_ERR(pc)) {
+			dev_put(dev);
+			return PTR_ERR(pc);
+		}
 		ret = ACT_P_CREATED;
 	} else {
 		if (!ovr) {
 			tcf_mirred_release(to_mirred(pc), bind);
+			if (dev != NULL)
+				dev_put(dev);
 			return -EEXIST;
 		}
 	}

@@ -119,12 +128,11 @@ static int tcf_mirred_init(struct nlattr *nla, struct nlattr *est,
 	spin_lock_bh(&m->tcf_lock);
 	m->tcf_action = parm->action;
 	m->tcfm_eaction = parm->eaction;
-	if (parm->ifindex) {
+	if (dev != NULL) {
 		m->tcfm_ifindex = parm->ifindex;
 		if (ret != ACT_P_CREATED)
 			dev_put(m->tcfm_dev);
 		m->tcfm_dev = dev;
-		dev_hold(dev);
 		m->tcfm_ok_push = ok_push;
 	}
 	spin_unlock_bh(&m->tcf_lock);

@@ -146,59 +154,60 @@ static int tcf_mirred_cleanup(struct tc_action *a, int bind)
 static int tcf_mirred(struct sk_buff *skb, struct tc_action *a,
 		      struct tcf_result *res)
 {
-	struct tcf_mirred *m = a->priv;
 	struct net_device *dev;
-	struct sk_buff *skb2 = NULL;
-	u32 at = G_TC_AT(skb->tc_verd);
+	struct sk_buff *skb2;
+	u32 at;
+	struct tcf_mirred *m = a->priv;
+	int retval, err = 1;
 
 	spin_lock(&m->tcf_lock);
-
-	dev = m->tcfm_dev;
 	m->tcf_tm.lastuse = jiffies;
 
-	if (!(dev->flags&IFF_UP) ) {
+	dev = m->tcfm_dev;
+	if (!(dev->flags & IFF_UP) ) {
 		if (net_ratelimit())
 			printk("mirred to Houston: device %s is gone!\n",
 			       dev->name);
-bad_mirred:
-		if (skb2 != NULL)
-			kfree_skb(skb2);
-		m->tcf_qstats.overlimits++;
-		m->tcf_bstats.bytes += qdisc_pkt_len(skb);
-		m->tcf_bstats.packets++;
-		spin_unlock(&m->tcf_lock);
-		/* should we be asking for packet to be dropped?
-		 * may make sense for redirect case only
-		*/
-		return TC_ACT_SHOT;
+		goto out;
 	}
 
 	skb2 = skb_act_clone(skb, GFP_ATOMIC);
 	if (skb2 == NULL)
-		goto bad_mirred;
-	if (m->tcfm_eaction != TCA_EGRESS_MIRROR &&
-	    m->tcfm_eaction != TCA_EGRESS_REDIR) {
-		if (net_ratelimit())
-			printk("tcf_mirred unknown action %d\n",
-			       m->tcfm_eaction);
-		goto bad_mirred;
-	}
+		goto out;
 
 	m->tcf_bstats.bytes += qdisc_pkt_len(skb2);
 	m->tcf_bstats.packets++;
-	if (!(at & AT_EGRESS))
+	at = G_TC_AT(skb->tc_verd);
+	if (!(at & AT_EGRESS)) {
 		if (m->tcfm_ok_push)
 			skb_push(skb2, skb2->dev->hard_header_len);
+	}
 
 	/* mirror is always swallowed */
-	if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
+	if (m->tcfm_eaction != TCA_EGRESS_MIRROR &&
+	    m->tcfm_eaction != TCA_INGRESS_MIRROR)
 		skb2->tc_verd = SET_TC_FROM(skb2->tc_verd, at);
 
 	skb2->dev = dev;
 	skb2->iif = skb->dev->ifindex;
 	dev_queue_xmit(skb2);
+	err = 0;
+
+out:
+	if (err) {
+		m->tcf_qstats.overlimits++;
+		m->tcf_bstats.bytes += qdisc_pkt_len(skb);
+		m->tcf_bstats.packets++;
+		/* should we be asking for packet to be dropped?
+		 * may make sense for redirect case only
+		 */
+		retval = TC_ACT_SHOT;
+	} else {
+		retval = m->tcf_action;
+	}
 	spin_unlock(&m->tcf_lock);
-	return m->tcf_action;
+
+	return retval;
 }
 
 static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
#1 lore raw
Quoted 10 lines · Changli Gao

act_mirred: cleanup and optimization.

cleanup and optimization.
1. don't let go back using goto.
2. move checking if eaction is valid in tcf_mirred_init().
3. don't call skb_act_clone() until it is necessary.
4. one exit of the critical context.
5. allow eaction is TCA_INGRESS_MIRROR & TCA_INGRESS_REDIR.

net/sched/act_mirred.c static int tcf_mirred_init(struct nlattr *nla, struct nlattr *est, · line 89

Quoted 5 lines
net/sched/act_mirred.c +1 -1
 	if (parm->ifindex) {
-		dev = __dev_get_by_index(&init_net, parm->ifindex);
+		dev = dev_get_by_index(&init_net, parm->ifindex);
 		if (dev == NULL)
 			return -ENODEV;

This change is not mentioned in the changelog and introduces a leak. Please split your patches into reasonable portions doing one change at a time.

author
#2 lore raw
Quoted 17 lines · Patrick McHardy
Quoted 10 lines · Changli Gao

act_mirred: cleanup and optimization.

cleanup and optimization.
1. don't let go back using goto.
2. move checking if eaction is valid in tcf_mirred_init().
3. don't call skb_act_clone() until it is necessary.
4. one exit of the critical context.
5. allow eaction is TCA_INGRESS_MIRROR & TCA_INGRESS_REDIR.

Quoted 5 lines

      if (parm->ifindex) {
-             dev = __dev_get_by_index(&init_net, parm->ifindex);
+             dev = dev_get_by_index(&init_net, parm->ifindex); if (dev == NULL)
                      return -ENODEV;

Quoted 4 lines

This change is not mentioned in the changelog and introduces a leak. Please split your patches into reasonable portions doing one change at a time.

Leak? Do I forget to put it some where? I'll keep it as a whole until Jamal thinks it is in the right form and direction.

Signature · 2 lines
Regards，
Changli Gao([email protected])
#3 lore raw
Quoted 16 lines · Changli Gao
Quoted 15 lines · Patrick McHardy
Quoted 14 lines · Changli Gao

act_mirred: cleanup and optimization.

cleanup and optimization.
1. don't let go back using goto.
2. move checking if eaction is valid in tcf_mirred_init().
3. don't call skb_act_clone() until it is necessary.
4. one exit of the critical context.
5. allow eaction is TCA_INGRESS_MIRROR & TCA_INGRESS_REDIR.

      if (parm->ifindex) {
net/sched/act_mirred.c +1 -1
-             dev = __dev_get_by_index(&init_net, parm->ifindex);
+             dev = dev_get_by_index(&init_net, parm->ifindex);
              if (dev == NULL)
                      return -ENODEV;
Quoted 7 lines
Quoted 4 lines

This change is not mentioned in the changelog and introduces a leak. Please split your patches into reasonable portions doing one change at a time.

Leak? Do I forget to put it some where? I'll keep it as a whole until Jamal thinks it is in the right form and direction.

Ah sorry, I misread the patch.

#4 lore raw
In reply to Changli Gao
Quoted 9 lines · Changli Gao

act_mirred: cleanup and optimization.

cleanup and optimization.
1. don't let go back using goto.
2. move checking if eaction is valid in tcf_mirred_init().
3. don't call skb_act_clone() until it is necessary.
4. one exit of the critical context.
5. allow eaction is TCA_INGRESS_MIRROR & TCA_INGRESS_REDIR.

I would break this into the following patches:
patch 1: #1, #3, and #4 (this is what we discussed before) patch 2: #2 (Ive had this in my todo list forever)

For #5 i think you misunderstood me earlier - TCA_INGRESS_* means not to do dev xmit (that is the domain of TCA_EGRESS_*) but rather something along the lines of netif_rx; this needs a lot of validation - the code has changed quiet a bit since the early days and as it has turned out noone has exactly been shedding any tears for that feature. The challenge is in the variety of netdevices which have different semantics..

Sorry - I am in some hectic travel mode (hence delayed response) but to answer your earlier question: no you cant redirect to packet socket today.

cheers,
jamal