Re: [RFC PATCH net-next v2 03/10] ipv4: Add custom multipath hash policy

[Ido Schimmel <idosch@idosch.org>]

On Tue, May 11, 2021 at 09:46:27AM -0600, David Ahern wrote:
> On 5/9/21 9:16 AM, Ido Schimmel wrote:
> > diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> > index 9d61e969446e..a4c477475f4c 100644
> > --- a/net/ipv4/route.c
> > +++ b/net/ipv4/route.c
> > @@ -1906,6 +1906,121 @@ static void ip_multipath_l3_keys(const struct sk_buff *skb,
> >  	hash_keys->addrs.v4addrs.dst = key_iph->daddr;
> >  }
> >  
> > +static u32 fib_multipath_custom_hash_outer(const struct net *net,
> > +					   const struct sk_buff *skb,
> > +					   bool *p_has_inner)
> > +{
> > +	u32 hash_fields = net->ipv4.sysctl_fib_multipath_hash_fields;
> > +	struct flow_keys keys, hash_keys;
> > +
> > +	if (!(hash_fields & FIB_MULTIPATH_HASH_FIELD_OUTER_MASK))
> > +		return 0;
> > +
> > +	memset(&hash_keys, 0, sizeof(hash_keys));
> > +	skb_flow_dissect_flow_keys(skb, &keys, FLOW_DISSECTOR_F_STOP_AT_ENCAP);
> > +
> > +	hash_keys.control.addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
> > +	if (hash_fields & FIB_MULTIPATH_HASH_FIELD_SRC_IP)
> > +		hash_keys.addrs.v4addrs.src = keys.addrs.v4addrs.src;
> > +	if (hash_fields & FIB_MULTIPATH_HASH_FIELD_DST_IP)
> > +		hash_keys.addrs.v4addrs.dst = keys.addrs.v4addrs.dst;
> > +	if (hash_fields & FIB_MULTIPATH_HASH_FIELD_IP_PROTO)
> > +		hash_keys.basic.ip_proto = keys.basic.ip_proto;
> > +	if (hash_fields & FIB_MULTIPATH_HASH_FIELD_SRC_PORT)
> > +		hash_keys.ports.src = keys.ports.src;
> > +	if (hash_fields & FIB_MULTIPATH_HASH_FIELD_DST_PORT)
> > +		hash_keys.ports.dst = keys.ports.dst;
> > +
> > +	*p_has_inner = !!(keys.control.flags & FLOW_DIS_ENCAPSULATION);
> > +	return flow_hash_from_keys(&hash_keys);
> > +}
> > +
> > +static u32 fib_multipath_custom_hash_inner(const struct net *net,
> > +					   const struct sk_buff *skb,
> > +					   bool has_inner)
> > +{
> > +	u32 hash_fields = net->ipv4.sysctl_fib_multipath_hash_fields;
> > +	struct flow_keys keys, hash_keys;
> > +
> > +	/* We assume the packet carries an encapsulation, but if none was
> > +	 * encountered during dissection of the outer flow, then there is no
> > +	 * point in calling the flow dissector again.
> > +	 */
> > +	if (!has_inner)
> > +		return 0;
> > +
> > +	if (!(hash_fields & FIB_MULTIPATH_HASH_FIELD_INNER_MASK))
> > +		return 0;
> > +
> > +	memset(&hash_keys, 0, sizeof(hash_keys));
> > +	skb_flow_dissect_flow_keys(skb, &keys, 0);
> > +
> > +	if (!(keys.control.flags & FLOW_DIS_ENCAPSULATION))
> > +		return 0;
> > +
> > +	if (keys.control.addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS) {
> > +		hash_keys.control.addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
> > +		if (hash_fields & FIB_MULTIPATH_HASH_FIELD_INNER_SRC_IP)
> > +			hash_keys.addrs.v4addrs.src = keys.addrs.v4addrs.src;
> > +		if (hash_fields & FIB_MULTIPATH_HASH_FIELD_INNER_DST_IP)
> > +			hash_keys.addrs.v4addrs.dst = keys.addrs.v4addrs.dst;
> > +	} else if (keys.control.addr_type == FLOW_DISSECTOR_KEY_IPV6_ADDRS) {
> > +		hash_keys.control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
> > +		if (hash_fields & FIB_MULTIPATH_HASH_FIELD_INNER_SRC_IP)
> > +			hash_keys.addrs.v6addrs.src = keys.addrs.v6addrs.src;
> > +		if (hash_fields & FIB_MULTIPATH_HASH_FIELD_INNER_DST_IP)
> > +			hash_keys.addrs.v6addrs.dst = keys.addrs.v6addrs.dst;
> > +		if (hash_fields & FIB_MULTIPATH_HASH_FIELD_INNER_FLOWLABEL)
> > +			hash_keys.tags.flow_label = keys.tags.flow_label;
> > +	}
> > +
> > +	if (hash_fields & FIB_MULTIPATH_HASH_FIELD_INNER_IP_PROTO)
> > +		hash_keys.basic.ip_proto = keys.basic.ip_proto;
> > +	if (hash_fields & FIB_MULTIPATH_HASH_FIELD_INNER_SRC_PORT)
> > +		hash_keys.ports.src = keys.ports.src;
> > +	if (hash_fields & FIB_MULTIPATH_HASH_FIELD_INNER_DST_PORT)
> > +		hash_keys.ports.dst = keys.ports.dst;
> > +
> > +	return flow_hash_from_keys(&hash_keys);
> > +}
> > +
> > +static u32 fib_multipath_custom_hash_skb(const struct net *net,
> > +					 const struct sk_buff *skb)
> > +{
> > +	u32 mhash, mhash_inner;
> > +	bool has_inner = true;
> > +
> 
> Is it not possible to do the dissect once here and pass keys to outer
> and inner functions?
> 
> 	memset(&hash_keys, 0, sizeof(hash_keys));
> 	skb_flow_dissect_flow_keys(skb, &keys, flag);

Not that I'm aware. For outer flow we need to pass
'FLOW_DISSECTOR_F_STOP_AT_ENCAP'. For inner flow, we shouldn't pass any
flags, but make sure encapsulation was encountered by checking
'keys.control.flags & FLOW_DIS_ENCAPSULATION'.

Also, 'struct flow_keys' has keys for a single flow.

> 
> 
> > +	mhash = fib_multipath_custom_hash_outer(net, skb, &has_inner);
> > +	mhash_inner = fib_multipath_custom_hash_inner(net, skb, has_inner);
> > +
> > +	return jhash_2words(mhash, mhash_inner, 0);
> > +}
> > +
> > +static u32 fib_multipath_custom_hash_fl4(const struct net *net,
> > +					 const struct flowi4 *fl4)
> > +{
> > +	u32 hash_fields = net->ipv4.sysctl_fib_multipath_hash_fields;
> > +	struct flow_keys hash_keys;
> > +
> > +	if (!(hash_fields & FIB_MULTIPATH_HASH_FIELD_OUTER_MASK))
> > +		return 0;
> > +
> > +	memset(&hash_keys, 0, sizeof(hash_keys));
> > +	hash_keys.control.addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
> > +	if (hash_fields & FIB_MULTIPATH_HASH_FIELD_SRC_IP)
> > +		hash_keys.addrs.v4addrs.src = fl4->saddr;
> > +	if (hash_fields & FIB_MULTIPATH_HASH_FIELD_DST_IP)
> > +		hash_keys.addrs.v4addrs.dst = fl4->daddr;
> > +	if (hash_fields & FIB_MULTIPATH_HASH_FIELD_IP_PROTO)
> > +		hash_keys.basic.ip_proto = fl4->flowi4_proto;
> > +	if (hash_fields & FIB_MULTIPATH_HASH_FIELD_SRC_PORT)
> > +		hash_keys.ports.src = fl4->fl4_sport;
> > +	if (hash_fields & FIB_MULTIPATH_HASH_FIELD_DST_PORT)
> > +		hash_keys.ports.dst = fl4->fl4_dport;
> > +
> > +	return flow_hash_from_keys(&hash_keys);
> > +}
> > +
> >  /* if skb is set it will be used and fl4 can be NULL */
> >  int fib_multipath_hash(const struct net *net, const struct flowi4 *fl4,
> >  		       const struct sk_buff *skb, struct flow_keys *flkeys)