[PATCH] mac80211: Fix bug in getting rx status for frames pending in reorder buffer

[PATCH] mac80211: Fix bug in getting rx status for frames pending in reorder buffer

[Vasanthakumar Thiagarajan]

Currently rx status for frames which are completed from reorder buffer
is taken from it's cb area which is wrong, cb is not holding the rx status.
This results in dropping almost all frames from reorder buffer when security
is enabled by doing double decryption (first in hw, second in sw because of wrong
rx status). This patch copies rx status into cb area before the frame is put
into reorder buffer. After this patch, there is a significant improvement in
throughput with ath9k + WPA2(AES).

Signed-off-by: Vasanthakumar Thiagarajan <vasanth@atheros.com>
---
 net/mac80211/rx.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 64ebe66..5fa7aed 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -29,6 +29,7 @@
 static u8 ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
 					   struct tid_ampdu_rx *tid_agg_rx,
 					   struct sk_buff *skb,
+					   struct ieee80211_rx_status *status,
 					   u16 mpdu_seq_num,
 					   int bar_req);
 /*
@@ -1688,7 +1689,7 @@ ieee80211_rx_h_ctrl(struct ieee80211_rx_data *rx)
 		/* manage reordering buffer according to requested */
 		/* sequence number */
 		rcu_read_lock();
-		ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, NULL,
+		ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, NULL, NULL,
 						 start_seq_num, 1);
 		rcu_read_unlock();
 		return RX_DROP_UNUSABLE;
@@ -2293,6 +2294,7 @@ static inline u16 seq_sub(u16 sq1, u16 sq2)
 static u8 ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
 					   struct tid_ampdu_rx *tid_agg_rx,
 					   struct sk_buff *skb,
+					   struct ieee80211_rx_status *rxstatus,
 					   u16 mpdu_seq_num,
 					   int bar_req)
 {
@@ -2374,6 +2376,8 @@ static u8 ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
 
 	/* put the frame in the reordering buffer */
 	tid_agg_rx->reorder_buf[index] = skb;
+	memcpy(tid_agg_rx->reorder_buf[index]->cb, rxstatus,
+	       sizeof(*rxstatus));
 	tid_agg_rx->stored_mpdu_num++;
 	/* release the buffer until next missing frame */
 	index = seq_sub(tid_agg_rx->head_seq_num, tid_agg_rx->ssn)
@@ -2399,7 +2403,8 @@ static u8 ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
 }
 
 static u8 ieee80211_rx_reorder_ampdu(struct ieee80211_local *local,
-				     struct sk_buff *skb)
+				     struct sk_buff *skb,
+				     struct ieee80211_rx_status *status)
 {
 	struct ieee80211_hw *hw = &local->hw;
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
@@ -2448,7 +2453,7 @@ static u8 ieee80211_rx_reorder_ampdu(struct ieee80211_local *local,
 
 	/* according to mpdu sequence number deal with reordering buffer */
 	mpdu_seq_num = (sc & IEEE80211_SCTL_SEQ) >> 4;
-	ret = ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, skb,
+	ret = ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, skb, status,
 						mpdu_seq_num, 0);
  end_reorder:
 	return ret;
@@ -2512,7 +2517,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
 		return;
 	}
 
-	if (!ieee80211_rx_reorder_ampdu(local, skb))
+	if (!ieee80211_rx_reorder_ampdu(local, skb, status))
 		__ieee80211_rx_handle_packet(hw, skb, status, rate);
 
 	rcu_read_unlock();
-- 
1.5.5.1

Re: [PATCH] mac80211: Fix bug in getting rx status for frames pending in reorder buffer

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 3746 bytes --]

On Wed, 2009-03-25 at 16:23 +0530, Vasanthakumar Thiagarajan wrote:
> Currently rx status for frames which are completed from reorder buffer
> is taken from it's cb area which is wrong, cb is not holding the rx status.
> This results in dropping almost all frames from reorder buffer when security
> is enabled by doing double decryption (first in hw, second in sw because of wrong
> rx status). This patch copies rx status into cb area before the frame is put
> into reorder buffer. After this patch, there is a significant improvement in
> throughput with ath9k + WPA2(AES).

Interesting. For ieee80211_rx_irqsafe() packets this is already in the
cb area.

I've been pondering for a while just requiring drivers to put it in
there to start with -- thoughts?

What's the case where the Rx status is NULL?

johannes

> Signed-off-by: Vasanthakumar Thiagarajan <vasanth@atheros.com>
> ---
>  net/mac80211/rx.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index 64ebe66..5fa7aed 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -29,6 +29,7 @@
>  static u8 ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
>  					   struct tid_ampdu_rx *tid_agg_rx,
>  					   struct sk_buff *skb,
> +					   struct ieee80211_rx_status *status,
>  					   u16 mpdu_seq_num,
>  					   int bar_req);
>  /*
> @@ -1688,7 +1689,7 @@ ieee80211_rx_h_ctrl(struct ieee80211_rx_data *rx)
>  		/* manage reordering buffer according to requested */
>  		/* sequence number */
>  		rcu_read_lock();
> -		ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, NULL,
> +		ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, NULL, NULL,
>  						 start_seq_num, 1);
>  		rcu_read_unlock();
>  		return RX_DROP_UNUSABLE;
> @@ -2293,6 +2294,7 @@ static inline u16 seq_sub(u16 sq1, u16 sq2)
>  static u8 ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
>  					   struct tid_ampdu_rx *tid_agg_rx,
>  					   struct sk_buff *skb,
> +					   struct ieee80211_rx_status *rxstatus,
>  					   u16 mpdu_seq_num,
>  					   int bar_req)
>  {
> @@ -2374,6 +2376,8 @@ static u8 ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
>  
>  	/* put the frame in the reordering buffer */
>  	tid_agg_rx->reorder_buf[index] = skb;
> +	memcpy(tid_agg_rx->reorder_buf[index]->cb, rxstatus,
> +	       sizeof(*rxstatus));
>  	tid_agg_rx->stored_mpdu_num++;
>  	/* release the buffer until next missing frame */
>  	index = seq_sub(tid_agg_rx->head_seq_num, tid_agg_rx->ssn)
> @@ -2399,7 +2403,8 @@ static u8 ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
>  }
>  
>  static u8 ieee80211_rx_reorder_ampdu(struct ieee80211_local *local,
> -				     struct sk_buff *skb)
> +				     struct sk_buff *skb,
> +				     struct ieee80211_rx_status *status)
>  {
>  	struct ieee80211_hw *hw = &local->hw;
>  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
> @@ -2448,7 +2453,7 @@ static u8 ieee80211_rx_reorder_ampdu(struct ieee80211_local *local,
>  
>  	/* according to mpdu sequence number deal with reordering buffer */
>  	mpdu_seq_num = (sc & IEEE80211_SCTL_SEQ) >> 4;
> -	ret = ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, skb,
> +	ret = ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, skb, status,
>  						mpdu_seq_num, 0);
>   end_reorder:
>  	return ret;
> @@ -2512,7 +2517,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
>  		return;
>  	}
>  
> -	if (!ieee80211_rx_reorder_ampdu(local, skb))
> +	if (!ieee80211_rx_reorder_ampdu(local, skb, status))
>  		__ieee80211_rx_handle_packet(hw, skb, status, rate);
>  
>  	rcu_read_unlock();

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mac80211: Fix bug in getting rx status for frames pending in reorder buffer

[Vasanthakumar Thiagarajan]

On Wed, Mar 25, 2009 at 04:32:53PM +0530, Johannes Berg wrote:
> On Wed, 2009-03-25 at 16:23 +0530, Vasanthakumar Thiagarajan wrote:
> > Currently rx status for frames which are completed from reorder buffer
> > is taken from it's cb area which is wrong, cb is not holding the rx status.
> > This results in dropping almost all frames from reorder buffer when security
> > is enabled by doing double decryption (first in hw, second in sw because of wrong
> > rx status). This patch copies rx status into cb area before the frame is put
> > into reorder buffer. After this patch, there is a significant improvement in
> > throughput with ath9k + WPA2(AES).
> 
> Interesting. For ieee80211_rx_irqsafe() packets this is already in the
> cb area.

right. I will change the commit log.
> 
> I've been pondering for a while just requiring drivers to put it in
> there to start with -- thoughts?

I'm not seeing any reason fot not doing this in the driver itself.
> 
> What's the case where the Rx status is NULL?

This is while completing reorder buffer on reception of BAR.
> 
> johannes
> 
> > Signed-off-by: Vasanthakumar Thiagarajan <vasanth@atheros.com>
> > ---
> >  net/mac80211/rx.c |   13 +++++++++----
> >  1 files changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > index 64ebe66..5fa7aed 100644
> > --- a/net/mac80211/rx.c
> > +++ b/net/mac80211/rx.c
> > @@ -29,6 +29,7 @@
> >  static u8 ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
> >  					   struct tid_ampdu_rx *tid_agg_rx,
> >  					   struct sk_buff *skb,
> > +					   struct ieee80211_rx_status *status,
> >  					   u16 mpdu_seq_num,
> >  					   int bar_req);
> >  /*
> > @@ -1688,7 +1689,7 @@ ieee80211_rx_h_ctrl(struct ieee80211_rx_data *rx)
> >  		/* manage reordering buffer according to requested */
> >  		/* sequence number */
> >  		rcu_read_lock();
> > -		ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, NULL,
> > +		ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, NULL, NULL,
> >  						 start_seq_num, 1);
> >  		rcu_read_unlock();
> >  		return RX_DROP_UNUSABLE;
> > @@ -2293,6 +2294,7 @@ static inline u16 seq_sub(u16 sq1, u16 sq2)
> >  static u8 ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
> >  					   struct tid_ampdu_rx *tid_agg_rx,
> >  					   struct sk_buff *skb,
> > +					   struct ieee80211_rx_status *rxstatus,
> >  					   u16 mpdu_seq_num,
> >  					   int bar_req)
> >  {
> > @@ -2374,6 +2376,8 @@ static u8 ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
> >  
> >  	/* put the frame in the reordering buffer */
> >  	tid_agg_rx->reorder_buf[index] = skb;
> > +	memcpy(tid_agg_rx->reorder_buf[index]->cb, rxstatus,
> > +	       sizeof(*rxstatus));
> >  	tid_agg_rx->stored_mpdu_num++;
> >  	/* release the buffer until next missing frame */
> >  	index = seq_sub(tid_agg_rx->head_seq_num, tid_agg_rx->ssn)
> > @@ -2399,7 +2403,8 @@ static u8 ieee80211_sta_manage_reorder_buf(struct ieee80211_hw *hw,
> >  }
> >  
> >  static u8 ieee80211_rx_reorder_ampdu(struct ieee80211_local *local,
> > -				     struct sk_buff *skb)
> > +				     struct sk_buff *skb,
> > +				     struct ieee80211_rx_status *status)
> >  {
> >  	struct ieee80211_hw *hw = &local->hw;
> >  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
> > @@ -2448,7 +2453,7 @@ static u8 ieee80211_rx_reorder_ampdu(struct ieee80211_local *local,
> >  
> >  	/* according to mpdu sequence number deal with reordering buffer */
> >  	mpdu_seq_num = (sc & IEEE80211_SCTL_SEQ) >> 4;
> > -	ret = ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, skb,
> > +	ret = ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, skb, status,
> >  						mpdu_seq_num, 0);
> >   end_reorder:
> >  	return ret;
> > @@ -2512,7 +2517,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
> >  		return;
> >  	}
> >  
> > -	if (!ieee80211_rx_reorder_ampdu(local, skb))
> > +	if (!ieee80211_rx_reorder_ampdu(local, skb, status))
> >  		__ieee80211_rx_handle_packet(hw, skb, status, rate);
> >  
> >  	rcu_read_unlock();

Re: [PATCH] mac80211: Fix bug in getting rx status for frames pending in reorder buffer

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

On Wed, 2009-03-25 at 16:49 +0530, Vasanthakumar Thiagarajan wrote:
> On Wed, Mar 25, 2009 at 04:32:53PM +0530, Johannes Berg wrote:
> > On Wed, 2009-03-25 at 16:23 +0530, Vasanthakumar Thiagarajan wrote:
> > > Currently rx status for frames which are completed from reorder buffer
> > > is taken from it's cb area which is wrong, cb is not holding the rx status.
> > > This results in dropping almost all frames from reorder buffer when security
> > > is enabled by doing double decryption (first in hw, second in sw because of wrong
> > > rx status). This patch copies rx status into cb area before the frame is put
> > > into reorder buffer. After this patch, there is a significant improvement in
> > > throughput with ath9k + WPA2(AES).
> > 
> > Interesting. For ieee80211_rx_irqsafe() packets this is already in the
> > cb area.
> 
> right. I will change the commit log.

No worries -- just pointing it out -- this is the reason it worked ok
with iwlwifi.

> This is while completing reorder buffer on reception of BAR.

ok... this code is a little convoluted, so I didn't see that at first,
thanks.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]