From: "Toke Høiland-Jørgensen" <toke@redhat.com> To: Kan Yan <kyan@google.com> Cc: Johannes Berg <johannes@sipsolutions.net>, linux-wireless <linux-wireless@vger.kernel.org>, Make-Wifi-fast <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>, Rajkumar Manoharan <rmanohar@codeaurora.org>, Kevin Hayes <kevinhayes@google.com> Subject: Re: [PATCH v9 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue Date: Sat, 16 Nov 2019 12:55:23 +0100 [thread overview] Message-ID: <87y2wgjas4.fsf@toke.dk> (raw) In-Reply-To: <CA+iem5tF+TN-osfGxu=EU5Xt1Uq+PcKgBVaoAmZY3a3JzS5JzQ@mail.gmail.com> Kan Yan <kyan@google.com> writes: >> +static inline u16 >> +ieee80211_info_set_tx_time_est(struct ieee80211_tx_info *info, u16 tx_time_est) >> +{ >> + /* We only have 10 bits in tx_time_est, so store airtime >> + * in increments of 4us and clamp the maximum to 2**12-1 >> + */ >> + info->tx_time_est = min_t(u16, tx_time_est, 4095) >> 2; >> + return info->tx_time_est; >> +} >> + >> +static inline u16 >> +ieee80211_info_get_tx_time_est(struct ieee80211_tx_info *info) >> +{ >> + return info->tx_time_est << 2; >> +} >> + > > set_tx_time_est() returns airtime in different units (4us) than > get_tx_time_est(), this will cause the pending_airtime out of whack. Huh, you're quite right; oops! I meant to shift that back before returning. Will fix. > Given the fact that AQL is only tested in very limited platforms, > should we set the default to disabled by removing this change in the > next update? > > - local->airtime_flags = AIRTIME_USE_TX | AIRTIME_USE_RX; > + > + local->airtime_flags = AIRTIME_USE_TX | > + AIRTIME_USE_RX | > + AIRTIME_USE_AQL; > + local->aql_threshold = IEEE80211_AQL_THRESHOLD; > + atomic_set(&local->aql_total_pending_airtime, 0); Well, we have the whole -rc series to get more testing in if we merge it as-is. It's up to the maintainers, of course, but I would be in favour of merging as-is, then optionally backing out the default before the final release if problems do turn up. But I would hope that the limits are sufficiently conservative that it would not result in any problems :) -Toke
WARNING: multiple messages have this Message-ID (diff)
From: "Toke Høiland-Jørgensen" <toke@redhat.com> To: Kan Yan <kyan@google.com> Cc: Rajkumar Manoharan <rmanohar@codeaurora.org>, Kevin Hayes <kevinhayes@google.com>, Make-Wifi-fast <make-wifi-fast@lists.bufferbloat.net>, linux-wireless <linux-wireless@vger.kernel.org>, ath10k@lists.infradead.org, John Crispin <john@phrozen.org>, Johannes Berg <johannes@sipsolutions.net>, Lorenzo Bianconi <lorenzo@kernel.org>, Felix Fietkau <nbd@nbd.name> Subject: Re: [PATCH v9 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue Date: Sat, 16 Nov 2019 12:55:23 +0100 [thread overview] Message-ID: <87y2wgjas4.fsf@toke.dk> (raw) In-Reply-To: <CA+iem5tF+TN-osfGxu=EU5Xt1Uq+PcKgBVaoAmZY3a3JzS5JzQ@mail.gmail.com> Kan Yan <kyan@google.com> writes: >> +static inline u16 >> +ieee80211_info_set_tx_time_est(struct ieee80211_tx_info *info, u16 tx_time_est) >> +{ >> + /* We only have 10 bits in tx_time_est, so store airtime >> + * in increments of 4us and clamp the maximum to 2**12-1 >> + */ >> + info->tx_time_est = min_t(u16, tx_time_est, 4095) >> 2; >> + return info->tx_time_est; >> +} >> + >> +static inline u16 >> +ieee80211_info_get_tx_time_est(struct ieee80211_tx_info *info) >> +{ >> + return info->tx_time_est << 2; >> +} >> + > > set_tx_time_est() returns airtime in different units (4us) than > get_tx_time_est(), this will cause the pending_airtime out of whack. Huh, you're quite right; oops! I meant to shift that back before returning. Will fix. > Given the fact that AQL is only tested in very limited platforms, > should we set the default to disabled by removing this change in the > next update? > > - local->airtime_flags = AIRTIME_USE_TX | AIRTIME_USE_RX; > + > + local->airtime_flags = AIRTIME_USE_TX | > + AIRTIME_USE_RX | > + AIRTIME_USE_AQL; > + local->aql_threshold = IEEE80211_AQL_THRESHOLD; > + atomic_set(&local->aql_total_pending_airtime, 0); Well, we have the whole -rc series to get more testing in if we merge it as-is. It's up to the maintainers, of course, but I would be in favour of merging as-is, then optionally backing out the default before the final release if problems do turn up. But I would hope that the limits are sufficiently conservative that it would not result in any problems :) -Toke _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
next prev parent reply other threads:[~2019-11-16 11:55 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-15 13:20 [PATCH v9 0/4] Add Airtime Queue Limits (AQL) to mac80211 Toke Høiland-Jørgensen 2019-11-15 13:20 ` Toke Høiland-Jørgensen 2019-11-15 13:20 ` [PATCH v9 1/4] mac80211: Add new sta_info getter by sta/vif addrs Toke Høiland-Jørgensen 2019-11-15 13:20 ` Toke Høiland-Jørgensen 2019-11-15 13:20 ` [PATCH v9 2/4] mac80211: Import airtime calculation code from mt76 Toke Høiland-Jørgensen 2019-11-15 13:20 ` Toke Høiland-Jørgensen 2019-11-15 13:20 ` [PATCH v9 3/4] mac80211: Implement Airtime-based Queue Limit (AQL) Toke Høiland-Jørgensen 2019-11-15 13:20 ` Toke Høiland-Jørgensen 2019-11-18 23:12 ` Kan Yan 2019-11-18 23:12 ` Kan Yan 2019-11-15 13:20 ` [PATCH v9 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue Toke Høiland-Jørgensen 2019-11-15 13:20 ` Toke Høiland-Jørgensen 2019-11-16 2:51 ` Kan Yan 2019-11-16 2:51 ` Kan Yan 2019-11-16 11:55 ` Toke Høiland-Jørgensen [this message] 2019-11-16 11:55 ` Toke Høiland-Jørgensen 2019-11-17 20:24 ` Kan Yan 2019-11-17 20:24 ` Kan Yan 2019-11-18 10:10 ` Toke Høiland-Jørgensen 2019-11-18 10:10 ` 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 * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87y2wgjas4.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.