| Message ID | 1372351227-25575-1-git-send-email-jp.tosoni@acksys.fr (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 2013-06-27 6:40 PM, Jean-Pierre Tosoni wrote: > From: J.P. Tosoni <jp.tosoni@acksys.fr> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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:nbd@openwrt.org]
> Envoyé : samedi 29 juin 2013 22:09
> À : Jean-Pierre Tosoni
> Cc : linux-wireless@vger.kernel.org
> 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 <jp.tosoni@acksys.fr>
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Felix, > -----Message d'origine----- > De : Felix Fietkau [mailto:nbd@openwrt.org] > > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-07-02 3:28 PM, Jean-Pierre Tosoni wrote: > Hi Felix, > >> -----Message d'origine----- >> De : Felix Fietkau [mailto:nbd@openwrt.org] >> > --- >> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; } }
From: J.P. Tosoni <jp.tosoni@acksys.fr> 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(-)