* [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly
@ 2011-07-13 17:58 Mat Martineau
2011-07-13 17:58 ` [PATCH 1/3] Bluetooth: Linearize skbs for use in BNEP, CMTP, HIDP, and RFCOMM Mat Martineau
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Mat Martineau @ 2011-07-13 17:58 UTC (permalink / raw)
To: linux-bluetooth; +Cc: padovan, Mat Martineau
This patch series changes SDU reassembly to link together PDU skbs
using a frag list instead of copying all data bytes to a single,
linear skb. The reduction in overhead is especially helpful with high
speed AMP links.
Mat Martineau (3):
Bluetooth: Linearize skbs for use in BNEP, CMTP, HIDP, and RFCOMM
Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg()
Bluetooth: Perform L2CAP SDU reassembly without copying data
include/net/bluetooth/l2cap.h | 3 +-
net/bluetooth/af_bluetooth.c | 30 +++++-
net/bluetooth/bnep/core.c | 5 +-
net/bluetooth/cmtp/core.c | 5 +-
net/bluetooth/hidp/core.c | 10 ++-
net/bluetooth/l2cap_core.c | 239 ++++++++++++++---------------------------
net/bluetooth/rfcomm/core.c | 5 +-
7 files changed, 128 insertions(+), 169 deletions(-)
--
1.7.6
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] Bluetooth: Linearize skbs for use in BNEP, CMTP, HIDP, and RFCOMM 2011-07-13 17:58 [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly Mat Martineau @ 2011-07-13 17:58 ` Mat Martineau 2011-07-13 17:58 ` [PATCH 2/3] Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg() Mat Martineau ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: Mat Martineau @ 2011-07-13 17:58 UTC (permalink / raw) To: linux-bluetooth; +Cc: padovan, Mat Martineau Fragmented skbs are only encountered when receiving ERTM or streaming mode L2CAP data. BNEP, CMTP, HIDP, and RFCOMM generally use basic mode, but they need to handle fragments without crashing. Signed-off-by: Mat Martineau <mathewm@codeaurora.org> --- net/bluetooth/bnep/core.c | 5 ++++- net/bluetooth/cmtp/core.c | 5 ++++- net/bluetooth/hidp/core.c | 10 ++++++++-- net/bluetooth/rfcomm/core.c | 5 ++++- 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c index ca39fcf..dfadb65 100644 --- a/net/bluetooth/bnep/core.c +++ b/net/bluetooth/bnep/core.c @@ -490,7 +490,10 @@ static int bnep_session(void *arg) /* RX */ while ((skb = skb_dequeue(&sk->sk_receive_queue))) { skb_orphan(skb); - bnep_rx_frame(s, skb); + if (!skb_linearize(skb)) + bnep_rx_frame(s, skb); + else + kfree_skb(skb); } if (sk->sk_state != BT_CONNECTED) diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c index c5b11af..2487b84 100644 --- a/net/bluetooth/cmtp/core.c +++ b/net/bluetooth/cmtp/core.c @@ -300,7 +300,10 @@ static int cmtp_session(void *arg) while ((skb = skb_dequeue(&sk->sk_receive_queue))) { skb_orphan(skb); - cmtp_recv_frame(session, skb); + if (!skb_linearize(skb)) + cmtp_recv_frame(session, skb); + else + kfree_skb(skb); } cmtp_process_transmit(session); diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c index c405a95..2506509 100644 --- a/net/bluetooth/hidp/core.c +++ b/net/bluetooth/hidp/core.c @@ -715,12 +715,18 @@ static int hidp_session(void *arg) while ((skb = skb_dequeue(&ctrl_sk->sk_receive_queue))) { skb_orphan(skb); - hidp_recv_ctrl_frame(session, skb); + if (!skb_linearize(skb)) + hidp_recv_ctrl_frame(session, skb); + else + kfree_skb(skb); } while ((skb = skb_dequeue(&intr_sk->sk_receive_queue))) { skb_orphan(skb); - hidp_recv_intr_frame(session, skb); + if (!skb_linearize(skb)) + hidp_recv_intr_frame(session, skb); + else + kfree_skb(skb); } hidp_process_transmit(session); diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c index 5759bb7..6bbd317 100644 --- a/net/bluetooth/rfcomm/core.c +++ b/net/bluetooth/rfcomm/core.c @@ -1855,7 +1855,10 @@ static inline void rfcomm_process_rx(struct rfcomm_session *s) /* Get data directly from socket receive queue without copying it. */ while ((skb = skb_dequeue(&sk->sk_receive_queue))) { skb_orphan(skb); - rfcomm_recv_frame(s, skb); + if (!skb_linearize(skb)) + rfcomm_recv_frame(s, skb); + else + kfree_skb(skb); } if (sk->sk_state == BT_CLOSED) { -- 1.7.6 -- Mat Martineau Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg() 2011-07-13 17:58 [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly Mat Martineau 2011-07-13 17:58 ` [PATCH 1/3] Bluetooth: Linearize skbs for use in BNEP, CMTP, HIDP, and RFCOMM Mat Martineau @ 2011-07-13 17:58 ` Mat Martineau 2011-07-22 16:43 ` Gustavo Padovan 2011-07-13 17:58 ` [PATCH 3/3] Bluetooth: Perform L2CAP SDU reassembly without copying data Mat Martineau 2011-07-18 20:02 ` [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly Mat Martineau 3 siblings, 1 reply; 11+ messages in thread From: Mat Martineau @ 2011-07-13 17:58 UTC (permalink / raw) To: linux-bluetooth; +Cc: padovan, Mat Martineau ERTM reassembly will be more efficient when skbs are linked together rather than copying every incoming data byte. The existing stream recv function assumes skbs are linear, so it needs to know how to handle fragments before reassembly is changed. bt_sock_recvmsg() already handles fragmented skbs. Signed-off-by: Mat Martineau <mathewm@codeaurora.org> --- net/bluetooth/af_bluetooth.c | 30 ++++++++++++++++++++++++++++-- 1 files changed, 28 insertions(+), 2 deletions(-) diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c index 8add9b4..9a43520 100644 --- a/net/bluetooth/af_bluetooth.c +++ b/net/bluetooth/af_bluetooth.c @@ -349,7 +349,7 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock, } chunk = min_t(unsigned int, skb->len, size); - if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) { + if (skb_copy_datagram_iovec(skb, 0, msg->msg_iov, chunk)) { skb_queue_head(&sk->sk_receive_queue, skb); if (!copied) copied = -EFAULT; @@ -361,7 +361,33 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock, sock_recv_ts_and_drops(msg, sk, skb); if (!(flags & MSG_PEEK)) { - skb_pull(skb, chunk); + int skb_len = skb_headlen(skb); + + if (chunk <= skb_len) { + __skb_pull(skb, chunk); + } else { + struct sk_buff *frag; + + __skb_pull(skb, skb_len); + chunk -= skb_len; + + skb_walk_frags(skb, frag) { + if (chunk <= frag->len) { + /* Pulling partial data */ + skb->len -= chunk; + skb->data_len -= chunk; + __skb_pull(frag, chunk); + break; + } else if (frag->len) { + /* Pulling all frag data */ + chunk -= frag->len; + skb->len -= frag->len; + skb->data_len -= frag->len; + __skb_pull(frag, frag->len); + } + } + } + if (skb->len) { skb_queue_head(&sk->sk_receive_queue, skb); break; -- 1.7.6 -- Mat Martineau Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg() 2011-07-13 17:58 ` [PATCH 2/3] Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg() Mat Martineau @ 2011-07-22 16:43 ` Gustavo Padovan 2011-07-22 19:20 ` Mat Martineau 0 siblings, 1 reply; 11+ messages in thread From: Gustavo Padovan @ 2011-07-22 16:43 UTC (permalink / raw) To: Mat Martineau; +Cc: linux-bluetooth * Mat Martineau <mathewm@codeaurora.org> [2011-07-13 10:58:38 -0700]: > ERTM reassembly will be more efficient when skbs are linked together > rather than copying every incoming data byte. The existing stream recv > function assumes skbs are linear, so it needs to know how to handle > fragments before reassembly is changed. > > bt_sock_recvmsg() already handles fragmented skbs. > > Signed-off-by: Mat Martineau <mathewm@codeaurora.org> > --- > net/bluetooth/af_bluetooth.c | 30 ++++++++++++++++++++++++++++-- > 1 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c > index 8add9b4..9a43520 100644 > --- a/net/bluetooth/af_bluetooth.c > +++ b/net/bluetooth/af_bluetooth.c > @@ -349,7 +349,7 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock, > } > > chunk = min_t(unsigned int, skb->len, size); > - if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) { > + if (skb_copy_datagram_iovec(skb, 0, msg->msg_iov, chunk)) { > skb_queue_head(&sk->sk_receive_queue, skb); > if (!copied) > copied = -EFAULT; > @@ -361,7 +361,33 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock, > sock_recv_ts_and_drops(msg, sk, skb); > > if (!(flags & MSG_PEEK)) { > - skb_pull(skb, chunk); > + int skb_len = skb_headlen(skb); > + > + if (chunk <= skb_len) { > + __skb_pull(skb, chunk); > + } else { > + struct sk_buff *frag; > + > + __skb_pull(skb, skb_len); > + chunk -= skb_len; > + > + skb_walk_frags(skb, frag) { > + if (chunk <= frag->len) { > + /* Pulling partial data */ > + skb->len -= chunk; > + skb->data_len -= chunk; > + __skb_pull(frag, chunk); > + break; > + } else if (frag->len) { > + /* Pulling all frag data */ > + chunk -= frag->len; > + skb->len -= frag->len; > + skb->data_len -= frag->len; > + __skb_pull(frag, frag->len); > + } > + } > + } > + ERTM and Streaming mode can also use SOCK_SEQPACKET, I think you also need to handle this for SOCK_SEQPACKET. Gustavo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg() 2011-07-22 16:43 ` Gustavo Padovan @ 2011-07-22 19:20 ` Mat Martineau 0 siblings, 0 replies; 11+ messages in thread From: Mat Martineau @ 2011-07-22 19:20 UTC (permalink / raw) To: Gustavo Padovan; +Cc: linux-bluetooth Hi Gustavo - On Fri, 22 Jul 2011, Gustavo Padovan wrote: > * Mat Martineau <mathewm@codeaurora.org> [2011-07-13 10:58:38 -0700]: > >> ERTM reassembly will be more efficient when skbs are linked together >> rather than copying every incoming data byte. The existing stream recv >> function assumes skbs are linear, so it needs to know how to handle >> fragments before reassembly is changed. >> >> bt_sock_recvmsg() already handles fragmented skbs. >> >> Signed-off-by: Mat Martineau <mathewm@codeaurora.org> >> --- >> net/bluetooth/af_bluetooth.c | 30 ++++++++++++++++++++++++++++-- >> 1 files changed, 28 insertions(+), 2 deletions(-) >> >> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c >> index 8add9b4..9a43520 100644 >> --- a/net/bluetooth/af_bluetooth.c >> +++ b/net/bluetooth/af_bluetooth.c >> @@ -349,7 +349,7 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock, >> } >> >> chunk = min_t(unsigned int, skb->len, size); >> - if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) { >> + if (skb_copy_datagram_iovec(skb, 0, msg->msg_iov, chunk)) { >> skb_queue_head(&sk->sk_receive_queue, skb); >> if (!copied) >> copied = -EFAULT; >> @@ -361,7 +361,33 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock, >> sock_recv_ts_and_drops(msg, sk, skb); >> >> if (!(flags & MSG_PEEK)) { >> - skb_pull(skb, chunk); >> + int skb_len = skb_headlen(skb); >> + >> + if (chunk <= skb_len) { >> + __skb_pull(skb, chunk); >> + } else { >> + struct sk_buff *frag; >> + >> + __skb_pull(skb, skb_len); >> + chunk -= skb_len; >> + >> + skb_walk_frags(skb, frag) { >> + if (chunk <= frag->len) { >> + /* Pulling partial data */ >> + skb->len -= chunk; >> + skb->data_len -= chunk; >> + __skb_pull(frag, chunk); >> + break; >> + } else if (frag->len) { >> + /* Pulling all frag data */ >> + chunk -= frag->len; >> + skb->len -= frag->len; >> + skb->data_len -= frag->len; >> + __skb_pull(frag, frag->len); >> + } >> + } >> + } >> + > > ERTM and Streaming mode can also use SOCK_SEQPACKET, I think you also need to > handle this for SOCK_SEQPACKET. bt_sock_recvmsg() is used with SOCK_SEQPACKET, and it already handles fragmented skbs without any changes. See skb_copy_datagram_iovec(), which is called by bt_sock_recvmsg() to copy the data out of the skb. -- Mat Martineau Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] Bluetooth: Perform L2CAP SDU reassembly without copying data 2011-07-13 17:58 [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly Mat Martineau 2011-07-13 17:58 ` [PATCH 1/3] Bluetooth: Linearize skbs for use in BNEP, CMTP, HIDP, and RFCOMM Mat Martineau 2011-07-13 17:58 ` [PATCH 2/3] Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg() Mat Martineau @ 2011-07-13 17:58 ` Mat Martineau 2011-07-22 16:46 ` Gustavo Padovan 2011-07-18 20:02 ` [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly Mat Martineau 3 siblings, 1 reply; 11+ messages in thread From: Mat Martineau @ 2011-07-13 17:58 UTC (permalink / raw) To: linux-bluetooth; +Cc: padovan, Mat Martineau Use sk_buff fragment capabilities to link together incoming skbs instead of allocating a new skb for reassembly and copying. The new reassembly code works equally well for ERTM and streaming mode, so there is now one reassembly function instead of two. Signed-off-by: Mat Martineau <mathewm@codeaurora.org> --- include/net/bluetooth/l2cap.h | 3 +- net/bluetooth/l2cap_core.c | 239 ++++++++++++++--------------------------- 2 files changed, 80 insertions(+), 162 deletions(-) diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h index 4f34ad2..6fa1140 100644 --- a/include/net/bluetooth/l2cap.h +++ b/include/net/bluetooth/l2cap.h @@ -354,8 +354,8 @@ struct l2cap_chan { __u8 retry_count; __u8 num_acked; __u16 sdu_len; - __u16 partial_sdu_len; struct sk_buff *sdu; + struct sk_buff *sdu_last_frag; __u8 remote_tx_win; __u8 remote_max_tx; @@ -454,7 +454,6 @@ enum { #define L2CAP_CONF_MAX_CONF_RSP 2 enum { - CONN_SAR_SDU, CONN_SREJ_SENT, CONN_WAIT_F, CONN_SREJ_ACT, diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index 52c791e..e82fff1 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -3120,102 +3120,104 @@ static int l2cap_add_to_srej_queue(struct l2cap_chan *chan, struct sk_buff *skb, return 0; } -static int l2cap_ertm_reassembly_sdu(struct l2cap_chan *chan, struct sk_buff *skb, u16 control) +static void append_skb_frag(struct sk_buff *skb, + struct sk_buff *new_frag, struct sk_buff **last_frag) { - struct sk_buff *_skb; - int err; + /* skb->len reflects data in skb as well as all fragments + * skb->data_len reflects only data in fragments + */ + if (!skb_has_frag_list(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; +} + +static int l2cap_reassemble_sdu(struct l2cap_chan *chan, struct sk_buff *skb, u16 control) +{ + int err = -EINVAL; switch (control & L2CAP_CTRL_SAR) { case L2CAP_SDU_UNSEGMENTED: - if (test_bit(CONN_SAR_SDU, &chan->conn_state)) - goto drop; + if (chan->sdu) + break; - return chan->ops->recv(chan->data, skb); + err = chan->ops->recv(chan->data, skb); + break; case L2CAP_SDU_START: - if (test_bit(CONN_SAR_SDU, &chan->conn_state)) - goto drop; + if (chan->sdu) + break; chan->sdu_len = get_unaligned_le16(skb->data); + skb_pull(skb, 2); - if (chan->sdu_len > chan->imtu) - goto disconnect; - - chan->sdu = bt_skb_alloc(chan->sdu_len, GFP_ATOMIC); - if (!chan->sdu) - return -ENOMEM; + if (chan->sdu_len > chan->imtu) { + err = -EMSGSIZE; + break; + } - /* pull sdu_len bytes only after alloc, because of Local Busy - * condition we have to be sure that this will be executed - * only once, i.e., when alloc does not fail */ - skb_pull(skb, 2); + if (skb->len >= chan->sdu_len) + break; - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len); + chan->sdu = skb; + chan->sdu_last_frag = skb; - set_bit(CONN_SAR_SDU, &chan->conn_state); - chan->partial_sdu_len = skb->len; + skb = NULL; + err = 0; break; case L2CAP_SDU_CONTINUE: - if (!test_bit(CONN_SAR_SDU, &chan->conn_state)) - goto disconnect; - if (!chan->sdu) - goto disconnect; + break; - chan->partial_sdu_len += skb->len; - if (chan->partial_sdu_len > chan->sdu_len) - goto drop; + append_skb_frag(chan->sdu, skb, + &chan->sdu_last_frag); + skb = NULL; - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len); + if (chan->sdu->len >= chan->sdu_len) + break; + err = 0; break; case L2CAP_SDU_END: - if (!test_bit(CONN_SAR_SDU, &chan->conn_state)) - goto disconnect; - if (!chan->sdu) - goto disconnect; - - chan->partial_sdu_len += skb->len; - - if (chan->partial_sdu_len > chan->imtu) - goto drop; + break; - if (chan->partial_sdu_len != chan->sdu_len) - goto drop; + append_skb_frag(chan->sdu, skb, + &chan->sdu_last_frag); + skb = NULL; - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len); + if (chan->sdu->len != chan->sdu_len) + break; - _skb = skb_clone(chan->sdu, GFP_ATOMIC); - if (!_skb) { - return -ENOMEM; - } + err = chan->ops->recv(chan->data, chan->sdu); - err = chan->ops->recv(chan->data, _skb); - if (err < 0) { - kfree_skb(_skb); - return err; + if (!err) { + /* Reassembly complete */ + chan->sdu = NULL; + chan->sdu_last_frag = NULL; + chan->sdu_len = 0; } - - clear_bit(CONN_SAR_SDU, &chan->conn_state); - - kfree_skb(chan->sdu); break; } - kfree_skb(skb); - return 0; - -drop: - kfree_skb(chan->sdu); - chan->sdu = NULL; + if (err) { + kfree_skb(skb); + kfree_skb(chan->sdu); + chan->sdu = NULL; + chan->sdu_last_frag = NULL; + chan->sdu_len = 0; + } -disconnect: - l2cap_send_disconn_req(chan->conn, chan, ECONNRESET); - kfree_skb(skb); - return 0; + return err; } static void l2cap_ertm_enter_local_busy(struct l2cap_chan *chan) @@ -3269,99 +3271,6 @@ void l2cap_chan_busy(struct l2cap_chan *chan, int busy) } } -static int l2cap_streaming_reassembly_sdu(struct l2cap_chan *chan, struct sk_buff *skb, u16 control) -{ - struct sk_buff *_skb; - int err = -EINVAL; - - /* - * TODO: We have to notify the userland if some data is lost with the - * Streaming Mode. - */ - - switch (control & L2CAP_CTRL_SAR) { - case L2CAP_SDU_UNSEGMENTED: - if (test_bit(CONN_SAR_SDU, &chan->conn_state)) { - kfree_skb(chan->sdu); - break; - } - - err = chan->ops->recv(chan->data, skb); - if (!err) - return 0; - - break; - - case L2CAP_SDU_START: - if (test_bit(CONN_SAR_SDU, &chan->conn_state)) { - kfree_skb(chan->sdu); - break; - } - - chan->sdu_len = get_unaligned_le16(skb->data); - skb_pull(skb, 2); - - if (chan->sdu_len > chan->imtu) { - err = -EMSGSIZE; - break; - } - - chan->sdu = bt_skb_alloc(chan->sdu_len, GFP_ATOMIC); - if (!chan->sdu) { - err = -ENOMEM; - break; - } - - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len); - - set_bit(CONN_SAR_SDU, &chan->conn_state); - chan->partial_sdu_len = skb->len; - err = 0; - break; - - case L2CAP_SDU_CONTINUE: - if (!test_bit(CONN_SAR_SDU, &chan->conn_state)) - break; - - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len); - - chan->partial_sdu_len += skb->len; - if (chan->partial_sdu_len > chan->sdu_len) - kfree_skb(chan->sdu); - else - err = 0; - - break; - - case L2CAP_SDU_END: - if (!test_bit(CONN_SAR_SDU, &chan->conn_state)) - break; - - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len); - - clear_bit(CONN_SAR_SDU, &chan->conn_state); - chan->partial_sdu_len += skb->len; - - if (chan->partial_sdu_len > chan->imtu) - goto drop; - - if (chan->partial_sdu_len == chan->sdu_len) { - _skb = skb_clone(chan->sdu, GFP_ATOMIC); - err = chan->ops->recv(chan->data, _skb); - if (err < 0) - kfree_skb(_skb); - } - err = 0; - -drop: - kfree_skb(chan->sdu); - break; - } - - kfree_skb(skb); - return err; -} - static void l2cap_check_srej_gap(struct l2cap_chan *chan, u8 tx_seq) { struct sk_buff *skb; @@ -3376,7 +3285,7 @@ static void l2cap_check_srej_gap(struct l2cap_chan *chan, u8 tx_seq) skb = skb_dequeue(&chan->srej_q); control = bt_cb(skb)->sar << L2CAP_CTRL_SAR_SHIFT; - err = l2cap_ertm_reassembly_sdu(chan, skb, control); + err = l2cap_reassemble_sdu(chan, skb, control); if (err < 0) { l2cap_send_disconn_req(chan->conn, chan, ECONNRESET); @@ -3536,7 +3445,7 @@ expected: return 0; } - err = l2cap_ertm_reassembly_sdu(chan, skb, rx_control); + err = l2cap_reassemble_sdu(chan, skb, rx_control); chan->buffer_seq = (chan->buffer_seq + 1) % 64; if (err < 0) { l2cap_send_disconn_req(chan->conn, chan, ECONNRESET); @@ -3854,10 +3763,20 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk if (chan->expected_tx_seq == tx_seq) chan->expected_tx_seq = (chan->expected_tx_seq + 1) % 64; - else + else { chan->expected_tx_seq = (tx_seq + 1) % 64; - l2cap_streaming_reassembly_sdu(chan, skb, control); + /* Frame(s) missing - must discard partial SDU */ + kfree_skb(chan->sdu); + chan->sdu = NULL; + chan->sdu_last_frag = NULL; + chan->sdu_len = 0; + + /* TODO: Notify userland of missing data */ + } + + if (l2cap_reassemble_sdu(chan, skb, control) == -EMSGSIZE) + l2cap_send_disconn_req(chan->conn, chan, ECONNRESET); goto done; -- 1.7.6 -- Mat Martineau Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] Bluetooth: Perform L2CAP SDU reassembly without copying data 2011-07-13 17:58 ` [PATCH 3/3] Bluetooth: Perform L2CAP SDU reassembly without copying data Mat Martineau @ 2011-07-22 16:46 ` Gustavo Padovan 0 siblings, 0 replies; 11+ messages in thread From: Gustavo Padovan @ 2011-07-22 16:46 UTC (permalink / raw) To: Mat Martineau; +Cc: linux-bluetooth Hi Mat, * Mat Martineau <mathewm@codeaurora.org> [2011-07-13 10:58:39 -0700]: > Use sk_buff fragment capabilities to link together incoming skbs > instead of allocating a new skb for reassembly and copying. > > The new reassembly code works equally well for ERTM and streaming > mode, so there is now one reassembly function instead of two. > > Signed-off-by: Mat Martineau <mathewm@codeaurora.org> > --- > include/net/bluetooth/l2cap.h | 3 +- > net/bluetooth/l2cap_core.c | 239 ++++++++++++++--------------------------- > 2 files changed, 80 insertions(+), 162 deletions(-) > > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h > index 4f34ad2..6fa1140 100644 > --- a/include/net/bluetooth/l2cap.h > +++ b/include/net/bluetooth/l2cap.h > @@ -354,8 +354,8 @@ struct l2cap_chan { > __u8 retry_count; > __u8 num_acked; > __u16 sdu_len; > - __u16 partial_sdu_len; > struct sk_buff *sdu; > + struct sk_buff *sdu_last_frag; > > __u8 remote_tx_win; > __u8 remote_max_tx; > @@ -454,7 +454,6 @@ enum { > #define L2CAP_CONF_MAX_CONF_RSP 2 > > enum { > - CONN_SAR_SDU, > CONN_SREJ_SENT, > CONN_WAIT_F, > CONN_SREJ_ACT, > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index 52c791e..e82fff1 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -3120,102 +3120,104 @@ static int l2cap_add_to_srej_queue(struct l2cap_chan *chan, struct sk_buff *skb, > return 0; > } > > -static int l2cap_ertm_reassembly_sdu(struct l2cap_chan *chan, struct sk_buff *skb, u16 control) > +static void append_skb_frag(struct sk_buff *skb, > + struct sk_buff *new_frag, struct sk_buff **last_frag) > { > - struct sk_buff *_skb; > - int err; > + /* skb->len reflects data in skb as well as all fragments > + * skb->data_len reflects only data in fragments > + */ > + if (!skb_has_frag_list(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; > +} > + > +static int l2cap_reassemble_sdu(struct l2cap_chan *chan, struct sk_buff *skb, u16 control) > +{ > + int err = -EINVAL; > > switch (control & L2CAP_CTRL_SAR) { > case L2CAP_SDU_UNSEGMENTED: > - if (test_bit(CONN_SAR_SDU, &chan->conn_state)) > - goto drop; > + if (chan->sdu) > + break; > > - return chan->ops->recv(chan->data, skb); > + err = chan->ops->recv(chan->data, skb); > + break; > > case L2CAP_SDU_START: > - if (test_bit(CONN_SAR_SDU, &chan->conn_state)) > - goto drop; > + if (chan->sdu) > + break; > > chan->sdu_len = get_unaligned_le16(skb->data); > + skb_pull(skb, 2); > > - if (chan->sdu_len > chan->imtu) > - goto disconnect; > - > - chan->sdu = bt_skb_alloc(chan->sdu_len, GFP_ATOMIC); > - if (!chan->sdu) > - return -ENOMEM; > + if (chan->sdu_len > chan->imtu) { > + err = -EMSGSIZE; > + break; > + } > > - /* pull sdu_len bytes only after alloc, because of Local Busy > - * condition we have to be sure that this will be executed > - * only once, i.e., when alloc does not fail */ > - skb_pull(skb, 2); > + if (skb->len >= chan->sdu_len) > + break; > > - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len); > + chan->sdu = skb; > + chan->sdu_last_frag = skb; > > - set_bit(CONN_SAR_SDU, &chan->conn_state); > - chan->partial_sdu_len = skb->len; > + skb = NULL; > + err = 0; > break; > > case L2CAP_SDU_CONTINUE: > - if (!test_bit(CONN_SAR_SDU, &chan->conn_state)) > - goto disconnect; > - > if (!chan->sdu) > - goto disconnect; > + break; > > - chan->partial_sdu_len += skb->len; > - if (chan->partial_sdu_len > chan->sdu_len) > - goto drop; > + append_skb_frag(chan->sdu, skb, > + &chan->sdu_last_frag); > + skb = NULL; > > - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len); > + if (chan->sdu->len >= chan->sdu_len) > + break; > > + err = 0; > break; > > case L2CAP_SDU_END: > - if (!test_bit(CONN_SAR_SDU, &chan->conn_state)) > - goto disconnect; > - > if (!chan->sdu) > - goto disconnect; > - > - chan->partial_sdu_len += skb->len; > - > - if (chan->partial_sdu_len > chan->imtu) > - goto drop; > + break; > > - if (chan->partial_sdu_len != chan->sdu_len) > - goto drop; > + append_skb_frag(chan->sdu, skb, > + &chan->sdu_last_frag); > + skb = NULL; > > - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len); > + if (chan->sdu->len != chan->sdu_len) > + break; > > - _skb = skb_clone(chan->sdu, GFP_ATOMIC); > - if (!_skb) { > - return -ENOMEM; > - } > + err = chan->ops->recv(chan->data, chan->sdu); > > - err = chan->ops->recv(chan->data, _skb); > - if (err < 0) { > - kfree_skb(_skb); > - return err; > + if (!err) { > + /* Reassembly complete */ > + chan->sdu = NULL; > + chan->sdu_last_frag = NULL; > + chan->sdu_len = 0; > } > - > - clear_bit(CONN_SAR_SDU, &chan->conn_state); > - > - kfree_skb(chan->sdu); > break; > } > > - kfree_skb(skb); > - return 0; > - > -drop: > - kfree_skb(chan->sdu); > - chan->sdu = NULL; > + if (err) { > + kfree_skb(skb); > + kfree_skb(chan->sdu); > + chan->sdu = NULL; > + chan->sdu_last_frag = NULL; > + chan->sdu_len = 0; > + } > > -disconnect: > - l2cap_send_disconn_req(chan->conn, chan, ECONNRESET); > - kfree_skb(skb); > - return 0; > + return err; > } > > static void l2cap_ertm_enter_local_busy(struct l2cap_chan *chan) > @@ -3269,99 +3271,6 @@ void l2cap_chan_busy(struct l2cap_chan *chan, int busy) > } > } > > -static int l2cap_streaming_reassembly_sdu(struct l2cap_chan *chan, struct sk_buff *skb, u16 control) > -{ > - struct sk_buff *_skb; > - int err = -EINVAL; > - > - /* > - * TODO: We have to notify the userland if some data is lost with the > - * Streaming Mode. > - */ > - > - switch (control & L2CAP_CTRL_SAR) { > - case L2CAP_SDU_UNSEGMENTED: > - if (test_bit(CONN_SAR_SDU, &chan->conn_state)) { > - kfree_skb(chan->sdu); > - break; > - } > - > - err = chan->ops->recv(chan->data, skb); > - if (!err) > - return 0; > - > - break; > - > - case L2CAP_SDU_START: > - if (test_bit(CONN_SAR_SDU, &chan->conn_state)) { > - kfree_skb(chan->sdu); > - break; > - } > - > - chan->sdu_len = get_unaligned_le16(skb->data); > - skb_pull(skb, 2); > - > - if (chan->sdu_len > chan->imtu) { > - err = -EMSGSIZE; > - break; > - } > - > - chan->sdu = bt_skb_alloc(chan->sdu_len, GFP_ATOMIC); > - if (!chan->sdu) { > - err = -ENOMEM; > - break; > - } > - > - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len); > - > - set_bit(CONN_SAR_SDU, &chan->conn_state); > - chan->partial_sdu_len = skb->len; > - err = 0; > - break; > - > - case L2CAP_SDU_CONTINUE: > - if (!test_bit(CONN_SAR_SDU, &chan->conn_state)) > - break; > - > - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len); > - > - chan->partial_sdu_len += skb->len; > - if (chan->partial_sdu_len > chan->sdu_len) > - kfree_skb(chan->sdu); > - else > - err = 0; > - > - break; > - > - case L2CAP_SDU_END: > - if (!test_bit(CONN_SAR_SDU, &chan->conn_state)) > - break; > - > - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len); > - > - clear_bit(CONN_SAR_SDU, &chan->conn_state); > - chan->partial_sdu_len += skb->len; > - > - if (chan->partial_sdu_len > chan->imtu) > - goto drop; > - > - if (chan->partial_sdu_len == chan->sdu_len) { > - _skb = skb_clone(chan->sdu, GFP_ATOMIC); > - err = chan->ops->recv(chan->data, _skb); > - if (err < 0) > - kfree_skb(_skb); > - } > - err = 0; > - > -drop: > - kfree_skb(chan->sdu); > - break; > - } > - > - kfree_skb(skb); > - return err; > -} > - > static void l2cap_check_srej_gap(struct l2cap_chan *chan, u8 tx_seq) > { > struct sk_buff *skb; > @@ -3376,7 +3285,7 @@ static void l2cap_check_srej_gap(struct l2cap_chan *chan, u8 tx_seq) > > skb = skb_dequeue(&chan->srej_q); > control = bt_cb(skb)->sar << L2CAP_CTRL_SAR_SHIFT; > - err = l2cap_ertm_reassembly_sdu(chan, skb, control); > + err = l2cap_reassemble_sdu(chan, skb, control); > > if (err < 0) { > l2cap_send_disconn_req(chan->conn, chan, ECONNRESET); > @@ -3536,7 +3445,7 @@ expected: > return 0; > } > > - err = l2cap_ertm_reassembly_sdu(chan, skb, rx_control); > + err = l2cap_reassemble_sdu(chan, skb, rx_control); > chan->buffer_seq = (chan->buffer_seq + 1) % 64; > if (err < 0) { > l2cap_send_disconn_req(chan->conn, chan, ECONNRESET); > @@ -3854,10 +3763,20 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk > > if (chan->expected_tx_seq == tx_seq) > chan->expected_tx_seq = (chan->expected_tx_seq + 1) % 64; Coding style issue: you also need {} in the 'if'. If you need it on 'else' then also add it to 'if.' > - else > + else { Gustavo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly 2011-07-13 17:58 [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly Mat Martineau ` (2 preceding siblings ...) 2011-07-13 17:58 ` [PATCH 3/3] Bluetooth: Perform L2CAP SDU reassembly without copying data Mat Martineau @ 2011-07-18 20:02 ` Mat Martineau 2011-07-18 20:33 ` Marcel Holtmann 3 siblings, 1 reply; 11+ messages in thread From: Mat Martineau @ 2011-07-18 20:02 UTC (permalink / raw) To: padovan; +Cc: linux-bluetooth Gustavo - On Wed, 13 Jul 2011, Mat Martineau wrote: > This patch series changes SDU reassembly to link together PDU skbs > using a frag list instead of copying all data bytes to a single, > linear skb. The reduction in overhead is especially helpful with high > speed AMP links. > > > Mat Martineau (3): > Bluetooth: Linearize skbs for use in BNEP, CMTP, HIDP, and RFCOMM > Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg() > Bluetooth: Perform L2CAP SDU reassembly without copying data > > include/net/bluetooth/l2cap.h | 3 +- > net/bluetooth/af_bluetooth.c | 30 +++++- > net/bluetooth/bnep/core.c | 5 +- > net/bluetooth/cmtp/core.c | 5 +- > net/bluetooth/hidp/core.c | 10 ++- > net/bluetooth/l2cap_core.c | 239 ++++++++++++++--------------------------- > net/bluetooth/rfcomm/core.c | 5 +- > 7 files changed, 128 insertions(+), 169 deletions(-) > > -- > 1.7.6 Have you had a chance to look at these patches? Thanks, -- Mat Martineau Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly 2011-07-18 20:02 ` [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly Mat Martineau @ 2011-07-18 20:33 ` Marcel Holtmann 2011-07-18 22:18 ` Mat Martineau 0 siblings, 1 reply; 11+ messages in thread From: Marcel Holtmann @ 2011-07-18 20:33 UTC (permalink / raw) To: Mat Martineau; +Cc: padovan, linux-bluetooth Hi Mat, > > This patch series changes SDU reassembly to link together PDU skbs > > using a frag list instead of copying all data bytes to a single, > > linear skb. The reduction in overhead is especially helpful with high > > speed AMP links. I have not looked at these at all, but one thing that came up in a discussion between Johan and myself with OBEX zero-copy was that we like to also see MSG_MORE support for L2CAP and RFCOMM sockets. Something to keep in mind ;) Regards Marcel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly 2011-07-18 20:33 ` Marcel Holtmann @ 2011-07-18 22:18 ` Mat Martineau 2011-07-18 22:43 ` Marcel Holtmann 0 siblings, 1 reply; 11+ messages in thread From: Mat Martineau @ 2011-07-18 22:18 UTC (permalink / raw) To: Marcel Holtmann; +Cc: padovan, linux-bluetooth Hi Marcel - On Mon, 18 Jul 2011, Marcel Holtmann wrote: > Hi Mat, > >>> This patch series changes SDU reassembly to link together PDU skbs >>> using a frag list instead of copying all data bytes to a single, >>> linear skb. The reduction in overhead is especially helpful with high >>> speed AMP links. > > I have not looked at these at all, but one thing that came up in a > discussion between Johan and myself with OBEX zero-copy was that we like > to also see MSG_MORE support for L2CAP and RFCOMM sockets. Something to > keep in mind ;) Noted! It would be nice to build up OBEX packets in the kernel, rather than having to assemble entire packets in an application buffer. -- Mat Martineau Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly 2011-07-18 22:18 ` Mat Martineau @ 2011-07-18 22:43 ` Marcel Holtmann 0 siblings, 0 replies; 11+ messages in thread From: Marcel Holtmann @ 2011-07-18 22:43 UTC (permalink / raw) To: Mat Martineau; +Cc: padovan, linux-bluetooth Hi Mat, > >>> This patch series changes SDU reassembly to link together PDU skbs > >>> using a frag list instead of copying all data bytes to a single, > >>> linear skb. The reduction in overhead is especially helpful with high > >>> speed AMP links. > > > > I have not looked at these at all, but one thing that came up in a > > discussion between Johan and myself with OBEX zero-copy was that we like > > to also see MSG_MORE support for L2CAP and RFCOMM sockets. Something to > > keep in mind ;) > > Noted! It would be nice to build up OBEX packets in the kernel, > rather than having to assemble entire packets in an application > buffer. hence the request for MSG_MORE support. That would allow us to build frames properly. Could also become a pretty nice enhancement for A2DP. Regards Marcel ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-07-22 19:20 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-07-13 17:58 [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly Mat Martineau 2011-07-13 17:58 ` [PATCH 1/3] Bluetooth: Linearize skbs for use in BNEP, CMTP, HIDP, and RFCOMM Mat Martineau 2011-07-13 17:58 ` [PATCH 2/3] Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg() Mat Martineau 2011-07-22 16:43 ` Gustavo Padovan 2011-07-22 19:20 ` Mat Martineau 2011-07-13 17:58 ` [PATCH 3/3] Bluetooth: Perform L2CAP SDU reassembly without copying data Mat Martineau 2011-07-22 16:46 ` Gustavo Padovan 2011-07-18 20:02 ` [PATCH 0/3] Zero-copy L2CAP ERTM & streaming reassembly Mat Martineau 2011-07-18 20:33 ` Marcel Holtmann 2011-07-18 22:18 ` Mat Martineau 2011-07-18 22:43 ` Marcel Holtmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).