From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: linux-wireless@vger.kernel.org,
make-wifi-fast@lists.bufferbloat.net, ath10k@lists.infradead.org,
John Crispin <john@phrozen.org>,
Lorenzo Bianconi <lorenzo@kernel.org>,
Felix Fietkau <nbd@nbd.name>, Kan Yan <kyan@google.com>,
Rajkumar Manoharan <rmanohar@codeaurora.org>,
Kevin Hayes <kevinhayes@google.com>
Subject: Re: [PATCH v6 2/4] mac80211: Import airtime calculation code from mt76
Date: Fri, 08 Nov 2019 11:55:35 +0100 [thread overview]
Message-ID: <87d0e2fxi0.fsf@toke.dk> (raw)
In-Reply-To: <42266e95f45d5772e04c9c016bd6179ea14ffefa.camel@sipsolutions.net>
Johannes Berg <johannes@sipsolutions.net> writes:
> On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>
>> Felix recently added code to calculate airtime of packets to the mt76
>> driver. Import this into mac80211 so we can use it for airtime queue limit
>> calculations later.
>>
>> The airtime.c file is copied verbatim from the mt76 driver, and adjusted to
>> use mac80211 data structures instead (which is fairly straight forward).
>> The per-rate TX rate calculation is split out to its own
>> function (ieee80211_calc_tx_airtime_rate()) so it can be used directly for
>> the AQL calculations added in a subsequent patch.
>
> Any way it could be exposed by mac80211 back to the drivers, perhaps, to
> share it?
Didn't I already export some of the functions? My intention was to do
that, certainly, and to patch mt76 to switch to using them once the
trees have converged...
>> The only thing that it was not possible to port directly was the bit that
>> read the internal driver flags of struct ieee80211_rate to determine
>> whether a rate is using CCK or OFDM encoding. Instead, just look at the
>> rate index, since at least mt76 and ath10k both seem to have the same
>> number of CCK rates (4) in their tables.
>
> This is highly questionable ...
>
>> + switch (status->encoding) {
>> + case RX_ENC_LEGACY:
>> + if (WARN_ON_ONCE(status->band > NL80211_BAND_5GHZ))
>> + return 0;
>> +
>> + sband = hw->wiphy->bands[status->band];
>> + if (!sband || status->rate_idx > sband->n_bitrates)
>> + return 0;
>> +
>> + rate = &sband->bitrates[status->rate_idx];
>> + cck = (status->rate_idx < CCK_NUM_RATES);
Heh, yeah this did feel like a hack to me as well ;)
> Why not
>
> cck = rate->flags & IEEE80211_RATE_MANDATORY_B;
>
> I mean .. we know that IEEE80211_RATE_MANDATORY_B rates are exactly the
> CCK rates, and that's not really going to change?
>
> Alternatively, we could do
>
> cck = sband->band == NL80211_BAND_2GHZ &&
> !(rate->flags & IEEE80211_RATE_ERP_G);
>
> or even
>
> cck = rate->bitrate == 10 || rate->bitrate == 20 ||
> rate->bitrate == 55 || rate->bitrate == 110;
I am fine with either of those; I just wasn't sure what assumptions I
could actually make here. I guess I'll just pick one :)
>> + default:
>> + WARN_ON_ONCE(1);
>
> You can't do that in mac80211 either. That might be fine for mt76, but
> mac80211 already supports HE.
Good point, will fix.
-Toke
next prev parent reply other threads:[~2019-11-08 10:55 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-23 9:58 [PATCH v6 0/4] Add Airtime Queue Limits (AQL) to mac80211 Toke Høiland-Jørgensen
2019-10-23 9:59 ` [PATCH v6 1/4] mac80211: Shrink the size of ack_frame_id to make room for tx_time_est Toke Høiland-Jørgensen
2019-11-08 10:03 ` Johannes Berg
2019-10-23 9:59 ` [PATCH v6 2/4] mac80211: Import airtime calculation code from mt76 Toke Høiland-Jørgensen
2019-11-08 10:07 ` Johannes Berg
2019-11-08 10:55 ` Toke Høiland-Jørgensen [this message]
2019-11-08 10:57 ` Johannes Berg
2019-10-23 9:59 ` [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL) Toke Høiland-Jørgensen
2019-11-08 10:09 ` Johannes Berg
2019-11-08 10:56 ` Toke Høiland-Jørgensen
2019-11-08 10:59 ` Johannes Berg
2019-11-08 11:10 ` Toke Høiland-Jørgensen
2019-11-08 11:17 ` Johannes Berg
2019-11-09 1:22 ` Kan Yan
2019-11-09 11:22 ` Toke Høiland-Jørgensen
2019-10-23 9:59 ` [PATCH v6 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue Toke Høiland-Jørgensen
2019-11-08 10:17 ` Johannes Berg
2019-11-08 11:01 ` Toke Høiland-Jørgensen
2019-11-08 11:05 ` Johannes Berg
2019-11-07 6:14 ` [PATCH v6 0/4] Add Airtime Queue Limits (AQL) to mac80211 Kan Yan
2019-11-07 10:55 ` Toke Høiland-Jørgensen
2019-11-07 21:24 ` [Make-wifi-fast] " Dave Taht
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87d0e2fxi0.fsf@toke.dk \
--to=toke@redhat.com \
--cc=ath10k@lists.infradead.org \
--cc=johannes@sipsolutions.net \
--cc=john@phrozen.org \
--cc=kevinhayes@google.com \
--cc=kyan@google.com \
--cc=linux-wireless@vger.kernel.org \
--cc=lorenzo@kernel.org \
--cc=make-wifi-fast@lists.bufferbloat.net \
--cc=nbd@nbd.name \
--cc=rmanohar@codeaurora.org \
/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;
as well as URLs for NNTP newsgroup(s).