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

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?

2014-02-27 10:36 GMT-08:00 Ben Greear <[email protected]>:

Quoted 4 lines

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.

Quoted 8 lines

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?

Quoted 14 lines · Florian Fainelli

2014-02-27 10:36 GMT-08:00 Ben Greear <[email protected]>:

Quoted 12 lines

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?

Quoted 5 lines

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

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;
				}

Quoted 20 lines · Ben Greear

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) &&

Quoted 8 lines

					    (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 ?

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