From: "Toke Høiland-Jørgensen" <toke@toke.dk>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: make-wifi-fast@lists.bufferbloat.net, linux-wireless@vger.kernel.org
Subject: Re: [Make-wifi-fast] [PATCH v8] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.
Date: Mon, 12 Sep 2016 15:08:58 +0200 [thread overview]
Message-ID: <87wpihe045.fsf@toke.dk> (raw)
In-Reply-To: <1473683716.29016.37.camel@sipsolutions.net> (Johannes Berg's message of "Mon, 12 Sep 2016 14:35:16 +0200")
Johannes Berg <johannes@sipsolutions.net> writes:
>> +static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx);
>> +static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data *sdata,
>> + struct sta_info *sta, u8 pn_offs,
>> + struct ieee80211_key_conf *key_conf,
>> + struct sk_buff *skb);
>> +
>
> I'm not very happy with this - I think you should do some
> refactoring/code move in a separate prior patch to avoid this.
Noted, will do.
>> + if (txq->sta && info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) {
>> struct sta_info *sta = container_of(txq->sta, struct sta_info,
>> sta);
>> - struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
>> + u8 pn_offs = 0;
>>
>> - hdr->seq_ctrl = ieee80211_tx_next_seq(sta, txq->tid);
>> - if (test_bit(IEEE80211_TXQ_AMPDU, &txqi->flags))
>> - info->flags |= IEEE80211_TX_CTL_AMPDU;
>> - else
>> - info->flags &= ~IEEE80211_TX_CTL_AMPDU;
>> + if (info->control.hw_key)
>> + pn_offs = ieee80211_hdrlen(hdr->frame_control);
>
> Not very happy with this either - the fast-xmit path explicitly tries
> to avoid all these calculations.
Well, the TXQ already adds a lot of other overhead (hashing on the
packet header, for one), so my guess would be that this would be
negligible compared to all that?
> I suppose I don't have to care all that much about the TXQs, but ...
>
> Then again, adding a field in the skb->cb for the sake of this? No,
> not really either.
So that's a "keep it", then? :)
>> + ieee80211_xmit_fast_finish(sta->sdata, sta, pn_offs,
>> + info->control.hw_key, skb);
>
> I don't see how keeping the info->control.hw_key pointer across the
> TXQ/FQ/Codel queueing isn't a potential bug? Probably one that already
> exists in your code today, before this patch, of course.
You mean the key could get removed from the hardware while the packet
was queued? Can certainly add a check for that. Under what conditions
does that happen? Does it make sense to try to recover from it (I guess
by calling tx_h_select_key), or is it rare enough that giving up and
dropping the packet makes more sense?
>> + } else {
>> + struct ieee80211_tx_data tx = { };
>> +
>> + __skb_queue_head_init(&tx.skbs);
>> + tx.local = local;
>> + tx.skb = skb;
>
> an empty initializer is weird - why not at least move local/skb
> initializations into it? Even txq->sta, I guess, since you can assign
> txq->sta either way.
Yup, makes sense. Noted.
>> - CALL_TXH(ieee80211_tx_h_select_key);
>> +
>> if (!ieee80211_hw_check(&tx->local->hw, HAS_RATE_CONTROL))
>> CALL_TXH(ieee80211_tx_h_rate_ctrl);
> [...]
>> if (unlikely(info->flags & IEEE80211_TX_INTFL_RETRANSMISSION)) {
>> __skb_queue_tail(&tx->skbs, tx->skb);
>> tx->skb = NULL;
>> goto txh_done;
>> }
>>
>> + CALL_TXH(ieee80211_tx_h_select_key);
>
> What happens for the IEEE80211_TX_INTFL_RETRANSMISSION packets wrt.
> key selection? Why is it OK to change this?
You're right, that's an oversight on my part. Will fix.
-Toke
next prev parent reply other threads:[~2016-09-12 13:09 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-17 12:58 [Make-wifi-fast] [PATCH] mac80211: Move crypto IV generation " Toke Høiland-Jørgensen
2016-08-17 13:08 ` Johannes Berg
2016-08-17 13:16 ` Toke Høiland-Jørgensen
2016-08-17 13:18 ` Johannes Berg
2016-08-17 13:23 ` Toke Høiland-Jørgensen
2016-08-17 14:45 ` [Make-wifi-fast] [PATCH v2] " Toke Høiland-Jørgensen
2016-08-17 15:47 ` Noah Causin
2016-08-17 22:33 ` Toke Høiland-Jørgensen
2016-08-19 3:06 ` Noah Causin
2016-08-22 14:24 ` Toke Høiland-Jørgensen
2016-08-23 17:06 ` Noah Causin
2016-08-23 17:51 ` Toke Høiland-Jørgensen
2016-08-17 19:49 ` Johannes Berg
2016-08-17 20:07 ` Dave Taht
2016-08-17 20:43 ` Johannes Berg
2016-08-22 14:47 ` Toke Høiland-Jørgensen
2016-08-26 8:38 ` Johannes Berg
2016-08-26 8:54 ` Toke Høiland-Jørgensen
2016-08-24 16:20 ` [Make-wifi-fast] [PATCH v3] mac80211: Move reorder-sensitive TX handlers " Toke Høiland-Jørgensen
2016-08-24 22:40 ` Noah Causin
2016-08-25 12:45 ` Toke Høiland-Jørgensen
2016-08-26 14:30 ` Toke Høiland-Jørgensen
2016-08-26 14:51 ` Dave Taht
2016-08-30 13:15 ` [Make-wifi-fast] [PATCH v4] " Toke Høiland-Jørgensen
2016-08-30 13:17 ` Toke Høiland-Jørgensen
2016-08-31 21:06 ` Johannes Berg
2016-09-01 8:23 ` Toke Høiland-Jørgensen
2016-09-01 8:34 ` Johannes Berg
2016-09-01 8:38 ` Toke Høiland-Jørgensen
2016-09-01 9:07 ` Johannes Berg
2016-09-01 9:20 ` Toke Høiland-Jørgensen
2016-09-01 9:27 ` Johannes Berg
2016-09-01 9:42 ` Toke Høiland-Jørgensen
2016-09-01 16:03 ` [Make-wifi-fast] [PATCH v5] " Toke Høiland-Jørgensen
2016-09-01 17:59 ` Johannes Berg
2016-09-01 18:30 ` Toke Høiland-Jørgensen
2016-09-01 18:35 ` Johannes Berg
2016-09-02 2:48 ` Jason Andryuk
2016-09-02 9:27 ` Toke Høiland-Jørgensen
2016-09-02 13:41 ` [Make-wifi-fast] [PATCH v6] " Toke Høiland-Jørgensen
2016-09-02 14:44 ` Toke Høiland-Jørgensen
2016-09-05 11:30 ` [Make-wifi-fast] [PATCH v7] " Toke Høiland-Jørgensen
2016-09-05 16:06 ` Toke Høiland-Jørgensen
2016-09-05 17:00 ` Dave Taht
2016-09-05 17:26 ` Toke Høiland-Jørgensen
2016-09-05 17:59 ` Dave Taht
2016-09-05 20:23 ` Dave Taht
2016-09-05 20:45 ` Toke Høiland-Jørgensen
2016-09-05 21:02 ` Dave Taht
2016-09-05 21:25 ` Toke Høiland-Jørgensen
2016-09-05 21:29 ` Dave Taht
2016-09-05 21:35 ` Toke Høiland-Jørgensen
2016-09-05 21:42 ` Dave Taht
2016-09-05 22:04 ` Dave Taht
2016-09-05 22:01 ` Toke Høiland-Jørgensen
2016-09-05 22:08 ` Dave Taht
2016-09-05 22:31 ` Dave Taht
2016-09-05 17:49 ` Felix Fietkau
2016-09-05 17:59 ` Toke Høiland-Jørgensen
2016-09-05 18:45 ` Felix Fietkau
2016-09-06 11:43 ` Toke Høiland-Jørgensen
2016-09-06 11:45 ` Toke Høiland-Jørgensen
2016-09-06 11:44 ` [Make-wifi-fast] [PATCH v8] " Toke Høiland-Jørgensen
2016-09-06 22:04 ` Felix Fietkau
2016-09-12 12:35 ` Johannes Berg
2016-09-12 13:08 ` Toke Høiland-Jørgensen [this message]
2016-09-12 13:19 ` Johannes Berg
2016-09-22 17:04 ` [Make-wifi-fast] [PATCH v9 0/2] mac80211: TXQ dequeue path rework Toke Høiland-Jørgensen
2016-09-22 17:04 ` [Make-wifi-fast] [PATCH v9 1/2] mac80211: Move ieee802111_tx_dequeue() to later in tx.c Toke Høiland-Jørgensen
2016-09-30 11:13 ` Johannes Berg
2016-09-22 17:04 ` [Make-wifi-fast] [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue Toke Høiland-Jørgensen
2016-09-30 10:27 ` Johannes Berg
2016-09-30 12:39 ` Toke Høiland-Jørgensen
2016-09-30 12:43 ` Johannes Berg
2016-09-30 12:45 ` Toke Høiland-Jørgensen
2016-09-30 12:49 ` Johannes Berg
2016-09-30 14:01 ` Toke Høiland-Jørgensen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://lists.bufferbloat.net/postorius/lists/make-wifi-fast.lists.bufferbloat.net/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87wpihe045.fsf@toke.dk \
--to=toke@toke.dk \
--cc=johannes@sipsolutions.net \
--cc=linux-wireless@vger.kernel.org \
--cc=make-wifi-fast@lists.bufferbloat.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox