Re: [PATCH] CHOKe flow scheduler (0.10)

[Eric Dumazet <eric.dumazet@gmail.com>]

Le jeudi 20 janvier 2011 à 19:19 +0100, Eric Dumazet a écrit :
> Le jeudi 20 janvier 2011 à 09:38 -0800, Stephen Hemminger a écrit :
> 
> ...
> 
> Hi Stephen
> 
> I am contemplating choke_linearize_header() (yet another hash
> function... oh well...) and say :
> 
> Instead of this boolean return, just use skb_get_rxhash(). It performs
> what you want, and return 32bits of information, instead of one bit.
> 
> (if result (rxhash) is zero, it means you can drop packet because it is
> not linearized)
> 
> So choke_match_flow() can be faster if rxhash differs (no cache miss on
> skb->data on an old skb)
> (Do the full check only if rxhash is equal on skb1 / skb2)
> 
> More over, the skb_get_rxhash() call is _really_ needed only if CHOKe
> triggers :
> if (p->qavg <= p->qth_min)
> 	we dont have to compute rxhash at all.
> 

> If you want, I can send a diff on your v0.10, just tell me !

Here is my diff against v 0.10

Seems to work well here

qdisc choke 11: parent 1:11 limit 130000b min 10833b max 32500b ewma 13 Plog 21 Scell_log 30
 Sent 459876256 bytes 836183 pkt (dropped 318428, overlimits 28 requeues 0) 
 rate 51823Kbit 11779pps backlog 5926323b 10781p requeues 0 
  marked 0 early 28 pdrop 0 other 0 matched 159200

Thanks !


 net/sched/sch_choke.c |  114 ++++++++++++----------------------------
 1 file changed, 37 insertions(+), 77 deletions(-)

diff --git a/net/sched/sch_choke.c b/net/sched/sch_choke.c
index e1ac560..0e898ed 100644
--- a/net/sched/sch_choke.c
+++ b/net/sched/sch_choke.c
@@ -141,63 +141,24 @@ static void choke_drop_by_idx(struct Qdisc *sch, unsigned int idx)
 	--sch->q.qlen;
 }
 
-/* Make sure network and transport headers can be dereferenced */
-static bool choke_linearize_header(struct sk_buff *skb)
-{
-	int nhoff, poff;
-	struct ipv6hdr *ip6;
-	struct iphdr *ip;
-	u8 ip_proto;
-	u32 ihl;
-
-	nhoff = skb_network_offset(skb);
-
-	switch (skb->protocol) {
-	case __constant_htons(ETH_P_IP):
-		if (!pskb_may_pull(skb, sizeof(*ip) + nhoff))
-			return false;
-
-		ip = (struct iphdr *) (skb->data + nhoff);
-		if (ip->frag_off & htons(IP_MF | IP_OFFSET))
-			return false;
-
-		ip_proto = ip->protocol;
-		ihl = ip->ihl;
-		break;
-	case __constant_htons(ETH_P_IPV6):
-		if (!pskb_may_pull(skb, sizeof(*ip6) + nhoff))
-			return false;
-
-		ip6 = (struct ipv6hdr *) (skb->data + nhoff);
-		ip_proto = ip6->nexthdr;
-		ihl = (40 >> 2);
-		break;
-	default:
-		return true;
-	}
-
-	poff = proto_ports_offset(ip_proto);
-	if (poff >= 0) {
-		nhoff += ihl * 4 + poff;
-		if (!pskb_may_pull(skb, nhoff + 4))
-			return false;
-	}
-
-	return true;
-}
-
 /*
  * Compare flow of two packets
  *  Returns true only if source and destination address and port match.
  *          false for special cases
  */
-static bool choke_match_flow(const struct sk_buff *skb1,
-			     const struct sk_buff *skb2)
+static bool choke_match_flow(struct sk_buff *skb1,
+			     struct sk_buff *skb2)
 {
 	int off1, off2, poff;
+	const u32 *ports1, *ports2;
+	u8 ip_proto;
 
 	if (skb1->protocol != skb2->protocol)
 		return false;
+	if (skb_get_rxhash(skb1) != skb_get_rxhash(skb2))
+		return false;
+	if (!skb_get_rxhash(skb1))
+		return true;
 
 	off1 = skb_network_offset(skb1);
 	off2 = skb_network_offset(skb2);
@@ -209,27 +170,14 @@ static bool choke_match_flow(const struct sk_buff *skb1,
 		ip1 = (struct iphdr *) (skb1->data + off1);
 		ip2 = (struct iphdr *) (skb2->data + off2);
 
-		if (ip1->protocol != ip2->protocol ||
+		ip_proto = ip1->protocol;
+		if (ip_proto != ip2->protocol ||
 		    ip1->saddr != ip2->saddr || ip1->daddr != ip2->daddr)
 			return false;
-
-		if (ip1->frag_off & htons(IP_MF | IP_OFFSET) ||
-		    ip2->frag_off & htons(IP_MF | IP_OFFSET))
-			return ip1->id == ip2->id;
-
-		poff = proto_ports_offset(ip1->protocol);
-		if (poff < 0)
-			return true;
-		else {
-			const u32 *ports1, *ports2;
-
-			ports1 = (__force u32 *) (skb1->data + off1
-						  + ip1->ihl * 4 + poff);
-			ports2 = (__force u32 *) (skb2->data + off2
-						  + ip2->ihl * 4 + poff);
-
-			return *ports1 == *ports2;
-		}
+		if ((ip1->frag_off | ip2->frag_off) & htons(IP_MF | IP_OFFSET))
+			ip_proto = 0;
+		off1 += ip1->ihl * 4;
+		off2 += ip2->ihl * 4;
 		break;
 	}
 
@@ -239,15 +187,32 @@ static bool choke_match_flow(const struct sk_buff *skb1,
 		ip1 = (struct ipv6hdr *) (skb1->data + off1);
 		ip2 = (struct ipv6hdr *) (skb2->data + off2);
 
-		return (ip1->nexthdr == ip2->nexthdr &&
-			ipv6_addr_cmp(&ip1->saddr, &ip2->saddr) == 0 &&
-			ipv6_addr_cmp(&ip1->daddr, &ip2->daddr) == 0);
+		ip_proto = ip1->nexthdr;
+		if (ip_proto != ip2->nexthdr ||
+		    ipv6_addr_cmp(&ip1->saddr, &ip2->saddr) ||
+		    ipv6_addr_cmp(&ip1->daddr, &ip2->daddr))
+			return false;
+		off1 += 40;
+		off2 += 40;
 	}
 
 	default: /* Maybe compare MAC header here? */
 		return false;
 	}
-	/* not reached */
+	poff = proto_ports_offset(ip_proto);
+	if (poff < 0)
+		return true;
+	off1 += poff;
+	off2 += poff;
+	if (!pskb_may_pull(skb1, off1 + 4))
+		return false;
+
+	if (!pskb_may_pull(skb2, off2 + 4))
+		return false;
+
+	ports1 = (__force u32 *)(skb1->data + off1);
+	ports2 = (__force u32 *)(skb2->data + off2);
+	return *ports1 == *ports2;
 }
 
 static inline void choke_set_classid(struct sk_buff *skb, u16 classid)
@@ -319,10 +284,10 @@ static struct sk_buff *choke_peek_random(const struct choke_sched_data *q,
  * returns true if matched and sets *pidx
  */
 static bool choke_match_random(const struct choke_sched_data *q,
-			       const struct sk_buff *nskb,
+			       struct sk_buff *nskb,
 			       unsigned int *pidx)
 {
-	const struct sk_buff *oskb;
+	struct sk_buff *oskb;
 
 	if (q->head == q->tail)
 		return false;
@@ -331,7 +296,6 @@ static bool choke_match_random(const struct choke_sched_data *q,
 	if (q->filter_list)
 		return choke_get_classid(nskb) == choke_get_classid(oskb);
 
-
 	return choke_match_flow(oskb, nskb);
 }
 
@@ -345,10 +309,6 @@ static int choke_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 		/* If using external classifiers, get result and record it. */
 		if (!choke_classify(skb, sch, &ret))
 			goto other_drop;	/* Packet was eaten by filter */
-	} else {
-		/* for internal classifier, make header accessible */
-		if (!choke_linearize_header(skb))
-			goto other_drop;
 	}
 
 	/* Compute average queue usage (see RED) */