Received-SPF: softfail (google.com: domain of transitioning linux-wireless-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <65e6897d-e1ec-ffda-5f7a-4aec37621377@nbd.name>
Date:   Sun, 8 May 2022 07:44:14 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.9.0
Subject: Re: [PATCH] mac80211: Use full queue selection code for control port
 tx
Content-Language: en-US
To:     =?UTF-8?Q?Toke_H=c3=b8iland-J=c3=b8rgensen?= <toke@kernel.org>,
        Alexander Wetzel <alexander@wetzel-home.de>,
        Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org, Pierre Asselin <pa@panix.com>
References: <20220507083706.384513-1-alexander@wetzel-home.de>
 <87r1551t4c.fsf@toke.dk>
From:   Felix Fietkau <nbd@nbd.name>
In-Reply-To: <87r1551t4c.fsf@toke.dk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 07.05.22 13:26, Toke Høiland-Jørgensen wrote:
> Alexander Wetzel <alexander@wetzel-home.de> writes:
> 
>> Calling only __ieee80211_select_queue() for control port TX exposes
>> drivers which do not support QoS to non-zero values in
>> skb->queue_mapping and even can assign not available queues to
>> info->hw_queue.
>> This can cause issues for drivers like we did e.g. see in
>> '746285cf81dc ("rtl818x: Prevent using not initialized queues")'.
>>
>> This also prevents a redundant call to __ieee80211_select_queue() when
>> using control port TX with iTXQ (pull path).
>> And it starts to prioritize 802.11 preauthentication frames
>> (ETH_P_PREAUTH) on all TX paths.
>>
>> Pierre Asselin confirmed that this patch indeed prevents crashing his
>> system without '746285cf81dc ("rtl818x: Prevent using not initialized
>> queues")'.
>>
>> Tested-by: Pierre Asselin <pa@panix.com>
>> Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
>> ---
>>
>> Starting to prioritize ETH_P_PREAUTH was just added since I noticed that
>> contradictory to at least my expectations control port does accept
>> ETH_P_PREAUTH but handles these like a normal frame for the priority.
>> That can be broken out or even drop, when needed.
>>
>> While looking at the code I also tripped over multiple other questions
>> and I'll probably propose a much more invasive change how to handle
>> the queue assignment. (End2end we seem to do some quite stupid things.)
>>
>> Additionally I really don't get why we call skb_get_hash() on queue
>> assignment:
>> I found the commit '180ac48ee62f ("mac80211: calculate skb hash early
>> when using itxq")' but don't see why calculating the hash early is
>> useful. Any hints here are appreciated. fq_flow_idx() seems to do that
>> when needed and I can't find any other usage of the hash...
> 
> The commit message of that commit has a hint:
> 
>      This avoids flow separation issues when using software encryption.
> 
> The idea being that the packet contents can change on encryption, but
> skb->hash is preserved, so you want it to run before encryption happens
> to keep flows in the same queue.
> 
> However, AFAICT ieee80211_tx_h_encrypt() is called after frames are
> dequeued from the TXQs, so not actually sure this is needed. Adding
> Felix, in the hope that he can explain the reasoning behind that commit :)Sorry, I don't remember the details on that one. One advantage that I 
can think of (which isn't mentioned in the commit msg) is that it is 
likely better for performance to calculate the hash early since there is 
a good chance that more of the skb data is still in the CPU cache.

- Felix