mac80211: Add debugfs file to show station-hash counts.
Subsystems (2)
PATCH 1/6 mac80211: Add debugfs file to show station-hash counts.
From: Ben Greear <[email protected]>
Helps debug bad hash spreads, like when you have lots of station interfaces all connected to the same AP.
--- net/mac80211/debugfs.c | 37 +++++++++++++++++++++++++++++++++++++ 1 files changed, 37 insertions(+), 0 deletions(-)
net/mac80211/debugfs.c +37 -0
@@ -175,8 +175,44 @@ static ssize_t queues_read(struct file *file, char __user *user_buf,return simple_read_from_buffer (user_buf, count, ppos, buf, res); } +static ssize_t sta_hash_read (struct file *file,char __user *user_buf, +size_t count,loff_t *ppos) +{ +struct ieee80211_local *local = file->private_data; +int mxln =STA_HASH_SIZE *10 ; +char *buf =kzalloc (mxln,GFP_KERNEL ); +int q, res =0 ; +struct sta_info *sta; + +if (!buf) +return 0 ; + +mutex_lock (&local->sta_mtx); +for (q =0 ; q <STA_HASH_SIZE ; q++) { +int cnt =0 ; + sta = local->sta_hash[q]; +while (sta) { + cnt++; + sta = sta->hnext; + } +if (cnt) { + res +=sprintf (buf + res,"%i: %i\n" , q, cnt); +if (res >= (STA_HASH_SIZE *10 )) { + res =STA_HASH_SIZE *10 ; +break ; + } + } + } +mutex_unlock (&local->sta_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);/* statistics stuff */
@@ -245,6 +281,7 @@ void debugfs_hw_add(struct ieee80211_local *local)DEBUGFS_ADD (total_ps_buffered);DEBUGFS_ADD (wep_iv);DEBUGFS_ADD (queues); +DEBUGFS_ADD (sta_hash);#ifdef CONFIG_PM DEBUGFS_ADD_MODE (reset,0200 );#endif
Signature · 1 line
1.7.3.4
PATCH 2/6 mac80211: Make un-found-rate splat a warn-once.
From: Ben Greear <[email protected]>
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.
--- net/mac80211/tx.c | 21 ++++++++++++++------- 1 files changed, 14 insertions(+), 7 deletions(-)
net/mac80211/tx.c +14 -7
@@ -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
Signature · 1 line
1.7.3.4
Johannes Berg
maintainerRe: [WT PATCH 2/6] mac80211: Make un-found-rate splat a warn-once.
Quoted 5 lines · On Sat, 2013-06-29 at 15:58 -0700, [email protected] wrote:
From: Ben Greear <[email protected]>
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.
I've hardly seen this in practice, let's just make it ratelimited always.
I think maybe sdata_info_ratelimited() or so would be worthwhile.
Might also stick in an unlikely().
johannes
PATCH 3/6 wireless: Add memory usage debugging.
In reply to [email protected]From: Ben Greear <[email protected]>
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.
--- 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(-)
net/wireless/core.c +7 -2
@@ -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 ();
net/wireless/core.h +16 -0
@@ -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;
net/wireless/debugfs.c +113 -0
129 lines not on this page · raw message
net/wireless/debugfs.h +2 -0
@@ -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 */
net/wireless/scan.c +124 -15
200 lines not on this page · raw message
Signature · 1 line
1.7.3.4
Johannes Berg
maintainerRe: [WT PATCH 3/6] wireless: Add memory usage debugging.
Quoted 6 lines · On Sat, 2013-06-29 at 15:58 -0700, [email protected] wrote:
From: Ben Greear <[email protected]>
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.
I still think this is too intrusive, not in terms of runtime but code maintenance penalty, so I'm not taking this.
johannes
PATCH 4/6 mac80211: Add per-sdata station hash, and sdata hash.
In reply to [email protected]From: Ben Greear <[email protected]>
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.
--- 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(-)
net/mac80211/ieee80211_i.h +34 -0
@@ -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;
net/mac80211/iface.c +49 -1
75 lines not on this page · raw message
net/mac80211/rx.c +16 -0
@@ -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;
net/mac80211/sta_info.c +77 -20
156 lines not on this page · raw message
net/mac80211/sta_info.h +16 -2
@@ -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,
net/mac80211/status.c +6 -0
@@ -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 ();
Signature · 1 line
1.7.3.4
Johannes Berg
maintainerRe: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
Quoted 9 lines · On Sat, 2013-06-29 at 15:58 -0700, [email protected] wrote:
From: Ben Greear <[email protected]>
Add sdata hash (based on
sdata->vif.addr) to local structure.Add
sta_vhash(based onsta->sta.addr) to sdata struct.Make
STA_HASHgive a better hash spread more often.
Quoted 8 lines
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
Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
Quoted 19 lines · Johannes Berg
Quoted 9 lines · On Sat, 2013-06-29 at 15:58 -0700, [email protected] wrote:
From: Ben Greear <[email protected]>
Add sdata hash (based on
sdata->vif.addr) to local structure.Add
sta_vhash(based onsta->sta.addr) to sdata struct.Make
STA_HASHgive a better hash spread more often.Quoted 8 lines
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.
Quoted 8 lines
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
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Johannes Berg
maintainerRe: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
Quoted 7 lines · Ben Greear
Quoted 5 lines
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
Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
Quoted 10 lines · Johannes Berg
Quoted 7 lines · Ben Greear
Quoted 5 lines
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?
Quoted 8 lines
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
Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
Quoted 19 lines · Felix Fietkau
Quoted 10 lines · Johannes Berg
Quoted 7 lines · Ben Greear
Quoted 5 lines
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?Quoted 8 lines
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.
Quoted 7 lines
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
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
Quoted 28 lines · Ben Greear
Quoted 19 lines · Felix Fietkau
Quoted 10 lines · Johannes Berg
Quoted 7 lines · Ben Greear
Quoted 5 lines
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?Quoted 8 lines
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.
Quoted 7 lines
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.
Quoted 8 lines
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
Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
Quoted 44 lines · Felix Fietkau
Quoted 28 lines · Ben Greear
Quoted 19 lines · Felix Fietkau
Quoted 10 lines · Johannes Berg
Quoted 7 lines · Ben Greear
Quoted 5 lines
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?Quoted 8 lines
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.
Quoted 7 lines
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.Quoted 8 lines
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.
Quoted 3 lines
I'm not convinced that keeping separate hashes is cleaner. Especially in the
AP_VLANcase, 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
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
Quoted 8 lines · Ben Greear
Quoted 7 lines · Felix Fietkau
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.
Quoted 8 lines
Quoted 3 lines
I'm not convinced that keeping separate hashes is cleaner. Especially in the
AP_VLANcase, 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
Re: [WT PATCH 4/6] mac80211: Add per-sdata station hash, and sdata hash.
In reply to Johannes BergQuoted 19 lines · Johannes Berg
Quoted 7 lines · Ben Greear
Quoted 5 lines
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.
Quoted 6 lines
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
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
PATCH 5/6 mac80211: Add debugfs for sdata and sdata->sta_vhash
In reply to [email protected]From: Ben Greear <[email protected]>
This gives the user some idea how well the hash functions are working.
--- net/mac80211/debugfs.c | 43 ++++++++++++++++++++++++++++++++ net/mac80211/debugfs_netdev.c | 54 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 95 insertions(+), 2 deletions(-)
net/mac80211/debugfs.c +43 -0
@@ -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
net/mac80211/debugfs_netdev.c +52 -2
81 lines not on this page · raw message
Signature · 1 line
1.7.3.4
PATCH 6/6 mac80211: Tell user why beacons fail to parse.
In reply to [email protected]From: Ben Greear <[email protected]>
Should help better debug dodgy APs and such.
--- 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(-)
net/mac80211/ieee80211_i.h +2 -0
@@ -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(
net/mac80211/mlme.c +2 -2
@@ -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 ;
net/mac80211/scan.c +6 -0
@@ -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 */
net/mac80211/util.c +71 -8
149 lines not on this page · raw message
Signature · 1 line
1.7.3.4
Johannes Berg
maintainerRe: [WT PATCH 6/6] mac80211: Tell user why beacons fail to parse.
Quoted 3 lines · On Sat, 2013-06-29 at 15:58 -0700, [email protected] wrote:
From: Ben Greear <[email protected]>
Should help better debug dodgy APs and such.
This isn't a bad idea, but I think instead of storing the message:
net/mac80211/ieee80211_i.h struct ieee80211_bss { · line 113
Quoted 5 lines
net/mac80211/ieee80211_i.h +1
-0@@ -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
net/mac80211/mlme.c int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata, · line 4345
Quoted 8 lines
net/mac80211/mlme.c +2
-2@@ -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.
net/mac80211/util.c u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action, · line 740
Quoted 3 lines
net/mac80211/util.c +3
-0+snprintf (elems->parse_err_msg, +sizeof (elems->parse_err_msg), +"seen id: %i already" , id);
Your snprintf() usage is also unsafe.
johannes
Re: [WT PATCH 6/6] mac80211: Tell user why beacons fail to parse.
Quoted 7 lines · Johannes Berg
Quoted 3 lines · On Sat, 2013-06-29 at 15:58 -0700, [email protected] wrote:
From: Ben Greear <[email protected]>
Should help better debug dodgy APs and such.
This isn't a bad idea, but I think instead of storing the message:
Quoted 8 lines
net/mac80211/ieee80211_i.h struct ieee80211_bss { · line 113
Quoted 5 lines
net/mac80211/ieee80211_i.h +1
-0@@ -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.
Quoted 5 lines
net/mac80211/util.c u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action, · line 740
Quoted 3 lines
net/mac80211/util.c +3
-0+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
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Johannes Berg
maintainerRe: [WT PATCH 6/6] mac80211: Tell user why beacons fail to parse.
Quoted 11 lines · Ben Greear
Quoted 6 lines
net/mac80211/ieee80211_i.h struct ieee80211_bss { · line 113
Quoted 3 lines
/* Keep track of what bits of information we have valid info for. */ u8
valid_data;net/mac80211/ieee80211_i.h +1
-0+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;
Quoted 7 lines
Quoted 5 lines
net/mac80211/util.c u32 ieee802_11_parse_elems_crc(const u8 *start, size_t len, bool action, · line 740
Quoted 3 lines
net/mac80211/util.c +3
-0+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
Johannes Berg
maintainerQuoted 4 lines · On Sat, 2013-06-29 at 15:58 -0700, [email protected] wrote:
From: Ben Greear <[email protected]>
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
net/mac80211/debugfs.c static ssize_t queues_read(struct file *file, char __user *user_buf, · line 202
Quoted 4 lines
net/mac80211/debugfs.c +4
-0+ 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