[PATCH 1/2] mac80211: support paged rx SKBs

[PATCH 1/2] mac80211: support paged rx SKBs

[Zhu Yi]

Mac80211 drivers can now pass paged SKBs to mac80211 via
ieee80211_rx{_irqsafe}. The implementation currently use
skb_linearize() in a few places i.e. management frame handling,
software decryption, defragmentation and A-MSDU process. We can
optimize them one by one later.

Signed-off-by: Zhu Yi <yi.zhu@intel.com>
---
 net/mac80211/rx.c   |   35 +++++++++++++++++++++++++++++++----
 net/wireless/util.c |    6 +++++-
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 1da57c8..063aa84 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -38,7 +38,7 @@ static struct sk_buff *remove_monitor_info(struct ieee80211_local *local,
 {
 	if (local->hw.flags & IEEE80211_HW_RX_INCLUDES_FCS) {
 		if (likely(skb->len > FCS_LEN))
-			skb_trim(skb, skb->len - FCS_LEN);
+			__pskb_trim(skb, skb->len - FCS_LEN);
 		else {
 			/* driver bug */
 			WARN_ON(1);
@@ -227,6 +227,11 @@ ieee80211_rx_monitor(struct ieee80211_local *local, struct sk_buff *origskb,
 	if (local->hw.flags & IEEE80211_HW_RX_INCLUDES_FCS)
 		present_fcs_len = FCS_LEN;
 
+	if (!pskb_may_pull(origskb, sizeof(struct ieee80211_hdr))) {
+		dev_kfree_skb(origskb);
+		return NULL;
+	}
+
 	if (!local->monitors) {
 		if (should_drop_frame(origskb, present_fcs_len)) {
 			dev_kfree_skb(origskb);
@@ -931,6 +936,9 @@ ieee80211_rx_h_decrypt(struct ieee80211_rx_data *rx)
 		return RX_DROP_MONITOR;
 	}
 
+	if (skb_linearize(rx->skb))
+		return RX_DROP_MONITOR;
+
 	/* Check for weak IVs if possible */
 	if (rx->sta && rx->key->conf.alg == ALG_WEP &&
 	    ieee80211_is_data(hdr->frame_control) &&
@@ -1231,6 +1239,9 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
 	}
 	I802_DEBUG_INC(rx->local->rx_handlers_fragments);
 
+	if (skb_linearize(rx->skb))
+		return RX_DROP_MONITOR;
+
 	seq = (sc & IEEE80211_SCTL_SEQ) >> 4;
 
 	if (frag == 0) {
@@ -1588,6 +1599,9 @@ ieee80211_rx_h_amsdu(struct ieee80211_rx_data *rx)
 	skb->dev = dev;
 	__skb_queue_head_init(&frame_list);
 
+	if (skb_linearize(skb))
+		return RX_DROP_MONITOR;
+
 	ieee80211_amsdu_to_8023s(skb, &frame_list, dev->dev_addr,
 				 rx->sdata->vif.type,
 				 rx->local->hw.extra_tx_headroom);
@@ -2357,29 +2371,42 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
 	struct ieee80211_local *local = hw_to_local(hw);
 	struct ieee80211_sub_if_data *sdata;
 	struct ieee80211_hdr *hdr;
+	__le16 fc;
 	struct ieee80211_rx_data rx;
 	int prepares;
 	struct ieee80211_sub_if_data *prev = NULL;
 	struct sk_buff *skb_new;
 	struct sta_info *sta, *tmp;
 	bool found_sta = false;
+	int err = 0;
 
-	hdr = (struct ieee80211_hdr *)skb->data;
+	fc = ((struct ieee80211_hdr *)skb->data)->frame_control;
 	memset(&rx, 0, sizeof(rx));
 	rx.skb = skb;
 	rx.local = local;
 
-	if (ieee80211_is_data(hdr->frame_control) || ieee80211_is_mgmt(hdr->frame_control))
+	if (ieee80211_is_data(fc) || ieee80211_is_mgmt(fc))
 		local->dot11ReceivedFragmentCount++;
 
 	if (unlikely(test_bit(SCAN_HW_SCANNING, &local->scanning) ||
 		     test_bit(SCAN_OFF_CHANNEL, &local->scanning)))
 		rx.flags |= IEEE80211_RX_IN_SCAN;
 
+	if (ieee80211_is_mgmt(fc))
+		err = skb_linearize(skb);
+	else
+		err = !pskb_may_pull(skb, ieee80211_hdrlen(fc));
+
+	if (err) {
+		dev_kfree_skb(skb);
+		return;
+	}
+
+	hdr = (struct ieee80211_hdr *)skb->data;
 	ieee80211_parse_qos(&rx);
 	ieee80211_verify_alignment(&rx);
 
-	if (ieee80211_is_data(hdr->frame_control)) {
+	if (ieee80211_is_data(fc)) {
 		for_each_sta_info(local, hdr->addr2, sta, tmp) {
 			rx.sta = sta;
 			found_sta = true;
diff --git a/net/wireless/util.c b/net/wireless/util.c
index be2ab8c..1764043 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -314,6 +314,10 @@ int ieee80211_data_to_8023(struct sk_buff *skb, const u8 *addr,
 	memcpy(dst, ieee80211_get_DA(hdr), ETH_ALEN);
 	memcpy(src, ieee80211_get_SA(hdr), ETH_ALEN);
 
+	if (iftype == NL80211_IFTYPE_MESH_POINT &&
+	    !pskb_may_pull(skb, hdrlen + sizeof(struct ieee80211s_hdr)))
+		return -1;
+
 	switch (hdr->frame_control &
 		cpu_to_le16(IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS)) {
 	case cpu_to_le16(IEEE80211_FCTL_TODS):
@@ -357,7 +361,7 @@ int ieee80211_data_to_8023(struct sk_buff *skb, const u8 *addr,
 		break;
 	}
 
-	if (unlikely(skb->len - hdrlen < 8))
+	if (!pskb_may_pull(skb, hdrlen + 8))
 		return -1;
 
 	payload = skb->data + hdrlen;
-- 
1.6.0.4

[PATCH 2/2] iwlwifi: remove skb_linearize for rx frames

[Zhu Yi]

Remove skb_linearize in the driver since mac80211 now supports
paged rx SKBs.

Signed-off-by: Zhu Yi <yi.zhu@intel.com>
---
 drivers/net/wireless/iwlwifi/iwl-dev.h |    7 -------
 drivers/net/wireless/iwlwifi/iwl-rx.c  |   30 +-----------------------------
 2 files changed, 1 insertions(+), 36 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-dev.h b/drivers/net/wireless/iwlwifi/iwl-dev.h
index e847e61..c2b907d 100644
--- a/drivers/net/wireless/iwlwifi/iwl-dev.h
+++ b/drivers/net/wireless/iwlwifi/iwl-dev.h
@@ -363,13 +363,6 @@ enum {
 
 #define DEF_CMD_PAYLOAD_SIZE 320
 
-/*
- * IWL_LINK_HDR_MAX should include ieee80211_hdr, radiotap header,
- * SNAP header and alignment. It should also be big enough for 802.11
- * control frames.
- */
-#define IWL_LINK_HDR_MAX 64
-
 /**
  * struct iwl_device_cmd
  *
diff --git a/drivers/net/wireless/iwlwifi/iwl-rx.c b/drivers/net/wireless/iwlwifi/iwl-rx.c
index b6a64d8..05dd057 100644
--- a/drivers/net/wireless/iwlwifi/iwl-rx.c
+++ b/drivers/net/wireless/iwlwifi/iwl-rx.c
@@ -1075,7 +1075,6 @@ static void iwl_pass_packet_to_mac80211(struct iwl_priv *priv,
 					struct ieee80211_rx_status *stats)
 {
 	struct sk_buff *skb;
-	int ret = 0;
 	__le16 fc = hdr->frame_control;
 
 	/* We only process data packets if the interface is open */
@@ -1090,45 +1089,18 @@ static void iwl_pass_packet_to_mac80211(struct iwl_priv *priv,
 	    iwl_set_decrypted_flag(priv, hdr, ampdu_status, stats))
 		return;
 
-	skb = alloc_skb(IWL_LINK_HDR_MAX * 2, GFP_ATOMIC);
+	skb = dev_alloc_skb(128);
 	if (!skb) {
 		IWL_ERR(priv, "alloc_skb failed\n");
 		return;
 	}
 
-	skb_reserve(skb, IWL_LINK_HDR_MAX);
 	skb_add_rx_frag(skb, 0, rxb->page, (void *)hdr - rxb_addr(rxb), len);
 
-	/* mac80211 currently doesn't support paged SKB. Convert it to
-	 * linear SKB for management frame and data frame requires
-	 * software decryption or software defragementation. */
-	if (ieee80211_is_mgmt(fc) ||
-	    ieee80211_has_protected(fc) ||
-	    ieee80211_has_morefrags(fc) ||
-	    le16_to_cpu(hdr->seq_ctrl) & IEEE80211_SCTL_FRAG ||
-	    (ieee80211_is_data_qos(fc) &&
-	     *ieee80211_get_qos_ctl(hdr) &
-	     IEEE80211_QOS_CONTROL_A_MSDU_PRESENT))
-		ret = skb_linearize(skb);
-	else
-		ret = __pskb_pull_tail(skb, min_t(u16, IWL_LINK_HDR_MAX, len)) ?
-			 0 : -ENOMEM;
-
-	if (ret) {
-		kfree_skb(skb);
-		goto out;
-	}
-
-	/*
-	 * XXX: We cannot touch the page and its virtual memory (hdr) after
-	 * here. It might have already been freed by the above skb change.
-	 */
-
 	iwl_update_stats(priv, false, fc, len);
 	memcpy(IEEE80211_SKB_RXCB(skb), stats, sizeof(*stats));
 
 	ieee80211_rx(priv->hw, skb);
- out:
 	priv->alloc_rxb_page--;
 	rxb->page = NULL;
 }
-- 
1.6.0.4

Re: [PATCH 1/2] mac80211: support paged rx SKBs

[Johannes Berg]

On Wed, 2010-03-24 at 10:57 +0800, Zhu Yi wrote:
> Mac80211 drivers can now pass paged SKBs to mac80211 via
> ieee80211_rx{_irqsafe}. The implementation currently use
> skb_linearize() in a few places i.e. management frame handling,
> software decryption, defragmentation and A-MSDU process. We can
> optimize them one by one later.


Cool, thanks, I'll go over it in the morning to double-check you caught
all the places :)

johannes

Re: [PATCH 1/2] mac80211: support paged rx SKBs

[Stanislaw Gruszka]

On Wed, 24 Mar 2010 10:57:42 +0800
Zhu Yi <yi.zhu@intel.com> wrote:

> Mac80211 drivers can now pass paged SKBs to mac80211 via
> ieee80211_rx{_irqsafe}. The implementation currently use
> skb_linearize() in a few places i.e. management frame handling,
> software decryption, defragmentation and A-MSDU process. 

What benefits this gives? Only driver which need non linear skbs
is iwliwifi and nicely handle that by itself. I do not see any 
other potential users for that. Can we wait for some other
users before put that feature into mac stack?

Stanislaw

Re: [PATCH 1/2] mac80211: support paged rx SKBs

[Johannes Berg]

On Wed, 2010-03-24 at 14:00 +0100, Stanislaw Gruszka wrote:
> On Wed, 24 Mar 2010 10:57:42 +0800
> Zhu Yi <yi.zhu@intel.com> wrote:
> 
> > Mac80211 drivers can now pass paged SKBs to mac80211 via
> > ieee80211_rx{_irqsafe}. The implementation currently use
> > skb_linearize() in a few places i.e. management frame handling,
> > software decryption, defragmentation and A-MSDU process. 
> 
> What benefits this gives? Only driver which need non linear skbs
> is iwliwifi and nicely handle that by itself. I do not see any 
> other potential users for that. Can we wait for some other
> users before put that feature into mac stack?

Hell no. We will get more users, and even putting it into the stack now
gives us the benefit of being able to improve it -- iwlwifi doesn't
"nicely" handle it after all.

johannes

Re: [PATCH 1/2] mac80211: support paged rx SKBs

[Kalle Valo]

Zhu Yi <yi.zhu@intel.com> writes:

> Mac80211 drivers can now pass paged SKBs to mac80211 via
> ieee80211_rx{_irqsafe}. The implementation currently use
> skb_linearize() in a few places i.e. management frame handling,
> software decryption, defragmentation and A-MSDU process. We can
> optimize them one by one later.

I think ieee80211_rx{_irqsafe} documentation should now mention that
paged skbs are supported. Better to make this suppot explicit for the
driver authors.

-- 
Kalle Valo

Re: [PATCH 1/2] mac80211: support paged rx SKBs

[Stanislaw Gruszka]

On Wed, Mar 24, 2010 at 09:16:02AM -0700, Johannes Berg wrote:
> > > Mac80211 drivers can now pass paged SKBs to mac80211 via
> > > ieee80211_rx{_irqsafe}. The implementation currently use
> > > skb_linearize() in a few places i.e. management frame handling,
> > > software decryption, defragmentation and A-MSDU process. 
> > 
> > What benefits this gives? Only driver which need non linear skbs
> > is iwliwifi and nicely handle that by itself. I do not see any 
> > other potential users for that. Can we wait for some other
> > users before put that feature into mac stack?
> 
> Hell no. We will get more users, and even putting it into the stack now
> gives us the benefit of being able to improve it

But wait, isn't that all just workaround for bad performing allocator,
what should be fixed in proper place not in mac80211 nor iwlwifi. What
for exactly this is needed, concretes please - that is not described in
patch changelog.

>  -- iwlwifi doesn't
> "nicely" handle it after all

Oh well, at least now we have no more bugs with that (I hope).

Stanislaw

Re: [PATCH 1/2] mac80211: support paged rx SKBs

[Zhu Yi]

On Thu, 2010-03-25 at 15:26 +0800, Stanislaw Gruszka wrote:
> 
> But wait, isn't that all just workaround for bad performing allocator,
> what should be fixed in proper place not in mac80211 nor iwlwifi. What
> for exactly this is needed, concretes please - that is not described
> in patch changelog. 

If a devices is capable of DMA a frame to/from physically discontinuous
host memory, the paged SKB handling can mitigate memory subsystem
pressure for large chunk of continuous memory allocation and also avoid
CPU cycles for memcpy. In Linux kernel, since TCP/IP stacks and
(capable) devices support paged SKBs, mac80211 will become the
bottleneck if it still couldn't provide the support. For devices don't
support paged SKBs, this patch doesn't bring performance issues. So it
should be a good thing to have for us.

Thanks,
-yi

Re: [PATCH 1/2] mac80211: support paged rx SKBs

[Zhu Yi]

On Thu, 2010-03-25 at 14:33 +0800, Kalle Valo wrote:
> Zhu Yi <yi.zhu@intel.com> writes:
> 
> > Mac80211 drivers can now pass paged SKBs to mac80211 via
> > ieee80211_rx{_irqsafe}. The implementation currently use
> > skb_linearize() in a few places i.e. management frame handling,
> > software decryption, defragmentation and A-MSDU process. We can
> > optimize them one by one later.
> 
> I think ieee80211_rx{_irqsafe} documentation should now mention that
> paged skbs are supported. Better to make this suppot explicit for the
> driver authors.

Good idea it should be documented somewhere (after the patch is
accepted). Maybe the developer wiki page? For the code itself, I think
Linux drivers can always assume the network stacks support paged
buffers. In case some stack doesn't support paged buffer, it should do
skb_linearize() itself in the very beginning of its rcv handling (e.g.
sctp_rcv, tipc_recv_msg, etc).

Thanks,
-yi

Re: [PATCH 1/2] mac80211: support paged rx SKBs

[Kalle Valo]

Zhu Yi <yi.zhu@intel.com> writes:

>> I think ieee80211_rx{_irqsafe} documentation should now mention that
>> paged skbs are supported. Better to make this suppot explicit for the
>> driver authors.
>
> Good idea it should be documented somewhere (after the patch is
> accepted). Maybe the developer wiki page?

I was thinking to add it to include/net/mac80211.h. Better to have all
documentation in one place.

> For the code itself, I think Linux drivers can always assume the
> network stacks support paged buffers.

But this wasn't true with mac80211 before your patch, right? So now
it's better to state this clearly in mac80211 documentation.

-- 
Kalle Valo

Re: [PATCH 1/2] mac80211: support paged rx SKBs

[Zhu Yi]

On Fri, 2010-03-26 at 14:15 +0800, Kalle Valo wrote:
> 
> I was thinking to add it to include/net/mac80211.h. Better to have all
> documentation in one place. 

OK. I'll send out V2.

Thanks,
-yi