> > 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 :) Sounds good. The current default limits are reasonably conservative and are tunable via debugfs. I will give the v10 version of this patch serial a quick test and hopefully we can wrap it up soon. -Kan On Sat, Nov 16, 2019 at 3:55 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote: > > Kan Yan <kyan@xxxxxxxxxx> 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 >