Re: [PATCH] CHOKe flow scheduler (0.10)

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

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.

> +/* 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)
> +{
> +	int off1, off2, poff;
> +
> +	if (skb1->protocol != skb2->protocol)
> +		return false;
> +
> +	off1 = skb_network_offset(skb1);
> +	off2 = skb_network_offset(skb2);
> +
> +	switch (skb1->protocol) {
> +	case __constant_htons(ETH_P_IP): {
> +		const struct iphdr *ip1, *ip2;
> +
> +		ip1 = (struct iphdr *) (skb1->data + off1);
> +		ip2 = (struct iphdr *) (skb2->data + off2);
> +
> +		if (ip1->protocol != 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;
> +		}
> +		break;
> +	}
> +
> +	case __constant_htons(ETH_P_IPV6): {
> +		const struct ipv6hdr *ip1, *ip2;
> +
> +		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);

Hmm, we also need to check ports, you might just move the check from
case __constant_htons(ETH_P_IP) after the switch(skb1->protocol)


> +	}
> +
> +	default: /* Maybe compare MAC header here? */
> +		return false;
> +	}
> +	/* not reached */
> +}
> +

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

Thanks !