From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <netdev-owner@vger.kernel.org>
Received: from mail-pg0-f48.google.com ([74.125.83.48]:33326 "EHLO
        mail-pg0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S934140AbeBURyE (ORCPT
        <rfc822;netdev@vger.kernel.org>); Wed, 21 Feb 2018 12:54:04 -0500
Received: by mail-pg0-f48.google.com with SMTP id g12so934452pgs.0
        for <netdev@vger.kernel.org>; Wed, 21 Feb 2018 09:54:04 -0800 (PST)
Subject: Re: [PATCH RFC net-next 5/7] net/ipv6: Add support for path selection
 using hash of 5-tuple
To: Ido Schimmel <idosch@idosch.org>
Cc: netdev@vger.kernel.org, roopa@cumulusnetworks.com,
        nikolay@cumulusnetworks.com, idosch@mellanox.com
References: <20180213000602.12150-1-dsahern@gmail.com>
 <20180213000602.12150-6-dsahern@gmail.com>
 <20180221162259.GA6597@splinter.mtl.com>
From: David Ahern <dsahern@gmail.com>
Message-ID: <3ece390a-401c-ac13-06a0-a23a7fab1a8a@gmail.com>
Date: Wed, 21 Feb 2018 10:54:33 -0700
MIME-Version: 1.0
In-Reply-To: <20180221162259.GA6597@splinter.mtl.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 2/21/18 9:22 AM, Ido Schimmel wrote:

>> +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.

Yes, I think you are correct. The oversight here is based on the missing
set in the ipv4 variant.

> 
>> +			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?

will remove. It is supposed to be an L4 hash.