Gustavo - On Wed, 11 Aug 2010, Gustavo F. Padovan wrote:
Hi Mat, * Mat Martineau <mathewm@xxxxxxxxxxxxxx> [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@xxxxxxxxxxxxxx> --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
