| Message ID | 20191216170302.29543-8-Jerome.Pouiller@silabs.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Improve wfx driver | expand |
On 2019-12-16 18:03, Jérôme Pouiller wrote: > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > When not using HT mode, minstrel always includes 1Mbps as fallback rate. > But, when using HT mode, this fallback is not included. Yet, it seems > that it could save some frames. So, this patch add it unconditionally. > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> Are you sure that's a good idea? Sometimes a little packet loss can be preferable over a larger amount of airtime wasted through using really low rates. Especially when you consider bufferbloat. - Felix
On Monday 16 December 2019 19:08:39 CET Felix Fietkau wrote: > On 2019-12-16 18:03, Jérôme Pouiller wrote: > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > When not using HT mode, minstrel always includes 1Mbps as fallback rate. > > But, when using HT mode, this fallback is not included. Yet, it seems > > that it could save some frames. So, this patch add it unconditionally. > > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > Are you sure that's a good idea? Sometimes a little packet loss can be > preferable over a larger amount of airtime wasted through using really > low rates. Especially when you consider bufferbloat. Hello Felix, I have observed that, in some circumstances, TCP throughput was far better with 802.11g than with 802.11n. I found that 802.11n had more Tx failures. These failures have big impacts on the congestion window. When the congestion window is low, it impacts the capacity of aggregation of the link. Thus, it does not help to improve the congestion windows. By investigating deeper, it appears that the minstrel (used by 802.11g) always add rate 1Mbps to the rate list while minstrel_ht (used by 802.11n) don't (compare minstrel_update_rates() and minstrel_ht_update_rates()). This difference seems to be correlated to the difference of TCP throughput I can observe. I did some search in git history and I did not find any explanation for this difference between minstrel and minstrel_ht (however, it seems you are the right person to ask :) ). I didn't find why it would be efficient on minstrel and inefficient on minstrel_ht. And since this change fix the issue that I observed, I have tried to apply it and wait for feedback.
Hello Jérôme, On 2019-12-17 12:01, Jérôme Pouiller wrote: > On Monday 16 December 2019 19:08:39 CET Felix Fietkau wrote: >> On 2019-12-16 18:03, Jérôme Pouiller wrote: >> > From: Jérôme Pouiller <jerome.pouiller@silabs.com> >> > >> > When not using HT mode, minstrel always includes 1Mbps as fallback rate. >> > But, when using HT mode, this fallback is not included. Yet, it seems >> > that it could save some frames. So, this patch add it unconditionally. >> > >> > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> >> Are you sure that's a good idea? Sometimes a little packet loss can be >> preferable over a larger amount of airtime wasted through using really >> low rates. Especially when you consider bufferbloat. > I have observed that, in some circumstances, TCP throughput was far > better with 802.11g than with 802.11n. I found that 802.11n had more Tx > failures. These failures have big impacts on the congestion window. When > the congestion window is low, it impacts the capacity of aggregation of > the link. Thus, it does not help to improve the congestion windows. > > By investigating deeper, it appears that the minstrel (used by 802.11g) > always add rate 1Mbps to the rate list while minstrel_ht (used by > 802.11n) don't (compare minstrel_update_rates() and > minstrel_ht_update_rates()). This difference seems to be correlated to > the difference of TCP throughput I can observe. > > I did some search in git history and I did not find any explanation for > this difference between minstrel and minstrel_ht (however, it seems you > are the right person to ask :) ). I didn't find why it would be > efficient on minstrel and inefficient on minstrel_ht. And since this > change fix the issue that I observed, I have tried to apply it and wait > for feedback. I have found that in many cases when minstrel_ht selects sub-optimal rates that cause too many re-transmissions or re-transmission failures, it was because there was an issue in tx status reporting. Another possible reason is buffering too many packets without having the ability to alter the rates for in-flight packets based on bad tx status results. To find out what the driver/hardware is doing, I took a quick look and it seems to be managing multiple tx rate policies based on per-packet rate info. Based on that I have an idea of what you could try to make things better: Instead of using per-packet rate info, implement the .sta_rate_tbl_update callback to maintain a primary tx policy used for all non-probing non-fixed-rate packets, which you can alter while packets using it are queued already. The existing approach using per-packet tx_info data should then be used only for probing or fixed-rate packets. You then probably have to be a bit clever in the tx status path for figuring out what rates were actually used. - Felix
On Tuesday 17 December 2019 12:20:40 CET Felix Fietkau wrote: [...] > Instead of using per-packet rate info, implement the > .sta_rate_tbl_update callback to maintain a primary tx policy used for > all non-probing non-fixed-rate packets, which you can alter while > packets using it are queued already. > The existing approach using per-packet tx_info data should then be used > only for probing or fixed-rate packets. > You then probably have to be a bit clever in the tx status path for > figuring out what rates were actually used. Indeed, I have noticed that we are are to react to any changes on the link quality. Your idea may helps a lot. Thank you. Do you know if I can safely rely on IEEE80211_TX_CTL_RATE_CTRL_PROBE and IEEE80211_TX_CTL_USE_MINRATE to detect probing and fixed-rate packets? I currently work on others parts, but I think I will try your suggestion in January. One last thing, do you know why minstrel appends the lowest rate and minstrel_ht don't? They should be identical, not?
On 2019-12-17 16:15, Jérôme Pouiller wrote: > On Tuesday 17 December 2019 12:20:40 CET Felix Fietkau wrote: > [...] >> Instead of using per-packet rate info, implement the >> .sta_rate_tbl_update callback to maintain a primary tx policy used for >> all non-probing non-fixed-rate packets, which you can alter while >> packets using it are queued already. >> The existing approach using per-packet tx_info data should then be used >> only for probing or fixed-rate packets. >> You then probably have to be a bit clever in the tx status path for >> figuring out what rates were actually used. > > Indeed, I have noticed that we are are to react to any changes on the > link quality. Your idea may helps a lot. Thank you. > > Do you know if I can safely rely on IEEE80211_TX_CTL_RATE_CTRL_PROBE and > IEEE80211_TX_CTL_USE_MINRATE to detect probing and fixed-rate packets? Set the hw flag SUPPORTS_RC_TABLE. That way the skb tx_info will only have control->rates set for if there are overrides, probing rate, fixed rate, etc. That way you can use the default policy if info->control.rates[0].idx == -1 > I currently work on others parts, but I think I will try your suggestion > in January. > > One last thing, do you know why minstrel appends the lowest rate and > minstrel_ht don't? They should be identical, not? The minstrel_ht algorithm is newer and minstrel just hasn't been updated for a number of algorithm changes yet. - Felix
diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c index 46ad83b95f52..738a6ca5edad 100644 --- a/drivers/staging/wfx/data_tx.c +++ b/drivers/staging/wfx/data_tx.c @@ -538,6 +538,17 @@ static void wfx_tx_fixup_rates(struct ieee80211_tx_rate *rates) } } } while (!finished); + // Ensure that MCS0 or 1Mbps is present at the end of the retry list + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) { + if (rates[i].idx == 0) + break; + if (rates[i].idx == -1) { + rates[i].idx = 0; + rates[i].count = 8; // == hw->max_rate_tries + rates[i].flags = rates[i - 1].flags & IEEE80211_TX_RC_MCS; + break; + } + } // All retries use long GI for (i = 1; i < IEEE80211_TX_MAX_RATES; i++) rates[i].flags &= ~IEEE80211_TX_RC_SHORT_GI;