From: J.P. Tosoni <[email protected]>
In the rate control algorithms, the maximum retry count is limited by
a) a constant value obtained from the hardware driver
b) a constant limit (6ms) on the time allowed for all
retries of each frame.
Replace the retry count by existing configurable values from nl80211.
Use wiphy->retry_long for frames whose length exceed rts_threshold.
Use wiphy->retry_short for all other frames.
Check that the configured value does not exceed driver capabilities.
Use the new values as soon as the next frame is transmitted.
Caveat: the retry count for frames sent outside the context of a STA is
still taken from the hardware driver.
---
What I am seeking with this patch:
I believe the configuration of the retries will help making recovery
much faster when an AP (in infrastructure mode) or a peer (in mesh
mode) suddenly disappears.
>From Felix Fietkau's comments:
- short retries arbitrarily reserved for management frames: now it depends on
the use_rts flag which is set when frame length > rts_threshold, which matches
the standard.
- "minstrel_alloc ... never update (long_frame_max_tx_count) again"
The value is now directly used each time we send a frame, the configuration
should now apply immediately. (not yet tested, need to change iw)
net/mac80211/cfg.c | 6 ++++++
net/mac80211/rate.c | 31 ++++++++++++++++++++++++++++++-
2 files changed, 36 insertions(+), 1 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 082f270..4f43beb 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2203,11 +2203,17 @@ static int ieee80211_set_wiphy_params(struct wiphy *wiphy, u32 changed)
if (changed & WIPHY_PARAM_RETRY_SHORT) {
if (wiphy->retry_short > IEEE80211_MAX_TX_RETRY)
return -EINVAL;
+ if (wiphy->retry_short >
+ local->hw.max_rate_tries*local->hw.max_rates)
+ return -EINVAL;
local->hw.conf.short_frame_max_tx_count = wiphy->retry_short;
}
if (changed & WIPHY_PARAM_RETRY_LONG) {
if (wiphy->retry_long > IEEE80211_MAX_TX_RETRY)
return -EINVAL;
+ if (wiphy->retry_long >
+ local->hw.max_rate_tries*local->hw.max_rates)
+ return -EINVAL;
local->hw.conf.long_frame_max_tx_count = wiphy->retry_long;
}
if (changed &
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index a02bef3..a8eaca1 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -537,18 +537,42 @@ static void rate_control_fill_sta_table(struct ieee80211_sta *sta,
{
struct ieee80211_sta_rates *ratetbl = NULL;
int i;
+ int max_retries;
if (sta && !info->control.skip_table)
ratetbl = rcu_dereference(sta->rates);
+ if (sta) {
+ struct ieee80211_hw *hw;
+
+ hw = &container_of(sta, struct sta_info, sta)->local->hw;
+ if (info->control.use_rts)
+ max_retries = hw->conf.long_frame_max_tx_count;
+ else
+ max_retries = hw->conf.short_frame_max_tx_count;
+ } else {
+ /*
+ * No STA, we cannot access hw. Set to a max value, so that
+ * the values computed by the rate control algorithm will be
+ * used unlimited.
+ */
+ max_retries = max_rates * 256; /* garanteed max value of u8 */
+ }
+
/* Fill remaining rate slots with data from the sta rate table. */
max_rates = min_t(int, max_rates, IEEE80211_TX_RATE_TABLE_SIZE);
for (i = 0; i < max_rates; i++) {
- if (i < ARRAY_SIZE(info->control.rates) &&
+ if (max_retries <= 0) {
+ rates[i].idx = -1;
+ rates[i].count = 0;
+ } else if (i < ARRAY_SIZE(info->control.rates) &&
info->control.rates[i].idx >= 0 &&
info->control.rates[i].count) {
if (rates != info->control.rates)
rates[i] = info->control.rates[i];
+ if (max_retries < rates[i].count) {
+ rates[i].count = max_retries;
+ }
} else if (ratetbl) {
rates[i].idx = ratetbl->rate[i].idx;
rates[i].flags = ratetbl->rate[i].flags;
@@ -558,6 +582,9 @@ static void rate_control_fill_sta_table(struct ieee80211_sta *sta,
rates[i].count = ratetbl->rate[i].count_cts;
else
rates[i].count = ratetbl->rate[i].count;
+ if (max_retries < rates[i].count) {
+ rates[i].count = max_retries;
+ }
} else {
rates[i].idx = -1;
rates[i].count = 0;
@@ -565,6 +592,8 @@ static void rate_control_fill_sta_table(struct ieee80211_sta *sta,
if (rates[i].idx < 0 || !rates[i].count)
break;
+
+ max_retries -= rates[i].count;
}
}
--
1.7.2.5
On 2013-06-27 6:40 PM, Jean-Pierre Tosoni wrote:
> From: J.P. Tosoni <[email protected]>
>
> In the rate control algorithms, the maximum retry count is limited by
> a) a constant value obtained from the hardware driver
> b) a constant limit (6ms) on the time allowed for all
> retries of each frame.
>
> Replace the retry count by existing configurable values from nl80211.
> Use wiphy->retry_long for frames whose length exceed rts_threshold.
> Use wiphy->retry_short for all other frames.
> Check that the configured value does not exceed driver capabilities.
> Use the new values as soon as the next frame is transmitted.
>
> Caveat: the retry count for frames sent outside the context of a STA is
> still taken from the hardware driver.
> ---
> What I am seeking with this patch:
> I believe the configuration of the retries will help making recovery
> much faster when an AP (in infrastructure mode) or a peer (in mesh
> mode) suddenly disappears.
I'm all for reducing retries, but I think the way you're applying the
limit is problematic.
If minstrel decides to use many retries for a high rate and you're
simply cutting off all retries that exceed the configured limit, you're
potentially inviting quite a bit of unnecessary packet loss.
I'm planning to remove the use of retry rate slot 4 (fallback to lowest
rate) from minstrel, since max_prob_rate should already provide quite
decent reliability.
- Felix
Hi Felix,
Sorry to use your time again...
> But much more important than that is to not cause regressions for other
> people via aggressive packet dropping.
Agreed, but see below.
> If you put the code in minstrel (and minstrel_ht), it not only allows
> making a better tradeoff for retry handling, the code also doesn't have
> to be run for every single packet. You can run it during the rate
> control stats update.
OK, I'll have a look at that part now.
>
> The reduction of retry attempts definitely needs to be balanced
> properly. Retries with max_prob_rate can be more important than retries
> with max_tp_rate, but there needs to be a minimum for each of those.
This leads to a question about regressions and backward compatibility:
Since minstrel can compute as much as 28 retries for a frame,
And since the (standard) default value for "short_frame_max_tx_count" is 7,
... there is no way I can enforce the configured value while keeping
minstrel counts by default !
The standard itself gives a very aggressive limit! Or am I mistaken about
the significance of this configuration parameter ?
Jean-Pierre
Thanks Felix.
I am using the ath9k, but I have seen this in the b43 driver:
rates[0].count = dev->wl->hw->conf.short_frame_max_tx_count;
Do you think that short/long_frame_max_tx_count should rather be applied at
the driver level (not mac80211) ?
The ath9k driver currently enforces a minimum retry count of 30 (constant),
it could be replaced with short/long_frame_max_tx_count ?
Jean-Pierre
> -----Message d'origine-----
> De?: Felix Fietkau [mailto:[email protected]]
> Envoy??: samedi 29 juin 2013 22:09
> ??: Jean-Pierre Tosoni
> Cc?: [email protected]
> Objet?: Re: [RFC v2] mac80211: Use libnl-configurable values for retry
> counts
>
> On 2013-06-27 6:40 PM, Jean-Pierre Tosoni wrote:
> > From: J.P. Tosoni <[email protected]>
> >
> > In the rate control algorithms, the maximum retry count is limited by
> > a) a constant value obtained from the hardware driver
> > b) a constant limit (6ms) on the time allowed for all
> > retries of each frame.
> >
> > Replace the retry count by existing configurable values from nl80211.
> > Use wiphy->retry_long for frames whose length exceed rts_threshold.
> > Use wiphy->retry_short for all other frames.
> > Check that the configured value does not exceed driver capabilities.
> > Use the new values as soon as the next frame is transmitted.
> >
> > Caveat: the retry count for frames sent outside the context of a STA is
> > still taken from the hardware driver.
> > ---
> > What I am seeking with this patch:
> > I believe the configuration of the retries will help making recovery
> > much faster when an AP (in infrastructure mode) or a peer (in mesh
> > mode) suddenly disappears.
> I'm all for reducing retries, but I think the way you're applying the
> limit is problematic.
> If minstrel decides to use many retries for a high rate and you're
> simply cutting off all retries that exceed the configured limit, you're
> potentially inviting quite a bit of unnecessary packet loss.
> I'm planning to remove the use of retry rate slot 4 (fallback to lowest
> rate) from minstrel, since max_prob_rate should already provide quite
> decent reliability.
>
> - Felix
On 2013-07-02 3:28 PM, Jean-Pierre Tosoni wrote:
> Hi Felix,
>
>> -----Message d'origine-----
>> De : Felix Fietkau [mailto:[email protected]]
>> > ---
>> > What I am seeking with this patch:
>> > I believe the configuration of the retries will help making recovery
>> > much faster when an AP (in infrastructure mode) or a peer (in mesh
>> > mode) suddenly disappears.
>> I'm all for reducing retries, but I think the way you're applying the
>> limit is problematic.
>> If minstrel decides to use many retries for a high rate and you're
>> simply cutting off all retries that exceed the configured limit, you're
>> potentially inviting quite a bit of unnecessary packet loss.
>> I'm planning to remove the use of retry rate slot 4 (fallback to lowest
>> rate) from minstrel, since max_prob_rate should already provide quite
>> decent reliability.
>
> Well, on one hand, people who want to reduce retries are more concerned with
> low latency and jitter than with reliable delivery of data.
Makes sense.
> On another hand the code should work for any rate control plugin, not just
> minstrel.
But much more important than that is to not cause regressions for other
people via aggressive packet dropping.
> Finally this code is executed for each frame, and I don?t want to bloat it
> more than necessary...
If you put the code in minstrel (and minstrel_ht), it not only allows
making a better tradeoff for retry handling, the code also doesn't have
to be run for every single packet. You can run it during the rate
control stats update.
The reduction of retry attempts definitely needs to be balanced
properly. Retries with max_prob_rate can be more important than retries
with max_tp_rate, but there needs to be a minimum for each of those.
- Felix
Hi Felix,
> -----Message d'origine-----
> De?: Felix Fietkau [mailto:[email protected]]
> > ---
> > What I am seeking with this patch:
> > I believe the configuration of the retries will help making recovery
> > much faster when an AP (in infrastructure mode) or a peer (in mesh
> > mode) suddenly disappears.
> I'm all for reducing retries, but I think the way you're applying the
> limit is problematic.
> If minstrel decides to use many retries for a high rate and you're
> simply cutting off all retries that exceed the configured limit, you're
> potentially inviting quite a bit of unnecessary packet loss.
> I'm planning to remove the use of retry rate slot 4 (fallback to lowest
> rate) from minstrel, since max_prob_rate should already provide quite
> decent reliability.
Well, on one hand, people who want to reduce retries are more concerned with
low latency and jitter than with reliable delivery of data.
On another hand the code should work for any rate control plugin, not just
minstrel.
Finally this code is executed for each frame, and I don?t want to bloat it
more than necessary...
I am thinking of trimming only the largest retry count in the table, but not
below 2 (one try and one retry). I'll test this today.
Do you feel this is a good path ?
Or should I just wait for the slot 4 removal ?
I could also split the trimming between 2 rates, the largest count and the
next-to-largest. Or I could use a minimum of 1 instead of 2.
>
> - Felix
Jean-Pierre
On 2013-07-02 7:40 PM, Jean-Pierre Tosoni wrote:
> Hi Felix,
>
> Sorry to use your time again...
>
>> But much more important than that is to not cause regressions for other
>> people via aggressive packet dropping.
>
> Agreed, but see below.
>
>> If you put the code in minstrel (and minstrel_ht), it not only allows
>> making a better tradeoff for retry handling, the code also doesn't have
>> to be run for every single packet. You can run it during the rate
>> control stats update.
>
> OK, I'll have a look at that part now.
>
>>
>> The reduction of retry attempts definitely needs to be balanced
>> properly. Retries with max_prob_rate can be more important than retries
>> with max_tp_rate, but there needs to be a minimum for each of those.
>
> This leads to a question about regressions and backward compatibility:
>
> Since minstrel can compute as much as 28 retries for a frame,
> And since the (standard) default value for "short_frame_max_tx_count" is 7,
>
> ... there is no way I can enforce the configured value while keeping
> minstrel counts by default !
> The standard itself gives a very aggressive limit! Or am I mistaken about
> the significance of this configuration parameter ?
I think you're right about this. Specifically because the standard limit
is much lower than what is being used now, we need to really make sure
that it's applied in a way that it does not harm the normal use case in
any visible way.
- Felix
On 2013-07-01 10:34 AM, Jean-Pierre Tosoni wrote:
> Thanks Felix.
>
> I am using the ath9k, but I have seen this in the b43 driver:
>
> rates[0].count = dev->wl->hw->conf.short_frame_max_tx_count;
>
> Do you think that short/long_frame_max_tx_count should rather be applied at
> the driver level (not mac80211) ?
> The ath9k driver currently enforces a minimum retry count of 30 (constant),
> it could be replaced with short/long_frame_max_tx_count ?
No, I think driver level is even more wrong than generic mac80211 rate
table code. The driver doesn't know more than mac80211 about how to
properly distribute a limited set of retries across different rates.
The only place that can properly control this is the rate control module.
The ath9k retry count of 30 that you're mentioning is software retry -
you should leave that one alone for now and focus on hardware retries.
- Felix