Skip to contentSkip to conversation

improve code in TX/RX paths

PATCH In review Johannes Berg 2 participants 6 messages Jun 19, 2008 mac80211

lore
Share & tools
lore

Johannes Berg

author maintainer
#0 lore raw

cover improve code in TX/RX paths

As mentioned previously, this improves code size a lot if all the functions get inlined by the compiler. It actually makes the code slightly larger in the debug case because now there is code rather than .data with all the function calls.

The patch series will require the fragmentation fix Tomas sent to be applied.

johannes

author maintainer
#1 lore raw

PATCH 1/3 mac80211: add single function calling tx handlers

This modifies mac80211 to only have a single function calling the TX handlers rather than them being invoked in multiple places.

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/tx.c |   82 ++++++++++++++++++++++++++----------------------------
 1 file changed, 40 insertions(+), 42 deletions(-)
net/mac80211/tx.c +40 -42
@@ -1106,13 +1106,46 @@ static int __ieee80211_tx(struct ieee802
 	return IEEE80211_TX_OK;
 }
 
+/*
+ * Invoke TX handlers, return 0 on success and non-zero if the
+ * frame was dropped or queued.
+ */
+static int invoke_tx_handlers(struct ieee80211_tx_data *tx)
+{
+	struct ieee80211_local *local = tx->local;
+	struct sk_buff *skb = tx->skb;
+	ieee80211_tx_handler *handler;
+	ieee80211_tx_result res = TX_DROP;
+	int i;
+
+	for (handler = ieee80211_tx_handlers; *handler != NULL; handler++) {
+		res = (*handler)(tx);
+		if (res != TX_CONTINUE)
+			break;
+	}
+
+	if (unlikely(res == TX_DROP)) {
+		I802_DEBUG_INC(local->tx_handlers_drop);
+		dev_kfree_skb(skb);
+		for (i = 0; i < tx->num_extra_frag; i++)
+			if (tx->extra_frag[i])
+				dev_kfree_skb(tx->extra_frag[i]);
+		kfree(tx->extra_frag);
+		return -1;
+	} else if (unlikely(res == TX_QUEUED)) {
+		I802_DEBUG_INC(local->tx_handlers_queued);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int ieee80211_tx(struct net_device *dev, struct sk_buff *skb)
 {
 	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
 	struct sta_info *sta;
-	ieee80211_tx_handler *handler;
 	struct ieee80211_tx_data tx;
-	ieee80211_tx_result res = TX_DROP, res_prepare;
+	ieee80211_tx_result res_prepare;
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 	int ret, i, retries = 0;
 	u16 queue;

@@ -1141,26 +1174,8 @@ static int ieee80211_tx(struct net_devic
 	tx.channel = local->hw.conf.channel;
 	info->band = tx.channel->band;
 
-	for (handler = ieee80211_tx_handlers; *handler != NULL;
-	     handler++) {
-		res = (*handler)(&tx);
-		if (res != TX_CONTINUE)
-			break;
-	}
-
-	if (WARN_ON(tx.skb != skb))
-		goto drop;
-
-	if (unlikely(res == TX_DROP)) {
-		I802_DEBUG_INC(local->tx_handlers_drop);
-		goto drop;
-	}
-
-	if (unlikely(res == TX_QUEUED)) {
-		I802_DEBUG_INC(local->tx_handlers_queued);
-		rcu_read_unlock();
-		return 0;
-	}
+	if (invoke_tx_handlers(&tx))
+		goto out;
 
 retry:
 	ret = __ieee80211_tx(local, skb, &tx);

@@ -1210,6 +1225,7 @@ retry:
 		store->last_frag_rate_ctrl_probe =
 			!!(tx.flags & IEEE80211_TX_PROBE_LAST_FRAG);
 	}
+ out:
 	rcu_read_unlock();
 	return 0;
 

@@ -1960,9 +1976,7 @@ ieee80211_get_buffered_bc(struct ieee802
 	struct ieee80211_local *local = hw_to_local(hw);
 	struct sk_buff *skb = NULL;
 	struct sta_info *sta;
-	ieee80211_tx_handler *handler;
 	struct ieee80211_tx_data tx;
-	ieee80211_tx_result res = TX_DROP;
 	struct net_device *bdev;
 	struct ieee80211_sub_if_data *sdata;
 	struct ieee80211_if_ap *bss = NULL;

@@ -2013,25 +2027,9 @@ ieee80211_get_buffered_bc(struct ieee802
 	tx.channel = local->hw.conf.channel;
 	info->band = tx.channel->band;
 
-	for (handler = ieee80211_tx_handlers; *handler != NULL; handler++) {
-		res = (*handler)(&tx);
-		if (res == TX_DROP || res == TX_QUEUED)
-			break;
-	}
-
-	if (WARN_ON(tx.skb != skb))
-		res = TX_DROP;
-
-	if (res == TX_DROP) {
-		I802_DEBUG_INC(local->tx_handlers_drop);
-		dev_kfree_skb(skb);
-		skb = NULL;
-	} else if (res == TX_QUEUED) {
-		I802_DEBUG_INC(local->tx_handlers_queued);
+	if (invoke_tx_handlers(&tx))
 		skb = NULL;
-	}
-
-out:
+ out:
 	rcu_read_unlock();
 
 	return skb;
author maintainer
#2 lore raw

PATCH 2/3 mac80211: get rid of function pointers in TX path

In reply to Johannes Berg

This changes the TX path to no longer use function pointers for TX handlers but rather invoke them directly. If debugging is enabled, mark the TX handlers noinline because otherwise they all get inlined into invoke_tx_handlers() which makes it harder to see where a bug is.

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/ieee80211_i.h |    6 ++++
 net/mac80211/tx.c          |   63 ++++++++++++++++++++-------------------------
 2 files changed, 35 insertions(+), 34 deletions(-)
net/mac80211/tx.c +29 -34
@@ -222,7 +222,7 @@ static int inline is_ieee80211_device(st
 
 /* tx handlers */
 
-static ieee80211_tx_result
+static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_check_assoc(struct ieee80211_tx_data *tx)
 {
 #ifdef CONFIG_MAC80211_VERBOSE_DEBUG

@@ -276,7 +276,7 @@ ieee80211_tx_h_check_assoc(struct ieee80
 	return TX_CONTINUE;
 }
 
-static ieee80211_tx_result
+static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_sequence(struct ieee80211_tx_data *tx)
 {
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;

@@ -434,7 +434,7 @@ ieee80211_tx_h_unicast_ps_buf(struct iee
 	return TX_CONTINUE;
 }
 
-static ieee80211_tx_result
+static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_ps_buf(struct ieee80211_tx_data *tx)
 {
 	if (unlikely(tx->flags & IEEE80211_TX_PS_BUFFERED))

@@ -446,7 +446,7 @@ ieee80211_tx_h_ps_buf(struct ieee80211_t
 		return ieee80211_tx_h_multicast_ps_buf(tx);
 }
 
-static ieee80211_tx_result
+static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 {
 	struct ieee80211_key *key;

@@ -495,7 +495,7 @@ ieee80211_tx_h_select_key(struct ieee802
 	return TX_CONTINUE;
 }
 
-static ieee80211_tx_result
+static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
 {
 	struct rate_selection rsel;

@@ -539,7 +539,7 @@ ieee80211_tx_h_rate_ctrl(struct ieee8021
 	return TX_CONTINUE;
 }
 
-static ieee80211_tx_result
+static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_misc(struct ieee80211_tx_data *tx)
 {
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) tx->skb->data;

@@ -635,7 +635,7 @@ ieee80211_tx_h_misc(struct ieee80211_tx_
 	return TX_CONTINUE;
 }
 
-static ieee80211_tx_result
+static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_fragment(struct ieee80211_tx_data *tx)
 {
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) tx->skb->data;

@@ -726,7 +726,7 @@ ieee80211_tx_h_fragment(struct ieee80211
 	return TX_DROP;
 }
 
-static ieee80211_tx_result
+static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_encrypt(struct ieee80211_tx_data *tx)
 {
 	if (!tx->key)

@@ -746,7 +746,7 @@ ieee80211_tx_h_encrypt(struct ieee80211_
 	return TX_DROP;
 }
 
-static ieee80211_tx_result
+static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_calculate_duration(struct ieee80211_tx_data *tx)
 {
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;

@@ -776,7 +776,7 @@ ieee80211_tx_h_calculate_duration(struct
 	return TX_CONTINUE;
 }
 
-static ieee80211_tx_result
+static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_stats(struct ieee80211_tx_data *tx)
 {
 	int i;

@@ -797,24 +797,6 @@ ieee80211_tx_h_stats(struct ieee80211_tx
 }
 
 
-typedef ieee80211_tx_result (*ieee80211_tx_handler)(struct ieee80211_tx_data *);
-static ieee80211_tx_handler ieee80211_tx_handlers[] =
-{
-	ieee80211_tx_h_check_assoc,
-	ieee80211_tx_h_sequence,
-	ieee80211_tx_h_ps_buf,
-	ieee80211_tx_h_select_key,
-	ieee80211_tx_h_michael_mic_add,
-	ieee80211_tx_h_rate_ctrl,
-	ieee80211_tx_h_misc,
-	ieee80211_tx_h_fragment,
-	/* handlers after fragment must be aware of tx info fragmentation! */
-	ieee80211_tx_h_encrypt,
-	ieee80211_tx_h_calculate_duration,
-	ieee80211_tx_h_stats,
-	NULL
-};
-
 /* actual transmit path */
 
 /*

@@ -1114,16 +1096,29 @@ static int invoke_tx_handlers(struct iee
 {
 	struct ieee80211_local *local = tx->local;
 	struct sk_buff *skb = tx->skb;
-	ieee80211_tx_handler *handler;
 	ieee80211_tx_result res = TX_DROP;
 	int i;
 
-	for (handler = ieee80211_tx_handlers; *handler != NULL; handler++) {
-		res = (*handler)(tx);
-		if (res != TX_CONTINUE)
-			break;
-	}
+#define CALL_TXH(txh)		\
+	res = txh(tx);		\
+	if (res != TX_CONTINUE)	\
+		goto txh_done;
+
+	CALL_TXH(ieee80211_tx_h_check_assoc)
+	CALL_TXH(ieee80211_tx_h_sequence);
+	CALL_TXH(ieee80211_tx_h_ps_buf);
+	CALL_TXH(ieee80211_tx_h_select_key);
+	CALL_TXH(ieee80211_tx_h_michael_mic_add);
+	CALL_TXH(ieee80211_tx_h_rate_ctrl);
+	CALL_TXH(ieee80211_tx_h_misc);
+	CALL_TXH(ieee80211_tx_h_fragment);
+	/* handlers after fragment must be aware of tx info fragmentation! */
+	CALL_TXH(ieee80211_tx_h_encrypt);
+	CALL_TXH(ieee80211_tx_h_calculate_duration);
+	CALL_TXH(ieee80211_tx_h_stats);
+#undef CALL_TXH
 
+ txh_done:
 	if (unlikely(res == TX_DROP)) {
 		I802_DEBUG_INC(local->tx_handlers_drop);
 		dev_kfree_skb(skb);
net/mac80211/ieee80211_i.h +6 -0
@@ -942,4 +942,10 @@ int ieee80211_frame_duration(struct ieee
 void mac80211_ev_michael_mic_failure(struct net_device *dev, int keyidx,
 				     struct ieee80211_hdr *hdr);
 
+#ifdef CONFIG_MAC80211_DEBUG
+#define debug_noinline noinline
+#else
+#define debug_noinline
+#endif
+
 #endif /* IEEE80211_I_H */
#3 lore raw

Re: [PATCH 2/3] mac80211: get rid of function pointers in TX path

Quoted 8 lines · Johannes Berg
net/mac80211/tx.c +8 -0
+#define CALL_TXH(txh)		\
+	res = txh(tx);		\
+	if (res != TX_CONTINUE)	\
+		goto txh_done;
+
+	CALL_TXH(ieee80211_tx_h_check_assoc)
+	CALL_TXH(ieee80211_tx_h_sequence);
+	CALL_TXH(ieee80211_tx_h_ps_buf);

net/mac80211/tx.c static int invoke_tx_handlers(struct iee · line 1119

Quoted 10 lines
net/mac80211/tx.c +10 -0
+	CALL_TXH(ieee80211_tx_h_select_key);
+	CALL_TXH(ieee80211_tx_h_michael_mic_add);
+	CALL_TXH(ieee80211_tx_h_rate_ctrl);
+	CALL_TXH(ieee80211_tx_h_misc);
+	CALL_TXH(ieee80211_tx_h_fragment);
+	/* handlers after fragment must be aware of tx info fragmentation! */
+	CALL_TXH(ieee80211_tx_h_encrypt);
+	CALL_TXH(ieee80211_tx_h_calculate_duration);
+	CALL_TXH(ieee80211_tx_h_stats);
+#undef CALL_TXH

Unnecessary ; after each CALL_TXH()...except the first.

Same thing for 3/3 on the receive side.

Other than that (small) bit, looks ok at first glance.

Harvey

author maintainer
#4 lore raw

Re: [PATCH 2/3] mac80211: get rid of function pointers in TX path

Quoted 22 lines · Harvey Harrison
Quoted 8 lines · Johannes Berg
net/mac80211/tx.c +8 -0
+#define CALL_TXH(txh)		\
+	res = txh(tx);		\
+	if (res != TX_CONTINUE)	\
+		goto txh_done;
+
+	CALL_TXH(ieee80211_tx_h_check_assoc)
+	CALL_TXH(ieee80211_tx_h_sequence);
+	CALL_TXH(ieee80211_tx_h_ps_buf);

net/mac80211/tx.c static int invoke_tx_handlers(struct iee · line 1119

Quoted 10 lines
net/mac80211/tx.c +10 -0
+	CALL_TXH(ieee80211_tx_h_select_key);
+	CALL_TXH(ieee80211_tx_h_michael_mic_add);
+	CALL_TXH(ieee80211_tx_h_rate_ctrl);
+	CALL_TXH(ieee80211_tx_h_misc);
+	CALL_TXH(ieee80211_tx_h_fragment);
+	/* handlers after fragment must be aware of tx info fragmentation! */
+	CALL_TXH(ieee80211_tx_h_encrypt);
+	CALL_TXH(ieee80211_tx_h_calculate_duration);
+	CALL_TXH(ieee80211_tx_h_stats);
+#undef CALL_TXH

Unnecessary ; after each CALL_TXH()...except the first.

Quoted 1 line

Same thing for 3/3 on the receive side.

Yeah, that's fair, it's not a function call in any way so it need not look like one either. I'll resend tomorrow (just in case somebody else finds something else).

johannes

author maintainer
#5 lore raw

PATCH 3/3 mac80211: get rid of function pointers in RX path

In reply to Johannes Berg

This changes the RX path to no longer use function pointers for RX handlers but rather invoke them directly. If debugging is enabled, mark the RX handlers noinline because otherwise they all get inlined into ieee80211_invoke_rx_handlers() which makes it harder to see where a bug is.

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/rx.c |   93 ++++++++++++++++++++++++------------------------------
 1 file changed, 42 insertions(+), 51 deletions(-)
net/mac80211/rx.c +42 -51
@@ -386,7 +386,7 @@ static void ieee80211_verify_ip_alignmen
 
 /* rx handlers */
 
-static ieee80211_rx_result
+static ieee80211_rx_result debug_noinline
 ieee80211_rx_h_passive_scan(struct ieee80211_rx_data *rx)
 {
 	struct ieee80211_local *local = rx->local;

@@ -463,7 +463,7 @@ ieee80211_rx_mesh_check(struct ieee80211
 }
 
 
-static ieee80211_rx_result
+static ieee80211_rx_result debug_noinline
 ieee80211_rx_h_check(struct ieee80211_rx_data *rx)
 {
 	struct ieee80211_hdr *hdr;

@@ -522,7 +522,7 @@ ieee80211_rx_h_check(struct ieee80211_rx
 }
 
 
-static ieee80211_rx_result
+static ieee80211_rx_result debug_noinline
 ieee80211_rx_h_decrypt(struct ieee80211_rx_data *rx)
 {
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) rx->skb->data;

@@ -710,7 +710,7 @@ static int ap_sta_ps_end(struct net_devi
 	return sent;
 }
 
-static ieee80211_rx_result
+static ieee80211_rx_result debug_noinline
 ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
 {
 	struct sta_info *sta = rx->sta;

@@ -858,7 +858,7 @@ ieee80211_reassemble_find(struct ieee802
 	return NULL;
 }
 
-static ieee80211_rx_result
+static ieee80211_rx_result debug_noinline
 ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
 {
 	struct ieee80211_hdr *hdr;

@@ -974,7 +974,7 @@ ieee80211_rx_h_defragment(struct ieee802
 	return RX_CONTINUE;
 }
 
-static ieee80211_rx_result
+static ieee80211_rx_result debug_noinline
 ieee80211_rx_h_ps_poll(struct ieee80211_rx_data *rx)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(rx->dev);

@@ -1049,7 +1049,7 @@ ieee80211_rx_h_ps_poll(struct ieee80211_
 	return RX_QUEUED;
 }
 
-static ieee80211_rx_result
+static ieee80211_rx_result debug_noinline
 ieee80211_rx_h_remove_qos_control(struct ieee80211_rx_data *rx)
 {
 	u16 fc = rx->fc;

@@ -1367,7 +1367,7 @@ ieee80211_deliver_skb(struct ieee80211_r
 	}
 }
 
-static ieee80211_rx_result
+static ieee80211_rx_result debug_noinline
 ieee80211_rx_h_amsdu(struct ieee80211_rx_data *rx)
 {
 	struct net_device *dev = rx->dev;

@@ -1484,7 +1484,7 @@ ieee80211_rx_h_amsdu(struct ieee80211_rx
 	return RX_QUEUED;
 }
 
-static ieee80211_rx_result
+static ieee80211_rx_result debug_noinline
 ieee80211_rx_h_data(struct ieee80211_rx_data *rx)
 {
 	struct net_device *dev = rx->dev;

@@ -1515,7 +1515,7 @@ ieee80211_rx_h_data(struct ieee80211_rx_
 	return RX_QUEUED;
 }
 
-static ieee80211_rx_result
+static ieee80211_rx_result debug_noinline
 ieee80211_rx_h_ctrl(struct ieee80211_rx_data *rx)
 {
 	struct ieee80211_local *local = rx->local;

@@ -1559,7 +1559,7 @@ ieee80211_rx_h_ctrl(struct ieee80211_rx_
 	return RX_CONTINUE;
 }
 
-static ieee80211_rx_result
+static ieee80211_rx_result debug_noinline
 ieee80211_rx_h_mgmt(struct ieee80211_rx_data *rx)
 {
 	struct ieee80211_sub_if_data *sdata;

@@ -1732,66 +1732,57 @@ static void ieee80211_rx_cooked_monitor(
 	dev_kfree_skb(skb);
 }
 
-typedef ieee80211_rx_result (*ieee80211_rx_handler)(struct ieee80211_rx_data *);
-static ieee80211_rx_handler ieee80211_rx_handlers[] =
-{
-	ieee80211_rx_h_passive_scan,
-	ieee80211_rx_h_check,
-	ieee80211_rx_h_decrypt,
-	ieee80211_rx_h_sta_process,
-	ieee80211_rx_h_defragment,
-	ieee80211_rx_h_ps_poll,
-	ieee80211_rx_h_michael_mic_verify,
-	/* this must be after decryption - so header is counted in MPDU mic
-	 * must be before pae and data, so QOS_DATA format frames
-	 * are not passed to user space by these functions
-	 */
-	ieee80211_rx_h_remove_qos_control,
-	ieee80211_rx_h_amsdu,
-	ieee80211_rx_h_data,
-	ieee80211_rx_h_ctrl,
-	ieee80211_rx_h_mgmt,
-	NULL
-};
 
 static void ieee80211_invoke_rx_handlers(struct ieee80211_sub_if_data *sdata,
 					 struct ieee80211_rx_data *rx,
 					 struct sk_buff *skb)
 {
-	ieee80211_rx_handler *handler;
 	ieee80211_rx_result res = RX_DROP_MONITOR;
 
 	rx->skb = skb;
 	rx->sdata = sdata;
 	rx->dev = sdata->dev;
 
-	for (handler = ieee80211_rx_handlers; *handler != NULL; handler++) {
-		res = (*handler)(rx);
+#define CALL_RXH(rxh)		\
+	res = rxh(rx);		\
+	if (res != RX_CONTINUE)	\
+		goto rxh_done;
+
+	CALL_RXH(ieee80211_rx_h_passive_scan);
+	CALL_RXH(ieee80211_rx_h_check);
+	CALL_RXH(ieee80211_rx_h_decrypt);
+	CALL_RXH(ieee80211_rx_h_sta_process);
+	CALL_RXH(ieee80211_rx_h_defragment);
+	CALL_RXH(ieee80211_rx_h_ps_poll);
+	CALL_RXH(ieee80211_rx_h_michael_mic_verify);
+	/* must be after MMIC verify so header is counted in MPDU mic */
+	CALL_RXH(ieee80211_rx_h_remove_qos_control);
+	CALL_RXH(ieee80211_rx_h_amsdu);
+	CALL_RXH(ieee80211_rx_h_data);
+	CALL_RXH(ieee80211_rx_h_ctrl);
+	CALL_RXH(ieee80211_rx_h_mgmt);
 
-		switch (res) {
-		case RX_CONTINUE:
-			continue;
-		case RX_DROP_UNUSABLE:
-		case RX_DROP_MONITOR:
-			I802_DEBUG_INC(sdata->local->rx_handlers_drop);
-			if (rx->sta)
-				rx->sta->rx_dropped++;
-			break;
-		case RX_QUEUED:
-			I802_DEBUG_INC(sdata->local->rx_handlers_queued);
-			break;
-		}
-		break;
-	}
+#undef CALL_RXH
 
+ rxh_done:
 	switch (res) {
-	case RX_CONTINUE:
 	case RX_DROP_MONITOR:
+		I802_DEBUG_INC(sdata->local->rx_handlers_drop);
+		if (rx->sta)
+			rx->sta->rx_dropped++;
+		/* fall through */
+	case RX_CONTINUE:
 		ieee80211_rx_cooked_monitor(rx);
 		break;
 	case RX_DROP_UNUSABLE:
+		I802_DEBUG_INC(sdata->local->rx_handlers_drop);
+		if (rx->sta)
+			rx->sta->rx_dropped++;
 		dev_kfree_skb(rx->skb);
 		break;
+	case RX_QUEUED:
+		I802_DEBUG_INC(sdata->local->rx_handlers_queued);
+		break;
 	}
 }