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.
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(-)
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);
--
1.7.11.7
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).
>
> 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(-)
>
> 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ł
[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.
>
> 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
--
Kalle Valo
On 02/27/2014 11:51 PM, Michal Kazior wrote:
> 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();
As far as I can tell, packets with bad FCS are dropped earlier in that
rx method?
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On 3 March 2014 23:08, Ben Greear <[email protected]> wrote:
> On 02/27/2014 11:51 PM, Michal Kazior wrote:
>> 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();
>
> 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ł