I notice that the rx logic currently walks through all stations when a NIC receives
a packet.
With the attached patch, total TCP download throughput goes from 70Mbps to 190Mbps on
my test system (Atom 1.6Ghz) with 50 station VIFs receiving TCP data streams.
The basic idea is to hash on the VIF addr (ie, what you see in 'ifconfig wlan0' as MAC
address), and then look up stations using the hdr->addr1 in the rx logic.
The attached patch probably breaks monitor interfaces and other VIFs other than AP and
STA. It also changes the behaviour of PROMISC, but I'm not sure that is bad (is
the old behaviour needed for anything useful?)
I'm thinking to store a count of all VIF types on a radio, and make
this hash code only be enabled when only STA and AP exist. Maybe later optimize
so we can quickly find monitor or other VIF types to handle them properly.
Comments and suggestions are welcome.
Thanks,
Ben
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index b985722..84f1b73 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -985,7 +985,8 @@ struct ieee80211_local {
spinlock_t tim_lock;
unsigned long num_sta;
struct list_head sta_list;
- struct sta_info __rcu *sta_hash[STA_HASH_SIZE];
+ struct sta_info __rcu *sta_hash[STA_HASH_SIZE]; /* By station addr */
+ struct sta_info __rcu *sta_vhash[STA_HASH_SIZE]; /* By VIF mac addr */
struct timer_list sta_cleanup;
int sta_generation;
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index c6844ad..1fbf5b4 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -3152,6 +3152,20 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
if (ieee80211_is_data(fc)) {
prev_sta = NULL;
+ /* If we have only station and AP interfaces, then hash on
+ * the destination MAC (ie, local sdata MAC). Could add other
+ * device types as well, perhaps. This changes 'promisc' behaviour,
+ * but not sure that is a bad thing.
+ */
+ if (!is_multicast_ether_addr(hdr->addr1)) {
+ sta = sta_info_get_by_vif(local, hdr->addr1);
+ 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;
@@ -3169,6 +3183,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 c465b1d..ff7ac08 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -77,19 +77,42 @@ static int sta_info_hash_del(struct ieee80211_local *local,
s = rcu_dereference_protected(local->sta_hash[STA_HASH(sta->sta.addr)],
lockdep_is_held(&local->sta_mtx));
if (!s)
- return -ENOENT;
+ goto try_lhash;
+
if (s == sta) {
rcu_assign_pointer(local->sta_hash[STA_HASH(sta->sta.addr)],
s->hnext);
- return 0;
+ goto try_lhash;
}
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);
+ goto try_lhash;
+ }
+
+ /* Remove from the local VIF addr hash */
+try_lhash:
+ s = rcu_dereference_protected(local->sta_vhash[STA_HASH(sta->sdata->vif.addr)],
+ lockdep_is_held(&local->sta_mtx));
+ if (!s)
+ return -ENONET;
+
+ if (s == sta) {
+ rcu_assign_pointer(local->sta_vhash[STA_HASH(sta->sdata->vif.addr)],
+ s->vnext);
+ return 0;
+ }
+
+ 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 0;
}
@@ -251,6 +274,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 *addr) {
+ struct sta_info *sta;
+
+ sta = rcu_dereference_check(local->sta_vhash[STA_HASH(addr)],
+ lockdep_is_held(&local->sta_mtx));
+ while (sta) {
+ if (ether_addr_equal(sta->sdata->vif.addr, addr))
+ break;
+ sta = rcu_dereference_check(sta->vnext,
+ lockdep_is_held(&local->sta_mtx));
+ }
+ return sta;
+}
+
+
struct sta_info *sta_info_get_by_idx(struct ieee80211_sub_if_data *sdata,
int idx)
{
@@ -300,6 +339,9 @@ static void sta_info_hash_add(struct ieee80211_local *local,
sta->hnext = local->sta_hash[STA_HASH(sta->sta.addr)];
rcu_assign_pointer(local->sta_hash[STA_HASH(sta->sta.addr)], sta);
+ sta->vnext = local->sta_vhash[STA_HASH(sta->sdata->vif.addr)];
+ rcu_assign_pointer(local->sta_vhash[STA_HASH(sta->sdata->vif.addr)], sta);
+
rcu_assign_pointer(sta->sdata->some_sta, sta);
}
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 4947341..d2a53ae 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -227,7 +227,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, by (remote) station MAC
+ * @vnext: hash table linked list pointer, by VIF MAC
* @local: pointer to the global information
* @sdata: virtual interface this station belongs to
* @ptk: peer key negotiated with this station, if any
@@ -304,6 +305,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];
@@ -507,6 +509,9 @@ 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);
+struct sta_info *sta_info_get_by_vif(struct ieee80211_local *local,
+ const u8 *addr);
+
static inline
void for_each_sta_info_type_check(struct ieee80211_local *local,
const u8 *addr,
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On Tue, 2013-04-02 at 16:45 -0700, Ben Greear wrote:
> I notice that the rx logic currently walks through all stations when a NIC receives
> a packet.
>
> With the attached patch, total TCP download throughput goes from 70Mbps to 190Mbps on
> my test system (Atom 1.6Ghz) with 50 station VIFs receiving TCP data streams.
>
> The basic idea is to hash on the VIF addr (ie, what you see in 'ifconfig wlan0' as MAC
> address), and then look up stations using the hdr->addr1 in the rx logic.
>
> The attached patch probably breaks monitor interfaces and other VIFs other than AP and
> STA. It also changes the behaviour of PROMISC, but I'm not sure that is bad (is
> the old behaviour needed for anything useful?)
>
> I'm thinking to store a count of all VIF types on a radio, and make
> this hash code only be enabled when only STA and AP exist. Maybe later optimize
> so we can quickly find monitor or other VIF types to handle them properly.
>
> Comments and suggestions are welcome.
Hmmm. I'm not really convinced this will make sense upstream. I'm kinda
fine with the single-station cache, but maintaining a whole other hash
table seems too much overhead for every use case but yours.
> + /* If we have only station and AP interfaces, then hash on
> + * the destination MAC (ie, local sdata MAC). Could add other
> + * device types as well, perhaps. This changes 'promisc' behaviour,
> + * but not sure that is a bad thing.
> + */
> + if (!is_multicast_ether_addr(hdr->addr1)) {
> + sta = sta_info_get_by_vif(local, hdr->addr1);
AFAICT, this is also wrong for TDLS and other cases where we might
receive a frame that's not from the AP, even if it's only by accident or
from an attacker.
johannes
On Wed, 2013-04-03 at 06:37 -0700, Ben Greear wrote:
> > Hmmm. I'm not really convinced this will make sense upstream. I'm kinda
> > fine with the single-station cache, but maintaining a whole other hash
> > table seems too much overhead for every use case but yours.
>
> Yeah, aside from multiple stations, I'm not sure it helps anything. It would
> require a different scheme to help with multiple VAP I think, and I'm not
> sure there are any other multi-vif use cases out there...
Yes, likely.
> >> + if (!is_multicast_ether_addr(hdr->addr1)) {
> >> + sta = sta_info_get_by_vif(local, hdr->addr1);
> >
> > AFAICT, this is also wrong for TDLS and other cases where we might
> > receive a frame that's not from the AP, even if it's only by accident or
> > from an attacker.
>
> I think patch is probably wrong for any VIF that can be associated with more than
> one station (such as APs). I'm going to re-work the vhash to only include
> Station VIFS and see how that works for my test case.
Well it's wrong even as is, even with just a single station on that VIF,
because as far as I can tell it assumes that *any* frame, no matter who
it came from, was from that station.
johannes
On 04/03/2013 05:46 AM, Johannes Berg wrote:
> On Tue, 2013-04-02 at 16:45 -0700, Ben Greear wrote:
>> I notice that the rx logic currently walks through all stations when a NIC receives
>> a packet.
>>
>> With the attached patch, total TCP download throughput goes from 70Mbps to 190Mbps on
>> my test system (Atom 1.6Ghz) with 50 station VIFs receiving TCP data streams.
>>
>> The basic idea is to hash on the VIF addr (ie, what you see in 'ifconfig wlan0' as MAC
>> address), and then look up stations using the hdr->addr1 in the rx logic.
>>
>> The attached patch probably breaks monitor interfaces and other VIFs other than AP and
>> STA. It also changes the behaviour of PROMISC, but I'm not sure that is bad (is
>> the old behaviour needed for anything useful?)
>>
>> I'm thinking to store a count of all VIF types on a radio, and make
>> this hash code only be enabled when only STA and AP exist. Maybe later optimize
>> so we can quickly find monitor or other VIF types to handle them properly.
>>
>> Comments and suggestions are welcome.
>
> Hmmm. I'm not really convinced this will make sense upstream. I'm kinda
> fine with the single-station cache, but maintaining a whole other hash
> table seems too much overhead for every use case but yours.
Yeah, aside from multiple stations, I'm not sure it helps anything. It would
require a different scheme to help with multiple VAP I think, and I'm not
sure there are any other multi-vif use cases out there...
>> + /* If we have only station and AP interfaces, then hash on
>> + * the destination MAC (ie, local sdata MAC). Could add other
>> + * device types as well, perhaps. This changes 'promisc' behaviour,
>> + * but not sure that is a bad thing.
>> + */
>> + if (!is_multicast_ether_addr(hdr->addr1)) {
>> + sta = sta_info_get_by_vif(local, hdr->addr1);
>
> AFAICT, this is also wrong for TDLS and other cases where we might
> receive a frame that's not from the AP, even if it's only by accident or
> from an attacker.
I think patch is probably wrong for any VIF that can be associated with more than
one station (such as APs). I'm going to re-work the vhash to only include
Station VIFS and see how that works for my test case.
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com