Message ID | 1393550452-1302-1-git-send-email-greearb@candelatech.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
greearb@candelatech.com writes: > From: Ben Greear <greearb@candelatech.com> > > 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. > > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > > 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
On 28 February 2014 02:20, <greearb@candelatech.com> wrote: > From: Ben Greear <greearb@candelatech.com> > > 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). > > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > > 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(-) > > diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c > index 9f3def7..a5070a9 100644 > --- a/drivers/net/wireless/ath/ath10k/htt_rx.c > +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c > @@ -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. > + */ It'd be great if we could use kernel-provided function to merge skbs instead of rolling out our own ;-) > + > + 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. > + > + /* 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); 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? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/27/2014 11:51 PM, Michal Kazior wrote: > On 28 February 2014 02:20, <greearb@candelatech.com> wrote: >> From: Ben Greear <greearb@candelatech.com> >> >> 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(); As far as I can tell, packets with bad FCS are dropped earlier in that rx method? Thanks, Ben
On 3 March 2014 23:08, Ben Greear <greearb@candelatech.com> wrote: > On 02/27/2014 11:51 PM, Michal Kazior wrote: >> On 28 February 2014 02:20, <greearb@candelatech.com> wrote: >>> From: Ben Greear <greearb@candelatech.com> >>> >>> 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(); > > 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? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c index 9f3def7..a5070a9 100644 --- a/drivers/net/wireless/ath/ath10k/htt_rx.c +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c @@ -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);