Re: [PATCH RFC net-next 5/7] net/ipv6: Add support for path selection using hash of 5-tuple

[Ido Schimmel <idosch@idosch.org>]

On Mon, Feb 12, 2018 at 04:06:00PM -0800, David Ahern wrote:
> Some operators prefer IPv6 path selection to use a standard 5-tuple
> hash rather than just an L3 hash with the flow the label. To that end
> add support to IPv6 for multipath hash policy similar to bf4e0a3db97eb
> ("net: ipv4: add support for ECMP hash policy choice"). The default
> is still L3 which covers source and destination addresses along with
> flow label and IPv6 protocol.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>

[...]

> @@ -1819,20 +1820,53 @@ static void ip6_multipath_l3_keys(const struct sk_buff *skb,
>  }
>  
>  /* if skb is set it will be used and fl6 can be NULL */
> -u32 rt6_multipath_hash(const struct flowi6 *fl6, const struct sk_buff *skb)
> +u32 rt6_multipath_hash(const struct net *net, const struct flowi6 *fl6,
> +		       const struct sk_buff *skb)
>  {
>  	struct flow_keys hash_keys;
>  	u32 mhash;
>  
> -	memset(&hash_keys, 0, sizeof(hash_keys));
> -	hash_keys.control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
> -	if (skb) {
> -		ip6_multipath_l3_keys(skb, &hash_keys);
> -	} else {
> -		hash_keys.addrs.v6addrs.src = fl6->saddr;
> -		hash_keys.addrs.v6addrs.dst = fl6->daddr;
> -		hash_keys.tags.flow_label = (__force u32)fl6->flowlabel;
> -		hash_keys.basic.ip_proto = fl6->flowi6_proto;
> +	switch (net->ipv6.sysctl.multipath_hash_policy) {
> +	case 0:
> +		memset(&hash_keys, 0, sizeof(hash_keys));
> +		hash_keys.control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
> +		if (skb) {
> +			ip6_multipath_l3_keys(skb, &hash_keys);
> +		} else {
> +			hash_keys.addrs.v6addrs.src = fl6->saddr;
> +			hash_keys.addrs.v6addrs.dst = fl6->daddr;
> +			hash_keys.tags.flow_label = (__force u32)fl6->flowlabel;
> +			hash_keys.basic.ip_proto = fl6->flowi6_proto;
> +		}
> +		break;
> +	case 1:
> +		if (skb) {
> +			unsigned int flag = FLOW_DISSECTOR_F_STOP_AT_ENCAP;
> +			struct flow_keys keys;
> +
> +			/* short-circuit if we already have L4 hash present */
> +			if (skb->l4_hash)
> +				return skb_get_hash_raw(skb) >> 1;
> +
> +			memset(&hash_keys, 0, sizeof(hash_keys));
> +
> +			skb_flow_dissect_flow_keys(skb, &keys, flag);
> +
> +			hash_keys.addrs.v6addrs.src = keys.addrs.v6addrs.src;
> +			hash_keys.addrs.v6addrs.dst = keys.addrs.v6addrs.dst;

Shouldn't you add:

hash_keys.control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;

?

Otherwise flow_hash_from_keys() will not consistentify the ports and the
addresses and it will also not use the addresses for the hash
computation.

It's missing from fib_multipath_hash() as well, so I might be missing
something.

> +			hash_keys.ports.src = keys.ports.src;
> +			hash_keys.ports.dst = keys.ports.dst;
> +			hash_keys.tags.flow_label = keys.tags.flow_label;

Why are you using the flow label?

> +			hash_keys.basic.ip_proto = keys.basic.ip_proto;
> +		} else {
> +			memset(&hash_keys, 0, sizeof(hash_keys));
> +			hash_keys.control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
> +			hash_keys.addrs.v6addrs.src = fl6->saddr;
> +			hash_keys.addrs.v6addrs.dst = fl6->daddr;
> +			hash_keys.ports.src = fl6->fl6_sport;
> +			hash_keys.ports.dst = fl6->fl6_dport;
> +			hash_keys.basic.ip_proto = fl6->flowi6_proto;
> +		}
>  	}
>  	mhash = flow_hash_from_keys(&hash_keys);
>  
> @@ -1858,7 +1892,7 @@ void ip6_route_input(struct sk_buff *skb)
>  	if (tun_info && !(tun_info->mode & IP_TUNNEL_INFO_TX))
>  		fl6.flowi6_tun_key.tun_id = tun_info->key.tun_id;
>  	if (unlikely(fl6.flowi6_proto == IPPROTO_ICMPV6))
> -		fl6.mp_hash = rt6_multipath_hash(&fl6, skb);
> +		fl6.mp_hash = rt6_multipath_hash(net, &fl6, skb);
>  	skb_dst_drop(skb);
>  	skb_dst_set(skb, ip6_route_input_lookup(net, skb->dev, &fl6, flags));
>  }