Allow rate control modules to pass a rate selection table to mac80211
and the driver. This allows drivers to fetch the most recent rate
selection from the sta pointer for already buffered frames. This allows
rate control to respond faster to sudden link changes and it is also a
step towards adding minstrel_ht support to drivers like iwlwifi.
When a driver sets IEEE80211_HW_SUPPORTS_RC_TABLE, mac80211 will not
fill info->control.rates with rates from the rate table (to preserve
explicit overrides by the rate control module). The driver then
explicitly calls ieee80211_get_tx_rates to merge overrides from
info->control.rates with defaults from the sta rate table.
Signed-off-by: Felix Fietkau <[email protected]>
---
include/net/mac80211.h | 66 +++++++++
net/mac80211/ieee80211_i.h | 1 +
net/mac80211/rate.c | 327 +++++++++++++++++++++++++++++++++++++--------
net/mac80211/tx.c | 142 +++++---------------
4 files changed, 369 insertions(+), 167 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 4f693a5..4ecff78 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -561,6 +561,9 @@ enum mac80211_rate_control_flags {
/* maximum number of rate stages */
#define IEEE80211_TX_MAX_RATES 4
+/* maximum number of rate table entries */
+#define IEEE80211_TX_RATE_TABLE_SIZE 4
+
/**
* struct ieee80211_tx_rate - rate selection/status
*
@@ -657,6 +660,8 @@ struct ieee80211_tx_info {
s8 rts_cts_rate_idx;
u8 use_rts:1;
u8 use_cts_prot:1;
+ u8 short_preamble:1;
+ u8 skip_table:1;
/* 2 bytes free */
};
/* only needed before rate control */
@@ -678,6 +683,8 @@ struct ieee80211_tx_info {
struct {
struct ieee80211_tx_rate driver_rates[
IEEE80211_TX_MAX_RATES];
+ u8 pad[4];
+
void *rate_driver_data[
IEEE80211_TX_INFO_RATE_DRIVER_DATA_SIZE / sizeof(void *)];
};
@@ -1223,6 +1230,24 @@ enum ieee80211_sta_rx_bandwidth {
};
/**
+ * struct ieee80211_sta_rates - station rate selection table
+ *
+ * @rcu_head: RCU head used for freeing the table on update
+ * @rates: transmit rates/flags to be used by default.
+ * Overriding entries per-packet is possible by using cb tx control.
+ */
+struct ieee80211_sta_rates {
+ struct rcu_head rcu_head;
+ struct {
+ s8 idx;
+ u8 count;
+ u8 count_cts;
+ u8 count_rts;
+ u16 flags;
+ } rate[IEEE80211_TX_RATE_TABLE_SIZE];
+};
+
+/**
* struct ieee80211_sta - station table entry
*
* A station table entry represents a station we are possibly
@@ -1249,6 +1274,7 @@ enum ieee80211_sta_rx_bandwidth {
* notifications and capabilities. The value is only valid after
* the station moves to associated state.
* @smps_mode: current SMPS mode (off, static or dynamic)
+ * @tx_rates: rate control selection table
*/
struct ieee80211_sta {
u32 supp_rates[IEEE80211_NUM_BANDS];
@@ -1262,6 +1288,7 @@ struct ieee80211_sta {
u8 rx_nss;
enum ieee80211_sta_rx_bandwidth bandwidth;
enum ieee80211_smps_mode smps_mode;
+ struct ieee80211_sta_rates __rcu *rates;
/* must be last */
u8 drv_priv[0] __aligned(sizeof(void *));
@@ -1417,6 +1444,9 @@ struct ieee80211_tx_control {
* for different virtual interfaces. See the doc section on HW queue
* control for more details.
*
+ * @IEEE80211_HW_SUPPORTS_RC_TABLE: The driver supports using a rate
+ * selection table provided by the rate control algorithm.
+ *
* @IEEE80211_HW_P2P_DEV_ADDR_FOR_INTF: Use the P2P Device address for any
* P2P Interface. This will be honoured even if more than one interface
* is supported.
@@ -1449,6 +1479,7 @@ enum ieee80211_hw_flags {
IEEE80211_HW_SUPPORTS_PER_STA_GTK = 1<<21,
IEEE80211_HW_AP_LINK_PS = 1<<22,
IEEE80211_HW_TX_AMPDU_SETUP_IN_HW = 1<<23,
+ IEEE80211_HW_SUPPORTS_RC_TABLE = 1<<24,
IEEE80211_HW_P2P_DEV_ADDR_FOR_INTF = 1<<25,
IEEE80211_HW_TIMING_BEACON_ONLY = 1<<26,
};
@@ -3135,6 +3166,25 @@ void ieee80211_sta_set_buffered(struct ieee80211_sta *sta,
u8 tid, bool buffered);
/**
+ * ieee80211_get_tx_rates - get the selected transmit rates for a packet
+ *
+ * Call this function in a driver with per-packet rate selection support
+ * to combine the rate info in the packet tx info with the most recent
+ * rate selection table for the station entry.
+ *
+ * @vif: &struct ieee80211_vif pointer from the add_interface callback.
+ * @sta: the receiver station to which this packet is sent.
+ * @skb: the frame to be transmitted.
+ * @dest: buffer for extracted rate/retry information
+ * @max_rates: maximum number of rates to fetch
+ */
+void ieee80211_get_tx_rates(struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
+ struct sk_buff *skb,
+ struct ieee80211_tx_rate *dest,
+ int max_rates);
+
+/**
* ieee80211_tx_status - transmit status callback
*
* Call this function for all transmitted frames after they have been
@@ -4210,6 +4260,22 @@ bool rate_usable_index_exists(struct ieee80211_supported_band *sband,
return false;
}
+/**
+ * rate_control_set_rates - pass the sta rate selection to mac80211/driver
+ *
+ * When not doing a rate control probe to test rates, rate control should pass
+ * its rate selection to mac80211. If the driver supports receiving a station
+ * rate table, it will use it to ensure that frames are always sent based on
+ * the most recent rate control module decision.
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @pubsta: &struct ieee80211_sta pointer to the target destination.
+ * @rates: new tx rate set to be used for this station.
+ */
+int rate_control_set_rates(struct ieee80211_hw *hw,
+ struct ieee80211_sta *pubsta,
+ struct ieee80211_sta_rates *rates);
+
int ieee80211_rate_control_register(struct rate_control_ops *ops);
void ieee80211_rate_control_unregister(struct rate_control_ops *ops);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index af8410e..158e6eb 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -156,6 +156,7 @@ struct ieee80211_tx_data {
struct ieee80211_sub_if_data *sdata;
struct sta_info *sta;
struct ieee80211_key *key;
+ struct ieee80211_tx_rate rate;
unsigned int flags;
};
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 5d545dd..0d51877 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -252,6 +252,25 @@ rate_lowest_non_cck_index(struct ieee80211_supported_band *sband,
return 0;
}
+static void __rate_control_send_low(struct ieee80211_hw *hw,
+ struct ieee80211_supported_band *sband,
+ struct ieee80211_sta *sta,
+ struct ieee80211_tx_info *info)
+{
+ if ((sband->band != IEEE80211_BAND_2GHZ) ||
+ !(info->flags & IEEE80211_TX_CTL_NO_CCK_RATE))
+ info->control.rates[0].idx = rate_lowest_index(sband, sta);
+ else
+ info->control.rates[0].idx =
+ rate_lowest_non_cck_index(sband, sta);
+
+ info->control.rates[0].count =
+ (info->flags & IEEE80211_TX_CTL_NO_ACK) ?
+ 1 : hw->max_rate_tries;
+
+ info->control.skip_table = 1;
+}
+
bool rate_control_send_low(struct ieee80211_sta *sta,
void *priv_sta,
@@ -262,16 +281,8 @@ bool rate_control_send_low(struct ieee80211_sta *sta,
int mcast_rate;
if (!sta || !priv_sta || rc_no_data_or_no_ack_use_min(txrc)) {
- if ((sband->band != IEEE80211_BAND_2GHZ) ||
- !(info->flags & IEEE80211_TX_CTL_NO_CCK_RATE))
- info->control.rates[0].idx =
- rate_lowest_index(txrc->sband, sta);
- else
- info->control.rates[0].idx =
- rate_lowest_non_cck_index(txrc->sband, sta);
- info->control.rates[0].count =
- (info->flags & IEEE80211_TX_CTL_NO_ACK) ?
- 1 : txrc->hw->max_rate_tries;
+ __rate_control_send_low(txrc->hw, sband, sta, info);
+
if (!sta && txrc->bss) {
mcast_rate = txrc->bss_conf->mcast_rate[sband->band];
if (mcast_rate > 0) {
@@ -355,7 +366,8 @@ static bool rate_idx_match_mcs_mask(struct ieee80211_tx_rate *rate,
static void rate_idx_match_mask(struct ieee80211_tx_rate *rate,
- struct ieee80211_tx_rate_control *txrc,
+ struct ieee80211_supported_band *sband,
+ enum nl80211_chan_width chan_width,
u32 mask,
u8 mcs_mask[IEEE80211_HT_MCS_MASK_LEN])
{
@@ -375,27 +387,17 @@ static void rate_idx_match_mask(struct ieee80211_tx_rate *rate,
IEEE80211_TX_RC_USE_SHORT_PREAMBLE);
alt_rate.count = rate->count;
if (rate_idx_match_legacy_mask(&alt_rate,
- txrc->sband->n_bitrates,
- mask)) {
+ sband->n_bitrates, mask)) {
*rate = alt_rate;
return;
}
} else {
- struct sk_buff *skb = txrc->skb;
- struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
- __le16 fc;
-
/* handle legacy rates */
- if (rate_idx_match_legacy_mask(rate, txrc->sband->n_bitrates,
- mask))
+ if (rate_idx_match_legacy_mask(rate, sband->n_bitrates, mask))
return;
/* if HT BSS, and we handle a data frame, also try HT rates */
- if (txrc->bss_conf->chandef.width == NL80211_CHAN_WIDTH_20_NOHT)
- return;
-
- fc = hdr->frame_control;
- if (!ieee80211_is_data(fc))
+ if (chan_width == NL80211_CHAN_WIDTH_20_NOHT)
return;
alt_rate.idx = 0;
@@ -408,7 +410,7 @@ static void rate_idx_match_mask(struct ieee80211_tx_rate *rate,
alt_rate.flags |= IEEE80211_TX_RC_MCS;
- if (txrc->bss_conf->chandef.width == NL80211_CHAN_WIDTH_40)
+ if (chan_width == NL80211_CHAN_WIDTH_40)
alt_rate.flags |= IEEE80211_TX_RC_40_MHZ_WIDTH;
if (rate_idx_match_mcs_mask(&alt_rate, mcs_mask)) {
@@ -426,6 +428,228 @@ static void rate_idx_match_mask(struct ieee80211_tx_rate *rate,
*/
}
+static void rate_fixup_ratelist(struct ieee80211_vif *vif,
+ struct ieee80211_supported_band *sband,
+ struct ieee80211_tx_info *info,
+ struct ieee80211_tx_rate *rates,
+ int max_rates)
+{
+ struct ieee80211_rate *rate;
+ bool inval = false;
+ int i;
+
+ /*
+ * Set up the RTS/CTS rate as the fastest basic rate
+ * that is not faster than the data rate unless there
+ * is no basic rate slower than the data rate, in which
+ * case we pick the slowest basic rate
+ *
+ * XXX: Should this check all retry rates?
+ */
+ if (!(rates[0].flags & IEEE80211_TX_RC_MCS)) {
+ u32 basic_rates = vif->bss_conf.basic_rates;
+ s8 baserate = basic_rates ? ffs(basic_rates - 1) : 0;
+
+ rate = &sband->bitrates[rates[0].idx];
+
+ for (i = 0; i < sband->n_bitrates; i++) {
+ /* must be a basic rate */
+ if (!(basic_rates & BIT(i)))
+ continue;
+ /* must not be faster than the data rate */
+ if (sband->bitrates[i].bitrate > rate->bitrate)
+ continue;
+ /* maximum */
+ if (sband->bitrates[baserate].bitrate <
+ sband->bitrates[i].bitrate)
+ baserate = i;
+ }
+
+ info->control.rts_cts_rate_idx = baserate;
+ }
+
+ for (i = 0; i < max_rates; i++) {
+ /*
+ * make sure there's no valid rate following
+ * an invalid one, just in case drivers don't
+ * take the API seriously to stop at -1.
+ */
+ if (inval) {
+ rates[i].idx = -1;
+ continue;
+ }
+ if (rates[i].idx < 0) {
+ inval = true;
+ continue;
+ }
+
+ /*
+ * For now assume MCS is already set up correctly, this
+ * needs to be fixed.
+ */
+ if (rates[i].flags & IEEE80211_TX_RC_MCS) {
+ WARN_ON(rates[i].idx > 76);
+
+ if (!(rates[i].flags & IEEE80211_TX_RC_USE_RTS_CTS) &&
+ info->control.use_cts_prot)
+ rates[i].flags |=
+ IEEE80211_TX_RC_USE_CTS_PROTECT;
+ continue;
+ }
+
+ if (rates[i].flags & IEEE80211_TX_RC_VHT_MCS) {
+ WARN_ON(ieee80211_rate_get_vht_mcs(&rates[i]) > 9);
+ continue;
+ }
+
+ /* set up RTS protection if desired */
+ if (info->control.use_rts) {
+ rates[i].flags |= IEEE80211_TX_RC_USE_RTS_CTS;
+ info->control.use_cts_prot = false;
+ }
+
+ /* RC is busted */
+ if (WARN_ON_ONCE(rates[i].idx >= sband->n_bitrates)) {
+ rates[i].idx = -1;
+ continue;
+ }
+
+ rate = &sband->bitrates[rates[i].idx];
+
+ /* set up short preamble */
+ if (info->control.short_preamble &&
+ rate->flags & IEEE80211_RATE_SHORT_PREAMBLE)
+ rates[i].flags |= IEEE80211_TX_RC_USE_SHORT_PREAMBLE;
+
+ /* set up G protection */
+ if (!(rates[i].flags & IEEE80211_TX_RC_USE_RTS_CTS) &&
+ info->control.use_cts_prot &&
+ rate->flags & IEEE80211_RATE_ERP_G)
+ rates[i].flags |= IEEE80211_TX_RC_USE_CTS_PROTECT;
+ }
+}
+
+
+static void rate_control_fill_sta_table(struct ieee80211_sta *sta,
+ struct ieee80211_tx_info *info,
+ struct ieee80211_tx_rate *rates,
+ int max_rates)
+{
+ struct ieee80211_sta_rates *ratetbl = NULL;
+ int i;
+
+ if (sta && !info->control.skip_table)
+ ratetbl = rcu_dereference(sta->rates);
+
+ /* 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) &&
+ info->control.rates[i].idx >= 0 &&
+ info->control.rates[i].count) {
+ if (rates != info->control.rates)
+ rates[i] = info->control.rates[i];
+ } else if (ratetbl) {
+ rates[i].idx = ratetbl->rate[i].idx;
+ rates[i].flags = ratetbl->rate[i].flags;
+ if (info->control.use_rts)
+ rates[i].count = ratetbl->rate[i].count_rts;
+ else if (info->control.use_cts_prot)
+ rates[i].count = ratetbl->rate[i].count_cts;
+ else
+ rates[i].count = ratetbl->rate[i].count;
+ } else {
+ rates[i].idx = -1;
+ rates[i].count = 0;
+ }
+
+ if (rates[i].idx < 0 || !rates[i].count)
+ break;
+ }
+}
+
+static void rate_control_apply_mask(struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_sta *sta,
+ struct ieee80211_supported_band *sband,
+ struct ieee80211_tx_info *info,
+ struct ieee80211_tx_rate *rates,
+ int max_rates)
+{
+ enum nl80211_chan_width chan_width;
+ u8 mcs_mask[IEEE80211_HT_MCS_MASK_LEN];
+ bool has_mcs_mask;
+ u32 mask;
+ int i;
+
+ /*
+ * Try to enforce the rateidx mask the user wanted. skip this if the
+ * default mask (allow all rates) is used to save some processing for
+ * the common case.
+ */
+ mask = sdata->rc_rateidx_mask[info->band];
+ has_mcs_mask = sdata->rc_has_mcs_mask[info->band];
+ if (mask == (1 << sband->n_bitrates) - 1 && !has_mcs_mask)
+ return;
+
+ if (has_mcs_mask)
+ memcpy(mcs_mask, sdata->rc_rateidx_mcs_mask[info->band],
+ sizeof(mcs_mask));
+ else
+ memset(mcs_mask, 0xff, sizeof(mcs_mask));
+
+ if (sta) {
+ /* Filter out rates that the STA does not support */
+ mask &= sta->supp_rates[info->band];
+ for (i = 0; i < sizeof(mcs_mask); i++)
+ mcs_mask[i] &= sta->ht_cap.mcs.rx_mask[i];
+ }
+
+ /*
+ * Make sure the rate index selected for each TX rate is
+ * included in the configured mask and change the rate indexes
+ * if needed.
+ */
+ chan_width = sdata->vif.bss_conf.chandef.width;
+ for (i = 0; i < max_rates; i++) {
+ /* Skip invalid rates */
+ if (rates[i].idx < 0)
+ break;
+
+ rate_idx_match_mask(&rates[i], sband, mask, chan_width,
+ mcs_mask);
+ }
+}
+
+void ieee80211_get_tx_rates(struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
+ struct sk_buff *skb,
+ struct ieee80211_tx_rate *dest,
+ int max_rates)
+{
+ struct ieee80211_sub_if_data *sdata;
+ struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
+ struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+ struct ieee80211_supported_band *sband;
+
+ rate_control_fill_sta_table(sta, info, dest, max_rates);
+
+ if (!vif)
+ return;
+
+ sdata = vif_to_sdata(vif);
+ sband = sdata->local->hw.wiphy->bands[info->band];
+
+ if (ieee80211_is_data(hdr->frame_control))
+ rate_control_apply_mask(sdata, sta, sband, info, dest, max_rates);
+
+ if (dest[0].idx < 0)
+ __rate_control_send_low(&sdata->local->hw, sband, sta, info);
+
+ if (sta)
+ rate_fixup_ratelist(vif, sband, info, dest, max_rates);
+}
+EXPORT_SYMBOL(ieee80211_get_tx_rates);
+
void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
struct sta_info *sta,
struct ieee80211_tx_rate_control *txrc)
@@ -435,8 +659,6 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
struct ieee80211_sta *ista = NULL;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(txrc->skb);
int i;
- u32 mask;
- u8 mcs_mask[IEEE80211_HT_MCS_MASK_LEN];
if (sta && test_sta_flag(sta, WLAN_STA_RATE_CONTROL)) {
ista = &sta->sta;
@@ -454,40 +676,27 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
ref->ops->get_rate(ref->priv, ista, priv_sta, txrc);
- /*
- * Try to enforce the rateidx mask the user wanted. skip this if the
- * default mask (allow all rates) is used to save some processing for
- * the common case.
- */
- mask = sdata->rc_rateidx_mask[info->band];
- if (mask != (1 << txrc->sband->n_bitrates) - 1 || txrc->rate_idx_mcs_mask) {
- if (txrc->rate_idx_mcs_mask)
- memcpy(mcs_mask, txrc->rate_idx_mcs_mask, sizeof(mcs_mask));
- else
- memset(mcs_mask, 0xff, sizeof(mcs_mask));
-
- if (sta) {
- /* Filter out rates that the STA does not support */
- mask &= sta->sta.supp_rates[info->band];
- for (i = 0; i < sizeof(mcs_mask); i++)
- mcs_mask[i] &= sta->sta.ht_cap.mcs.rx_mask[i];
- }
- /*
- * Make sure the rate index selected for each TX rate is
- * included in the configured mask and change the rate indexes
- * if needed.
- */
- for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
- /* Skip invalid rates */
- if (info->control.rates[i].idx < 0)
- break;
- rate_idx_match_mask(&info->control.rates[i], txrc,
- mask, mcs_mask);
- }
- }
+ if (sdata->local->hw.flags & IEEE80211_HW_SUPPORTS_RC_TABLE)
+ return;
+
+ ieee80211_get_tx_rates(&sdata->vif, ista, txrc->skb,
+ info->control.rates,
+ ARRAY_SIZE(info->control.rates));
+}
- BUG_ON(info->control.rates[0].idx < 0);
+int rate_control_set_rates(struct ieee80211_hw *hw,
+ struct ieee80211_sta *pubsta,
+ struct ieee80211_sta_rates *rates)
+{
+ struct ieee80211_sta_rates *old = rcu_dereference(pubsta->rates);
+
+ rcu_assign_pointer(pubsta->rates, rates);
+ if (old)
+ kfree_rcu(old, rcu_head);
+
+ return 0;
}
+EXPORT_SYMBOL(rate_control_set_rates);
int ieee80211_init_rate_ctrl_alg(struct ieee80211_local *local,
const char *name)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 6ca857f..4a5fbf8 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -48,15 +48,15 @@ static __le16 ieee80211_duration(struct ieee80211_tx_data *tx,
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
/* assume HW handles this */
- if (info->control.rates[0].flags & IEEE80211_TX_RC_MCS)
+ if (tx->rate.flags & IEEE80211_TX_RC_MCS)
return 0;
/* uh huh? */
- if (WARN_ON_ONCE(info->control.rates[0].idx < 0))
+ if (WARN_ON_ONCE(tx->rate.idx < 0))
return 0;
sband = local->hw.wiphy->bands[info->band];
- txrate = &sband->bitrates[info->control.rates[0].idx];
+ txrate = &sband->bitrates[tx->rate.idx];
erp = txrate->flags & IEEE80211_RATE_ERP_G;
@@ -617,11 +617,9 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
struct ieee80211_hdr *hdr = (void *)tx->skb->data;
struct ieee80211_supported_band *sband;
- struct ieee80211_rate *rate;
- int i;
u32 len;
- bool inval = false, rts = false, short_preamble = false;
struct ieee80211_tx_rate_control txrc;
+ struct ieee80211_sta_rates *ratetbl = NULL;
bool assoc = false;
memset(&txrc, 0, sizeof(txrc));
@@ -653,10 +651,10 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
/* set up RTS protection if desired */
if (len > tx->local->hw.wiphy->rts_threshold) {
- txrc.rts = rts = true;
+ txrc.rts = true;
}
- info->control.use_rts = rts;
+ info->control.use_rts = txrc.rts;
info->control.use_cts_prot = tx->sdata->vif.bss_conf.use_cts_prot;
/*
@@ -668,7 +666,9 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
if (tx->sdata->vif.bss_conf.use_short_preamble &&
(ieee80211_is_data(hdr->frame_control) ||
(tx->sta && test_sta_flag(tx->sta, WLAN_STA_SHORT_PREAMBLE))))
- txrc.short_preamble = short_preamble = true;
+ txrc.short_preamble = true;
+
+ info->control.short_preamble = txrc.short_preamble;
if (tx->sta)
assoc = test_sta_flag(tx->sta, WLAN_STA_ASSOC);
@@ -692,16 +692,38 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
*/
rate_control_get_rate(tx->sdata, tx->sta, &txrc);
- if (unlikely(info->control.rates[0].idx < 0))
- return TX_DROP;
+ if (tx->sta && !info->control.skip_table)
+ ratetbl = rcu_dereference(tx->sta->sta.rates);
+
+ if (unlikely(info->control.rates[0].idx < 0)) {
+ if (ratetbl) {
+ struct ieee80211_tx_rate rate = {
+ .idx = ratetbl->rate[0].idx,
+ .flags = ratetbl->rate[0].flags,
+ .count = ratetbl->rate[0].count
+ };
+
+ if (ratetbl->rate[0].idx < 0)
+ return TX_DROP;
+
+ tx->rate = rate;
+ } else {
+ return TX_DROP;
+ }
+ } else {
+ tx->rate = info->control.rates[0];
+ }
if (txrc.reported_rate.idx < 0) {
- txrc.reported_rate = info->control.rates[0];
+ txrc.reported_rate = tx->rate;
if (tx->sta && ieee80211_is_data(hdr->frame_control))
tx->sta->last_tx_rate = txrc.reported_rate;
} else if (tx->sta)
tx->sta->last_tx_rate = txrc.reported_rate;
+ if (ratetbl)
+ return TX_CONTINUE;
+
if (unlikely(!info->control.rates[0].count))
info->control.rates[0].count = 1;
@@ -709,102 +731,6 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
(info->flags & IEEE80211_TX_CTL_NO_ACK)))
info->control.rates[0].count = 1;
- if (is_multicast_ether_addr(hdr->addr1)) {
- /*
- * XXX: verify the rate is in the basic rateset
- */
- return TX_CONTINUE;
- }
-
- /*
- * Set up the RTS/CTS rate as the fastest basic rate
- * that is not faster than the data rate unless there
- * is no basic rate slower than the data rate, in which
- * case we pick the slowest basic rate
- *
- * XXX: Should this check all retry rates?
- */
- if (!(info->control.rates[0].flags & IEEE80211_TX_RC_MCS)) {
- u32 basic_rates = tx->sdata->vif.bss_conf.basic_rates;
- s8 baserate = basic_rates ? ffs(basic_rates - 1) : 0;
-
- rate = &sband->bitrates[info->control.rates[0].idx];
-
- for (i = 0; i < sband->n_bitrates; i++) {
- /* must be a basic rate */
- if (!(basic_rates & BIT(i)))
- continue;
- /* must not be faster than the data rate */
- if (sband->bitrates[i].bitrate > rate->bitrate)
- continue;
- /* maximum */
- if (sband->bitrates[baserate].bitrate <
- sband->bitrates[i].bitrate)
- baserate = i;
- }
-
- info->control.rts_cts_rate_idx = baserate;
- }
-
- for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
- struct ieee80211_tx_rate *rc_rate = &info->control.rates[i];
-
- /*
- * make sure there's no valid rate following
- * an invalid one, just in case drivers don't
- * take the API seriously to stop at -1.
- */
- if (inval) {
- rc_rate->idx = -1;
- continue;
- }
- if (rc_rate->idx < 0) {
- inval = true;
- continue;
- }
-
- /*
- * For now assume MCS is already set up correctly, this
- * needs to be fixed.
- */
- if (rc_rate->flags & IEEE80211_TX_RC_MCS) {
- WARN_ON(rc_rate->idx > 76);
-
- if (!(rc_rate->flags & IEEE80211_TX_RC_USE_RTS_CTS) &&
- tx->sdata->vif.bss_conf.use_cts_prot)
- rc_rate->flags |=
- IEEE80211_TX_RC_USE_CTS_PROTECT;
- continue;
- }
-
- if (rc_rate->flags & IEEE80211_TX_RC_VHT_MCS) {
- WARN_ON(ieee80211_rate_get_vht_mcs(rc_rate) > 9);
- continue;
- }
-
- /* set up RTS protection if desired */
- if (rts)
- rc_rate->flags |= IEEE80211_TX_RC_USE_RTS_CTS;
-
- /* RC is busted */
- if (WARN_ON_ONCE(rc_rate->idx >= sband->n_bitrates)) {
- rc_rate->idx = -1;
- continue;
- }
-
- rate = &sband->bitrates[rc_rate->idx];
-
- /* set up short preamble */
- if (short_preamble &&
- rate->flags & IEEE80211_RATE_SHORT_PREAMBLE)
- rc_rate->flags |= IEEE80211_TX_RC_USE_SHORT_PREAMBLE;
-
- /* set up G protection */
- if (!rts && tx->sdata->vif.bss_conf.use_cts_prot &&
- rate->flags & IEEE80211_RATE_ERP_G)
- rc_rate->flags |= IEEE80211_TX_RC_USE_CTS_PROTECT;
- }
-
return TX_CONTINUE;
}
--
1.8.0.2
Pass the rate selection table to mac80211 from minstrel_ht_update_stats.
Only rates for sample attempts are set in info->control.rates.
Signed-off-by: Felix Fietkau <[email protected]>
---
net/mac80211/rc80211_minstrel_ht.c | 150 ++++++++++++++++++++-----------------
net/mac80211/rc80211_minstrel_ht.h | 2 +
2 files changed, 84 insertions(+), 68 deletions(-)
diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
index a8e979e..5b2d301 100644
--- a/net/mac80211/rc80211_minstrel_ht.c
+++ b/net/mac80211/rc80211_minstrel_ht.c
@@ -126,6 +126,9 @@ const struct mcs_group minstrel_mcs_groups[] = {
static u8 sample_table[SAMPLE_COLUMNS][MCS_GROUP_RATES];
+static void
+minstrel_ht_update_rates(struct minstrel_priv *mp, struct minstrel_ht_sta *mi);
+
/*
* Look up an MCS group index based on mac80211 rate information
*/
@@ -465,7 +468,7 @@ minstrel_ht_tx_status(void *priv, struct ieee80211_supported_band *sband,
struct ieee80211_tx_rate *ar = info->status.rates;
struct minstrel_rate_stats *rate, *rate2;
struct minstrel_priv *mp = priv;
- bool last;
+ bool last, update = false;
int i;
if (!msp->is_ht)
@@ -514,21 +517,29 @@ minstrel_ht_tx_status(void *priv, struct ieee80211_supported_band *sband,
rate = minstrel_get_ratestats(mi, mi->max_tp_rate);
if (rate->attempts > 30 &&
MINSTREL_FRAC(rate->success, rate->attempts) <
- MINSTREL_FRAC(20, 100))
+ MINSTREL_FRAC(20, 100)) {
minstrel_downgrade_rate(mi, &mi->max_tp_rate, true);
+ update = true;
+ }
rate2 = minstrel_get_ratestats(mi, mi->max_tp_rate2);
if (rate2->attempts > 30 &&
MINSTREL_FRAC(rate2->success, rate2->attempts) <
- MINSTREL_FRAC(20, 100))
+ MINSTREL_FRAC(20, 100)) {
minstrel_downgrade_rate(mi, &mi->max_tp_rate2, false);
+ update = true;
+ }
if (time_after(jiffies, mi->stats_update + (mp->update_interval / 2 * HZ) / 1000)) {
+ update = true;
minstrel_ht_update_stats(mp, mi);
if (!(info->flags & IEEE80211_TX_CTL_AMPDU) &&
mi->max_prob_rate / MCS_GROUP_RATES != MINSTREL_CCK_GROUP)
minstrel_aggr_check(sta, skb);
}
+
+ if (update)
+ minstrel_ht_update_rates(mp, mi);
}
static void
@@ -592,36 +603,71 @@ minstrel_calc_retransmit(struct minstrel_priv *mp, struct minstrel_ht_sta *mi,
static void
minstrel_ht_set_rate(struct minstrel_priv *mp, struct minstrel_ht_sta *mi,
- struct ieee80211_tx_rate *rate, int index,
- bool sample, bool rtscts)
+ struct ieee80211_sta_rates *ratetbl, int offset, int index)
{
const struct mcs_group *group = &minstrel_mcs_groups[index / MCS_GROUP_RATES];
struct minstrel_rate_stats *mr;
+ u8 idx;
+ u16 flags;
mr = minstrel_get_ratestats(mi, index);
if (!mr->retry_updated)
minstrel_calc_retransmit(mp, mi, index);
- if (sample)
- rate->count = 1;
- else if (mr->probability < MINSTREL_FRAC(20, 100))
- rate->count = 2;
- else if (rtscts)
- rate->count = mr->retry_count_rtscts;
- else
- rate->count = mr->retry_count;
-
- rate->flags = 0;
- if (rtscts)
- rate->flags |= IEEE80211_TX_RC_USE_RTS_CTS;
+ if (mr->probability < MINSTREL_FRAC(20, 100) || !mr->retry_count) {
+ ratetbl->rate[offset].count = 2;
+ ratetbl->rate[offset].count_rts = 2;
+ ratetbl->rate[offset].count_cts = 2;
+ } else {
+ ratetbl->rate[offset].count = mr->retry_count;
+ ratetbl->rate[offset].count_cts = mr->retry_count;
+ ratetbl->rate[offset].count_rts = mr->retry_count_rtscts;
+ }
if (index / MCS_GROUP_RATES == MINSTREL_CCK_GROUP) {
- rate->idx = mp->cck_rates[index % ARRAY_SIZE(mp->cck_rates)];
+ idx = mp->cck_rates[index % ARRAY_SIZE(mp->cck_rates)];
+ flags = 0;
+ } else {
+ idx = index % MCS_GROUP_RATES +
+ (group->streams - 1) * MCS_GROUP_RATES;
+ flags = IEEE80211_TX_RC_MCS | group->flags;
+ }
+
+ if (offset > 0) {
+ ratetbl->rate[offset].count = ratetbl->rate[offset].count_rts;
+ flags |= IEEE80211_TX_RC_USE_RTS_CTS;
+ }
+
+ ratetbl->rate[offset].idx = idx;
+ ratetbl->rate[offset].flags = flags;
+}
+
+static void
+minstrel_ht_update_rates(struct minstrel_priv *mp, struct minstrel_ht_sta *mi)
+{
+ struct ieee80211_sta_rates *rates;
+ int i = 0;
+
+ rates = kzalloc(sizeof(*rates), GFP_ATOMIC);
+ if (!rates)
return;
+
+ /* Start with max_tp_rate */
+ minstrel_ht_set_rate(mp, mi, rates, i++, mi->max_tp_rate);
+
+ if (mp->hw->max_rates >= 3) {
+ /* At least 3 tx rates supported, use max_tp_rate2 next */
+ minstrel_ht_set_rate(mp, mi, rates, i++, mi->max_tp_rate2);
+ }
+
+ if (mp->hw->max_rates >= 2) {
+ /*
+ * At least 2 tx rates supported, use max_prob_rate next */
+ minstrel_ht_set_rate(mp, mi, rates, i++, mi->max_prob_rate);
}
- rate->flags |= IEEE80211_TX_RC_MCS | group->flags;
- rate->idx = index % MCS_GROUP_RATES + (group->streams - 1) * MCS_GROUP_RATES;
+ rates->rate[i].idx = -1;
+ rate_control_set_rates(mp->hw, mi->sta, rates);
}
static inline int
@@ -711,13 +757,13 @@ static void
minstrel_ht_get_rate(void *priv, struct ieee80211_sta *sta, void *priv_sta,
struct ieee80211_tx_rate_control *txrc)
{
+ const struct mcs_group *sample_group;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(txrc->skb);
- struct ieee80211_tx_rate *ar = info->status.rates;
+ struct ieee80211_tx_rate *rate = &info->status.rates[0];
struct minstrel_ht_sta_priv *msp = priv_sta;
struct minstrel_ht_sta *mi = &msp->ht;
struct minstrel_priv *mp = priv;
int sample_idx;
- bool sample = false;
if (rate_control_send_low(sta, priv_sta, txrc))
return;
@@ -745,51 +791,6 @@ minstrel_ht_get_rate(void *priv, struct ieee80211_sta *sta, void *priv_sta,
}
#endif
- if (sample_idx >= 0) {
- sample = true;
- minstrel_ht_set_rate(mp, mi, &ar[0], sample_idx,
- true, false);
- info->flags |= IEEE80211_TX_CTL_RATE_CTRL_PROBE;
- } else {
- minstrel_ht_set_rate(mp, mi, &ar[0], mi->max_tp_rate,
- false, false);
- }
-
- if (mp->hw->max_rates >= 3) {
- /*
- * At least 3 tx rates supported, use
- * sample_rate -> max_tp_rate -> max_prob_rate for sampling and
- * max_tp_rate -> max_tp_rate2 -> max_prob_rate by default.
- */
- if (sample_idx >= 0)
- minstrel_ht_set_rate(mp, mi, &ar[1], mi->max_tp_rate,
- false, false);
- else
- minstrel_ht_set_rate(mp, mi, &ar[1], mi->max_tp_rate2,
- false, true);
-
- minstrel_ht_set_rate(mp, mi, &ar[2], mi->max_prob_rate,
- false, !sample);
-
- ar[3].count = 0;
- ar[3].idx = -1;
- } else if (mp->hw->max_rates == 2) {
- /*
- * Only 2 tx rates supported, use
- * sample_rate -> max_prob_rate for sampling and
- * max_tp_rate -> max_prob_rate by default.
- */
- minstrel_ht_set_rate(mp, mi, &ar[1], mi->max_prob_rate,
- false, !sample);
-
- ar[2].count = 0;
- ar[2].idx = -1;
- } else {
- /* Not using MRR, only use the first rate */
- ar[1].count = 0;
- ar[1].idx = -1;
- }
-
mi->total_packets++;
/* wraparound */
@@ -797,6 +798,16 @@ minstrel_ht_get_rate(void *priv, struct ieee80211_sta *sta, void *priv_sta,
mi->total_packets = 0;
mi->sample_packets = 0;
}
+
+ if (sample_idx < 0)
+ return;
+
+ sample_group = &minstrel_mcs_groups[sample_idx / MCS_GROUP_RATES];
+ info->flags |= IEEE80211_TX_CTL_RATE_CTRL_PROBE;
+ rate->idx = sample_idx % MCS_GROUP_RATES +
+ (sample_group->streams - 1) * MCS_GROUP_RATES;
+ rate->flags = IEEE80211_TX_RC_MCS | sample_group->flags;
+ rate->count = 1;
}
static void
@@ -846,6 +857,8 @@ minstrel_ht_update_caps(void *priv, struct ieee80211_supported_band *sband,
msp->is_ht = true;
memset(mi, 0, sizeof(*mi));
+
+ mi->sta = sta;
mi->stats_update = jiffies;
ack_dur = ieee80211_frame_duration(sband->band, 10, 60, 1, 1);
@@ -907,8 +920,9 @@ minstrel_ht_update_caps(void *priv, struct ieee80211_supported_band *sband,
if (!n_supported)
goto use_legacy;
- /* init {mi,mi->groups[*]}->{max_tp_rate,max_tp_rate2,max_prob_rate} */
+ /* create an initial rate table with the lowest supported rates */
minstrel_ht_update_stats(mp, mi);
+ minstrel_ht_update_rates(mp, mi);
return;
diff --git a/net/mac80211/rc80211_minstrel_ht.h b/net/mac80211/rc80211_minstrel_ht.h
index 9b16e9d..d655586 100644
--- a/net/mac80211/rc80211_minstrel_ht.h
+++ b/net/mac80211/rc80211_minstrel_ht.h
@@ -65,6 +65,8 @@ struct minstrel_mcs_group_data {
};
struct minstrel_ht_sta {
+ struct ieee80211_sta *sta;
+
/* ampdu length (average, per sampling interval) */
unsigned int ampdu_len;
unsigned int ampdu_packets;
--
1.8.0.2
On Tuesday, April 23, 2013 08:48:28 AM Johannes Berg wrote:
> On Tue, 2013-04-23 at 02:58 +0200, Christian Lamparter wrote:
> > This patch fixes the following RCU debug splat:
> >
> > ===============================
> > [ INFO: suspicious RCU usage. ]
> > 3.9.0-rc8-wl+ #31 Tainted: G O
> > -------------------------------
> > net/mac80211/rate.c:691 suspicious rcu_dereference_check() usage!
> >
> > other info that might help us debug this:
> >
> > rcu_scheduler_active = 1, debug_locks = 1
> > 3 locks held by hostapd/9451:
> > #0: (genl_mutex){+.+.+.}, at: [<c1326365>] genl_lock+0xf/0x11
> > #1: (rtnl_mutex){+.+.+.}, at: [<c13133c4>] rtnl_lock+0xf/0x11
> > #2: (&rdev->mtx){+.+.+.}, at: [<f853395e>] nl80211_pre_doit+0x166/0x180 [cfg80211]
> >
> > stack backtrace:
> > Pid: 9451, comm: hostapd Tainted: G O 3.9.0-rc8-wl+ #31
> > Call Trace:
> > [<c107da0b>] lockdep_rcu_suspicious+0xe6/0xee
> > [<f8bf82ad>] rate_control_set_rates+0x43/0x5a [mac80211]
> > [<f8c2cacb>] minstrel_update_rates+0xdc/0xe2 [mac80211]
> > [<f8c2cfb0>] minstrel_rate_init+0x24c/0x33d [mac80211]
> > [<f8c2d9d3>] minstrel_ht_update_caps+0x206/0x234 [mac80211]
> > [<c1080a8d>] ? lock_release+0x1c9/0x226
> > [<f8c2da25>] minstrel_ht_rate_init+0x10/0x14 [mac80211]
> > [...]
> >
> > Signed-off-by: Christian Lamparter <[email protected]>
> > ---
> > Actually, rcu_read_lock() might not be necessary in this special
> > case [the RC is not yet initialized, so nothing bad can happen].
> >
> > But, since the rcu_read_lock() has a low overhead and
> > rate_control_set_rates mac80211.h doc does not mention
> > anything about locking, I think this is a viable way.
>
> I think that, on the contrary, it's completely strange/wrong. ;-)
Sorry, I think I cut too much from the stack trace and I didn't
explain how the code end up in this case. This time, I commented out
the rcu_read_(un)lock() [=> rate.c:694 is rate.c:691 in wireless-testing.git]
and started hostapd and let a station connect. (see attached log)
> > + rcu_read_lock();
> > + old = rcu_dereference(pubsta->rates);
>
> Here's have a dereference.
>
> > rcu_assign_pointer(pubsta->rates, rates);
>
> and here's an assignment. The assignment ought to be protected already
> by some locking, presumably, so similarly is the rcu_dereference() which
> then should just be rcu_dereference_protected()?
The issue seems to be in ieee80211_add_station in net/mac80211/cfg.c.
This function allocates, initializes and adds the new station for
hostapd. And of course: the alloc and (rate_)init part is done without
acquiring any special mac80211 locks. (just rtnl, genl and rdev->mtx).
[And why should it? After all, during initialization, the station is
not yet in the station hash table.]
So, what else can be done?
Obviously, the locking requirement needs to be added to the
doc entry for rate_control_set_rates in include/net/mac80211.h.
And one of the following changes:
1. move the rate_control_rate_init after sta_info_insert_rcu
and remove the rcu_read_locks from rate_control_set_rates.
However then we would add an incomplete station (this can't be right?!).
2. add rcu or other lock around rate_control_set_rates in
minstrel_update_rates() and minstrel_ht_update_rates().
3. add a new function: rate_control_init_rates which is
reserved for this case and only does the assignment.
(4. use rcu_dereference_protected and test the rtnl_lock - really?)
(5. some other way?)
Regards,
Christian
---
===============================
[ INFO: suspicious RCU usage. ]
3.9.0-rc8-wl+ #32 Tainted: G O
------------------------------
net/mac80211/rate.c:694 suspicious rcu_dereference_check() usage!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 1
3 locks held by hostapd/2906:
#0: (genl_mutex){+.+.+.}, at: [<c1326365>] genl_lock+0xf/0x11
#1: (rtnl_mutex){+.+.+.}, at: [<c13133c4>] rtnl_lock+0xf/0x11
#2: (&rdev->mtx){+.+.+.}, at: [<f852195e>] nl80211_pre_doit+0x166/0x180 [cfg80211]
stack backtrace:
Pid: 2906, comm: hostapd Tainted: G O 3.9.0-rc8-wl+ #32
Call Trace:
[<c107da0b>] lockdep_rcu_suspicious+0xe6/0xee
[<f884835f>] rate_control_set_rates+0x43/0x5a [mac80211]
[<f8882e52>] minstrel_ht_update_rates+0x9f/0xa7 [mac80211]
[<f88833ec>] minstrel_ht_update_caps+0x1cf/0x234 [mac80211]
[<c1080a8d>] ? lock_release+0x1c9/0x226
[<f8883475>] minstrel_ht_rate_init+0x10/0x14 [mac80211]
[<f884d326>] rate_control_rate_init+0xc4/0xd8 [mac80211]
[<f884e219>] ieee80211_add_station+0xdc/0x11b [mac80211]
[<f8526595>] nl80211_new_station+0x27e/0x2c7 [cfg80211]
[<c132653d>] genl_rcv_msg+0x1b6/0x1ee
[<c1326387>] ? genl_rcv+0x20/0x20
[The full unaltered trace is available at: <http://pastebin.com/gYc8yAqB>]
Pass the rate selection table to mac80211 from minstrel_update_stats.
Only rates for sample attempts are set in info->control.rates, with deferred
sampling, only the second slot gets changed.
Signed-off-by: Felix Fietkau <[email protected]>
---
net/mac80211/rc80211_minstrel.c | 200 ++++++++++++++++++++++------------------
net/mac80211/rc80211_minstrel.h | 2 +
2 files changed, 111 insertions(+), 91 deletions(-)
diff --git a/net/mac80211/rc80211_minstrel.c b/net/mac80211/rc80211_minstrel.c
index eda290f..ac7ef54 100644
--- a/net/mac80211/rc80211_minstrel.c
+++ b/net/mac80211/rc80211_minstrel.c
@@ -84,6 +84,50 @@ minstrel_sort_best_tp_rates(struct minstrel_sta_info *mi, int i, u8 *tp_list)
}
static void
+minstrel_set_rate(struct minstrel_sta_info *mi, struct ieee80211_sta_rates *ratetbl,
+ int offset, int idx)
+{
+ struct minstrel_rate *r = &mi->r[idx];
+
+ ratetbl->rate[offset].idx = r->rix;
+ ratetbl->rate[offset].count = r->adjusted_retry_count;
+ ratetbl->rate[offset].count_cts = r->retry_count_cts;
+ ratetbl->rate[offset].count_rts = r->retry_count_rtscts;
+}
+
+static void
+minstrel_update_rates(struct minstrel_priv *mp, struct minstrel_sta_info *mi)
+{
+ struct ieee80211_sta_rates *ratetbl;
+ int i = 0;
+
+ ratetbl = kzalloc(sizeof(*ratetbl), GFP_ATOMIC);
+ if (!ratetbl)
+ return;
+
+ /* Start with max_tp_rate */
+ minstrel_set_rate(mi, ratetbl, i++, mi->max_tp_rate[0]);
+
+ if (mp->hw->max_rates >= 3) {
+ /* At least 3 tx rates supported, use max_tp_rate2 next */
+ minstrel_set_rate(mi, ratetbl, i++, mi->max_tp_rate[1]);
+ }
+
+ if (mp->hw->max_rates >= 2) {
+ /* At least 2 tx rates supported, use max_prob_rate next */
+ minstrel_set_rate(mi, ratetbl, i++, mi->max_prob_rate);
+ }
+
+ /* Use lowest rate last */
+ ratetbl->rate[i].idx = mi->lowest_rix;
+ ratetbl->rate[i].count = mp->max_retry;
+ ratetbl->rate[i].count_cts = mp->max_retry;
+ ratetbl->rate[i].count_rts = mp->max_retry;
+
+ rate_control_set_rates(mp->hw, mi->sta, ratetbl);
+}
+
+static void
minstrel_update_stats(struct minstrel_priv *mp, struct minstrel_sta_info *mi)
{
u8 tmp_tp_rate[MAX_THR_RATES];
@@ -161,6 +205,8 @@ minstrel_update_stats(struct minstrel_priv *mp, struct minstrel_sta_info *mi)
/* Reset update timer */
mi->stats_update = jiffies;
+
+ minstrel_update_rates(mp, mi);
}
static void
@@ -240,13 +286,12 @@ minstrel_get_rate(void *priv, struct ieee80211_sta *sta,
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
struct minstrel_sta_info *mi = priv_sta;
struct minstrel_priv *mp = priv;
- struct ieee80211_tx_rate *ar = info->control.rates;
- unsigned int ndx, sample_ndx = 0;
+ struct ieee80211_tx_rate *rate = &info->control.rates[0];
+ struct minstrel_rate *msr, *mr;
+ unsigned int ndx;
bool mrr_capable;
- bool indirect_rate_sampling = false;
- bool rate_sampling = false;
- int i, delta;
- int mrr_ndx[3];
+ bool prev_sample = mi->prev_sample;
+ int delta;
int sampling_ratio;
/* management/no-ack frames do not use rate control */
@@ -262,107 +307,75 @@ minstrel_get_rate(void *priv, struct ieee80211_sta *sta,
else
sampling_ratio = mp->lookaround_rate;
- /* init rateindex [ndx] with max throughput rate */
- ndx = mi->max_tp_rate[0];
-
/* increase sum packet counter */
mi->packet_count++;
delta = (mi->packet_count * sampling_ratio / 100) -
(mi->sample_count + mi->sample_deferred / 2);
- /* delta > 0: sampling required */
- if ((delta > 0) && (mrr_capable || !mi->prev_sample)) {
- struct minstrel_rate *msr;
- if (mi->packet_count >= 10000) {
- mi->sample_deferred = 0;
- mi->sample_count = 0;
- mi->packet_count = 0;
- } else if (delta > mi->n_rates * 2) {
- /* With multi-rate retry, not every planned sample
- * attempt actually gets used, due to the way the retry
- * chain is set up - [max_tp,sample,prob,lowest] for
- * sample_rate < max_tp.
- *
- * If there's too much sampling backlog and the link
- * starts getting worse, minstrel would start bursting
- * out lots of sampling frames, which would result
- * in a large throughput loss. */
- mi->sample_count += (delta - mi->n_rates * 2);
- }
+ /* delta < 0: no sampling required */
+ mi->prev_sample = false;
+ if (delta < 0 || (!mrr_capable && prev_sample))
+ return;
- /* get next random rate sample */
- sample_ndx = minstrel_get_next_sample(mi);
- msr = &mi->r[sample_ndx];
- rate_sampling = true;
-
- /* Decide if direct ( 1st mrr stage) or indirect (2nd mrr stage)
- * rate sampling method should be used.
- * Respect such rates that are not sampled for 20 interations.
- */
- if (mrr_capable &&
- msr->perfect_tx_time > mi->r[ndx].perfect_tx_time &&
- msr->sample_skipped < 20)
- indirect_rate_sampling = true;
-
- if (!indirect_rate_sampling) {
- if (msr->sample_limit != 0) {
- ndx = sample_ndx;
- mi->sample_count++;
- if (msr->sample_limit > 0)
- msr->sample_limit--;
- } else
- rate_sampling = false;
- } else {
- /* Only use IEEE80211_TX_CTL_RATE_CTRL_PROBE to mark
- * packets that have the sampling rate deferred to the
- * second MRR stage. Increase the sample counter only
- * if the deferred sample rate was actually used.
- * Use the sample_deferred counter to make sure that
- * the sampling is not done in large bursts */
- info->flags |= IEEE80211_TX_CTL_RATE_CTRL_PROBE;
- mi->sample_deferred++;
- }
+ if (mi->packet_count >= 10000) {
+ mi->sample_deferred = 0;
+ mi->sample_count = 0;
+ mi->packet_count = 0;
+ } else if (delta > mi->n_rates * 2) {
+ /* With multi-rate retry, not every planned sample
+ * attempt actually gets used, due to the way the retry
+ * chain is set up - [max_tp,sample,prob,lowest] for
+ * sample_rate < max_tp.
+ *
+ * If there's too much sampling backlog and the link
+ * starts getting worse, minstrel would start bursting
+ * out lots of sampling frames, which would result
+ * in a large throughput loss. */
+ mi->sample_count += (delta - mi->n_rates * 2);
+ }
+
+ /* get next random rate sample */
+ ndx = minstrel_get_next_sample(mi);
+ msr = &mi->r[ndx];
+ mr = &mi->r[mi->max_tp_rate[0]];
+
+ /* Decide if direct ( 1st mrr stage) or indirect (2nd mrr stage)
+ * rate sampling method should be used.
+ * Respect such rates that are not sampled for 20 interations.
+ */
+ if (mrr_capable &&
+ msr->perfect_tx_time > mr->perfect_tx_time &&
+ msr->sample_skipped < 20) {
+ /* Only use IEEE80211_TX_CTL_RATE_CTRL_PROBE to mark
+ * packets that have the sampling rate deferred to the
+ * second MRR stage. Increase the sample counter only
+ * if the deferred sample rate was actually used.
+ * Use the sample_deferred counter to make sure that
+ * the sampling is not done in large bursts */
+ info->flags |= IEEE80211_TX_CTL_RATE_CTRL_PROBE;
+ rate++;
+ mi->sample_deferred++;
+ } else {
+ if (!msr->sample_limit != 0)
+ return;
+
+ mi->sample_count++;
+ if (msr->sample_limit > 0)
+ msr->sample_limit--;
}
- mi->prev_sample = rate_sampling;
/* If we're not using MRR and the sampling rate already
* has a probability of >95%, we shouldn't be attempting
* to use it, as this only wastes precious airtime */
- if (!mrr_capable && rate_sampling &&
+ if (!mrr_capable &&
(mi->r[ndx].probability > MINSTREL_FRAC(95, 100)))
- ndx = mi->max_tp_rate[0];
-
- /* mrr setup for 1st stage */
- ar[0].idx = mi->r[ndx].rix;
- ar[0].count = minstrel_get_retry_count(&mi->r[ndx], info);
-
- /* non mrr setup for 2nd stage */
- if (!mrr_capable) {
- if (!rate_sampling)
- ar[0].count = mp->max_retry;
- ar[1].idx = mi->lowest_rix;
- ar[1].count = mp->max_retry;
return;
- }
- /* mrr setup for 2nd stage */
- if (rate_sampling) {
- if (indirect_rate_sampling)
- mrr_ndx[0] = sample_ndx;
- else
- mrr_ndx[0] = mi->max_tp_rate[0];
- } else {
- mrr_ndx[0] = mi->max_tp_rate[1];
- }
+ mi->prev_sample = true;
- /* mrr setup for 3rd & 4th stage */
- mrr_ndx[1] = mi->max_prob_rate;
- mrr_ndx[2] = 0;
- for (i = 1; i < 4; i++) {
- ar[i].idx = mi->r[mrr_ndx[i - 1]].rix;
- ar[i].count = mi->r[mrr_ndx[i - 1]].adjusted_retry_count;
- }
+ rate->idx = mi->r[ndx].rix;
+ rate->count = minstrel_get_retry_count(&mi->r[ndx], info);
}
@@ -412,12 +425,16 @@ minstrel_rate_init(void *priv, struct ieee80211_supported_band *sband,
unsigned int i, n = 0;
unsigned int t_slot = 9; /* FIXME: get real slot time */
+ mi->sta = sta;
mi->lowest_rix = rate_lowest_index(sband, sta);
ctl_rate = &sband->bitrates[mi->lowest_rix];
mi->sp_ack_dur = ieee80211_frame_duration(sband->band, 10,
ctl_rate->bitrate,
!!(ctl_rate->flags & IEEE80211_RATE_ERP_G), 1);
+ memset(mi->max_tp_rate, 0, sizeof(mi->max_tp_rate));
+ mi->max_prob_rate = 0;
+
for (i = 0; i < sband->n_bitrates; i++) {
struct minstrel_rate *mr = &mi->r[n];
unsigned int tx_time = 0, tx_time_cts = 0, tx_time_rtscts = 0;
@@ -473,6 +490,7 @@ minstrel_rate_init(void *priv, struct ieee80211_supported_band *sband,
mi->stats_update = jiffies;
init_sample_table(mi);
+ minstrel_update_rates(mp, mi);
}
static void *
diff --git a/net/mac80211/rc80211_minstrel.h b/net/mac80211/rc80211_minstrel.h
index b9f8535..f4301f4 100644
--- a/net/mac80211/rc80211_minstrel.h
+++ b/net/mac80211/rc80211_minstrel.h
@@ -63,6 +63,8 @@ struct minstrel_rate {
};
struct minstrel_sta_info {
+ struct ieee80211_sta *sta;
+
unsigned long stats_update;
unsigned int sp_ack_dur;
unsigned int rate_avg;
--
1.8.0.2
On Mon, 2013-04-22 at 16:14 +0200, Felix Fietkau wrote:
> Allow rate control modules to pass a rate selection table to mac80211
> and the driver. This allows drivers to fetch the most recent rate
> selection from the sta pointer for already buffered frames. This allows
> rate control to respond faster to sudden link changes and it is also a
> step towards adding minstrel_ht support to drivers like iwlwifi.
Applied all. Karl, thanks for the detailed review!
johannes
On Mon, Apr 22, 2013 at 04:18:31PM +0200, Johannes Berg wrote:
> On Mon, 2013-04-22 at 16:14 +0200, Felix Fietkau wrote:
> > Allow rate control modules to pass a rate selection table to mac80211
> > and the driver. This allows drivers to fetch the most recent rate
> > selection from the sta pointer for already buffered frames. This allows
> > rate control to respond faster to sudden link changes and it is also a
> > step towards adding minstrel_ht support to drivers like iwlwifi.
>
> Applied all. Karl, thanks for the detailed review!
>
No problem, it's nice to know it was appreciated ;)
Karl
On Tue, 2013-04-23 at 02:58 +0200, Christian Lamparter wrote:
> This patch fixes the following RCU debug splat:
>
> ===============================
> [ INFO: suspicious RCU usage. ]
> 3.9.0-rc8-wl+ #31 Tainted: G O
> -------------------------------
> net/mac80211/rate.c:691 suspicious rcu_dereference_check() usage!
>
> other info that might help us debug this:
>
> rcu_scheduler_active = 1, debug_locks = 1
> 3 locks held by hostapd/9451:
> #0: (genl_mutex){+.+.+.}, at: [<c1326365>] genl_lock+0xf/0x11
> #1: (rtnl_mutex){+.+.+.}, at: [<c13133c4>] rtnl_lock+0xf/0x11
> #2: (&rdev->mtx){+.+.+.}, at: [<f853395e>] nl80211_pre_doit+0x166/0x180 [cfg80211]
>
> stack backtrace:
> Pid: 9451, comm: hostapd Tainted: G O 3.9.0-rc8-wl+ #31
> Call Trace:
> [<c107da0b>] lockdep_rcu_suspicious+0xe6/0xee
> [<f8bf82ad>] rate_control_set_rates+0x43/0x5a [mac80211]
> [<f8c2cacb>] minstrel_update_rates+0xdc/0xe2 [mac80211]
> [<f8c2cfb0>] minstrel_rate_init+0x24c/0x33d [mac80211]
> [<f8c2d9d3>] minstrel_ht_update_caps+0x206/0x234 [mac80211]
> [<c1080a8d>] ? lock_release+0x1c9/0x226
> [<f8c2da25>] minstrel_ht_rate_init+0x10/0x14 [mac80211]
> [...]
>
> Signed-off-by: Christian Lamparter <[email protected]>
> ---
> Actually, rcu_read_lock() might not be necessary in this special
> case [the RC is not yet initialized, so nothing bad can happen].
>
> But, since the rcu_read_lock() has a low overhead and
> rate_control_set_rates mac80211.h doc does not mention
> anything about locking, I think this is a viable way.
I think that, on the contrary, it's completely strange/wrong. ;-)
> + rcu_read_lock();
> + old = rcu_dereference(pubsta->rates);
Here's have a dereference.
> rcu_assign_pointer(pubsta->rates, rates);
and here's an assignment. The assignment ought to be protected already
by some locking, presumably, so similarly is the rcu_dereference() which
then should just be rcu_dereference_protected()?
johannes
This patch fixes the following RCU debug splat:
===============================
[ INFO: suspicious RCU usage. ]
3.9.0-rc8-wl+ #31 Tainted: G O
-------------------------------
net/mac80211/rate.c:691 suspicious rcu_dereference_check() usage!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 1
3 locks held by hostapd/9451:
#0: (genl_mutex){+.+.+.}, at: [<c1326365>] genl_lock+0xf/0x11
#1: (rtnl_mutex){+.+.+.}, at: [<c13133c4>] rtnl_lock+0xf/0x11
#2: (&rdev->mtx){+.+.+.}, at: [<f853395e>] nl80211_pre_doit+0x166/0x180 [cfg80211]
stack backtrace:
Pid: 9451, comm: hostapd Tainted: G O 3.9.0-rc8-wl+ #31
Call Trace:
[<c107da0b>] lockdep_rcu_suspicious+0xe6/0xee
[<f8bf82ad>] rate_control_set_rates+0x43/0x5a [mac80211]
[<f8c2cacb>] minstrel_update_rates+0xdc/0xe2 [mac80211]
[<f8c2cfb0>] minstrel_rate_init+0x24c/0x33d [mac80211]
[<f8c2d9d3>] minstrel_ht_update_caps+0x206/0x234 [mac80211]
[<c1080a8d>] ? lock_release+0x1c9/0x226
[<f8c2da25>] minstrel_ht_rate_init+0x10/0x14 [mac80211]
[...]
Signed-off-by: Christian Lamparter <[email protected]>
---
Actually, rcu_read_lock() might not be necessary in this special
case [the RC is not yet initialized, so nothing bad can happen].
But, since the rcu_read_lock() has a low overhead and
rate_control_set_rates mac80211.h doc does not mention
anything about locking, I think this is a viable way.
---
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 0d51877..615d3a8 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -688,11 +688,15 @@ int rate_control_set_rates(struct ieee80211_hw *hw,
struct ieee80211_sta *pubsta,
struct ieee80211_sta_rates *rates)
{
- struct ieee80211_sta_rates *old = rcu_dereference(pubsta->rates);
+ struct ieee80211_sta_rates *old;
+
+ rcu_read_lock();
+ old = rcu_dereference(pubsta->rates);
rcu_assign_pointer(pubsta->rates, rates);
if (old)
kfree_rcu(old, rcu_head);
+ rcu_read_unlock();
return 0;
}
On Tue, 2013-04-23 at 15:26 +0200, Christian Lamparter wrote:
> > > Actually, rcu_read_lock() might not be necessary in this special
> > > case [the RC is not yet initialized, so nothing bad can happen].
> > >
> > > But, since the rcu_read_lock() has a low overhead and
> > > rate_control_set_rates mac80211.h doc does not mention
> > > anything about locking, I think this is a viable way.
> >
> > I think that, on the contrary, it's completely strange/wrong. ;-)
> Sorry, I think I cut too much from the stack trace and I didn't
> explain how the code end up in this case. This time, I commented out
> the rcu_read_(un)lock() [=> rate.c:694 is rate.c:691 in wireless-testing.git]
> and started hostapd and let a station connect. (see attached log)
Yes, I understand how you can get here. But every time the assignment
here happens, the value is completely overwritten. And when we free it
here, we don't look at the value.
> > > + rcu_read_lock();
> > > + old = rcu_dereference(pubsta->rates);
> >
> > Here's have a dereference.
> >
> > > rcu_assign_pointer(pubsta->rates, rates);
> >
> > and here's an assignment. The assignment ought to be protected already
> > by some locking, presumably, so similarly is the rcu_dereference() which
> > then should just be rcu_dereference_protected()?
> The issue seems to be in ieee80211_add_station in net/mac80211/cfg.c.
> This function allocates, initializes and adds the new station for
> hostapd. And of course: the alloc and (rate_)init part is done without
> acquiring any special mac80211 locks. (just rtnl, genl and rdev->mtx).
>
> [And why should it? After all, during initialization, the station is
> not yet in the station hash table.]
>
> So, what else can be done?
>
> Obviously, the locking requirement needs to be added to the
> doc entry for rate_control_set_rates in include/net/mac80211.h.
I don't see that any bug can happen here right now, even without
locking.
> And one of the following changes:
>
> 1. move the rate_control_rate_init after sta_info_insert_rcu
> and remove the rcu_read_locks from rate_control_set_rates.
> However then we would add an incomplete station (this can't be right?!).
>
> 2. add rcu or other lock around rate_control_set_rates in
> minstrel_update_rates() and minstrel_ht_update_rates().
Both seem wrong.
> 3. add a new function: rate_control_init_rates which is
> reserved for this case and only does the assignment.
I like that.
> (4. use rcu_dereference_protected and test the rtnl_lock - really?)
Nah that'll never work anyway.
> (5. some other way?)
The problem here is that even the rcu_read_lock() around here that's
actually there in most cases *isn't* what's protecting this code. What's
protecting this assignment is the fact that we require drivers to not
call ieee80211_tx_status() concurrently (and if they call
ieee80211_tx_status_irqsafe() then we serialize via the tasklet.)
If this wasn't the case, then calling the function could cause
double-free or so by having two CPUs read the old pointer and both call
kfree_rcu() on it.
Actually, looking at this code, this does seem possible in minstrel_ht
because it also calls this from minstrel_ht_rate_update() (indirectly),
which is called from the RX path which I'm not sure we require to be not
concurrent with the TX status path? Most drivers probably don't call
them concurrently, but I haven't checked all of them.
So as you can see, the RCU warning is just the tip of the iceberg.
johannes
Hi,
> static void
> @@ -846,6 +857,8 @@ minstrel_ht_update_caps(void *priv, struct
> ieee80211_supported_band *sband,
>
> msp->is_ht = true;
> memset(mi, 0, sizeof(*mi));
> +
> + mi->sta = sta;
> mi->stats_update = jiffies;
minstrel_ht_update_caps can be called on init and on different other changes
(rate_control_rate_update).
Which lock protects mi from following scenario?
context 1: memset(mi, 0, sizeof(*mi)); // mi->sta is now NULL
context 2: minstrel_ht_update_rates -> rate_control_set_rates(mp->hw, mi->sta, rates)
context 2: rate_control_set_rates dereferences pubsta->rates (mi->sta + 0x48) -> Kernel Oops
context 1: mi->sta = sta
The first context is from one of the many rate_control_rate_update in mac80211
and the second context is from ieee80211_tx_status.
The question came up when discovering the OpenWrt bug report
https://dev.openwrt.org/ticket/18388 (minstrel_ht_update_caps
the thing most likely behind minstrel_remove_sta_debugfs+0xe8c/0x1674 - at least
EPC is pointing inside this function for a build from this revision)
Kind regards,
Sven Eckelmann
Hi Felix,
On Friday 20 February 2015 15:12:10 Sven Eckelmann wrote:
> > static void
> >
> > @@ -846,6 +857,8 @@ minstrel_ht_update_caps(void *priv, struct
> > ieee80211_supported_band *sband,
> >
> > msp->is_ht = true;
> > memset(mi, 0, sizeof(*mi));
> >
> > +
> > + mi->sta = sta;
> >
> > mi->stats_update = jiffies;
>
> minstrel_ht_update_caps can be called on init and on different other changes
> (rate_control_rate_update).
>
> Which lock protects mi from following scenario?
>
> context 1: memset(mi, 0, sizeof(*mi)); // mi->sta is now NULL
> context 2: minstrel_ht_update_rates -> rate_control_set_rates(mp->hw,
> mi->sta, rates)
> context 2: rate_control_set_rates dereferences
> pubsta->rates (mi->sta + 0x48) -> Kernel Oops
> context 1: mi->sta = sta
>
> The first context is from one of the many rate_control_rate_update in
> mac80211 and the second context is from ieee80211_tx_status.
>
> The question came up when discovering the OpenWrt bug report
> https://dev.openwrt.org/ticket/18388 (minstrel_ht_update_caps
> the thing most likely behind minstrel_remove_sta_debugfs+0xe8c/0x1674 - at
> least EPC is pointing inside this function for a build from this revision)
I have someone here who says that he can reproduce this problem with a current
mac80211 from OpenWrt in ~40 min in a mesh setup with a lot of multicast. I
gave them following test patch to check if it could be related to the scenario
explained earlier:
--- a/net/mac80211/rc80211_minstrel_ht.c
+++ b/net/mac80211/rc80211_minstrel_ht.c
@@ -1126,7 +1126,8 @@ minstrel_ht_update_caps(void *priv, stru
use_vht = 0;
msp->is_ht = true;
- memset(mi, 0, sizeof(*mi));
+ /* don't reset the first entry of mi which is the sta pointer */
+ memset(((u8 *)mi) + sizeof(mi->sta), 0, sizeof(*mi) - sizeof(mi->sta));
mi->sta = sta;
mi->stats_update = jiffies;
He reported back that the mesh nodes were now running fine since 7 hours. It
is also tested in another network which now runs since 1 1/2 days and were not
able to run stable for more then 20 hours at max before applying that patch.
These numbers are no definitive proof but at least suggest that there could be
a connection. Maybe you already had some concept how to protect from this
problem and have not fully implemented it. Would be nice to hear back from
you.
Kind regards,
Sven