Return-Path: Date: Thu, 12 Aug 2010 17:11:02 -0700 (PDT) From: Mat Martineau To: "Gustavo F. Padovan" cc: linux-bluetooth@vger.kernel.org, marcel@holtmann.org, rshaffer@codeaurora.org Subject: Re: [RFC 6/7] Bluetooth: Reassemble enhanced L2CAP PDUs using skb fragments. In-Reply-To: <20100811052433.GA10773@vigoh> Message-ID: References: <1281467704-5378-1-git-send-email-mathewm@codeaurora.org> <1281467704-5378-7-git-send-email-mathewm@codeaurora.org> <20100811052433.GA10773@vigoh> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed List-ID: Gustavo - On Wed, 11 Aug 2010, Gustavo F. Padovan wrote: > Hi Mat, > > * Mat Martineau [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 >> --- >> 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