2018-05-15 05:18:27

by Sriram R

[permalink] [raw]
Subject: [RFC 0/2] nl80211/mac80211 Add support for per-rate rx statistics

This patchset adds support for the collection and propagating of
per-rate, per-station rx statistics when enabled by a userspace application.

These statistics can be useful in understanding the quality of
communication with our peers and in evaluating how different peers
are communicating in different MCS/BW/NSS during different time periods and environment.

Sriram R (2):
nl80211: Add support for collection of per rate rx statistics
mac80211: Add support for per-rate rx statistics

include/net/cfg80211.h | 25 +++++++
include/uapi/linux/nl80211.h | 50 +++++++++++++
net/mac80211/cfg.c | 47 ++++++++++++
net/mac80211/rx.c | 10 ++-
net/mac80211/sta_info.c | 165 +++++++++++++++++++++++++++++++++++++++++
net/mac80211/sta_info.h | 17 +++++
net/wireless/nl80211.c | 172 +++++++++++++++++++++++++++++++++++++++++++
net/wireless/rdev-ops.h | 30 ++++++++
net/wireless/trace.h | 26 +++++++
9 files changed, 540 insertions(+), 2 deletions(-)

--
2.7.4


2018-05-18 09:30:07

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC 0/2] nl80211/mac80211 Add support for per-rate rx statistics

On Wed, 2018-05-16 at 10:04 +0530, Sriram R wrote:

> But, I wanted to avoid,
> 1. Static indexing and memory allocation based on MCS count((8x3)24
> entries for HT and (10x3)30 for VHT within allocated 36 entries) so that
> it's scalable.

Do you expect that the rate control on the other side flips through
MCSes so fast that this little cache will need to be flushed
significantly?

> 2. Remote chance of dropping a stats(Though it does not have much
> impact)

Yeah this doesn't seem like a concern either way. How many packets and
how little time ... :)

> And to allow,
> 1. A 'station dump' kind of interface to dump the complete collected
> stats instead of returning only current snapshot of the stats within
> kernel.

This *completely* contradict keeping limits on the kernel memory
consumed, so basically I don't think this is feasible.

> Also, do you feel it would be good to have both ,i.e complete stats
> collection within kernel(this approach) and dump+clear of stats on
> reaching threshold(your approach) and have one of these two modes
> selected based on the requirement.

No, honestly, I don't. If an application wants these statistics, then I
feel that we can impose *some* requirements and not leave it at "let me
just enable it and have the kernel do all the work for me".

johannes

2018-05-15 05:18:39

by Sriram R

[permalink] [raw]
Subject: [RFC 2/2] mac80211: Add support for per-rate rx statistics

This patch adds support for the mac80211 to collect
rx statistics (Packet count and total bytes received)
per-rate, per-station when enabled by a user space application.

Note that the rate field passed on to the userspace from mac80211
is an encoded value where different rate attributes such as
type(VHT/HT/Legacy), MCS, BW, NSS, GI are collectively used to encode to
this rate and the userspace has to take care of decoding it.This is
done so as to allow scalability in future.

Once statistics collection is enabled, each packet received is recorded
per rate and stored as an entry(per rate) in a hashtable with the encoded rate being
the key for the hashtable.As the rate changes, new entries gets added
into this table and this information will be populated and sent back
to userspace when requested. The lifetime of this table is from the enabling
of the per-rate stats feature to disabling it or when the sta gets disconnected.

Memory usage(on a 32-bit machine) by this feature would be,
96 bytes (static value, per STA for hash table usage)
+28 bytes (increases per rate entry)
+ 4 bytes (increases per hash bucket and rate is used as key.
Default is 16 buckets)

Signed-off-by: Sriram R <[email protected]>
---
net/mac80211/cfg.c | 47 ++++++++++++++
net/mac80211/rx.c | 10 ++-
net/mac80211/sta_info.c | 165 ++++++++++++++++++++++++++++++++++++++++++++++++
net/mac80211/sta_info.h | 17 +++++
4 files changed, 237 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 5ce9d12..abee3ff 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -703,6 +703,30 @@ static int ieee80211_dump_station(struct wiphy *wiphy, struct net_device *dev,
return ret;
}

+static int
+ieee80211_dump_sta_rate_stats(struct wiphy *wiphy, struct net_device *dev,
+ int idx, u8 *mac,
+ struct sta_rate_stats **rate_stats_buf, int *len)
+{
+ struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+ struct ieee80211_local *local = sdata->local;
+ struct sta_info *sta;
+ int ret = -ENOENT;
+
+ mutex_lock(&local->sta_mtx);
+
+ sta = sta_info_get_by_idx(sdata, idx);
+ if (sta) {
+ memcpy(mac, sta->sta.addr, ETH_ALEN);
+ ret = ieee80211_sta_get_rate_stats_report(sta, rate_stats_buf,
+ len);
+ }
+
+ mutex_unlock(&local->sta_mtx);
+
+ return ret;
+}
+
static int ieee80211_dump_survey(struct wiphy *wiphy, struct net_device *dev,
int idx, struct survey_info *survey)
{
@@ -732,6 +756,27 @@ static int ieee80211_get_station(struct wiphy *wiphy, struct net_device *dev,
return ret;
}

+static int
+ieee80211_get_sta_rate_stats(struct wiphy *wiphy, struct net_device *dev,
+ const u8 *mac,
+ struct sta_rate_stats **rate_stats_buf, int *len)
+{
+ struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+ struct ieee80211_local *local = sdata->local;
+ struct sta_info *sta;
+ int ret = -ENOENT;
+
+ mutex_lock(&local->sta_mtx);
+
+ sta = sta_info_get_bss(sdata, mac);
+ if (sta)
+ ret = ieee80211_sta_get_rate_stats_report(sta, rate_stats_buf,
+ len);
+ mutex_unlock(&local->sta_mtx);
+
+ return ret;
+}
+
static int ieee80211_set_monitor_channel(struct wiphy *wiphy,
struct cfg80211_chan_def *chandef)
{
@@ -3822,6 +3867,8 @@ const struct cfg80211_ops mac80211_config_ops = {
.change_station = ieee80211_change_station,
.get_station = ieee80211_get_station,
.dump_station = ieee80211_dump_station,
+ .get_sta_rate_stats = ieee80211_get_sta_rate_stats,
+ .dump_sta_rate_stats = ieee80211_dump_sta_rate_stats,
.dump_survey = ieee80211_dump_survey,
#ifdef CONFIG_MAC80211_MESH
.add_mpath = ieee80211_add_mpath,
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 0a38cc1..0f63b18 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1561,9 +1561,11 @@ ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
test_sta_flag(sta, WLAN_STA_AUTHORIZED)) {
sta->rx_stats.last_rx = jiffies;
if (ieee80211_is_data(hdr->frame_control) &&
- !is_multicast_ether_addr(hdr->addr1))
+ !is_multicast_ether_addr(hdr->addr1)) {
sta->rx_stats.last_rate =
sta_stats_encode_rate(status);
+ ieee80211_sta_update_rate_stats(&sta);
+ }
}
} else if (rx->sdata->vif.type == NL80211_IFTYPE_OCB) {
sta->rx_stats.last_rx = jiffies;
@@ -1573,8 +1575,10 @@ ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
* match the current local configuration when processed.
*/
sta->rx_stats.last_rx = jiffies;
- if (ieee80211_is_data(hdr->frame_control))
+ if (ieee80211_is_data(hdr->frame_control)) {
sta->rx_stats.last_rate = sta_stats_encode_rate(status);
+ ieee80211_sta_update_rate_stats(&sta);
+ }
}

if (rx->sdata->vif.type == NL80211_IFTYPE_STATION)
@@ -4067,6 +4071,8 @@ static bool ieee80211_invoke_fast_rx(struct ieee80211_rx_data *rx,
stats->last_rx = jiffies;
stats->last_rate = sta_stats_encode_rate(status);

+ ieee80211_sta_update_rate_stats(&sta);
+
stats->fragments++;
stats->packets++;

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 43f34aa..7df30e7 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -31,6 +31,8 @@
#include "mesh.h"
#include "wme.h"

+static void ieee80211_sta_rate_table_free(struct sta_info *sta);
+
/**
* DOC: STA information lifetime rules
*
@@ -609,6 +611,11 @@ static int sta_info_insert_finish(struct sta_info *sta) __acquires(RCU)
if (ieee80211_vif_is_mesh(&sdata->vif))
mesh_accept_plinks_update(sdata);

+ /* Rate table can be allocated and initialized during rx
+ * if rate stats collection is enabled.
+ */
+ sta->rate_table = NULL;
+
return 0;
out_remove:
sta_info_hash_del(local, sta);
@@ -1006,6 +1013,8 @@ static void __sta_info_destroy_part2(struct sta_info *sta)

sta_dbg(sdata, "Removed STA %pM\n", sta->sta.addr);

+ ieee80211_sta_rate_table_free(sta);
+
sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL);
if (sinfo)
sta_set_sinfo(sta, sinfo);
@@ -2369,3 +2378,159 @@ void ieee80211_sta_set_expected_throughput(struct ieee80211_sta *pubsta,

sta_update_codel_params(sta, thr);
}
+
+static u32 rate_table_hash(const void *rate, u32 len, u32 seed)
+{
+ return jhash_1word(*(u32 *)rate, seed);
+}
+
+static const struct rhashtable_params rate_rht_params = {
+ .nelem_hint = 5,
+ .automatic_shrinking = true,
+ .key_len = IEEE80211_ENCODED_RATE_LEN,
+ .key_offset = offsetof(struct ieee80211_sta_rate_entry, rate),
+ .head_offset = offsetof(struct ieee80211_sta_rate_entry, rhash),
+ .hashfn = rate_table_hash,
+};
+
+static int
+ieee80211_sta_rate_table_init(struct sta_info *sta)
+{
+ sta->rate_table = kmalloc(sizeof(*sta->rate_table), GFP_KERNEL);
+
+ if (!sta->rate_table)
+ return -ENOMEM;
+
+ return rhashtable_init(sta->rate_table, &rate_rht_params);
+}
+
+static struct ieee80211_sta_rate_entry*
+ieee80211_sta_rate_table_lookup(struct sta_info *sta, u32 rate)
+{
+ if (!sta->rate_table)
+ return NULL;
+
+ return rhashtable_lookup_fast(sta->rate_table, &rate, rate_rht_params);
+}
+
+static int
+ieee80211_sta_rate_entry_insert(struct sta_info *sta,
+ struct ieee80211_sta_rate_entry *entry)
+{
+ if (!sta->rate_table)
+ return -EINVAL;
+
+ return rhashtable_lookup_insert_fast(sta->rate_table, &entry->rhash,
+ rate_rht_params);
+}
+
+static void ieee80211_sta_rate_entry_free(void *ptr, void *arg)
+{
+ struct ieee80211_sta_rate_entry *entry = ptr;
+
+ kfree_rcu(entry, rcu);
+}
+
+static void ieee80211_sta_rate_table_free(struct sta_info *sta)
+{
+ if (!sta->rate_table)
+ return;
+
+ rhashtable_free_and_destroy(sta->rate_table,
+ ieee80211_sta_rate_entry_free, NULL);
+ kfree(sta->rate_table);
+}
+
+int ieee80211_sta_get_rate_stats_report(struct sta_info *sta,
+ struct sta_rate_stats **report_buf,
+ int *len)
+{
+ int i = 0, ret, rate_table_len;
+ struct ieee80211_sta_rate_entry *entry = NULL;
+ struct rhashtable_iter iter;
+ struct sta_rate_stats *report;
+
+ if (!sta->rate_table)
+ return -EINVAL;
+
+ ret = rhashtable_walk_init(sta->rate_table, &iter, GFP_KERNEL);
+
+ if (ret)
+ return -EINVAL;
+
+ rhashtable_walk_start(&iter);
+
+ rate_table_len = atomic_read(&sta->rate_table->nelems);
+
+ /* Caller should take care of freeing this memory */
+ *report_buf = kzalloc(sizeof(struct sta_rate_stats) * rate_table_len,
+ GFP_KERNEL);
+
+ if (report_buf == NULL) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ while ((entry = rhashtable_walk_next(&iter))) {
+ if (IS_ERR(entry) && PTR_ERR(entry) == -EAGAIN)
+ continue;
+ if (IS_ERR(entry))
+ break;
+ if (i >= rate_table_len)
+ break;
+
+ report = *report_buf + i++;
+ report->rate = entry->rate;
+ report->packets = entry->packets;
+ report->bytes = entry->bytes;
+ }
+
+ *len = i;
+err:
+ rhashtable_walk_stop(&iter);
+ rhashtable_walk_exit(&iter);
+ return ret;
+}
+
+void ieee80211_sta_update_rate_stats(struct sta_info **sta_ptr)
+{
+ struct ieee80211_sta_rate_entry *entry;
+ struct sta_info *sta = *sta_ptr;
+ struct ieee80211_rx_data *rx;
+ u64 rx_pkt_len;
+ u32 rate;
+ int ret;
+
+ /* TODO CHECK rate_stats collection is enabled(via debugfs?(TODO))
+ * and return if it's not enabled
+ */
+
+ rx = container_of(sta_ptr, struct ieee80211_rx_data, sta);
+
+ rx_pkt_len = rx->skb->len;
+ rate = sta->rx_stats.last_rate;
+
+ if (!sta->rate_table) {
+ ret = ieee80211_sta_rate_table_init(sta);
+ if (ret)
+ return;
+ }
+
+ rcu_read_lock();
+ entry = ieee80211_sta_rate_table_lookup(sta, rate);
+ if (!entry) {
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ return;
+ entry->rate = rate;
+ ret = ieee80211_sta_rate_entry_insert(sta, entry);
+ if (ret) {
+ kfree(entry);
+ return;
+ }
+ }
+
+ entry->packets++;
+ entry->bytes += rx_pkt_len;
+ rcu_read_unlock();
+}
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index d79bd6e..ff3dcea 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -583,10 +583,22 @@ struct sta_info {

struct cfg80211_chan_def tdls_chandef;

+ struct rhashtable *rate_table;
+
/* keep last! */
struct ieee80211_sta sta;
};

+#define IEEE80211_ENCODED_RATE_LEN 4
+
+struct ieee80211_sta_rate_entry {
+ u32 rate;
+ u32 packets;
+ u64 bytes;
+ struct rcu_head rcu;
+ struct rhash_head rhash;
+};
+
static inline enum nl80211_plink_state sta_plink_state(struct sta_info *sta)
{
#ifdef CONFIG_MAC80211_MESH
@@ -758,6 +770,11 @@ void ieee80211_sta_ps_deliver_uapsd(struct sta_info *sta);

unsigned long ieee80211_sta_last_active(struct sta_info *sta);

+int ieee80211_sta_get_rate_stats_report(struct sta_info *sta,
+ struct sta_rate_stats **report_buf,
+ int *len);
+void ieee80211_sta_update_rate_stats(struct sta_info **sta_ptr);
+
enum sta_stats_type {
STA_STATS_RATE_TYPE_INVALID = 0,
STA_STATS_RATE_TYPE_LEGACY,
--
2.7.4

2018-05-15 07:00:06

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC 0/2] nl80211/mac80211 Add support for per-rate rx statistics

On Tue, 2018-05-15 at 10:47 +0530, Sriram R wrote:
> This patchset adds support for the collection and propagating of
> per-rate, per-station rx statistics when enabled by a userspace application.
>
> These statistics can be useful in understanding the quality of
> communication with our peers and in evaluating how different peers
> are communicating in different MCS/BW/NSS during different time periods and environment.

So ... I know that you're aware of my rate statistics collection code
(at least you should be, I showed it to Jouni), so I think you should
state why that approach isn't suitable.

In particular, I don't like the idea that you implement here of allowing
unresponsive (or dead) userspace to let the data pile up indefinitely. I
think we should be sending it out upon reaching a threshold to limit the
memory consumption in the kernel more reliably.

johannes

2018-05-15 05:18:35

by Sriram R

[permalink] [raw]
Subject: [RFC 1/2] nl80211: Add support for collection of per rate rx statistics

RX Statistics per rate, per station can be useful in
understanding the quality of communication with our peers
and can be helpful in evaluating the consistency at which a
specific peer has been communicating in different MCS/BW/NSS
after association at different time periods and in varied environments.

This patch adds support for collecting the rx statistics
from the kernel and providing it to the userspace.

Signed-off-by: Sriram R <[email protected]>
---
include/net/cfg80211.h | 25 +++++++
include/uapi/linux/nl80211.h | 50 +++++++++++++
net/wireless/nl80211.c | 172 +++++++++++++++++++++++++++++++++++++++++++
net/wireless/rdev-ops.h | 30 ++++++++
net/wireless/trace.h | 26 +++++++
5 files changed, 303 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 8db6071..8bd818f 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1235,6 +1235,19 @@ struct station_info {
s8 avg_ack_signal;
};

+/**
+ * struct sta_rate_stats - per rate statistics of station
+ *
+ * @rate: encoded rate value
+ * @packets: Packet count received at this @rate
+ * @bytes: Total number of bytes received at this @rate
+ */
+struct sta_rate_stats {
+ u32 rate;
+ u32 packets;
+ u64 bytes;
+};
+
#if IS_ENABLED(CONFIG_CFG80211)
/**
* cfg80211_get_station - retrieve information about a given station
@@ -3018,6 +3031,9 @@ struct cfg80211_external_auth_params {
*
* @tx_control_port: TX a control port frame (EAPoL). The noencrypt parameter
* tells the driver that the frame should not be encrypted.
+ *
+ * @get_sta_rate_stats: get per-rate stats of the station identified by @mac
+ * @dump_sta_rate_stats: get per-rate stats dump of stations from index @idx
*/
struct cfg80211_ops {
int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -3323,6 +3339,15 @@ struct cfg80211_ops {
const u8 *buf, size_t len,
const u8 *dest, const __be16 proto,
const bool noencrypt);
+
+ int (*get_sta_rate_stats)(struct wiphy *wiphy,
+ struct net_device *dev, const u8 *mac,
+ struct sta_rate_stats **rate_stats_buf,
+ int *len);
+ int (*dump_sta_rate_stats)(struct wiphy *wiphy,
+ struct net_device *dev, int idx, u8 *mac,
+ struct sta_rate_stats **rate_stats_buf,
+ int *len);
};

/*
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 83ed1dd..beeca81 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1031,6 +1031,10 @@
* &NL80211_ATTR_CHANNEL_WIDTH,&NL80211_ATTR_NSS attributes with its
* address(specified in &NL80211_ATTR_MAC).
*
+ * @NL80211_CMD_GET_RATE_STATS: Get Per rate statistics (rx packets and bytes)
+ * for a station identified by %NL80211_ATTR_MAC on the interface
+ * identified by %NL80211_ATTR_IFINDEX.
+ *
* @NL80211_CMD_MAX: highest used command number
* @__NL80211_CMD_AFTER_LAST: internal use
*/
@@ -1243,6 +1247,8 @@ enum nl80211_commands {

NL80211_CMD_CONTROL_PORT_FRAME,

+ NL80211_CMD_GET_RATE_STATS,
+
/* add new commands above here */

/* used to define NL80211_CMD_MAX below */
@@ -2236,6 +2242,10 @@ enum nl80211_commands {
* @NL80211_ATTR_TXQ_QUANTUM: TXQ scheduler quantum (bytes). Number of bytes
* a flow is assigned on each round of the DRR scheduler.
*
+ * @NL80211_ATTR_RATE_STATS: Indicates the presence of per-rate stats holding
+ * the info of packets and bytes received per rate identified by
+ * the attributes in @nl80211_rate_stats
+ *
* @NUM_NL80211_ATTR: total number of nl80211_attrs available
* @NL80211_ATTR_MAX: highest attribute number currently defined
* @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2675,6 +2685,8 @@ enum nl80211_attrs {
NL80211_ATTR_TXQ_MEMORY_LIMIT,
NL80211_ATTR_TXQ_QUANTUM,

+ NL80211_ATTR_RATE_STATS,
+
/* add attributes here, update the policy in nl80211.c */

__NL80211_ATTR_AFTER_LAST,
@@ -3114,6 +3126,44 @@ enum nl80211_txq_stats {
};

/**
+ * DOC: per-rate per-station statistics
+ * The nl80211 %NL80211_CMD_GET_RATE_STATS command provides an interface to
+ * get the statistics of number of packets and bytes received per rate
+ * per station.
+ * The kernel will start collecting this statistics only when it is required
+ * by the userspace application and rate statistics needs to be enabled for
+ * this purpose(TODO: Add comments on interface used to enable rate-stats)
+ * Once enabled, the packet (and corresponding bytes) count received at the
+ * station are recorded per rate on each rx and the userspace application
+ * can get the current stats information using the %NL80211_CMD_GET_RATE_STATS
+ * command.
+ * Note that the rate field which is provided by the statistics is a encoded
+ * value containing the information on VHT/HT/Legacy,BW,NSS,MCS,GI/SGI and this
+ * needs to be extracted/decoded by the userspace application.
+ */
+
+/**
+ * enum nl80211_rate_stats - per-rate stats attributes
+ * @__NL80211_RATE_STATS_INVALID :attribute number 0 is reserved
+ * @NL80211_ATTR_RATE_STATS_RATE :identifier for the encoded rate field.
+ * @NL80211_ATTR_RATE_STATS_RX_PACKETS: Number of packets which are received
+ * per @rate for this specific station identified by @NL80211_ATTR_MAC.
+ * @NL80211_ATTR_RATE_STATS_RX_BYTES: Number of bytes which are received
+ * per @rate for this specific station identified by @NL80211_ATTR_MAC.
+ * @NUM_NL80211_ATTR_RATE_STATS: Total number of rate-stats attributes
+ * @NL80211_ATTR_RATE_STATS_MAX: MAX rate-stats attribute number.
+ */
+enum nl80211_rate_stats {
+ __NL80211_RATE_STATS_INVALID,
+ NL80211_ATTR_RATE_STATS_RATE,
+ NL80211_ATTR_RATE_STATS_RX_PACKETS,
+ NL80211_ATTR_RATE_STATS_RX_BYTES,
+
+ /* keep last */
+ NUM_NL80211_ATTR_RATE_STATS,
+ NL80211_ATTR_RATE_STATS_MAX = NUM_NL80211_ATTR_RATE_STATS - 1
+};
+/**
* enum nl80211_mpath_flags - nl80211 mesh path flags
*
* @NL80211_MPATH_FLAG_ACTIVE: the mesh path is active
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index afbe510..fa78d05 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4826,6 +4826,170 @@ static int nl80211_get_station(struct sk_buff *skb, struct genl_info *info)
return err;
}

+static int
+nl80211_send_sta_rate_stats(struct sk_buff *msg, u32 cmd, u32 portid,
+ u32 seq, int flags,
+ struct cfg80211_registered_device *rdev,
+ struct net_device *dev, const u8 *mac_addr,
+ struct sta_rate_stats *rate_stats_buf,
+ int rate_table_len)
+{
+ void *hdr;
+ struct sta_rate_stats *report;
+ struct nlattr *rstatsattr, *rentryattr;
+ int rid;
+
+ hdr = nl80211hdr_put(msg, portid, seq, flags, cmd);
+ if (!hdr)
+ return -1;
+
+ if (nla_put_u32(msg, NL80211_ATTR_IFINDEX, dev->ifindex) ||
+ nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, mac_addr))
+ goto nla_put_failure;
+
+ if (rate_table_len && rate_stats_buf) {
+ rstatsattr = nla_nest_start(msg, NL80211_ATTR_RATE_STATS);
+
+ if (!rstatsattr)
+ goto nla_put_failure;
+
+ report = rate_stats_buf;
+
+ for (rid = 0; rid < rate_table_len ; rid++) {
+ rentryattr = nla_nest_start(msg, rid + 1);
+ if (!rentryattr)
+ goto nla_put_failure;
+
+ nla_put_u32(msg, NL80211_ATTR_RATE_STATS_RATE,
+ report->rate);
+ nla_put_u32(msg, NL80211_ATTR_RATE_STATS_RX_PACKETS,
+ report->packets);
+ nla_put_u64_64bit(msg,
+ NL80211_ATTR_RATE_STATS_RX_BYTES,
+ report->bytes, NL80211_ATTR_PAD);
+
+ nla_nest_end(msg, rentryattr);
+
+ if (rid + 1 < rate_table_len)
+ report++;
+ }
+
+ nla_nest_end(msg, rstatsattr);
+ }
+
+ genlmsg_end(msg, hdr);
+ return 0;
+
+ nla_put_failure:
+ genlmsg_cancel(msg, hdr);
+ return -EMSGSIZE;
+}
+
+static int nl80211_dump_sta_rate_stats(struct sk_buff *skb,
+ struct netlink_callback *cb)
+{
+ struct cfg80211_registered_device *rdev;
+ struct wireless_dev *wdev;
+ u8 mac_addr[ETH_ALEN];
+ int sta_idx = cb->args[2];
+ int err, num_rate_elems;
+ struct sta_rate_stats *rate_stats_buf = NULL;
+
+ rtnl_lock();
+ err = nl80211_prepare_wdev_dump(skb, cb, &rdev, &wdev);
+ if (err)
+ goto out_err;
+
+ if (!wdev->netdev) {
+ err = -EINVAL;
+ goto out_err;
+ }
+
+ if (!rdev->ops->dump_sta_rate_stats) {
+ err = -EOPNOTSUPP;
+ goto out_err;
+ }
+
+ while (1) {
+ err = rdev_dump_sta_rate_stats(rdev, wdev->netdev, sta_idx,
+ mac_addr, &rate_stats_buf,
+ &num_rate_elems);
+ if (err == -ENOENT)
+ break;
+ if (err)
+ goto out_err;
+
+ if (nl80211_send_sta_rate_stats(skb, NL80211_CMD_GET_RATE_STATS,
+ NETLINK_CB(cb->skb).portid,
+ cb->nlh->nlmsg_seq, NLM_F_MULTI,
+ rdev, wdev->netdev, mac_addr,
+ rate_stats_buf,
+ num_rate_elems) < 0) {
+ if (rate_stats_buf)
+ kfree(rate_stats_buf);
+ goto out;
+ }
+
+ sta_idx++;
+
+ if (rate_stats_buf)
+ kfree(rate_stats_buf);
+ }
+
+ out:
+ cb->args[2] = sta_idx;
+ err = skb->len;
+ out_err:
+ rtnl_unlock();
+
+ return err;
+}
+
+static int
+nl80211_get_sta_rate_stats(struct sk_buff *skb, struct genl_info *info)
+{
+ struct cfg80211_registered_device *rdev = info->user_ptr[0];
+ struct net_device *dev = info->user_ptr[1];
+ struct sk_buff *msg;
+ u8 *mac_addr = NULL;
+ int err, num_rate_elems;
+ struct sta_rate_stats *rate_stats_buf = NULL;
+
+ if (!info->attrs[NL80211_ATTR_MAC])
+ return -EINVAL;
+
+ mac_addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
+
+ if (!rdev->ops->get_sta_rate_stats)
+ return -EOPNOTSUPP;
+
+ err = rdev_get_sta_rate_stats(rdev, dev, mac_addr,
+ &rate_stats_buf, &num_rate_elems);
+ if (err)
+ return err;
+
+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ if (nl80211_send_sta_rate_stats(msg, NL80211_CMD_GET_RATE_STATS,
+ info->snd_portid, info->snd_seq, 0,
+ rdev, dev, mac_addr, rate_stats_buf,
+ num_rate_elems) < 0) {
+ nlmsg_free(msg);
+
+ if (rate_stats_buf)
+ kfree(rate_stats_buf);
+
+ return -ENOBUFS;
+ }
+
+ if (rate_stats_buf)
+ kfree(rate_stats_buf);
+
+ return genlmsg_reply(msg, info);
+}
+
int cfg80211_check_station_change(struct wiphy *wiphy,
struct station_parameters *params,
enum cfg80211_station_type statype)
@@ -13744,6 +13908,14 @@ static const struct genl_ops nl80211_ops[] = {
.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
NL80211_FLAG_NEED_RTNL,
},
+ {
+ .cmd = NL80211_CMD_GET_RATE_STATS,
+ .doit = nl80211_get_sta_rate_stats,
+ .dumpit = nl80211_dump_sta_rate_stats,
+ .policy = nl80211_policy,
+ .internal_flags = NL80211_FLAG_NEED_NETDEV |
+ NL80211_FLAG_NEED_RTNL,
+ },
};

static struct genl_family nl80211_fam __ro_after_init = {
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 364f5d6..1564ad8 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -222,6 +222,36 @@ static inline int rdev_dump_station(struct cfg80211_registered_device *rdev,
return ret;
}

+static inline int
+rdev_get_sta_rate_stats(struct cfg80211_registered_device *rdev,
+ struct net_device *dev, const u8 *mac,
+ struct sta_rate_stats **rate_stats_buf,
+ int *len)
+{
+ int ret;
+
+ trace_rdev_get_sta_rate_stats(&rdev->wiphy, dev, mac);
+ ret = rdev->ops->get_sta_rate_stats(&rdev->wiphy, dev, mac,
+ rate_stats_buf, len);
+ trace_rdev_return_int(&rdev->wiphy, ret);
+ return ret;
+}
+
+static inline int
+rdev_dump_sta_rate_stats(struct cfg80211_registered_device *rdev,
+ struct net_device *dev, int idx, u8 *mac,
+ struct sta_rate_stats **rate_stats_buf,
+ int *len)
+{
+ int ret;
+
+ trace_rdev_dump_sta_rate_stats(&rdev->wiphy, dev, idx, mac);
+ ret = rdev->ops->dump_sta_rate_stats(&rdev->wiphy, dev, idx, mac,
+ rate_stats_buf, len);
+ trace_rdev_return_int(&rdev->wiphy, ret);
+ return ret;
+}
+
static inline int rdev_add_mpath(struct cfg80211_registered_device *rdev,
struct net_device *dev, u8 *dst, u8 *next_hop)
{
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 2b417a2..09dd69b 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -759,6 +759,11 @@ DEFINE_EVENT(wiphy_netdev_mac_evt, rdev_get_station,
TP_ARGS(wiphy, netdev, mac)
);

+DEFINE_EVENT(wiphy_netdev_mac_evt, rdev_get_sta_rate_stats,
+ TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, const u8 *mac),
+ TP_ARGS(wiphy, netdev, mac)
+);
+
DEFINE_EVENT(wiphy_netdev_mac_evt, rdev_del_mpath,
TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, const u8 *mac),
TP_ARGS(wiphy, netdev, mac)
@@ -790,6 +795,27 @@ TRACE_EVENT(rdev_dump_station,
__entry->idx)
);

+TRACE_EVENT(rdev_dump_sta_rate_stats,
+ TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, int idx,
+ u8 *mac),
+ TP_ARGS(wiphy, netdev, idx, mac),
+ TP_STRUCT__entry(
+ WIPHY_ENTRY
+ NETDEV_ENTRY
+ MAC_ENTRY(sta_mac)
+ __field(int, idx)
+ ),
+ TP_fast_assign(
+ WIPHY_ASSIGN;
+ NETDEV_ASSIGN;
+ MAC_ASSIGN(sta_mac, mac);
+ __entry->idx = idx;
+ ),
+ TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", station mac: " MAC_PR_FMT ", idx: %d",
+ WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(sta_mac),
+ __entry->idx)
+);
+
TRACE_EVENT(rdev_return_int_station_info,
TP_PROTO(struct wiphy *wiphy, int ret, struct station_info *sinfo),
TP_ARGS(wiphy, ret, sinfo),
--
2.7.4

2018-05-15 08:51:40

by Toke Høiland-Jørgensen

[permalink] [raw]
Subject: Re: [RFC 0/2] nl80211/mac80211 Add support for per-rate rx statistics

Sriram R <[email protected]> writes:

> This patchset adds support for the collection and propagating of
> per-rate, per-station rx statistics when enabled by a userspace
> application.

Why not report TX rate statistics as well, when it is available?
Minstrel already collects this, and it is available via debugfs (the
rc_stats file); so if you're adding a netlink API, it seems odd to not
include TX stats as well...

-Toke

2018-05-21 03:46:28

by Sriram R

[permalink] [raw]
Subject: Re: [RFC 0/2] nl80211/mac80211 Add support for per-rate rx statistics

On 2018-05-18 15:00, Johannes Berg wrote:
> On Wed, 2018-05-16 at 10:04 +0530, Sriram R wrote:
>
>> But, I wanted to avoid,
>> 1. Static indexing and memory allocation based on MCS count((8x3)24
>> entries for HT and (10x3)30 for VHT within allocated 36 entries) so
>> that
>> it's scalable.
>
> Do you expect that the rate control on the other side flips through
> MCSes so fast that this little cache will need to be flushed
> significantly?
Not really Johannes. Dynamic allocation (with indexing based on encoded
rate)was tried out in this patch
so as to have the rate table independent of MCS Count(so that it works
without changes
when new mcs's is used in HE).
>
>> 2. Remote chance of dropping a stats(Though it does not have much
>> impact)
>
> Yeah this doesn't seem like a concern either way. How many packets and
> how little time ... :)
Right!
>
>> And to allow,
>> 1. A 'station dump' kind of interface to dump the complete collected
>> stats instead of returning only current snapshot of the stats within
>> kernel.
>
> This *completely* contradict keeping limits on the kernel memory
> consumed, so basically I don't think this is feasible.
Ok,Understood.
>
>> Also, do you feel it would be good to have both ,i.e complete stats
>> collection within kernel(this approach) and dump+clear of stats on
>> reaching threshold(your approach) and have one of these two modes
>> selected based on the requirement.
>
> No, honestly, I don't. If an application wants these statistics, then I
> feel that we can impose *some* requirements and not leave it at "let me
> just enable it and have the kernel do all the work for me".
Right, Thanks for the suggestions. I'll send the next revision based on
your approach
of flushing the rate table(whenever its' requested or when we hit a
limit)
Thanks and Regards,
Sriram.R
>
> johannes

2018-05-16 04:34:09

by Sriram R

[permalink] [raw]
Subject: Re: [RFC 0/2] nl80211/mac80211 Add support for per-rate rx statistics

On 2018-05-15 12:30, Johannes Berg wrote:
> On Tue, 2018-05-15 at 10:47 +0530, Sriram R wrote:
>> This patchset adds support for the collection and propagating of
>> per-rate, per-station rx statistics when enabled by a userspace
>> application.
>>
>> These statistics can be useful in understanding the quality of
>> communication with our peers and in evaluating how different peers
>> are communicating in different MCS/BW/NSS during different time
>> periods and environment.
>
> So ... I know that you're aware of my rate statistics collection code
> (at least you should be, I showed it to Jouni), so I think you should
> state why that approach isn't suitable.
>
Hi Johannes,

I really liked your approach of publishing the rate statistics only
when subscribers exist and to push the data when we hit some limits (pkt
count nearing UINT16_MAX or when 'escape' entries gets used).

But, I wanted to avoid,
1. Static indexing and memory allocation based on MCS count((8x3)24
entries for HT and (10x3)30 for VHT within allocated 36 entries) so that
it's scalable.
2. Remote chance of dropping a stats(Though it does not have much
impact)

And to allow,
1. A 'station dump' kind of interface to dump the complete collected
stats instead of returning only current snapshot of the stats within
kernel.

> In particular, I don't like the idea that you implement here of
> allowing
> unresponsive (or dead) userspace to let the data pile up indefinitely.
> I
> think we should be sending it out upon reaching a threshold to limit
> the
> memory consumption in the kernel more reliably.
I agree with you that this does not allow deterministic memory
allocation within kernel (when rate stats are enabled).

I'll revisit this approach to address your concern and to allow a data
aggregator kind of usersapce application to collect info and provide
complete stats.

Also, do you feel it would be good to have both ,i.e complete stats
collection within kernel(this approach) and dump+clear of stats on
reaching threshold(your approach) and have one of these two modes
selected based on the requirement.

Thanks,
Sriram.R
>
> johannes