Re: [RFC 6/7] Bluetooth: Reassemble enhanced L2CAP PDUs using skb fragments.

[Mat Martineau <mathewm@codeaurora.org>]

Gustavo -

On Wed, 11 Aug 2010, Gustavo F. Padovan wrote:

> Hi Mat,
>
> * Mat Martineau <mathewm@codeaurora.org> [2010-08-10 12:15:03 -0700]:
>
>> As enhanced L2CAP PDUs arrive, it is not necessary to copy them
>> in to a separate skbuff.  Instead, the skbuffs can be linked
>> together as fragments, only being copied in to a linear buffer
>> when the data is copied to userspace.  This avoids the need
>> to allocate additional buffers for incoming data, and
>> eliminates copying of data payloads during SDU reassembly.
>> This is of greater concern with high-speed AMP links than
>> with BR/EDR.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/l2cap.h |    1 +
>>  net/bluetooth/l2cap.c         |   66 +++++++++++++++++++++-------------------
>>  2 files changed, 36 insertions(+), 31 deletions(-)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 2f3222f..9384e87 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -357,6 +357,7 @@ struct l2cap_pinfo {
>>  	__u16		sdu_len;
>>  	__u16		partial_sdu_len;
>>  	struct sk_buff	*sdu;
>> +	struct sk_buff	*sdu_last_frag;
>>
>>  	__u8		ident;
>>
>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> index b485c4a..0212035 100644
>> --- a/net/bluetooth/l2cap.c
>> +++ b/net/bluetooth/l2cap.c
>> @@ -290,6 +290,9 @@ static void l2cap_chan_del(struct sock *sk, int err)
>>  		skb_queue_purge(SREJ_QUEUE(sk));
>>  		skb_queue_purge(BUSY_QUEUE(sk));
>>
>> +		if (l2cap_pi(sk)->sdu)
>> +			kfree_skb(l2cap_pi(sk)->sdu);
>> +
>>  		list_for_each_entry_safe(l, tmp, SREJ_LIST(sk), list) {
>>  			list_del(&l->list);
>>  			kfree(l);
>> @@ -3635,6 +3638,27 @@ static int l2cap_add_to_srej_queue(struct sock *sk, struct sk_buff *skb, u8 tx_s
>>  	return 0;
>>  }
>>
>> +static inline void append_skb_frag(struct sk_buff *skb,
>> +			struct sk_buff *new_frag, struct sk_buff **last_frag)
>
> Call this l2cap_append_skb_frag()

Ok.

>
>> +{
>> +	/* skb->len reflects data in skb as well as all fragments
>> +	   skb->data_len reflects only data in fragments
>> +	 */
>> +	BT_DBG("skb %p, new_frag %p, *last_frag %p", skb, new_frag, *last_frag);
>> +
>> +	if (!skb_has_frags(skb))
>> +		skb_shinfo(skb)->frag_list = new_frag;
>> +
>> +	new_frag->next = NULL;
>> +
>> +	(*last_frag)->next = new_frag;
>> +	*last_frag = new_frag;
>> +
>> +	skb->len += new_frag->len;
>> +	skb->data_len += new_frag->len;
>> +	skb->truesize += new_frag->truesize;
>> +}
>> +
>
> Wondering if it is possible to do that append more simple, but I looked
> through the kernel code it's not possible, we have to keep last frag
> pointer.
>
> This change should go to l2cap_streaming_reassembly_sdu() as well, then
> you can get ride of partial_sdu_len in the struct l2cap_pinfo.

I haven't had a chance to test streaming mode, so I was avoiding 
changes to it so far.  I agree that the same technique can be applied 
there.

>
>>  static int l2cap_ertm_reassembly_sdu(struct sock *sk, struct sk_buff *skb, u16 control)
>>  {
>>  	struct l2cap_pinfo *pi = l2cap_pi(sk);
>> @@ -3643,7 +3667,7 @@ static int l2cap_ertm_reassembly_sdu(struct sock *sk, struct sk_buff *skb, u16 c
>>
>>  	switch (control & L2CAP_CTRL_SAR) {
>>  	case L2CAP_SDU_UNSEGMENTED:
>> -		if (pi->conn_state & L2CAP_CONN_SAR_SDU)
>> +		if (pi->sdu)
>
> pi->sdu can do the work work of pi->conn_state & L2CAP_CONN_SAR_SDU, so
> a separated patch for that change sounds better (you can change that for
> the Streaming mode at the same time, it's a similar code, but more simple).
>

Ok.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum