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

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
>

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

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.

> On Nov 27, 2025, at 3:24 AM, Ido Schimmel <idosch@nvidia.com> wrote:
> 
> 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.

I thought about that, but the nice bit about doing it like I have it
is that the flow keys / L4 hash bits are getting evaluated properly.

If we do it like you’ve suggested, we’re asserting that L4 hash is always
true, right?

How about another helper, that only tun consumes, which does all of these
things, such that the code still stays clean on the flow dissector side
and we don’t have to mess with any other callers?

That would be the middle ground between what you suggested and what I did

Thoughts?

Thanks,
Jon

On Tue, Dec 02, 2025 at 04:53:43PM +0000, Jon Kohler wrote:
> I thought about that, but the nice bit about doing it like I have it
> is that the flow keys / L4 hash bits are getting evaluated properly.
> 
> If we do it like you’ve suggested, we’re asserting that L4 hash is always
> true, right?

Yes, for some reason I thought that the flow dissector always sets it in
this case.

> How about another helper, that only tun consumes, which does all of these
> things, such that the code still stays clean on the flow dissector side
> and we don’t have to mess with any other callers?
> 
> That would be the middle ground between what you suggested and what I did
> 
> Thoughts?

Not sure. We already have __skb_get_hash_symmetric_net() and
__skb_get_hash_symmetric() and now we will have a third variant. In this
case, maybe adding a 'save_hash' argument is better. It also means that
the next time someone needs to calculate a symmetric hash they will
pause to think if it needs to be set in the skb. I believe that when
skb_get_hash() was replaced with __skb_get_hash_symmetric() in tun the
assumption was that the hash will be stored in the skb as with
skb_get_hash().