Re: [PATCH 2/7] mac80211: force calculation of software hash for tx fair queueing

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Felix Fietkau <nbd@nbd.name> writes:

> On 2020-12-17 14:01, Toke Høiland-Jørgensen wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>> 
>>> On 2020-12-17 12:54, Toke Høiland-Jørgensen wrote:
>>>> Felix Fietkau <nbd@nbd.name> writes:
>>>> 
>>>>> Depending on the source, a hardware calculated hash may not provide the
>>>>> same level of collision resistance.
>>>> 
>>>> This seems like it would have performance implications?
>>>> 
>>>> Also, this can potentially discard information from tunnels that
>>>> preserve the hash before encapsulation (we added support for this to
>>>> Wireguard which had some nice effects on queueing of encapsulated
>>>> traffic).
>>> If the hash was calculated in software using the flow dissector, it will
>>> be preserved, even if it went through a few virtual interfaces.
>>> The only hashes discarded are hardware generated ones.
>> 
>> Yeah, but I was thinking something like:
>> 
>> Packet comes in with HW hash -> gets encapsulated (preserving the hash)
>> -> gets to mac80211 which discards the HW hash. So now you're replacing
>> a (possibly bad-quality) HW hash with a software hash of the *outer*
>> encapsulation header...
> If this becomes a problem, I think we should add a similar patch to
> wireguard, which already calls skb_get_hash before encapsulating.
> Other regular tunnels should already get a proper hash, since the flow
> dissector will take care of it.

But then we'd need to go around adding this to all the places that uses
the hash just to work around a particular piece of broken(ish) hardware.
And we're hard-coding a behaviour in mac80211 that means we'll *always*
recompute the hash, even for hardware that's not similarly broken.

> The reason I did this patch is because I have a patch to set the hw flow
> hash in the skb on mtk_eth_soc, which does help GRO, but leads to
> collisions on mac80211 fq.

So wouldn't the right thing to do here be to put a flag into the RX
device that makes the stack clear the hash after using it for GRO?

-Toke