Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>
To:     Felix Fietkau <nbd@nbd.name>, linux-wireless@vger.kernel.org
Cc:     johannes@sipsolutions.net
Subject: Re: [PATCH 2/7] mac80211: force calculation of software hash for tx
 fair queueing
In-Reply-To: <07ad2533-b477-abf1-5176-0521ca9ddf82@nbd.name>
References: <20201216204316.44498-1-nbd@nbd.name>
 <20201216204316.44498-2-nbd@nbd.name> <87czz8bqbj.fsf@toke.dk>
 <add3d1fa-c2ad-5aaf-83c7-31e919129bbf@nbd.name> <871rfobn8w.fsf@toke.dk>
 <07ad2533-b477-abf1-5176-0521ca9ddf82@nbd.name>
Date:   Thu, 17 Dec 2020 18:26:06 +0100
Message-ID: <874kkk9wep.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Felix Fietkau <nbd@nbd.name> writes:

> On 2020-12-17 14:01, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>>=20
>>> On 2020-12-17 12:54, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>>>> Felix Fietkau <nbd@nbd.name> writes:
>>>>=20
>>>>> Depending on the source, a hardware calculated hash may not provide t=
he
>>>>> same level of collision resistance.
>>>>=20
>>>> This seems like it would have performance implications?
>>>>=20
>>>> 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.
>>=20
>> Yeah, but I was thinking something like:
>>=20
>> 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