RFC: Hashing by VIF addr for rx of data packets.

PATCH Ben Greear 2 participants 4 messages Apr 2, 2013 – Apr 3, 2013 mac80211

Share & tools

Ben Greear

author

2013-04-02 23:45 UTC #0 lore raw

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

net/mac80211/ieee80211_i.h +2 -1

index b985722..84f1b73 100644
@@ -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;

net/mac80211/rx.c +15 -0

index c6844ad..1fbf5b4 100644
@@ -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;

net/mac80211/sta_info.c +45 -3

76 lines not on this page · raw message

net/mac80211/sta_info.h +6 -1

index 4947341..d2a53ae 100644
@@ -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,

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Johannes Berg

maintainer

2013-04-03 12:46 UTC #1 lore raw

Quoted 13 lines · Ben Greear

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?)

Quoted 5 lines

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.

net/mac80211/rx.c static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw, · line 3161

Quoted 7 lines

net/mac80211/rx.c +7 -0

+		/* 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

Ben Greear

author

2013-04-03 13:37 UTC #2 lore raw

Quoted 18 lines · Johannes Berg

Quoted 17 lines · Ben Greear
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.

Quoted 5 lines

Quoted 1 line
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...

Quoted 11 lines

net/mac80211/rx.c static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw, · line 3161
Quoted 7 lines
net/mac80211/rx.c +7 -0
+		/* 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

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Johannes Berg

maintainer

2013-04-03 13:42 UTC #3 lore raw

Quoted 7 lines · Ben Greear

Quoted 3 lines
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.

Quoted 10 lines

Quoted 6 lines
net/mac80211/rx.c static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw, · line 3161
Quoted 2 lines
net/mac80211/rx.c +2 -0
+		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