ath10k: support msdu chaining.
PATCH v2 1/1 ath10k: support msdu chaining.
From: Ben Greear <[email protected]>
Consolidate the list of msdu skbs into the msdu-head skb, delete the rest of the skbs, pass the msdu-head skb on up the stack as normal.
Tested with high-speed TCP and UDP traffic on modified firmware that supports raw-rx.
---
v2: Add note about skb_try_coalesce, though I am not sure it will
work or not.
Only grow skb once, which should save on some memory (re)allocation and copying when we have more than one chained skb.
drivers/net/wireless/ath/ath10k/htt_rx.c | 60 +++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 4 deletions(-)
drivers/net/wireless/ath/ath10k/htt_rx.c +56 -4
@@ -401,6 +401,7 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt, msdu_len =MS (__le32_to_cpu (rx_desc->msdu_start.info0),RX_MSDU_START_INFO0_MSDU_LENGTH ); msdu_chained = rx_desc->frag_info.ring2_more_count; + msdu_chaining = msdu_chained;if (msdu_len_invalid) msdu_len =0 ;
@@ -428,7 +429,6 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt, msdu->next = next; msdu = next; - msdu_chaining =1 ; } last_msdu = __le32_to_cpu(rx_desc->msdu_end.info0) &
@@ -1008,14 +1008,66 @@ continue_on:continue ; } -/* FIXME: we do not support chaining yet. -* this needs investigation */ if (msdu_chaining) { -ath10k_warn ("htt rx msdu_chaining is true\n" ); +struct sk_buff *next = msdu_head->next; +struct sk_buff *to_free = next; +static int do_once =1 ; +int space; +int total_len =0 ; + +/* TODO: Might could optimize this by using +* skb_try_coalesce or similar method to] +* decrease copying. +*/ + + msdu_head->next =NULL ; + +if (unlikely (do_once)) { +ath10k_warn ("htt rx msdu_chaining detected %d\n" , + msdu_chaining); + do_once =0 ; + } + +/* Allocate total length all at once. */ +while (next) { + total_len += next->len; + next = next->next; + } + + space = total_len -skb_tailroom (msdu_head); +if ((space >0 ) && + (pskb_expand_head (msdu_head,0 , space,GFP_ATOMIC ) <0 )) { +/* TODO: bump some rx-oom error stat */ + goto outside_continue; + } + +/* Walk list again, copying contents into +* msdu_head +*/ + next = to_free; +while (next) { +skb_copy_from_linear_data (next,skb_put (msdu_head, next->len), + next->len); + next = next->next; + } + +/* If here, we have consolidated skb. Free the +* fragments and pass the main skb on up the +* stack. +*/ +ath10k_htt_rx_free_msdu_chain (to_free); + goto have_good_msdu; + + outside_continue: +/* put it back together so we can free the +* whole list at once. +*/ + msdu_head->next = to_free;ath10k_htt_rx_free_msdu_chain (msdu_head);continue ; } + have_good_msdu: info.skb = msdu_head; info.fcs_err =ath10k_htt_rx_has_fcs_err (msdu_head); info.mic_err =ath10k_htt_rx_has_mic_err (msdu_head);
Signature · 7 lines
1.7.11.7 _______________________________________________ ath10k mailing list [email protected] http://lists.infradead.org/mailman/listinfo/ath10k
Quoted 8 lines · [email protected] writes:
From: Ben Greear <[email protected]>
Consolidate the list of msdu skbs into the msdu-head skb, delete the rest of the skbs, pass the msdu-head skb on up the stack as normal.
Tested with high-speed TCP and UDP traffic on modified firmware that supports raw-rx.
Quoted 7 lines
Signed-off-by Ben Greear <[email protected]>---
v2: Add note about
skb_try_coalesce, though I am not sure it will
work or not.
Only grow skb once, which should save on some memory (re)allocation and copying when we have more than one chained skb.
I didn't look at the patch itself yet, but I see checkpatch warnings:
drivers/net/wireless/ath/ath10k/htt_rx.c:1022: WARNING: line over 80 characters drivers/net/wireless/ath/ath10k/htt_rx.c:1032: WARNING: line over 80 characters drivers/net/wireless/ath/ath10k/htt_rx.c:1044: WARNING: labels should not be indented drivers/net/wireless/ath/ath10k/htt_rx.c:1053: WARNING: labels should not be indented
Signature · 1 line
Kalle Valo
Quoted 7 lines · On 28 February 2014 02:20, <[email protected]> wrote:
From: Ben Greear <[email protected]>
Consolidate the list of msdu skbs into the msdu-head skb, delete the rest of the skbs, pass the msdu-head skb on up the stack as normal.
Tested with high-speed TCP and UDP traffic on modified firmware that supports raw-rx.
As for an umodified firmware I expect ath10k will just re-assemble corrupted frames just to have them dropped by mac80211. It might be worth checking:
if (chaining && fcs_error)
drop();
..to avoid processing junk and flushing d-cache (this matters for less powerful host systems).
Quoted 40 lines
Signed-off-by Ben Greear <[email protected]>---
v2: Add note about
skb_try_coalesce, though I am not sure it will
work or not.
Only grow skb once, which should save on some memory (re)allocation and copying when we have more than one chained skb.drivers/net/wireless/ath/ath10k/htt_rx.c | 60 +++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 4 deletions(-)drivers/net/wireless/ath/ath10k/htt_rx.c +1
-4@@ -401,6 +401,7 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt, msdu_len =MS (__le32_to_cpu (rx_desc->msdu_start.info0),RX_MSDU_START_INFO0_MSDU_LENGTH ); msdu_chained = rx_desc->frag_info.ring2_more_count; + msdu_chaining = msdu_chained;if (msdu_len_invalid) msdu_len =0 ;@@ -428,7 +429,6 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt, msdu->next = next; msdu = next; - msdu_chaining =1 ; } last_msdu = __le32_to_cpu(rx_desc->msdu_end.info0) &@@ -1008,14 +1008,66 @@ continue_on:continue ; } -/* FIXME: we do not support chaining yet. -* this needs investigation */ if (msdu_chaining) { -ath10k_warn ("htt rx msdu_chaining is true\n" );
drivers/net/wireless/ath/ath10k/htt_rx.c continue_on: · line 1021
Quoted 10 lines
drivers/net/wireless/ath/ath10k/htt_rx.c +10
-0+struct sk_buff *next = msdu_head->next; +struct sk_buff *to_free = next; +static int do_once =1 ; +int space; +int total_len =0 ; + +/* TODO: Might could optimize this by using +* skb_try_coalesce or similar method to] +* decrease copying. +*/
It'd be great if we could use kernel-provided function to merge skbs instead of rolling out our own ;-)
drivers/net/wireless/ath/ath10k/htt_rx.c continue_on: · line 1029
Quoted 8 lines
drivers/net/wireless/ath/ath10k/htt_rx.c +8
-0+ + msdu_head->next =NULL ; + +if (unlikely (do_once)) { +ath10k_warn ("htt rx msdu_chaining detected %d\n" , + msdu_chaining); + do_once =0 ; + }
I don't really like this do_once thing. I wouldn't even bother with a warning, since, well, chaining parsing is implemented. This should be a simple debug print if anything.
Quoted 34 lines
drivers/net/wireless/ath/ath10k/htt_rx.c +34
-0+ +/* Allocate total length all at once. */ +while (next) { + total_len += next->len; + next = next->next; + } + + space = total_len -skb_tailroom (msdu_head); +if ((space >0 ) && + (pskb_expand_head (msdu_head,0 , space,GFP_ATOMIC ) <0 )) { +/* TODO: bump some rx-oom error stat */ + goto outside_continue; + } + +/* Walk list again, copying contents into +* msdu_head +*/ + next = to_free; +while (next) { +skb_copy_from_linear_data (next,skb_put (msdu_head, next->len), + next->len); + next = next->next; + } + +/* If here, we have consolidated skb. Free the +* fragments and pass the main skb on up the +* stack. +*/ +ath10k_htt_rx_free_msdu_chain (to_free); + goto have_good_msdu; + + outside_continue: +/* put it back together so we can free the +* whole list at once.
drivers/net/wireless/ath/ath10k/htt_rx.c continue_on: · line 1073
Quoted 10 lines
drivers/net/wireless/ath/ath10k/htt_rx.c +3
-0+*/ + msdu_head->next = to_free;ath10k_htt_rx_free_msdu_chain (msdu_head);continue ; } + have_good_msdu: info.skb = msdu_head; info.fcs_err =ath10k_htt_rx_has_fcs_err (msdu_head); info.mic_err =ath10k_htt_rx_has_mic_err (msdu_head);
If anything, please, make it into a separate function. The rx handler is already huge and it needs shrinking down, not expanding it more.
Michał
Quoted 9 lines · Michal Kazior
Quoted 7 lines · On 28 February 2014 02:20, <[email protected]> wrote:
From: Ben Greear <[email protected]>
Consolidate the list of msdu skbs into the msdu-head skb, delete the rest of the skbs, pass the msdu-head skb on up the stack as normal.
Tested with high-speed TCP and UDP traffic on modified firmware that supports raw-rx.
Quoted 6 lines
As for an umodified firmware I expect ath10k will just re-assemble corrupted frames just to have them dropped by mac80211. It might be worth checking:
if (chaining &&
fcs_error)drop();
As far as I can tell, packets with bad FCS are dropped earlier in that rx method?
Thanks,
Ben
Signature · 8 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com _______________________________________________ ath10k mailing list [email protected] http://lists.infradead.org/mailman/listinfo/ath10k
Quoted 14 lines · Ben Greear
Quoted 13 lines · Michal Kazior
Quoted 7 lines · On 28 February 2014 02:20, <[email protected]> wrote:
From: Ben Greear <[email protected]>
Consolidate the list of msdu skbs into the msdu-head skb, delete the rest of the skbs, pass the msdu-head skb on up the stack as normal.
Tested with high-speed TCP and UDP traffic on modified firmware that supports raw-rx.
As for an umodified firmware I expect ath10k will just re-assemble corrupted frames just to have them dropped by mac80211. It might be worth checking:
Quoted 5 lines
Quoted 2 lines
if (chaining &&
fcs_error)drop();As far as I can tell, packets with bad FCS are dropped earlier in that rx method?
Ah, good point. We're dropping some frames based on status value already.
Michał