| Message ID | 20220507083706.384513-1-alexander@wetzel-home.de |
|---|---|
| State | New |
| Headers | show |
| Series | mac80211: Use full queue selection code for control port tx | expand |
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
Felix Fietkau <nbd@nbd.name> writes: > 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. Right, that could be, I suppose. I don't think it'll hurt, at least; Alexander, did you have any particular reason for wanting to get rid of it? -Toke
>>>> 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. > > Right, that could be, I suppose. I don't think it'll hurt, at least; > Alexander, did you have any particular reason for wanting to get rid of > it? No, not really. I just do not want to move code around I do not understand. I'm looking into how mac80211 assigns the queues and ended up with the impression, that this could be simplified. Now I'm pretty sure that the distinction between iTXQ and other drivers for queue selection is (nowadays?) pointless. (I'll argue the case hopefully soon in another patch.) My problem was only, how to handle the calls to skb_get_hash() in my upcoming patch: I could not figure out how this call helps to "avoids flow separation issues when using software encryption", indicating that I still may have a critical knowledge gap. With the understanding that we try to get the hash calculated while the skb may still be in the CPU buffers that's sorted out. In fact I've now a first draft for the "queue simplification patch" and will share that here after testing it with a card which actually supports wake_tx_queue(). Alexander
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 0e4efc08c762..2fabf6c4547c 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -5727,7 +5727,6 @@ int ieee80211_tx_control_port(struct wiphy *wiphy, struct net_device *dev, { struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev); struct ieee80211_local *local = sdata->local; - struct sta_info *sta; struct sk_buff *skb; struct ethhdr *ehdr; u32 ctrl_flags = 0; @@ -5771,20 +5770,9 @@ int ieee80211_tx_control_port(struct wiphy *wiphy, struct net_device *dev, skb_reset_network_header(skb); skb_reset_mac_header(skb); - /* update QoS header to prioritize control port frames if possible, - * priorization also happens for control port frames send over - * AF_PACKET - */ - rcu_read_lock(); - - if (ieee80211_lookup_ra_sta(sdata, skb, &sta) == 0 && !IS_ERR(sta)) { - u16 queue = __ieee80211_select_queue(sdata, sta, skb); - - skb_set_queue_mapping(skb, queue); - skb_get_hash(skb); - } - - rcu_read_unlock(); + /* skb bypassed queue selection in net/core/dev.c, do it now */ + skb_set_queue_mapping(skb, ieee80211_select_queue(sdata, skb)); + skb_get_hash(skb); /* mutex lock is only needed for incrementing the cookie counter */ mutex_lock(&local->mtx); diff --git a/net/mac80211/wme.c b/net/mac80211/wme.c index 62c6733e0792..774afefbe0b0 100644 --- a/net/mac80211/wme.c +++ b/net/mac80211/wme.c @@ -160,7 +160,8 @@ u16 __ieee80211_select_queue(struct ieee80211_sub_if_data *sdata, return IEEE80211_AC_BE; } - if (skb->protocol == sdata->control_port_protocol) { + if (skb->protocol == sdata->control_port_protocol || + skb->protocol == cpu_to_be16(ETH_P_PREAUTH)) { skb->priority = 7; goto downgrade; }
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... net/mac80211/tx.c | 18 +++--------------- net/mac80211/wme.c | 3 ++- 2 files changed, 5 insertions(+), 16 deletions(-)