* [WT PATCH 2/6] mac80211: Make un-found-rate splat a warn-once.
2013-06-29 22:58 [WT PATCH 1/6] mac80211: Add debugfs file to show station-hash counts greearb
@ 2013-06-29 22:58 ` greearb
2013-07-11 8:52 ` Johannes Berg
2013-06-29 22:58 ` [WT PATCH 3/6] wireless: Add memory usage debugging greearb
` (4 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: greearb @ 2013-06-29 22:58 UTC (permalink / raw)
To: linux-wireless; +Cc: Ben Greear
From: Ben Greear <greearb@candelatech.com>
After that, print it out with net_ratelimit. We saw a system
continually hit this warning, for reasons unknown, and it
seems it bogged the system down enough to make it go OOM.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
net/mac80211/tx.c | 21 ++++++++++++++-------
1 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 4105d0c..8601f3f 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -678,14 +678,21 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
* Lets not bother rate control if we're associated and cannot
* talk to the sta. This should not happen.
*/
- if (WARN(test_bit(SCAN_SW_SCANNING, &tx->local->scanning) && assoc &&
- !rate_usable_index_exists(sband, &tx->sta->sta),
- "%s: Dropped data frame as no usable bitrate found while "
- "scanning and associated. Target station: "
- "%pM on %d GHz band\n",
- tx->sdata->name, hdr->addr1,
- info->band ? 5 : 2))
+ if (test_bit(SCAN_SW_SCANNING, &tx->local->scanning) && assoc &&
+ !rate_usable_index_exists(sband, &tx->sta->sta)) {
+ static bool do_once = true;
+ if (do_once) {
+ WARN(1, "%s: Dropped data frame as no usable bitrate found while scanning and associated. Target station: %pM on %d GHz band\n",
+ tx->sdata->name, hdr->addr1,
+ info->band ? 5 : 2);
+ do_once = false;
+ } else {
+ net_info_ratelimited("%s: Dropped data frame as no usable bitrate found while scanning and associated. Target station: %pM on %d GHz band\n",
+ tx->sdata->name, hdr->addr1,
+ info->band ? 5 : 2);
+ }
return TX_DROP;
+ }
/*
* If we're associated with the sta at this point we know we can at
--
1.7.3.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* [WT PATCH 3/6] wireless: Add memory usage debugging.
2013-06-29 22:58 [WT PATCH 1/6] mac80211: Add debugfs file to show station-hash counts greearb
2013-06-29 22:58 ` [WT PATCH 2/6] mac80211: Make un-found-rate splat a warn-once greearb
@ 2013-06-29 22:58 ` greearb
2013-07-11 8:53 ` Johannes Berg
2013-06-29 22:58 ` [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash greearb
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: greearb @ 2013-06-29 22:58 UTC (permalink / raw)
To: linux-wireless; +Cc: Ben Greear
From: Ben Greear <greearb@candelatech.com>
The bss objects are reference counted, and the ies
are also tricky to keep track of. Add module option to
track allocation and freeing of the ies and bss objects,
and add debugfs files to show the current objects.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
net/wireless/core.c | 9 +++-
net/wireless/core.h | 16 ++++++
net/wireless/debugfs.c | 113 +++++++++++++++++++++++++++++++++++++++
net/wireless/debugfs.h | 2 +
net/wireless/scan.c | 139 ++++++++++++++++++++++++++++++++++++++++++-----
5 files changed, 262 insertions(+), 17 deletions(-)
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 672459b..861fc37 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -50,6 +50,10 @@ static bool cfg80211_disable_40mhz_24ghz;
module_param(cfg80211_disable_40mhz_24ghz, bool, 0644);
MODULE_PARM_DESC(cfg80211_disable_40mhz_24ghz,
"Disable 40MHz support in the 2.4GHz band");
+bool cfg80211_mem_debugging;
+module_param(cfg80211_mem_debugging, bool, 0644);
+MODULE_PARM_DESC(cfg80211_mem_debugging,
+ "Enable memory tracking for ies and bss objects.");
struct cfg80211_registered_device *cfg80211_rdev_by_wiphy_idx(int wiphy_idx)
{
@@ -996,6 +1000,7 @@ static int __init cfg80211_init(void)
goto out_fail_nl80211;
ieee80211_debugfs_dir = debugfs_create_dir("ieee80211", NULL);
+ ieee80211_debugfs_add_glbl(ieee80211_debugfs_dir);
err = regulatory_init();
if (err)
@@ -1012,7 +1017,7 @@ static int __init cfg80211_init(void)
out_fail_wq:
regulatory_exit();
out_fail_reg:
- debugfs_remove(ieee80211_debugfs_dir);
+ debugfs_remove_recursive(ieee80211_debugfs_dir);
out_fail_nl80211:
unregister_netdevice_notifier(&cfg80211_netdev_notifier);
out_fail_notifier:
@@ -1026,7 +1031,7 @@ subsys_initcall(cfg80211_init);
static void __exit cfg80211_exit(void)
{
- debugfs_remove(ieee80211_debugfs_dir);
+ debugfs_remove_recursive(ieee80211_debugfs_dir);
nl80211_exit();
unregister_netdevice_notifier(&cfg80211_netdev_notifier);
wiphy_sysfs_exit();
diff --git a/net/wireless/core.h b/net/wireless/core.h
index a6b45bf..74d694f 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -114,6 +114,22 @@ extern struct workqueue_struct *cfg80211_wq;
extern struct list_head cfg80211_rdev_list;
extern int cfg80211_rdev_list_generation;
+extern bool cfg80211_mem_debugging; /* module option */
+#ifdef CONFIG_CFG80211_DEBUGFS
+struct wifi_mem_tracker {
+ struct list_head mylist;
+ char buf[40];
+ void *ptr;
+};
+extern struct list_head ies_list;
+extern spinlock_t ies_lock;
+extern atomic_t ies_count;
+
+extern struct list_head bss_list;
+extern spinlock_t bss_lock;
+extern atomic_t bss_count;
+#endif
+
struct cfg80211_internal_bss {
struct list_head list;
struct list_head hidden_list;
diff --git a/net/wireless/debugfs.c b/net/wireless/debugfs.c
index 90d0500..fd52bb1 100644
--- a/net/wireless/debugfs.c
+++ b/net/wireless/debugfs.c
@@ -31,6 +31,108 @@ static const struct file_operations name## _ops = { \
.llseek = generic_file_llseek, \
};
+#define DEBUGFS_READONLY_FILE_OPS(name) \
+static const struct file_operations name## _ops = { \
+ .read = name## _read, \
+ .open = simple_open, \
+ .llseek = generic_file_llseek, \
+};
+
+static ssize_t all_ies_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ int mxln = 31500;
+ char *buf = kzalloc(mxln, GFP_KERNEL);
+ int q, res = 0;
+ struct wifi_mem_tracker *iesm;
+
+ if (!buf)
+ return 0;
+
+ spin_lock_bh(&ies_lock);
+ res += sprintf(buf + res, "Total: %i\n", atomic_read(&ies_count));
+ list_for_each_entry(iesm, &ies_list, mylist) {
+ res += sprintf(buf + res, "%p: %s\n",
+ iesm->ptr, iesm->buf);
+ if (res >= mxln) {
+ res = mxln;
+ break;
+ }
+ }
+ spin_unlock_bh(&ies_lock);
+
+ q = simple_read_from_buffer(user_buf, count, ppos, buf, res);
+ kfree(buf);
+ return q;
+}
+
+static ssize_t all_bss_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ int mxln = 31500;
+ char *buf = kzalloc(mxln, GFP_KERNEL);
+ int q, res = 0;
+ struct wifi_mem_tracker *bssm;
+
+ if (!buf)
+ return 0;
+
+ spin_lock_bh(&bss_lock);
+ res += sprintf(buf + res, "Total: %i\n", atomic_read(&bss_count));
+ list_for_each_entry(bssm, &bss_list, mylist) {
+ struct cfg80211_internal_bss *bss;
+ bss = (struct cfg80211_internal_bss *)(bssm->ptr);
+ res += sprintf(buf + res, "%p: #%lu %s\n",
+ bssm->ptr, bss->refcount, bssm->buf);
+ if (res >= mxln) {
+ res = mxln;
+ break;
+ }
+ }
+ spin_unlock_bh(&bss_lock);
+
+ q = simple_read_from_buffer(user_buf, count, ppos, buf, res);
+ kfree(buf);
+ return q;
+}
+
+DEBUGFS_READONLY_FILE_OPS(all_ies);
+DEBUGFS_READONLY_FILE_OPS(all_bss);
+
+
+static ssize_t bss_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct wiphy *wiphy = file->private_data;
+ struct cfg80211_registered_device *dev = wiphy_to_dev(wiphy);
+ int mxln = 31500;
+ char *buf = kzalloc(mxln, GFP_KERNEL);
+ int q, res = 0;
+ struct cfg80211_internal_bss *bss;
+
+ if (!buf)
+ return 0;
+
+ spin_lock_bh(&dev->bss_lock);
+ list_for_each_entry(bss, &dev->bss_list, list) {
+ res += sprintf(buf + res,
+ "%p: #%lu bcn: %p pr: %p hidden: %p\n",
+ bss, bss->refcount,
+ rcu_access_pointer(bss->pub.beacon_ies),
+ rcu_access_pointer(bss->pub.proberesp_ies),
+ bss->pub.hidden_beacon_bss);
+ if (res >= mxln) {
+ res = mxln;
+ break;
+ }
+ }
+ spin_unlock_bh(&dev->bss_lock);
+
+ q = simple_read_from_buffer(user_buf, count, ppos, buf, res);
+ kfree(buf);
+ return q;
+}
+
DEBUGFS_READONLY_FILE(rts_threshold, 20, "%d",
wiphy->rts_threshold)
DEBUGFS_READONLY_FILE(fragmentation_threshold, 20, "%d",
@@ -39,6 +141,7 @@ DEBUGFS_READONLY_FILE(short_retry_limit, 20, "%d",
wiphy->retry_short)
DEBUGFS_READONLY_FILE(long_retry_limit, 20, "%d",
wiphy->retry_long);
+DEBUGFS_READONLY_FILE_OPS(bss);
static int ht_print_chan(struct ieee80211_channel *chan,
char *buf, int buf_size, int offset)
@@ -112,4 +215,14 @@ void cfg80211_debugfs_rdev_add(struct cfg80211_registered_device *rdev)
DEBUGFS_ADD(short_retry_limit);
DEBUGFS_ADD(long_retry_limit);
DEBUGFS_ADD(ht40allow_map);
+ DEBUGFS_ADD(bss);
+}
+
+#define DEBUGFS_ADD_GLBL(name) \
+ debugfs_create_file(#name, S_IRUGO, dir, NULL, &name## _ops);
+
+void ieee80211_debugfs_add_glbl(struct dentry *dir)
+{
+ DEBUGFS_ADD_GLBL(all_ies);
+ DEBUGFS_ADD_GLBL(all_bss);
}
diff --git a/net/wireless/debugfs.h b/net/wireless/debugfs.h
index 74fdd38..f644869 100644
--- a/net/wireless/debugfs.h
+++ b/net/wireless/debugfs.h
@@ -3,9 +3,11 @@
#ifdef CONFIG_CFG80211_DEBUGFS
void cfg80211_debugfs_rdev_add(struct cfg80211_registered_device *rdev);
+void ieee80211_debugfs_add_glbl(struct dentry *dir);
#else
static inline
void cfg80211_debugfs_rdev_add(struct cfg80211_registered_device *rdev) {}
+static inline void ieee80211_debugfs_add_glbl(struct dentry *dir) { }
#endif
#endif /* __CFG80211_DEBUGFS_H */
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index ae8c186..2d66334 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -57,6 +57,118 @@
#define IEEE80211_SCAN_RESULT_EXPIRE (30 * HZ)
+#ifdef CONFIG_CFG80211_DEBUGFS
+/* memory debugging structures, only useful when debugfs
+ * is enabled.
+ */
+LIST_HEAD(ies_list);
+DEFINE_SPINLOCK(ies_lock);
+atomic_t ies_count = ATOMIC_INIT(0);
+
+LIST_HEAD(bss_list);
+DEFINE_SPINLOCK(bss_lock);
+atomic_t bss_count = ATOMIC_INIT(0);
+
+
+static void my_kfree_rcu_ies(struct cfg80211_bss_ies *ies)
+{
+ if (cfg80211_mem_debugging) {
+ struct wifi_mem_tracker *iesm;
+ spin_lock_bh(&ies_lock);
+ list_for_each_entry(iesm, &ies_list, mylist) {
+ if (iesm->ptr == ies) {
+ list_del(&iesm->mylist);
+ kfree(iesm);
+ break;
+ }
+ }
+ spin_unlock_bh(&ies_lock);
+ }
+ atomic_sub(1, &ies_count);
+ kfree_rcu(ies, rcu_head);
+}
+
+#define my_kmalloc_ies(s, g) \
+ _my_kmalloc_ies(s, g, __LINE__);
+
+static void* _my_kmalloc_ies(size_t s, gfp_t gfp, int l)
+{
+ void *rv = kmalloc(s, gfp);
+ if (rv) {
+ atomic_add(1, &ies_count);
+ if (cfg80211_mem_debugging) {
+ struct wifi_mem_tracker *iesm;
+ iesm = kmalloc(sizeof(*iesm), gfp);
+ if (iesm) {
+ snprintf(iesm->buf, sizeof(iesm->buf), "%i", l);
+ iesm->buf[sizeof(iesm->buf)-1] = 0;
+ iesm->ptr = rv;
+ INIT_LIST_HEAD(&iesm->mylist);
+ spin_lock_bh(&ies_lock);
+ list_add(&iesm->mylist, &ies_list);
+ spin_unlock_bh(&ies_lock);
+ } else {
+ pr_err("ERROR: Could not allocate iesm.\n");
+ }
+ }
+ }
+ return rv;
+}
+
+static void my_kfree_bss(struct cfg80211_internal_bss *bss)
+{
+ if (cfg80211_mem_debugging) {
+ struct wifi_mem_tracker *bssm;
+ spin_lock_bh(&bss_lock);
+ list_for_each_entry(bssm, &bss_list, mylist) {
+ if (bssm->ptr == bss) {
+ list_del(&bssm->mylist);
+ kfree(bssm);
+ break;
+ }
+ }
+ spin_unlock_bh(&bss_lock);
+ }
+ atomic_sub(1, &bss_count);
+ kfree(bss);
+}
+
+#define my_kzalloc_bss(s, g) \
+ _my_kzalloc_bss(s, g, __LINE__);
+
+static void* _my_kzalloc_bss(size_t s, gfp_t gfp, int l)
+{
+ void *rv = kmalloc(s, gfp);
+ if (rv) {
+ atomic_add(1, &bss_count);
+ if (cfg80211_mem_debugging) {
+ struct wifi_mem_tracker *bssm;
+ bssm = kmalloc(sizeof(*bssm), gfp);
+ if (bssm) {
+ snprintf(bssm->buf, sizeof(bssm->buf), "%i", l);
+ bssm->buf[sizeof(bssm->buf)-1] = 0;
+ bssm->ptr = rv;
+ INIT_LIST_HEAD(&bssm->mylist);
+ spin_lock_bh(&bss_lock);
+ list_add(&bssm->mylist, &bss_list);
+ spin_unlock_bh(&bss_lock);
+ } else {
+ pr_err("ERROR: Could not allocate bssm for bss.\n");
+ }
+ }
+ }
+ return rv;
+}
+
+#else
+
+#define my_kfree_rcu_ies(ies) kfree_rcu(ies, rcu_head)
+#define my_kmalloc_ies(s, g) kmalloc(s, g)
+#define my_kfree_bss(a) kfree(a)
+#define my_kzalloc_bss(s, g) kzalloc(s, g)
+
+#endif
+
static void bss_free(struct cfg80211_internal_bss *bss)
{
struct cfg80211_bss_ies *ies;
@@ -66,10 +178,10 @@ static void bss_free(struct cfg80211_internal_bss *bss)
ies = (void *)rcu_access_pointer(bss->pub.beacon_ies);
if (ies && !bss->pub.hidden_beacon_bss)
- kfree_rcu(ies, rcu_head);
+ my_kfree_rcu_ies(ies);
ies = (void *)rcu_access_pointer(bss->pub.proberesp_ies);
if (ies)
- kfree_rcu(ies, rcu_head);
+ my_kfree_rcu_ies(ies);
/*
* This happens when the module is removed, it doesn't
@@ -78,7 +190,7 @@ static void bss_free(struct cfg80211_internal_bss *bss)
if (!list_empty(&bss->hidden_list))
list_del(&bss->hidden_list);
- kfree(bss);
+ my_kfree_bss(bss);
}
static inline void bss_ref_get(struct cfg80211_registered_device *dev,
@@ -713,8 +825,7 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
rcu_assign_pointer(found->pub.ies,
tmp->pub.proberesp_ies);
if (old)
- kfree_rcu((struct cfg80211_bss_ies *)old,
- rcu_head);
+ my_kfree_rcu_ies((struct cfg80211_bss_ies *)old);
} else if (rcu_access_pointer(tmp->pub.beacon_ies)) {
const struct cfg80211_bss_ies *old;
struct cfg80211_internal_bss *bss;
@@ -734,8 +845,7 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
*/
f = rcu_access_pointer(tmp->pub.beacon_ies);
- kfree_rcu((struct cfg80211_bss_ies *)f,
- rcu_head);
+ my_kfree_rcu_ies((struct cfg80211_bss_ies *)f);
goto drop;
}
@@ -762,8 +872,7 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
}
if (old)
- kfree_rcu((struct cfg80211_bss_ies *)old,
- rcu_head);
+ my_kfree_rcu_ies((struct cfg80211_bss_ies *)old);
}
found->pub.beacon_interval = tmp->pub.beacon_interval;
@@ -780,15 +889,15 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
* is allocated on the stack since it's not needed in the
* more common case of an update
*/
- new = kzalloc(sizeof(*new) + dev->wiphy.bss_priv_size,
- GFP_ATOMIC);
+ new = my_kzalloc_bss(sizeof(*new) + dev->wiphy.bss_priv_size,
+ GFP_ATOMIC);
if (!new) {
ies = (void *)rcu_dereference(tmp->pub.beacon_ies);
if (ies)
- kfree_rcu(ies, rcu_head);
+ my_kfree_rcu_ies(ies);
ies = (void *)rcu_dereference(tmp->pub.proberesp_ies);
if (ies)
- kfree_rcu(ies, rcu_head);
+ my_kfree_rcu_ies(ies);
goto drop;
}
memcpy(new, tmp, sizeof(*new));
@@ -903,7 +1012,7 @@ cfg80211_inform_bss(struct wiphy *wiphy,
* override the IEs pointer should we have received an earlier
* indication of Probe Response data.
*/
- ies = kmalloc(sizeof(*ies) + ielen, gfp);
+ ies = my_kmalloc_ies(sizeof(*ies) + ielen, gfp);
if (!ies)
return NULL;
ies->len = ielen;
@@ -961,7 +1070,7 @@ cfg80211_inform_bss_frame(struct wiphy *wiphy,
if (!channel)
return NULL;
- ies = kmalloc(sizeof(*ies) + ielen, gfp);
+ ies = my_kmalloc_ies(sizeof(*ies) + ielen, gfp);
if (!ies)
return NULL;
ies->len = ielen;
--
1.7.3.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
2013-06-29 22:58 [WT PATCH 1/6] mac80211: Add debugfs file to show station-hash counts greearb
2013-06-29 22:58 ` [WT PATCH 2/6] mac80211: Make un-found-rate splat a warn-once greearb
2013-06-29 22:58 ` [WT PATCH 3/6] wireless: Add memory usage debugging greearb
@ 2013-06-29 22:58 ` greearb
2013-07-11 8:55 ` Johannes Berg
2013-06-29 22:58 ` [WT PATCH 5/6] mac80211: Add debugfs for sdata and sdata->sta_vhash greearb
` (2 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: greearb @ 2013-06-29 22:58 UTC (permalink / raw)
To: linux-wireless; +Cc: Ben Greear
From: Ben Greear <greearb@candelatech.com>
Add sdata hash (based on sdata->vif.addr) to local
structure.
Add sta_vhash (based on sta->sta.addr) to sdata struct.
Make STA_HASH give a better hash spread more often.
Use new hashes where we can. Might be able to completely
get rid of the local->sta_hash, but didn't want to try that
quite yet.
This significantly improves performance when using lots
of station VIFs connected to the same AP. It will likely
help other cases where the old hash logic failed to create
a decent spread.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
net/mac80211/ieee80211_i.h | 34 +++++++++++++++
net/mac80211/iface.c | 50 ++++++++++++++++++++++-
net/mac80211/rx.c | 16 +++++++
net/mac80211/sta_info.c | 97 +++++++++++++++++++++++++++++++++++---------
net/mac80211/sta_info.h | 18 +++++++-
net/mac80211/status.c | 6 +++
6 files changed, 198 insertions(+), 23 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 8412a30..f36f120 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -670,6 +670,10 @@ struct ieee80211_chanctx {
struct ieee80211_sub_if_data {
struct list_head list;
+ struct ieee80211_sub_if_data *hnext; /* sdata hash list pointer */
+
+ /* Protected by local->sta_mtx */
+ struct sta_info __rcu *sta_vhash[STA_HASH_SIZE]; /* By station addr */
struct wireless_dev wdev;
@@ -794,6 +798,34 @@ sdata_assert_lock(struct ieee80211_sub_if_data *sdata)
lockdep_assert_held(&sdata->wdev.mtx);
}
+static inline
+void for_each_sdata_type_check(struct ieee80211_local *local,
+ const u8 *addr,
+ struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_sub_if_data *nxt)
+{
+}
+
+/* This deals with multiple sdata having same MAC */
+#define for_each_sdata(local, _addr, _sdata, nxt) \
+ for ( /* initialise loop */ \
+ _sdata = rcu_dereference(local->sdata_hash[STA_HASH(_addr)]), \
+ nxt = _sdata ? rcu_dereference(_sdata->hnext) : NULL; \
+ /* typecheck */ \
+ for_each_sdata_type_check(local, (_addr), _sdata, nxt), \
+ /* continue condition */ \
+ _sdata; \
+ /* advance loop */ \
+ _sdata = nxt, \
+ nxt = _sdata ? rcu_dereference(_sdata->hnext) : NULL \
+ ) \
+ /* compare address and run code only if it matches */ \
+ if (ether_addr_equal(_sdata->vif.addr, (_addr)))
+
+
+struct ieee80211_sub_if_data*
+ieee80211_find_sdata(struct ieee80211_local *local, const u8 *vif_addr);
+
static inline enum ieee80211_band
ieee80211_get_sdata_band(struct ieee80211_sub_if_data *sdata)
{
@@ -1009,6 +1041,8 @@ struct ieee80211_local {
u32 wep_iv;
/* see iface.c */
+ /* Hash interfaces by VIF mac addr */
+ struct ieee80211_sub_if_data __rcu *sdata_hash[STA_HASH_SIZE];
struct list_head interfaces;
struct mutex iflist_mtx;
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index a2a8250..1bf7996 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -208,6 +208,47 @@ static int ieee80211_verify_mac(struct ieee80211_sub_if_data *sdata, u8 *addr,
return ret;
}
+
+static void __ieee80211_if_add_hash(struct ieee80211_sub_if_data *sdata)
+{
+ struct ieee80211_local *local = sdata->local;
+ int idx = STA_HASH(sdata->vif.addr);
+
+ lockdep_assert_held(&local->iflist_mtx);
+ sdata->hnext = local->sdata_hash[idx];
+ rcu_assign_pointer(local->sdata_hash[idx], sdata);
+}
+
+static int __ieee80211_if_remove_hash(struct ieee80211_sub_if_data *sdata)
+{
+ struct ieee80211_sub_if_data *s;
+ struct ieee80211_local *local = sdata->local;
+ int idx = STA_HASH(sdata->vif.addr);
+
+ lockdep_assert_held(&local->iflist_mtx);
+ s = rcu_dereference_protected(local->sdata_hash[idx],
+ lockdep_is_held(&local->iflist_mtx));
+ if (!s)
+ return -ENOENT;
+
+ if (s == sdata) {
+ rcu_assign_pointer(local->sdata_hash[idx], s->hnext);
+ return 0;
+ }
+
+ while (rcu_access_pointer(s->hnext) &&
+ rcu_access_pointer(s->hnext) != sdata)
+ s = rcu_dereference_protected(s->hnext,
+ lockdep_is_held(&local->iflist_mtx));
+
+ if (rcu_access_pointer(s->hnext)) {
+ rcu_assign_pointer(s->hnext, sdata->hnext);
+ return 0;
+ }
+ return -ENOENT;
+}
+
+
static int ieee80211_change_mac(struct net_device *dev, void *addr)
{
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
@@ -228,8 +269,13 @@ static int ieee80211_change_mac(struct net_device *dev, void *addr)
ret = eth_mac_addr(dev, sa);
- if (ret == 0)
+ if (ret == 0) {
+ mutex_lock(&sdata->local->iflist_mtx);
+ __ieee80211_if_remove_hash(sdata);
memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN);
+ __ieee80211_if_add_hash(sdata);
+ mutex_unlock(&sdata->local->iflist_mtx);
+ }
return ret;
}
@@ -1688,6 +1734,7 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
mutex_lock(&local->iflist_mtx);
list_add_tail_rcu(&sdata->list, &local->interfaces);
+ __ieee80211_if_add_hash(sdata);
mutex_unlock(&local->iflist_mtx);
if (new_wdev)
@@ -1702,6 +1749,7 @@ void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata)
mutex_lock(&sdata->local->iflist_mtx);
list_del_rcu(&sdata->list);
+ __ieee80211_if_remove_hash(sdata);
mutex_unlock(&sdata->local->iflist_mtx);
synchronize_rcu();
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 23dbcfc..67efa59 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -3172,6 +3172,21 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
if (ieee80211_is_data(fc)) {
prev_sta = NULL;
+ /* Check for Station VIFS by hashing on the destination MAC
+ * (ie, local sdata MAC). This changes 'promisc' behaviour,
+ * but not sure that is a bad thing.
+ */
+ if ((!is_multicast_ether_addr(hdr->addr1)) &&
+ (local->monitors == 0) && (local->cooked_mntrs == 0)) {
+ sta = sta_info_get_by_vif(local, hdr->addr1,
+ hdr->addr2);
+ if (sta) {
+ rx.sta = sta;
+ rx.sdata = sta->sdata;
+ goto rx_and_done;
+ }
+ }
+
for_each_sta_info(local, hdr->addr2, sta, tmp) {
if (!prev_sta) {
prev_sta = sta;
@@ -3189,6 +3204,7 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
rx.sta = prev_sta;
rx.sdata = prev_sta->sdata;
+rx_and_done:
if (ieee80211_prepare_and_rx_handle(&rx, skb, true))
return;
goto out;
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index aeb967a..44bb89b 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -68,27 +68,54 @@ static int sta_info_hash_del(struct ieee80211_local *local,
struct sta_info *sta)
{
struct sta_info *s;
+ int rv = -ENOENT;
+ int idx = STA_HASH(sta->sta.addr);
+ struct ieee80211_sub_if_data *sdata = sta->sdata;
- s = rcu_dereference_protected(local->sta_hash[STA_HASH(sta->sta.addr)],
+ s = rcu_dereference_protected(local->sta_hash[idx],
lockdep_is_held(&local->sta_mtx));
if (!s)
- return -ENOENT;
+ /* If station is not in the main hash, then it definitely
+ * should not be in the vhash, so we can just return.
+ */
+ return rv;
+
if (s == sta) {
- rcu_assign_pointer(local->sta_hash[STA_HASH(sta->sta.addr)],
- s->hnext);
- return 0;
+ rcu_assign_pointer(local->sta_hash[idx], s->hnext);
+ rv = 0;
+ goto try_vhash;
}
while (rcu_access_pointer(s->hnext) &&
rcu_access_pointer(s->hnext) != sta)
s = rcu_dereference_protected(s->hnext,
- lockdep_is_held(&local->sta_mtx));
+ lockdep_is_held(&local->sta_mtx));
if (rcu_access_pointer(s->hnext)) {
rcu_assign_pointer(s->hnext, sta->hnext);
- return 0;
+ rv = 0;
+ goto try_vhash;
}
+ return rv;
- return -ENOENT;
+try_vhash:
+ s = rcu_dereference_protected(sdata->sta_vhash[idx],
+ lockdep_is_held(&local->sta_mtx));
+ if (!s)
+ return rv;
+
+ if (s == sta) {
+ rcu_assign_pointer(sdata->sta_vhash[idx], s->vnext);
+ return rv;
+ }
+
+ while (rcu_access_pointer(s->vnext) &&
+ rcu_access_pointer(s->vnext) != sta)
+ s = rcu_dereference_protected(s->vnext,
+ lockdep_is_held(&local->sta_mtx));
+ if (rcu_access_pointer(s->vnext))
+ rcu_assign_pointer(s->vnext, sta->vnext);
+
+ return rv;
}
static void cleanup_single_sta(struct sta_info *sta)
@@ -195,17 +222,15 @@ static void free_sta_rcu(struct rcu_head *h)
struct sta_info *sta_info_get(struct ieee80211_sub_if_data *sdata,
const u8 *addr)
{
- struct ieee80211_local *local = sdata->local;
struct sta_info *sta;
- sta = rcu_dereference_check(local->sta_hash[STA_HASH(addr)],
- lockdep_is_held(&local->sta_mtx));
+ sta = rcu_dereference_check(sdata->sta_vhash[STA_HASH(addr)],
+ lockdep_is_held(&sdata->local->sta_mtx));
while (sta) {
- if (sta->sdata == sdata &&
- ether_addr_equal(sta->sta.addr, addr))
+ if (ether_addr_equal(sta->sta.addr, addr))
break;
- sta = rcu_dereference_check(sta->hnext,
- lockdep_is_held(&local->sta_mtx));
+ sta = rcu_dereference_check(sta->vnext,
+ lockdep_is_held(&sdata->local->sta_mtx));
}
return sta;
}
@@ -220,6 +245,13 @@ struct sta_info *sta_info_get_bss(struct ieee80211_sub_if_data *sdata,
struct ieee80211_local *local = sdata->local;
struct sta_info *sta;
+ sta = sta_info_get(sdata, addr);
+ if (sta)
+ return sta;
+
+ /* Maybe it's on some other sdata matching the bss, try
+ * a bit harder.
+ */
sta = rcu_dereference_check(local->sta_hash[STA_HASH(addr)],
lockdep_is_held(&local->sta_mtx));
while (sta) {
@@ -233,6 +265,22 @@ struct sta_info *sta_info_get_bss(struct ieee80211_sub_if_data *sdata,
return sta;
}
+struct sta_info *sta_info_get_by_vif(struct ieee80211_local *local,
+ const u8 *vif_addr, const u8 *sta_addr)
+{
+ struct ieee80211_sub_if_data *sdata;
+ struct ieee80211_sub_if_data *nxt;
+ struct sta_info *sta;
+
+ for_each_sdata(local, vif_addr, sdata, nxt) {
+ sta = sta_info_get(sdata, sta_addr);
+ if (sta)
+ return sta;
+ }
+ return NULL;
+}
+
+
struct sta_info *sta_info_get_by_idx(struct ieee80211_sub_if_data *sdata,
int idx)
{
@@ -278,9 +326,14 @@ void sta_info_free(struct ieee80211_local *local, struct sta_info *sta)
static void sta_info_hash_add(struct ieee80211_local *local,
struct sta_info *sta)
{
+ int idx = STA_HASH(sta->sta.addr);
+
lockdep_assert_held(&local->sta_mtx);
- sta->hnext = local->sta_hash[STA_HASH(sta->sta.addr)];
- rcu_assign_pointer(local->sta_hash[STA_HASH(sta->sta.addr)], sta);
+ sta->hnext = local->sta_hash[idx];
+ rcu_assign_pointer(local->sta_hash[idx], sta);
+
+ sta->vnext = sta->sdata->sta_vhash[idx];
+ rcu_assign_pointer(sta->sdata->sta_vhash[idx], sta);
}
static void sta_unblock(struct work_struct *wk)
@@ -975,14 +1028,18 @@ struct ieee80211_sta *ieee80211_find_sta_by_ifaddr(struct ieee80211_hw *hw,
{
struct sta_info *sta, *nxt;
+ if (localaddr) {
+ sta = sta_info_get_by_vif(hw_to_local(hw), localaddr, addr);
+ if (sta && sta->uploaded)
+ return &sta->sta;
+ return NULL;
+ }
+
/*
* Just return a random station if localaddr is NULL
* ... first in list.
*/
for_each_sta_info(hw_to_local(hw), addr, sta, nxt) {
- if (localaddr &&
- !ether_addr_equal(sta->sdata->vif.addr, localaddr))
- continue;
if (!sta->uploaded)
return NULL;
return &sta->sta;
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 4208dbd..963de5e 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -15,6 +15,7 @@
#include <linux/workqueue.h>
#include <linux/average.h>
#include <linux/etherdevice.h>
+#include <linux/hash.h>
#include "key.h"
/**
@@ -228,7 +229,8 @@ struct sta_ampdu_mlme {
* mac80211 is communicating with.
*
* @list: global linked list entry
- * @hnext: hash table linked list pointer
+ * @hnext: hash table linked list pointer, used by local->sta_hash
+ * @vnext: hash table linked list pointer, used by sdata->sta_vhash.
* @local: pointer to the global information
* @sdata: virtual interface this station belongs to
* @ptk: peer key negotiated with this station, if any
@@ -307,6 +309,7 @@ struct sta_info {
struct list_head list;
struct rcu_head rcu_head;
struct sta_info __rcu *hnext;
+ struct sta_info __rcu *vnext;
struct ieee80211_local *local;
struct ieee80211_sub_if_data *sdata;
struct ieee80211_key __rcu *gtk[NUM_DEFAULT_KEYS + NUM_DEFAULT_MGMT_KEYS];
@@ -492,7 +495,12 @@ rcu_dereference_protected_tid_tx(struct sta_info *sta, int tid)
}
#define STA_HASH_SIZE 256
-#define STA_HASH(sta) (sta[5])
+static inline u32 STA_HASH(const unsigned char *addr)
+{
+ u32 v = (addr[0] << 8) | addr[1];
+ v ^= (addr[2] << 24) | (addr[3] << 16) | (addr[4] << 8) | addr[5];
+ return hash_32(v, 8);
+}
/* Maximum number of frames to buffer per power saving station per AC */
@@ -514,6 +522,12 @@ struct sta_info *sta_info_get(struct ieee80211_sub_if_data *sdata,
struct sta_info *sta_info_get_bss(struct ieee80211_sub_if_data *sdata,
const u8 *addr);
+/*
+ * Uses the local->sdata hash and sdata->sta_hash for fast lookup
+ * base on VIF (sdata) address and remote station address.
+ */
+struct sta_info *sta_info_get_by_vif(struct ieee80211_local *local,
+ const u8 *vif_addr, const u8 *sta_addr);
static inline
void for_each_sta_info_type_check(struct ieee80211_local *local,
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 4343920..0ecab1a 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -453,11 +453,16 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
sband = local->hw.wiphy->bands[info->band];
fc = hdr->frame_control;
+ sta = sta_info_get_by_vif(local, hdr->addr2, hdr->addr1);
+ if (sta)
+ goto found_it;
+
for_each_sta_info(local, hdr->addr1, sta, tmp) {
/* skip wrong virtual interface */
if (!ether_addr_equal(hdr->addr2, sta->sdata->vif.addr))
continue;
+found_it:
if (info->flags & IEEE80211_TX_STATUS_EOSP)
clear_sta_flag(sta, WLAN_STA_SP);
@@ -553,6 +558,7 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
if (acked)
sta->last_ack_signal = info->status.ack_signal;
+ break;
}
rcu_read_unlock();
--
1.7.3.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
2013-06-29 22:58 ` [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash greearb
@ 2013-07-11 8:55 ` Johannes Berg
2013-07-11 15:29 ` Ben Greear
0 siblings, 1 reply; 21+ messages in thread
From: Johannes Berg @ 2013-07-11 8:55 UTC (permalink / raw)
To: greearb; +Cc: linux-wireless
On Sat, 2013-06-29 at 15:58 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> Add sdata hash (based on sdata->vif.addr) to local
> structure.
>
> Add sta_vhash (based on sta->sta.addr) to sdata struct.
>
> Make STA_HASH give a better hash spread more often.
>
> Use new hashes where we can. Might be able to completely
> get rid of the local->sta_hash, but didn't want to try that
> quite yet.
>
> This significantly improves performance when using lots
> of station VIFs connected to the same AP. It will likely
> help other cases where the old hash logic failed to create
> a decent spread.
I think this is too much code for a corner case unlikely to happen
outside of your specific scenario, so I'm not taking this either.
I also don't like maintaining two separate hash tables and all that.
I'd reconsider if you actually remove the hash entirely, but that'll be
tricky to walk the station list and will quite possibly make the RX path
there more expensive?
johannes
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
2013-07-11 8:55 ` Johannes Berg
@ 2013-07-11 15:29 ` Ben Greear
2013-07-26 8:53 ` Johannes Berg
0 siblings, 1 reply; 21+ messages in thread
From: Ben Greear @ 2013-07-11 15:29 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
On 07/11/2013 01:55 AM, Johannes Berg wrote:
> On Sat, 2013-06-29 at 15:58 -0700, greearb@candelatech.com wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Add sdata hash (based on sdata->vif.addr) to local
>> structure.
>>
>> Add sta_vhash (based on sta->sta.addr) to sdata struct.
>>
>> Make STA_HASH give a better hash spread more often.
>>
>> Use new hashes where we can. Might be able to completely
>> get rid of the local->sta_hash, but didn't want to try that
>> quite yet.
>>
>> This significantly improves performance when using lots
>> of station VIFs connected to the same AP. It will likely
>> help other cases where the old hash logic failed to create
>> a decent spread.
>
> I think this is too much code for a corner case unlikely to happen
> outside of your specific scenario, so I'm not taking this either.
>
> I also don't like maintaining two separate hash tables and all that.
>
> I'd reconsider if you actually remove the hash entirely, but that'll be
> tricky to walk the station list and will quite possibly make the RX path
> there more expensive?
Remove local->sta_hash ?
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
2013-07-11 15:29 ` Ben Greear
@ 2013-07-26 8:53 ` Johannes Berg
2013-07-26 9:56 ` Felix Fietkau
2013-07-26 15:27 ` Ben Greear
0 siblings, 2 replies; 21+ messages in thread
From: Johannes Berg @ 2013-07-26 8:53 UTC (permalink / raw)
To: Ben Greear; +Cc: linux-wireless
On Thu, 2013-07-11 at 08:29 -0700, Ben Greear wrote:
> > I also don't like maintaining two separate hash tables and all that.
> >
> > I'd reconsider if you actually remove the hash entirely, but that'll be
> > tricky to walk the station list and will quite possibly make the RX path
> > there more expensive?
>
> Remove local->sta_hash ?
To be honest, I'm undecided. Yes, I was thinking that, but I also think
having a huge hashtable like that for each virtual interface is way
overkill, in particular for station interfaces that usually have one
peer (the AP) and maybe a few TDLS peers. Or P2P-Device interfaces that
have no peers at all ...
I don't see a good way to improve the hash either, since we don't always
(e.g. in RX path) have the interface address.
The basic problem really is that the hash now is designed to work well
for more regular use cases than yours, where you talk to any number of
different stations but degrades really badly when you talk only to a
single one many times. That use case is really special, and I don't want
to 'fix' that in a way that would make the other use case significantly
worse in memory consumption or CPU utilisation.
johannes
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
2013-07-26 8:53 ` Johannes Berg
@ 2013-07-26 9:56 ` Felix Fietkau
2013-07-26 15:22 ` Ben Greear
2013-07-26 15:27 ` Ben Greear
1 sibling, 1 reply; 21+ messages in thread
From: Felix Fietkau @ 2013-07-26 9:56 UTC (permalink / raw)
To: Johannes Berg; +Cc: Ben Greear, linux-wireless
On 2013-07-26 10:53 AM, Johannes Berg wrote:
> On Thu, 2013-07-11 at 08:29 -0700, Ben Greear wrote:
>
>> > I also don't like maintaining two separate hash tables and all that.
>> >
>> > I'd reconsider if you actually remove the hash entirely, but that'll be
>> > tricky to walk the station list and will quite possibly make the RX path
>> > there more expensive?
>>
>> Remove local->sta_hash ?
>
> To be honest, I'm undecided. Yes, I was thinking that, but I also think
> having a huge hashtable like that for each virtual interface is way
> overkill, in particular for station interfaces that usually have one
> peer (the AP) and maybe a few TDLS peers. Or P2P-Device interfaces that
> have no peers at all ...
>
> I don't see a good way to improve the hash either, since we don't always
> (e.g. in RX path) have the interface address.
How about mixing in the interface address into the hash. Theoretically
you should always have that available, even in the rx path. Multicast
data packets contain the BSSID, so you can get the address from there.
You just need to be careful about checking the DS bits to figure out
which address to use ;)
I think this is a much better solution than duplicating the hash, or
moving it into sdata entirely.
- Felix
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
2013-07-26 9:56 ` Felix Fietkau
@ 2013-07-26 15:22 ` Ben Greear
2013-07-26 15:38 ` Felix Fietkau
0 siblings, 1 reply; 21+ messages in thread
From: Ben Greear @ 2013-07-26 15:22 UTC (permalink / raw)
To: Felix Fietkau; +Cc: Johannes Berg, linux-wireless
On 07/26/2013 02:56 AM, Felix Fietkau wrote:
> On 2013-07-26 10:53 AM, Johannes Berg wrote:
>> On Thu, 2013-07-11 at 08:29 -0700, Ben Greear wrote:
>>
>>>> I also don't like maintaining two separate hash tables and all that.
>>>>
>>>> I'd reconsider if you actually remove the hash entirely, but that'll be
>>>> tricky to walk the station list and will quite possibly make the RX path
>>>> there more expensive?
>>>
>>> Remove local->sta_hash ?
>>
>> To be honest, I'm undecided. Yes, I was thinking that, but I also think
>> having a huge hashtable like that for each virtual interface is way
>> overkill, in particular for station interfaces that usually have one
>> peer (the AP) and maybe a few TDLS peers. Or P2P-Device interfaces that
>> have no peers at all ...
>>
>> I don't see a good way to improve the hash either, since we don't always
>> (e.g. in RX path) have the interface address.
> How about mixing in the interface address into the hash. Theoretically
> you should always have that available, even in the rx path. Multicast
> data packets contain the BSSID, so you can get the address from there.
> You just need to be careful about checking the DS bits to figure out
> which address to use ;)
> I think this is a much better solution than duplicating the hash, or
> moving it into sdata entirely.
I think I could probably get rid of the big global per wiphy hash and
use the per-wiphy sdata-hash and per-sdata station hashes.
To me, that is cleanest because it gives a nice ownership relationship
between wiphy, sdata, and stations.
For what it's worth, my hashing scheme has been working well on highly
loaded APs and Station machines.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
2013-07-26 15:22 ` Ben Greear
@ 2013-07-26 15:38 ` Felix Fietkau
2013-07-26 16:09 ` Ben Greear
0 siblings, 1 reply; 21+ messages in thread
From: Felix Fietkau @ 2013-07-26 15:38 UTC (permalink / raw)
To: Ben Greear; +Cc: Johannes Berg, linux-wireless
On 2013-07-26 5:22 PM, Ben Greear wrote:
> On 07/26/2013 02:56 AM, Felix Fietkau wrote:
>> On 2013-07-26 10:53 AM, Johannes Berg wrote:
>>> On Thu, 2013-07-11 at 08:29 -0700, Ben Greear wrote:
>>>
>>>>> I also don't like maintaining two separate hash tables and all that.
>>>>>
>>>>> I'd reconsider if you actually remove the hash entirely, but that'll be
>>>>> tricky to walk the station list and will quite possibly make the RX path
>>>>> there more expensive?
>>>>
>>>> Remove local->sta_hash ?
>>>
>>> To be honest, I'm undecided. Yes, I was thinking that, but I also think
>>> having a huge hashtable like that for each virtual interface is way
>>> overkill, in particular for station interfaces that usually have one
>>> peer (the AP) and maybe a few TDLS peers. Or P2P-Device interfaces that
>>> have no peers at all ...
>>>
>>> I don't see a good way to improve the hash either, since we don't always
>>> (e.g. in RX path) have the interface address.
>> How about mixing in the interface address into the hash. Theoretically
>> you should always have that available, even in the rx path. Multicast
>> data packets contain the BSSID, so you can get the address from there.
>> You just need to be careful about checking the DS bits to figure out
>> which address to use ;)
>> I think this is a much better solution than duplicating the hash, or
>> moving it into sdata entirely.
>
> I think I could probably get rid of the big global per wiphy hash and
> use the per-wiphy sdata-hash and per-sdata station hashes.
>
> To me, that is cleanest because it gives a nice ownership relationship
> between wiphy, sdata, and stations.
>
> For what it's worth, my hashing scheme has been working well on highly
> loaded APs and Station machines.
The global hash (with added vif-addr mixing) not only completely fixes
the many-STA-vif case, also has some other advantages compared to the
per-sdata hash:
- Lookup is easier in setups with multiple AP VLANs
- Better cache footprint (especially important for small embedded devices).
- You don't need a separate sdata lookup before the sta lookup.
I'm not convinced that keeping separate hashes is cleaner. Especially in
the AP_VLAN case, ownership is not clear in any way, since there's some
overlap between multiple sdata entities (belonging to the same BSS).
- Felix
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
2013-07-26 15:38 ` Felix Fietkau
@ 2013-07-26 16:09 ` Ben Greear
2013-07-26 17:59 ` Felix Fietkau
0 siblings, 1 reply; 21+ messages in thread
From: Ben Greear @ 2013-07-26 16:09 UTC (permalink / raw)
To: Felix Fietkau; +Cc: Johannes Berg, linux-wireless
On 07/26/2013 08:38 AM, Felix Fietkau wrote:
> On 2013-07-26 5:22 PM, Ben Greear wrote:
>> On 07/26/2013 02:56 AM, Felix Fietkau wrote:
>>> On 2013-07-26 10:53 AM, Johannes Berg wrote:
>>>> On Thu, 2013-07-11 at 08:29 -0700, Ben Greear wrote:
>>>>
>>>>>> I also don't like maintaining two separate hash tables and all that.
>>>>>>
>>>>>> I'd reconsider if you actually remove the hash entirely, but that'll be
>>>>>> tricky to walk the station list and will quite possibly make the RX path
>>>>>> there more expensive?
>>>>>
>>>>> Remove local->sta_hash ?
>>>>
>>>> To be honest, I'm undecided. Yes, I was thinking that, but I also think
>>>> having a huge hashtable like that for each virtual interface is way
>>>> overkill, in particular for station interfaces that usually have one
>>>> peer (the AP) and maybe a few TDLS peers. Or P2P-Device interfaces that
>>>> have no peers at all ...
>>>>
>>>> I don't see a good way to improve the hash either, since we don't always
>>>> (e.g. in RX path) have the interface address.
>>> How about mixing in the interface address into the hash. Theoretically
>>> you should always have that available, even in the rx path. Multicast
>>> data packets contain the BSSID, so you can get the address from there.
>>> You just need to be careful about checking the DS bits to figure out
>>> which address to use ;)
>>> I think this is a much better solution than duplicating the hash, or
>>> moving it into sdata entirely.
>>
>> I think I could probably get rid of the big global per wiphy hash and
>> use the per-wiphy sdata-hash and per-sdata station hashes.
>>
>> To me, that is cleanest because it gives a nice ownership relationship
>> between wiphy, sdata, and stations.
>>
>> For what it's worth, my hashing scheme has been working well on highly
>> loaded APs and Station machines.
> The global hash (with added vif-addr mixing) not only completely fixes
> the many-STA-vif case, also has some other advantages compared to the
> per-sdata hash:
> - Lookup is easier in setups with multiple AP VLANs
> - Better cache footprint (especially important for small embedded devices).
> - You don't need a separate sdata lookup before the sta lookup.
>
> I'm not convinced that keeping separate hashes is cleaner. Especially in
> the AP_VLAN case, ownership is not clear in any way, since there's some
> overlap between multiple sdata entities (belonging to the same BSS).
If someone wants to post such a patch, we can run it through our test
rigs, but I have little time or interest for re-doing the
hashing code again at this time. If your approach does fix the performance
issues we saw, then I'll be more than happy to drop my patch and use
your method.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
2013-07-26 16:09 ` Ben Greear
@ 2013-07-26 17:59 ` Felix Fietkau
0 siblings, 0 replies; 21+ messages in thread
From: Felix Fietkau @ 2013-07-26 17:59 UTC (permalink / raw)
To: Ben Greear; +Cc: Johannes Berg, linux-wireless
On 2013-07-26 6:09 PM, Ben Greear wrote:
> On 07/26/2013 08:38 AM, Felix Fietkau wrote:
>> The global hash (with added vif-addr mixing) not only completely fixes
>> the many-STA-vif case, also has some other advantages compared to the
>> per-sdata hash:
>> - Lookup is easier in setups with multiple AP VLANs
>> - Better cache footprint (especially important for small embedded devices).
>> - You don't need a separate sdata lookup before the sta lookup.
>>
>> I'm not convinced that keeping separate hashes is cleaner. Especially in
>> the AP_VLAN case, ownership is not clear in any way, since there's some
>> overlap between multiple sdata entities (belonging to the same BSS).
> If someone wants to post such a patch, we can run it through our test
> rigs, but I have little time or interest for re-doing the
> hashing code again at this time. If your approach does fix the performance
> issues we saw, then I'll be more than happy to drop my patch and use
> your method.
I don't have time to create such a patch myself at this point. I just
want to make sure that changes you post don't negatively affect small
embedded devices - and this is where the per-sdata hashing could be
problematic in my opinion.
- Felix
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
2013-07-26 8:53 ` Johannes Berg
2013-07-26 9:56 ` Felix Fietkau
@ 2013-07-26 15:27 ` Ben Greear
1 sibling, 0 replies; 21+ messages in thread
From: Ben Greear @ 2013-07-26 15:27 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
On 07/26/2013 01:53 AM, Johannes Berg wrote:
> On Thu, 2013-07-11 at 08:29 -0700, Ben Greear wrote:
>
>>> I also don't like maintaining two separate hash tables and all that.
>>>
>>> I'd reconsider if you actually remove the hash entirely, but that'll be
>>> tricky to walk the station list and will quite possibly make the RX path
>>> there more expensive?
>>
>> Remove local->sta_hash ?
>
> To be honest, I'm undecided. Yes, I was thinking that, but I also think
> having a huge hashtable like that for each virtual interface is way
> overkill, in particular for station interfaces that usually have one
> peer (the AP) and maybe a few TDLS peers. Or P2P-Device interfaces that
> have no peers at all ...
>
> I don't see a good way to improve the hash either, since we don't always
> (e.g. in RX path) have the interface address.
>
> The basic problem really is that the hash now is designed to work well
> for more regular use cases than yours, where you talk to any number of
> different stations but degrades really badly when you talk only to a
> single one many times. That use case is really special, and I don't want
> to 'fix' that in a way that would make the other use case significantly
> worse in memory consumption or CPU utilisation.
I could make the hash size configurable I suppose, or just make it always
be small for stations and larger for AP interfaces. That should
mitigate the memory usage issues. The sdata hash in the wiphy can
remain big, but there are rarely more than a few wiphy in a system, so
I think the cost is low for that.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [WT PATCH 5/6] mac80211: Add debugfs for sdata and sdata->sta_vhash
2013-06-29 22:58 [WT PATCH 1/6] mac80211: Add debugfs file to show station-hash counts greearb
` (2 preceding siblings ...)
2013-06-29 22:58 ` [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash greearb
@ 2013-06-29 22:58 ` greearb
2013-06-29 22:58 ` [WT PATCH 6/6] mac80211: Tell user why beacons fail to parse greearb
2013-07-11 8:51 ` [WT PATCH 1/6] mac80211: Add debugfs file to show station-hash counts Johannes Berg
5 siblings, 0 replies; 21+ messages in thread
From: greearb @ 2013-06-29 22:58 UTC (permalink / raw)
To: linux-wireless; +Cc: Ben Greear
From: Ben Greear <greearb@candelatech.com>
This gives the user some idea how well the hash functions
are working.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
net/mac80211/debugfs.c | 43 ++++++++++++++++++++++++++++++++
net/mac80211/debugfs_netdev.c | 54 +++++++++++++++++++++++++++++++++++++++-
2 files changed, 95 insertions(+), 2 deletions(-)
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 2bbe377..c18a6d1 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -210,9 +210,51 @@ static ssize_t sta_hash_read(struct file *file, char __user *user_buf,
return q;
}
+static ssize_t sdata_hash_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct ieee80211_local *local = file->private_data;
+ int mxln = 15000;
+ char *buf = kzalloc(mxln, GFP_KERNEL);
+ int q, res = 0;
+ struct ieee80211_sub_if_data *s;
+
+ if (!buf)
+ return 0;
+
+ mutex_lock(&local->iflist_mtx);
+ for (q = 0; q < STA_HASH_SIZE; q++) {
+ s = local->sdata_hash[q];
+ if (s) {
+ res += snprintf(buf + res, mxln - res, "%i: ", q);
+ if (res >= mxln)
+ goto done;
+ }
+ while (s) {
+ res += snprintf(buf + res, mxln - res, " %pM",
+ s->vif.addr);
+ if (res >= mxln)
+ goto done;
+ s = s->hnext;
+ }
+ if (local->sdata_hash[q]) {
+ res += snprintf(buf + res, mxln - res, "\n");
+ if (res >= mxln)
+ goto done;
+ }
+ }
+done:
+ mutex_unlock(&local->iflist_mtx);
+
+ q = simple_read_from_buffer(user_buf, count, ppos, buf, res);
+ kfree(buf);
+ return q;
+}
+
DEBUGFS_READONLY_FILE_OPS(hwflags);
DEBUGFS_READONLY_FILE_OPS(queues);
DEBUGFS_READONLY_FILE_OPS(sta_hash);
+DEBUGFS_READONLY_FILE_OPS(sdata_hash);
/* statistics stuff */
@@ -282,6 +324,7 @@ void debugfs_hw_add(struct ieee80211_local *local)
DEBUGFS_ADD(wep_iv);
DEBUGFS_ADD(queues);
DEBUGFS_ADD(sta_hash);
+ DEBUGFS_ADD(sdata_hash);
#ifdef CONFIG_PM
DEBUGFS_ADD_MODE(reset, 0200);
#endif
diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.c
index cafe614..321460d 100644
--- a/net/mac80211/debugfs_netdev.c
+++ b/net/mac80211/debugfs_netdev.c
@@ -30,17 +30,22 @@ static ssize_t ieee80211_if_read(
size_t count, loff_t *ppos,
ssize_t (*format)(const struct ieee80211_sub_if_data *, char *, int))
{
- char buf[70];
+ int mxln = STA_HASH_SIZE * 10;
+ char *buf = kzalloc(mxln, GFP_KERNEL);
ssize_t ret = -EINVAL;
+ if (!buf)
+ return 0;
+
read_lock(&dev_base_lock);
if (sdata->dev->reg_state == NETREG_REGISTERED)
- ret = (*format)(sdata, buf, sizeof(buf));
+ ret = (*format)(sdata, buf, mxln);
read_unlock(&dev_base_lock);
if (ret >= 0)
ret = simple_read_from_buffer(userbuf, count, ppos, buf, ret);
+ kfree(buf);
return ret;
}
@@ -201,6 +206,50 @@ ieee80211_if_fmt_hw_queues(const struct ieee80211_sub_if_data *sdata,
}
__IEEE80211_IF_FILE(hw_queues, NULL);
+static ssize_t
+ieee80211_if_fmt_sta_hash(const struct ieee80211_sub_if_data *sdata,
+ char *buf, int buflen)
+{
+ int q, res = 0;
+ struct sta_info *sta;
+
+ mutex_lock(&sdata->local->sta_mtx);
+ for (q = 0; q < STA_HASH_SIZE; q++) {
+ int cnt = 0;
+ sta = sdata->sta_vhash[q];
+ while (sta) {
+ cnt++;
+ sta = sta->vnext;
+ }
+ if (cnt) {
+ res += snprintf(buf + res, buflen - res, "%i: %i ",
+ q, cnt);
+ if (res >= buflen) {
+ res = buflen;
+ break;
+ }
+ sta = sdata->sta_vhash[q];
+ while (sta) {
+ res += snprintf(buf + res, buflen - res, " %pM",
+ sta->sta.addr);
+ if (res >= buflen) {
+ res = buflen;
+ break;
+ }
+ sta = sta->vnext;
+ }
+ res += snprintf(buf + res, buflen - res, "\n");
+ if (res >= buflen) {
+ res = buflen;
+ break;
+ }
+ }
+ }
+ mutex_unlock(&sdata->local->sta_mtx);
+ return res;
+}
+__IEEE80211_IF_FILE(sta_hash, NULL);
+
/* STA attributes */
IEEE80211_IF_FILE(bssid, u.mgd.bssid, MAC);
IEEE80211_IF_FILE(aid, u.mgd.aid, DEC);
@@ -545,6 +594,7 @@ static void add_common_files(struct ieee80211_sub_if_data *sdata)
DEBUGFS_ADD(rc_rateidx_mcs_mask_2ghz);
DEBUGFS_ADD(rc_rateidx_mcs_mask_5ghz);
DEBUGFS_ADD(hw_queues);
+ DEBUGFS_ADD(sta_hash);
}
static void add_sta_files(struct ieee80211_sub_if_data *sdata)
--
1.7.3.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* [WT PATCH 6/6] mac80211: Tell user why beacons fail to parse.
2013-06-29 22:58 [WT PATCH 1/6] mac80211: Add debugfs file to show station-hash counts greearb
` (3 preceding siblings ...)
2013-06-29 22:58 ` [WT PATCH 5/6] mac80211: Add debugfs for sdata and sdata->sta_vhash greearb
@ 2013-06-29 22:58 ` greearb
2013-07-11 8:59 ` Johannes Berg
2013-07-11 8:51 ` [WT PATCH 1/6] mac80211: Add debugfs file to show station-hash counts Johannes Berg
5 siblings, 1 reply; 21+ messages in thread
From: greearb @ 2013-06-29 22:58 UTC (permalink / raw)
To: linux-wireless; +Cc: Ben Greear
From: Ben Greear <greearb@candelatech.com>
Should help better debug dodgy APs and such.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
net/mac80211/ieee80211_i.h | 2 +
net/mac80211/mlme.c | 4 +-
net/mac80211/scan.c | 6 +++
net/mac80211/util.c | 79 +++++++++++++++++++++++++++++++++++++++----
4 files changed, 81 insertions(+), 10 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index f36f120..809c9a5 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -110,6 +110,7 @@ struct ieee80211_bss {
/* Keep track of what bits of information we have valid info for. */
u8 valid_data;
+ char corrupt_elems_msg[80];
};
/**
@@ -1258,6 +1259,7 @@ struct ieee802_11_elems {
/* whether a parse error occurred while retrieving these elements */
bool parse_error;
+ char parse_err_msg[80];
};
static inline struct ieee80211_local *hw_to_local(
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index ae31968..42edd6c 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -4341,8 +4341,8 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
corrupt_type = "beacon";
} else if (bss->corrupt_data & IEEE80211_BSS_CORRUPT_PROBE_RESP)
corrupt_type = "probe response";
- sdata_info(sdata, "associating with AP with corrupt %s\n",
- corrupt_type);
+ sdata_info(sdata, "associating with AP with corrupt %s, reason: %s\n",
+ corrupt_type, bss->corrupt_elems_msg);
}
return 0;
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 1b122a7..9dc9c98 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -86,6 +86,8 @@ ieee80211_bss_info_update(struct ieee80211_local *local,
bss->device_ts_presp = rx_status->device_timestamp;
if (elems->parse_error) {
+ strncpy(bss->corrupt_elems_msg, elems->parse_err_msg,
+ sizeof(bss->corrupt_elems_msg));
if (beacon)
bss->corrupt_data |= IEEE80211_BSS_CORRUPT_BEACON;
else
@@ -95,6 +97,10 @@ ieee80211_bss_info_update(struct ieee80211_local *local,
bss->corrupt_data &= ~IEEE80211_BSS_CORRUPT_BEACON;
else
bss->corrupt_data &= ~IEEE80211_BSS_CORRUPT_PROBE_RESP;
+ if (!(bss->corrupt_data &
+ (IEEE80211_BSS_CORRUPT_BEACON |
+ IEEE80211_BSS_CORRUPT_PROBE_RESP)))
+ bss->corrupt_elems_msg[0] = 0;
}
/* save the ERP value so that it is available at association time */
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 2265445..7e6a4f3 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -692,6 +692,10 @@ u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
if (elen > left) {
elems->parse_error = true;
+ snprintf(elems->parse_err_msg,
+ sizeof(elems->parse_err_msg),
+ "elen: %hhu > left: %zu",
+ elen, left);
break;
}
@@ -731,6 +735,9 @@ u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
*/
if (test_bit(id, seen_elems)) {
elems->parse_error = true;
+ snprintf(elems->parse_err_msg,
+ sizeof(elems->parse_err_msg),
+ "seen id: %i already", id);
left -= elen;
pos += elen;
continue;
@@ -762,8 +769,14 @@ u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
if (elen >= sizeof(struct ieee80211_tim_ie)) {
elems->tim = (void *)pos;
elems->tim_len = elen;
- } else
+ } else {
elem_parse_failed = true;
+ snprintf(elems->parse_err_msg,
+ sizeof(elems->parse_err_msg),
+ "EID_TIM size wrong, elen: %hhu sizeof(tim_ie): %zu",
+ elen,
+ sizeof(struct ieee80211_tim_ie));
+ }
break;
case WLAN_EID_CHALLENGE:
elems->challenge = pos;
@@ -806,32 +819,61 @@ u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
case WLAN_EID_HT_CAPABILITY:
if (elen >= sizeof(struct ieee80211_ht_cap))
elems->ht_cap_elem = (void *)pos;
- else
+ else {
elem_parse_failed = true;
+ snprintf(elems->parse_err_msg,
+ sizeof(elems->parse_err_msg),
+ "HT_CAPAB size wrong, elen: %hhu sizeof(ht_cap): %zu",
+ elen,
+ sizeof(struct ieee80211_ht_cap));
+ }
break;
case WLAN_EID_HT_OPERATION:
if (elen >= sizeof(struct ieee80211_ht_operation))
elems->ht_operation = (void *)pos;
- else
+ else {
elem_parse_failed = true;
+ snprintf(elems->parse_err_msg,
+ sizeof(elems->parse_err_msg),
+ "HT_OPER size wrong, elen: %hhu sizeof(ht_oper): %zu",
+ elen,
+ sizeof(struct ieee80211_ht_operation));
+ }
break;
case WLAN_EID_VHT_CAPABILITY:
if (elen >= sizeof(struct ieee80211_vht_cap))
elems->vht_cap_elem = (void *)pos;
- else
+ else {
elem_parse_failed = true;
+ snprintf(elems->parse_err_msg,
+ sizeof(elems->parse_err_msg),
+ "EID_VHT size wrong, elen: %hhu sizeof(vht_cap): %zu",
+ elen,
+ sizeof(struct ieee80211_vht_cap));
+ }
break;
case WLAN_EID_VHT_OPERATION:
if (elen >= sizeof(struct ieee80211_vht_operation))
elems->vht_operation = (void *)pos;
- else
+ else {
elem_parse_failed = true;
+ snprintf(elems->parse_err_msg,
+ sizeof(elems->parse_err_msg),
+ "VHT_OPER size wrong, elen: %hhu sizeof(vht_oper): %zu",
+ elen,
+ sizeof(struct ieee80211_vht_operation));
+ }
break;
case WLAN_EID_OPMODE_NOTIF:
if (elen > 0)
elems->opmode_notif = pos;
- else
+ else {
elem_parse_failed = true;
+ snprintf(elems->parse_err_msg,
+ sizeof(elems->parse_err_msg),
+ "OPMODE_NOTIF has elen > 0: %hhu",
+ elen);
+ }
break;
case WLAN_EID_MESH_ID:
elems->mesh_id = pos;
@@ -840,8 +882,14 @@ u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
case WLAN_EID_MESH_CONFIG:
if (elen >= sizeof(struct ieee80211_meshconf_ie))
elems->mesh_config = (void *)pos;
- else
+ else {
elem_parse_failed = true;
+ snprintf(elems->parse_err_msg,
+ sizeof(elems->parse_err_msg),
+ "MESH_CONFIG size wrong, elen: %hhu sizeof(meshconf_ie): %zu",
+ elen,
+ sizeof(struct ieee80211_meshconf_ie));
+ }
break;
case WLAN_EID_PEER_MGMT:
elems->peering = pos;
@@ -866,12 +914,23 @@ u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
case WLAN_EID_RANN:
if (elen >= sizeof(struct ieee80211_rann_ie))
elems->rann = (void *)pos;
- else
+ else {
elem_parse_failed = true;
+ snprintf(elems->parse_err_msg,
+ sizeof(elems->parse_err_msg),
+ "EID_RANN size wrong, elen: %hhu sizeof(rann_ie): %zu",
+ elen,
+ sizeof(struct ieee80211_rann_ie));
+ }
break;
case WLAN_EID_CHANNEL_SWITCH:
if (elen != sizeof(struct ieee80211_channel_sw_ie)) {
elem_parse_failed = true;
+ snprintf(elems->parse_err_msg,
+ sizeof(elems->parse_err_msg),
+ "CH_SWITCH size wrong, elen: %hhu sizeof(sw_ie): %zu",
+ elen,
+ sizeof(struct ieee80211_channel_sw_ie));
break;
}
elems->ch_switch_ie = (void *)pos;
@@ -925,6 +984,10 @@ u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action,
case WLAN_EID_PWR_CONSTRAINT:
if (elen != 1) {
elem_parse_failed = true;
+ snprintf(elems->parse_err_msg,
+ sizeof(elems->parse_err_msg),
+ "PWR_CONSTRAINT size not 1, elen: %hhu",
+ elen);
break;
}
elems->pwr_constr_elem = pos;
--
1.7.3.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [WT PATCH 6/6] mac80211: Tell user why beacons fail to parse.
2013-06-29 22:58 ` [WT PATCH 6/6] mac80211: Tell user why beacons fail to parse greearb
@ 2013-07-11 8:59 ` Johannes Berg
2013-07-11 15:10 ` Ben Greear
0 siblings, 1 reply; 21+ messages in thread
From: Johannes Berg @ 2013-07-11 8:59 UTC (permalink / raw)
To: greearb; +Cc: linux-wireless
On Sat, 2013-06-29 at 15:58 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> Should help better debug dodgy APs and such.
This isn't a bad idea, but I think instead of storing the message:
> @@ -110,6 +110,7 @@ struct ieee80211_bss {
>
> /* Keep track of what bits of information we have valid info for. */
> u8 valid_data;
> + char corrupt_elems_msg[80];
you should store a "what's bad" type field and the broken IE number or
so, to reduce memory usage
> @@ -4341,8 +4341,8 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
> corrupt_type = "beacon";
> } else if (bss->corrupt_data & IEEE80211_BSS_CORRUPT_PROBE_RESP)
> corrupt_type = "probe response";
> - sdata_info(sdata, "associating with AP with corrupt %s\n",
> - corrupt_type);
> + sdata_info(sdata, "associating with AP with corrupt %s, reason: %s\n",
> + corrupt_type, bss->corrupt_elems_msg);
and only actually format the message here. That also reduces overhead
during beacon/probe response processing.
> + snprintf(elems->parse_err_msg,
> + sizeof(elems->parse_err_msg),
> + "seen id: %i already", id);
Your snprintf() usage is also unsafe.
johannes
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [WT PATCH 6/6] mac80211: Tell user why beacons fail to parse.
2013-07-11 8:59 ` Johannes Berg
@ 2013-07-11 15:10 ` Ben Greear
2013-07-11 15:17 ` Johannes Berg
0 siblings, 1 reply; 21+ messages in thread
From: Ben Greear @ 2013-07-11 15:10 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
On 07/11/2013 01:59 AM, Johannes Berg wrote:
> On Sat, 2013-06-29 at 15:58 -0700, greearb@candelatech.com wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Should help better debug dodgy APs and such.
>
> This isn't a bad idea, but I think instead of storing the message:
>
>> @@ -110,6 +110,7 @@ struct ieee80211_bss {
>>
>> /* Keep track of what bits of information we have valid info for. */
>> u8 valid_data;
>> + char corrupt_elems_msg[80];
>
> you should store a "what's bad" type field and the broken IE number or
> so, to reduce memory usage
I thought of this, but the problem is then you cannot tell the details
(for instance the actual lengths when length is bad, the ID that is duplicated, etc).
I figure if we are going to provide the info to the user, we might as well
be specific about it.
>> + snprintf(elems->parse_err_msg,
>> + sizeof(elems->parse_err_msg),
>> + "seen id: %i already", id);
>
> Your snprintf() usage is also unsafe.
What is unsafe about it?
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [WT PATCH 6/6] mac80211: Tell user why beacons fail to parse.
2013-07-11 15:10 ` Ben Greear
@ 2013-07-11 15:17 ` Johannes Berg
0 siblings, 0 replies; 21+ messages in thread
From: Johannes Berg @ 2013-07-11 15:17 UTC (permalink / raw)
To: Ben Greear; +Cc: linux-wireless
On Thu, 2013-07-11 at 08:10 -0700, Ben Greear wrote:
> >> /* Keep track of what bits of information we have valid info for. */
> >> u8 valid_data;
> >> + char corrupt_elems_msg[80];
> >
> > you should store a "what's bad" type field and the broken IE number or
> > so, to reduce memory usage
>
> I thought of this, but the problem is then you cannot tell the details
> (for instance the actual lengths when length is bad, the ID that is duplicated, etc).
> I figure if we are going to provide the info to the user, we might as well
> be specific about it.
It doesn't seem so difficult?
enum ies_parse_error {
NO_ERROR,
BAD_IE_LEN, // uses eid+len
DUPLICATE_IE, // uses eid
...
} parse_error;
int parse_error_eid, parse_error_len;
> >> + snprintf(elems->parse_err_msg,
> >> + sizeof(elems->parse_err_msg),
> >> + "seen id: %i already", id);
> >
> > Your snprintf() usage is also unsafe.
>
> What is unsafe about it?
using printk("%s", result) is unsafe due to potentially missing
NUL-termination.
johannes
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [WT PATCH 1/6] mac80211: Add debugfs file to show station-hash counts.
2013-06-29 22:58 [WT PATCH 1/6] mac80211: Add debugfs file to show station-hash counts greearb
` (4 preceding siblings ...)
2013-06-29 22:58 ` [WT PATCH 6/6] mac80211: Tell user why beacons fail to parse greearb
@ 2013-07-11 8:51 ` Johannes Berg
5 siblings, 0 replies; 21+ messages in thread
From: Johannes Berg @ 2013-07-11 8:51 UTC (permalink / raw)
To: greearb; +Cc: linux-wireless
On Sat, 2013-06-29 at 15:58 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> Helps debug bad hash spreads, like when you have lots of
> station interfaces all connected to the same AP.
I'm willing to take this patch, but
> + res += sprintf(buf + res, "%i: %i\n", q, cnt);
> + if (res >= (STA_HASH_SIZE * 10)) {
> + res = STA_HASH_SIZE * 10;
> + break;
tihs needs to use "maxln" (what does that variable mean anyway? it's
more like "bufsize"?) and snprintf().
johannes
^ permalink raw reply [flat|nested] 21+ messages in thread