How to best consolidate list of skbs (msdu) for receive?

How to best consolidate list of skbs (msdu) for receive?

[Ben Greear]

In ath10k, it can receive msdu (in rx raw mode, at least).  These show up
as a list of skb.  Currently they are just dropped as un-handled, and I
need to fix this.

I think I need to consolidate this list into a single skb in order to pass
it up the stack.

What is the preferred way to go about doing this?

It seems I could just expand the head skb (pskb_expand_head) and copy the data from the others
onto the end of the head skb, but maybe there is a more efficient way to go about
doing this?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: How to best consolidate list of skbs (msdu) for receive?

[Florian Fainelli]

2014-02-27 10:36 GMT-08:00 Ben Greear <greearb@candelatech.com>:
> In ath10k, it can receive msdu (in rx raw mode, at least).  These show up
> as a list of skb.  Currently they are just dropped as un-handled, and I
> need to fix this.
>
> I think I need to consolidate this list into a single skb in order to pass
> it up the stack.
>
> What is the preferred way to go about doing this?
>
> It seems I could just expand the head skb (pskb_expand_head) and copy the data from the others
> onto the end of the head skb, but maybe there is a more efficient way to go about
> doing this?

Some Ethernet drivers you would in general get multiple RX fragments
for large frames, with the first fragment containing the headers, and
then buffers of a few KiB, it is the driver responsibility to
reassemble this as a large SKB with multiple fragments (grep for
skb_fill_page_desc), would that work in your case?
-- 
Florian

Re: How to best consolidate list of skbs (msdu) for receive?

[Eric Dumazet]

On Thu, 2014-02-27 at 12:37 -0800, Florian Fainelli wrote:
> 2014-02-27 10:36 GMT-08:00 Ben Greear <greearb@candelatech.com>:
> > In ath10k, it can receive msdu (in rx raw mode, at least).  These show up
> > as a list of skb.  Currently they are just dropped as un-handled, and I
> > need to fix this.
> >
> > I think I need to consolidate this list into a single skb in order to pass
> > it up the stack.
> >
> > What is the preferred way to go about doing this?
> >
> > It seems I could just expand the head skb (pskb_expand_head) and copy the data from the others
> > onto the end of the head skb, but maybe there is a more efficient way to go about
> > doing this?
> 
> Some Ethernet drivers you would in general get multiple RX fragments
> for large frames, with the first fragment containing the headers, and
> then buffers of a few KiB, it is the driver responsibility to
> reassemble this as a large SKB with multiple fragments (grep for
> skb_fill_page_desc), would that work in your case?

Also skb_try_coalesce() might feet the need, if all you have are a list
of skb, rather than an array of page fragments

Re: How to best consolidate list of skbs (msdu) for receive?

[Ben Greear]

On 02/27/2014 01:21 PM, Eric Dumazet wrote:
> On Thu, 2014-02-27 at 12:37 -0800, Florian Fainelli wrote:
>> 2014-02-27 10:36 GMT-08:00 Ben Greear <greearb@candelatech.com>:
>>> In ath10k, it can receive msdu (in rx raw mode, at least).  These show up
>>> as a list of skb.  Currently they are just dropped as un-handled, and I
>>> need to fix this.
>>>
>>> I think I need to consolidate this list into a single skb in order to pass
>>> it up the stack.
>>>
>>> What is the preferred way to go about doing this?
>>>
>>> It seems I could just expand the head skb (pskb_expand_head) and copy the data from the others
>>> onto the end of the head skb, but maybe there is a more efficient way to go about
>>> doing this?
>>
>> Some Ethernet drivers you would in general get multiple RX fragments
>> for large frames, with the first fragment containing the headers, and
>> then buffers of a few KiB, it is the driver responsibility to
>> reassemble this as a large SKB with multiple fragments (grep for
>> skb_fill_page_desc), would that work in your case?
> 
> Also skb_try_coalesce() might feet the need, if all you have are a list
> of skb, rather than an array of page fragments

Thanks for the hints...I'll go look at this.

That said, the code below appears to work, even if it is
not as efficient as it might be?

			if (msdu_chaining) {
				struct sk_buff *next = msdu_head->next;
				struct sk_buff *to_free = next;
				int space;
				static int do_once = 1;
				msdu_head->next = NULL;

				if (unlikely(do_once)) {
					ath10k_warn("htt rx msdu_chaining detected %d\n",
						    msdu_chaining);
					do_once = 0;
				}

				while (next) {
					space = next->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;
					}
					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;
			}



Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: How to best consolidate list of skbs (msdu) for receive?

[Eric Dumazet]

On Thu, 2014-02-27 at 13:51 -0800, Ben Greear wrote:

> That said, the code below appears to work, even if it is
> not as efficient as it might be?
> 
> 			if (msdu_chaining) {
> 				struct sk_buff *next = msdu_head->next;
> 				struct sk_buff *to_free = next;
> 				int space;
> 				static int do_once = 1;
> 				msdu_head->next = NULL;
> 
> 				if (unlikely(do_once)) {
> 					ath10k_warn("htt rx msdu_chaining detected %d\n",
> 						    msdu_chaining);
> 					do_once = 0;
> 				}
> 
> 				while (next) {
> 					space = next->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;
> 					}
> 					skb_copy_from_linear_data(next, skb_put(msdu_head, next->len),
> 								  next->len);
> 					next = next->next;
> 				}

Yep, this is very inefficient , you might copy data very often in
pskb_expand_head(), then in skb_copy_from_linear_data()

Also, if total length is big, you end up doing high order allocations,
since you generate a linear skb. This might fail under memory pressure.

What is the maximal skb->len of resulting skb ?

Re: How to best consolidate list of skbs (msdu) for receive?

[Ben Greear]

On 02/27/2014 02:31 PM, Eric Dumazet wrote:
> On Thu, 2014-02-27 at 13:51 -0800, Ben Greear wrote:
> 
>> That said, the code below appears to work, even if it is
>> not as efficient as it might be?
>>
>> 			if (msdu_chaining) {
>> 				struct sk_buff *next = msdu_head->next;
>> 				struct sk_buff *to_free = next;
>> 				int space;
>> 				static int do_once = 1;
>> 				msdu_head->next = NULL;
>>
>> 				if (unlikely(do_once)) {
>> 					ath10k_warn("htt rx msdu_chaining detected %d\n",
>> 						    msdu_chaining);
>> 					do_once = 0;
>> 				}
>>
>> 				while (next) {
>> 					space = next->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;
>> 					}
>> 					skb_copy_from_linear_data(next, skb_put(msdu_head, next->len),
>> 								  next->len);
>> 					next = next->next;
>> 				}
> 
> Yep, this is very inefficient , you might copy data very often in
> pskb_expand_head(), then in skb_copy_from_linear_data()

Based on the code above, do you think skb_try_coalesce() is
the best option for this logic?

> Also, if total length is big, you end up doing high order allocations,
> since you generate a linear skb. This might fail under memory pressure.
> 
> What is the maximal skb->len of resulting skb ?

According to the page below, msdu are limited to 7935 total.  In my testing, I have seen a maximum
of 2 chained skbs (3 total).  I do not know the average total combined skb size...but since it
has upper bound of 8k, perhaps that doesn't matter so much?

http://ergodicthoughts.blogspot.com/2012/02/difference-between-mpdu-msdu-ampdu-and.html

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: How to best consolidate list of skbs (msdu) for receive?

[Eric Dumazet]

On Thu, 2014-02-27 at 14:38 -0800, Ben Greear wrote:

> Based on the code above, do you think skb_try_coalesce() is
> the best option for this logic?

Yep, if you do not know how are built the skb in the chain, I would say
using skb_try_coalesce() seems the proper way.

We use it for example in tcp input path.

Take also a look at tipc_link_recv_fragment()