2010-10-13 22:48:53

by Paul Stewart

[permalink] [raw]
Subject: [PATCH 3/6] Add a function for evaluating rate changes for possible notification

Called from a work proc, this examines the last transmit rate change
to see if it warrants an event transmission.

Signed-off-by: Paul Stewart <[email protected]>

---
net/mac80211/rate.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++
net/mac80211/rate.h | 8 +++++
2 files changed, 82 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 4f772de..5e39494 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -399,3 +399,77 @@ void rate_control_deinitialize(struct ieee80211_local *local)
rate_control_put(ref);
}

+u32 ieee80211_rate_calculate(struct ieee80211_local *local,
+ struct ieee80211_if_managed *ifmgd)
+{
+ u32 rate_val;
+ u32 modulation, streams, base_rate, mcs, gi_div;
+ struct ieee80211_tx_rate *rate_ptr = &ifmgd->last_cqm_tx_rate;
+ struct ieee80211_supported_band *sband;
+ static int mod_table[8] = { 1, 2, 3, 4, 6, 8, 9, 10 };
+
+ if (!(rate_ptr->flags & IEEE80211_TX_RC_MCS)) {
+ sband = local->hw.wiphy->bands[local->hw.conf.channel->band];
+ return sband->bitrates[ifmgd->last_cqm_tx_rate.idx].bitrate;
+ }
+
+ mcs = rate_ptr->idx;
+
+ /* MCS values over 32 are not yet supported */
+ if (mcs >= 32)
+ return 0;
+
+ modulation = mcs & 7;
+ streams = (mcs >> 3) + 1;
+ base_rate = (rate_ptr->flags & IEEE80211_TX_RC_40_MHZ_WIDTH) ?
+ 13500000 : 6500000;
+ gi_div = (rate_ptr->flags & IEEE80211_TX_RC_SHORT_GI) ? 9 : 10;
+ rate_val = base_rate * mod_table[modulation] * streams / gi_div;
+
+ return ((rate_val + 5000) / 10000) * 100;
+}
+
+void ieee80211_cqm_bitrate_notify(struct ieee80211_local *local)
+{
+ struct ieee80211_sub_if_data *sdata;
+ struct ieee80211_if_managed *ifmgd;
+ struct ieee80211_bss_conf *bss_conf;
+ u32 bitrate, threshold;
+ int prev_state, new_state;
+
+ WARN_ON(!mutex_is_locked(&local->iflist_mtx));
+
+ list_for_each_entry(sdata, &local->interfaces, list) {
+ if (!netif_running(sdata->dev) ||
+ sdata->vif.type != NL80211_IFTYPE_STATION)
+ continue;
+
+ ifmgd = &sdata->u.mgd;
+ bss_conf = &sdata->vif.bss_conf;
+
+ if (!(ifmgd->flags & IEEE80211_STA_TX_RATE_CHANGED) ||
+ bss_conf->cqm_bitrate_thold == 0)
+ continue;
+
+ bitrate = ieee80211_rate_calculate(local, ifmgd);
+ threshold = bss_conf->cqm_bitrate_thold;
+ prev_state = (ifmgd->last_cqm_bitrate < threshold);
+ new_state = (bitrate < threshold);
+
+ /*
+ * Trigger a bitrate notification if one of the following is
+ * true:
+ * - We haven't sent one since the threshold was reconfigured
+ * - We have crossed the threshold in either direction
+ * - We are below threshold, and the bitrate has decreased yet
+ * again.
+ */
+ if (ifmgd->last_cqm_bitrate == 0 || prev_state != new_state ||
+ (new_state && bitrate < ifmgd->last_cqm_bitrate)) {
+ cfg80211_cqm_bitrate_notify(sdata->dev, bitrate,
+ GFP_KERNEL);
+ ifmgd->last_cqm_bitrate = bitrate;
+ }
+ ifmgd->flags &= ~IEEE80211_STA_TX_RATE_CHANGED;
+ }
+}
diff --git a/net/mac80211/rate.h b/net/mac80211/rate.h
index 168427b..33e4ca3 100644
--- a/net/mac80211/rate.h
+++ b/net/mac80211/rate.h
@@ -120,6 +120,14 @@ int ieee80211_init_rate_ctrl_alg(struct ieee80211_local *local,
void rate_control_deinitialize(struct ieee80211_local *local);


+/* Notify listeners about transmit rate changes */
+void ieee80211_cqm_bitrate_notify(struct ieee80211_local *local);
+
+/* Convert rate into cfg80211-compatible struct? */
+void ieee80211_rate_convert_cfg(struct ieee80211_local *local,
+ struct ieee80211_if_managed *ifmgd,
+ struct rate_info *rate);
+
/* Rate control algorithms */
#ifdef CONFIG_MAC80211_RC_PID
extern int rc80211_pid_init(void);
--
1.7.1



2010-10-14 13:50:22

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 3/6] Add a function for evaluating rate changes for possible notification

On Wed, 2010-10-13 at 15:13 -0700, Paul Stewart wrote:
> Called from a work proc, this examines the last transmit rate change
> to see if it warrants an event transmission.

> + list_for_each_entry(sdata, &local->interfaces, list) {
> + if (!netif_running(sdata->dev) ||

ieee80211_sdata_running

> + if (!(ifmgd->flags & IEEE80211_STA_TX_RATE_CHANGED) ||
> + bss_conf->cqm_bitrate_thold == 0)
> + continue;

why would this work item even be running with a threshold of zero?

Again, this won't compile without the next patch.

However, I think all of this is really expensive. For many devices,
you'll effectively be triggering an evaluation of the bitrate for every
packet. This will work well with minstrel, since it properly uses and
advertises the "best throughput" rate, but not necessarily with other
algorithms.

johannes


2010-10-14 14:31:47

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 3/6] Add a function for evaluating rate changes for possible notification

On Thu, 2010-10-14 at 07:24 -0700, Paul Stewart wrote:

> >> + if (!(ifmgd->flags & IEEE80211_STA_TX_RATE_CHANGED) ||
> >> + bss_conf->cqm_bitrate_thold == 0)
> >> + continue;
> >
> > why would this work item even be running with a threshold of zero?
>
> This would be in the case where the userspace reset the threshold back
> to zero (no longer interested in bitrate change events) but a work
> task was already scheduled it's a small window and I can remove that
> conditional if it suits.

Makes sense, I was just wondering. Maybe just add a comment?

> > However, I think all of this is really expensive. For many devices,
> > you'll effectively be triggering an evaluation of the bitrate for every
> > packet. This will work well with minstrel, since it properly uses and
> > advertises the "best throughput" rate, but not necessarily with other
> > algorithms.
>
> I could throttle-down the execution rate of this work proc to a
> minimum periodicity, at the expense of timely information. Is this
> along the lines of what you'd like done?

If I'd had a good idea I probably would've mentioned it. I guess that
would work, but I don't really know. I'm just worried that there is a
lot of calculation going on all the time, like all those divisions.
Those are _really_ expensive on embedded systems.

I wonder if "rate" is really the best interface for this. Since it's a
threshold event, does it matter? Maybe there should instead be events
for MCS changes and legacy rate changes so we can avoid all those
calculations?

johannes


2010-10-14 15:59:31

by Paul Stewart

[permalink] [raw]
Subject: Re: [PATCH 3/6] Add a function for evaluating rate changes for possible notification

On Thu, Oct 14, 2010 at 7:51 AM, Johannes Berg
<[email protected]> wrote:
> On Thu, 2010-10-14 at 16:32 +0200, Johannes Berg wrote:
>
>> I wonder if "rate" is really the best interface for this. Since it's a
>> threshold event, does it matter? Maybe there should instead be events
>> for MCS changes and legacy rate changes so we can avoid all those
>> calculations?
>
> In fact, since you'd only care about changes across a threshold etc, not
> the absolute rate, you might be able to build the system without ever
> doing the expensive rate calculation from MCS.

What if I change the calculate() so that it does a cached table
lookup? Since MCS numbers aren't linear or monotonic with regard to
bitrate, those numbers can't be compared directly to set thresholds.
I think it'd be a mistake to push it to userspace to interpret MCS and
alert on every MCS change since that would just tradeoff calculation
expense for nl80211 socket traffic, since without an understanding of
the MCS ranking, it'd would not be possible to set thresholds.

--
Paul

2010-10-14 14:50:26

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 3/6] Add a function for evaluating rate changes for possible notification

On Thu, 2010-10-14 at 16:32 +0200, Johannes Berg wrote:

> I wonder if "rate" is really the best interface for this. Since it's a
> threshold event, does it matter? Maybe there should instead be events
> for MCS changes and legacy rate changes so we can avoid all those
> calculations?

In fact, since you'd only care about changes across a threshold etc, not
the absolute rate, you might be able to build the system without ever
doing the expensive rate calculation from MCS.

johannes


2010-10-14 14:24:43

by Paul Stewart

[permalink] [raw]
Subject: Re: [PATCH 3/6] Add a function for evaluating rate changes for possible notification

On Thu, Oct 14, 2010 at 6:51 AM, Johannes Berg
<[email protected]> wrote:
> On Wed, 2010-10-13 at 15:13 -0700, Paul Stewart wrote:
>> Called from a work proc, this examines the last transmit rate change
>> to see if it warrants an event transmission.
>
>> + ? ? ? ?list_for_each_entry(sdata, &local->interfaces, list) {
>> + ? ? ? ? ? ? ? ?if (!netif_running(sdata->dev) ||
>
> ieee80211_sdata_running
>
>> + ? ? ? ? ? ? if (!(ifmgd->flags & IEEE80211_STA_TX_RATE_CHANGED) ||
>> + ? ? ? ? ? ? ? ? bss_conf->cqm_bitrate_thold == 0)
>> + ? ? ? ? ? ? ? ? ? ? continue;
>
> why would this work item even be running with a threshold of zero?

This would be in the case where the userspace reset the threshold back
to zero (no longer interested in bitrate change events) but a work
task was already scheduled it's a small window and I can remove that
conditional if it suits.

> Again, this won't compile without the next patch.

I'll fix.

> However, I think all of this is really expensive. For many devices,
> you'll effectively be triggering an evaluation of the bitrate for every
> packet. This will work well with minstrel, since it properly uses and
> advertises the "best throughput" rate, but not necessarily with other
> algorithms.

I could throttle-down the execution rate of this work proc to a
minimum periodicity, at the expense of timely information. Is this
along the lines of what you'd like done?

--
Paul