flow_dissector: save computed hash in __skb_get_hash_symmetric_net()

[PATCH net-next] flow_dissector: save computed hash in __skb_get_hash_symmetric_net()

Posted by Jon Kohler 6 days, 5 hours ago

tun.c changed from skb_get_hash() to __skb_get_hash_symmetric() on
commit feec084a7cf4 ("tun: use symmetric hash"), which exposes an
overhead for OVS datapath, where ovs_dp_process_packet() has to
calculate the hash again because __skb_get_hash_symmetric() does not
retain the hash that it calculates.

Save the computed hash in __skb_get_hash_symmetric_net() so that the
calcuation work does not go to waste.

Fixes: feec084a7cf4 ("tun: use symmetric hash")
Signed-off-by: Jon Kohler <jon@nutanix.com>
---
 include/linux/skbuff.h    | 4 ++--
 net/core/flow_dissector.c | 7 +++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index ff90281ddf90..f58afa49a50e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1568,9 +1568,9 @@ __skb_set_sw_hash(struct sk_buff *skb, __u32 hash, bool is_l4)
 	__skb_set_hash(skb, hash, true, is_l4);
 }
 
-u32 __skb_get_hash_symmetric_net(const struct net *net, const struct sk_buff *skb);
+u32 __skb_get_hash_symmetric_net(const struct net *net, struct sk_buff *skb);
 
-static inline u32 __skb_get_hash_symmetric(const struct sk_buff *skb)
+static inline u32 __skb_get_hash_symmetric(struct sk_buff *skb)
 {
 	return __skb_get_hash_symmetric_net(NULL, skb);
 }
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 1b61bb25ba0e..4a74dcc4799c 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1864,9 +1864,10 @@ EXPORT_SYMBOL(make_flow_keys_digest);
 
 static struct flow_dissector flow_keys_dissector_symmetric __read_mostly;
 
-u32 __skb_get_hash_symmetric_net(const struct net *net, const struct sk_buff *skb)
+u32 __skb_get_hash_symmetric_net(const struct net *net, struct sk_buff *skb)
 {
 	struct flow_keys keys;
+	u32 flow_hash;
 
 	__flow_hash_secret_init();
 
@@ -1874,7 +1875,9 @@ u32 __skb_get_hash_symmetric_net(const struct net *net, const struct sk_buff *sk
 	__skb_flow_dissect(net, skb, &flow_keys_dissector_symmetric,
 			   &keys, NULL, 0, 0, 0, 0);
 
-	return __flow_hash_from_keys(&keys, &hashrnd);
+	flow_hash = __flow_hash_from_keys(&keys, &hashrnd);
+	__skb_set_sw_hash(skb, flow_hash, flow_keys_have_l4(&keys));
+	return flow_hash;
 }
 EXPORT_SYMBOL_GPL(__skb_get_hash_symmetric_net);
 
-- 
2.43.0

Re: [PATCH net-next] flow_dissector: save computed hash in __skb_get_hash_symmetric_net()

Posted by Ido Schimmel 5 days, 15 hours ago

On Tue, Nov 25, 2025 at 11:19:27AM -0700, Jon Kohler wrote:
> tun.c changed from skb_get_hash() to __skb_get_hash_symmetric() on
> commit feec084a7cf4 ("tun: use symmetric hash"), which exposes an
> overhead for OVS datapath, where ovs_dp_process_packet() has to
> calculate the hash again because __skb_get_hash_symmetric() does not
> retain the hash that it calculates.

This seems to be intentional according to commit eb70db875671 ("packet:
Use symmetric hash for PACKET_FANOUT_HASH."): "This hash does not get
installed into and override the normal skb hash, so this change has no
effect whatsoever on the rest of the stack."

> 
> Save the computed hash in __skb_get_hash_symmetric_net() so that the
> calcuation work does not go to waste.
> 
> Fixes: feec084a7cf4 ("tun: use symmetric hash")
> Signed-off-by: Jon Kohler <jon@nutanix.com>
> ---
>  include/linux/skbuff.h    | 4 ++--
>  net/core/flow_dissector.c | 7 +++++--
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index ff90281ddf90..f58afa49a50e 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1568,9 +1568,9 @@ __skb_set_sw_hash(struct sk_buff *skb, __u32 hash, bool is_l4)
>  	__skb_set_hash(skb, hash, true, is_l4);
>  }
>  
> -u32 __skb_get_hash_symmetric_net(const struct net *net, const struct sk_buff *skb);
> +u32 __skb_get_hash_symmetric_net(const struct net *net, struct sk_buff *skb);
>  
> -static inline u32 __skb_get_hash_symmetric(const struct sk_buff *skb)
> +static inline u32 __skb_get_hash_symmetric(struct sk_buff *skb)
>  {
>  	return __skb_get_hash_symmetric_net(NULL, skb);
>  }
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 1b61bb25ba0e..4a74dcc4799c 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -1864,9 +1864,10 @@ EXPORT_SYMBOL(make_flow_keys_digest);
>  
>  static struct flow_dissector flow_keys_dissector_symmetric __read_mostly;
>  
> -u32 __skb_get_hash_symmetric_net(const struct net *net, const struct sk_buff *skb)
> +u32 __skb_get_hash_symmetric_net(const struct net *net, struct sk_buff *skb)
>  {
>  	struct flow_keys keys;
> +	u32 flow_hash;
>  
>  	__flow_hash_secret_init();
>  
> @@ -1874,7 +1875,9 @@ u32 __skb_get_hash_symmetric_net(const struct net *net, const struct sk_buff *sk
>  	__skb_flow_dissect(net, skb, &flow_keys_dissector_symmetric,
>  			   &keys, NULL, 0, 0, 0, 0);
>  
> -	return __flow_hash_from_keys(&keys, &hashrnd);
> +	flow_hash = __flow_hash_from_keys(&keys, &hashrnd);
> +	__skb_set_sw_hash(skb, flow_hash, flow_keys_have_l4(&keys));
> +	return flow_hash;
>  }
>  EXPORT_SYMBOL_GPL(__skb_get_hash_symmetric_net);
>  
> -- 
> 2.43.0
>

Re: [PATCH net-next] flow_dissector: save computed hash in __skb_get_hash_symmetric_net()

Posted by Jon Kohler 5 days, 7 hours ago


> On Nov 26, 2025, at 2:45 AM, Ido Schimmel <idosch@nvidia.com> wrote:
> 
> On Tue, Nov 25, 2025 at 11:19:27AM -0700, Jon Kohler wrote:
>> tun.c changed from skb_get_hash() to __skb_get_hash_symmetric() on
>> commit feec084a7cf4 ("tun: use symmetric hash"), which exposes an
>> overhead for OVS datapath, where ovs_dp_process_packet() has to
>> calculate the hash again because __skb_get_hash_symmetric() does not
>> retain the hash that it calculates.
> 
> This seems to be intentional according to commit eb70db875671 ("packet:
> Use symmetric hash for PACKET_FANOUT_HASH."): "This hash does not get
> installed into and override the normal skb hash, so this change has no
> effect whatsoever on the rest of the stack."

Ah! Good eye

What about a variant of this patch that had an arg like:
__skb_get_hash_symmetric(struct sk_buff *skb, bool save_hash)

Then we just make calls (like tun) opt in?

> 
>> 
>> Save the computed hash in __skb_get_hash_symmetric_net() so that the
>> calcuation work does not go to waste.
>> 
>> Fixes: feec084a7cf4 ("tun: use symmetric hash")
>> Signed-off-by: Jon Kohler <jon@nutanix.com>
>> ---
>> include/linux/skbuff.h    | 4 ++--
>> net/core/flow_dissector.c | 7 +++++--
>> 2 files changed, 7 insertions(+), 4 deletions(-)
>> 
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index ff90281ddf90..f58afa49a50e 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -1568,9 +1568,9 @@ __skb_set_sw_hash(struct sk_buff *skb, __u32 hash, bool is_l4)
>> __skb_set_hash(skb, hash, true, is_l4);
>> }
>> 
>> -u32 __skb_get_hash_symmetric_net(const struct net *net, const struct sk_buff *skb);
>> +u32 __skb_get_hash_symmetric_net(const struct net *net, struct sk_buff *skb);
>> 
>> -static inline u32 __skb_get_hash_symmetric(const struct sk_buff *skb)
>> +static inline u32 __skb_get_hash_symmetric(struct sk_buff *skb)
>> {
>> return __skb_get_hash_symmetric_net(NULL, skb);
>> }
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index 1b61bb25ba0e..4a74dcc4799c 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -1864,9 +1864,10 @@ EXPORT_SYMBOL(make_flow_keys_digest);
>> 
>> static struct flow_dissector flow_keys_dissector_symmetric __read_mostly;
>> 
>> -u32 __skb_get_hash_symmetric_net(const struct net *net, const struct sk_buff *skb)
>> +u32 __skb_get_hash_symmetric_net(const struct net *net, struct sk_buff *skb)
>> {
>> struct flow_keys keys;
>> + u32 flow_hash;
>> 
>> __flow_hash_secret_init();
>> 
>> @@ -1874,7 +1875,9 @@ u32 __skb_get_hash_symmetric_net(const struct net *net, const struct sk_buff *sk
>> __skb_flow_dissect(net, skb, &flow_keys_dissector_symmetric,
>>   &keys, NULL, 0, 0, 0, 0);
>> 
>> - return __flow_hash_from_keys(&keys, &hashrnd);
>> + flow_hash = __flow_hash_from_keys(&keys, &hashrnd);
>> + __skb_set_sw_hash(skb, flow_hash, flow_keys_have_l4(&keys));
>> + return flow_hash;
>> }
>> EXPORT_SYMBOL_GPL(__skb_get_hash_symmetric_net);
>> 
>> -- 
>> 2.43.0
>>

Re: [PATCH net-next] flow_dissector: save computed hash in __skb_get_hash_symmetric_net()

Posted by Ido Schimmel 4 days, 15 hours ago

On Wed, Nov 26, 2025 at 04:21:33PM +0000, Jon Kohler wrote:
> What about a variant of this patch that had an arg like:
> __skb_get_hash_symmetric(struct sk_buff *skb, bool save_hash)
> 
> Then we just make calls (like tun) opt in?

It will require changes in all the callers and I am not sure it's wise
to change a common function for a single user. Why not just patch tun to
call __skb_set_sw_hash(skb, hash, true)? IIUC, even in tun you only need
it in two out of the four callers of __skb_get_hash_symmetric():
tun_get_user() and tun_xdp_one() which both build an skb before
injecting it into the Rx path.