MSG_MORE enables us to save buffer space in userspace, the packet are
built directly in the kernel and sent only when a msg with the MSG_MORE
flag not set arrives. If a send() tries to add more chan->omtu bytes
-EMSGSIZE is returned.
Only MSG_MORE for Basic Mode is supported right now. chan->skb_more keeps
a pointer to the L2CAP packet that is being build through many calls to
send().
Signed-off-by: Gustavo Padovan <[email protected]>
---
include/net/bluetooth/l2cap.h | 2 +
net/bluetooth/l2cap_core.c | 112 ++++++++++++++++++++++++++++++++++++++---
2 files changed, 106 insertions(+), 8 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 1c7d1cd..5f2845d 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -520,6 +520,8 @@ struct l2cap_chan {
struct list_head list;
struct list_head global_l;
+ struct sk_buff *skb_more;
+
void *data;
struct l2cap_ops *ops;
struct mutex lock;
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index e5a4fd9..d42cc11 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -508,6 +508,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
test_bit(CONF_INPUT_DONE, &chan->conf_state)))
return;
+ kfree_skb(chan->skb_more);
+
skb_queue_purge(&chan->tx_q);
if (chan->mode == L2CAP_MODE_ERTM) {
@@ -1827,6 +1829,8 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
struct sk_buff **frag;
int sent = 0;
+ count = min_t(unsigned int, count, len);
+
if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
return -EFAULT;
@@ -1903,9 +1907,13 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
int err, count;
struct l2cap_hdr *lh;
- BT_DBG("chan %p len %d", chan, (int)len);
+ BT_DBG("chan %p len %d aclmtu %d" , chan, (int)len, conn->mtu);
- count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len);
+ if (msg->msg_flags & MSG_MORE)
+ count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
+ chan->omtu);
+ else
+ count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len);
skb = chan->ops->alloc_skb(chan, count + L2CAP_HDR_SIZE,
msg->msg_flags & MSG_DONTWAIT);
@@ -2048,11 +2056,76 @@ static int l2cap_segment_sdu(struct l2cap_chan *chan,
return err;
}
+static int l2cap_append_pdu(struct l2cap_chan *chan, struct msghdr *msg,
+ size_t len)
+{
+ struct l2cap_conn *conn = chan->conn;
+ struct sk_buff **frag, *skb = chan->skb_more;
+ int sent = 0;
+ unsigned int count;
+ struct l2cap_hdr *lh;
+
+ BT_DBG("chan %p len %ld", chan, len);
+
+ frag = &skb_shinfo(skb)->frag_list;
+ if (*frag)
+ goto frags;
+
+ count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
+ chan->omtu);
+ count = count - skb->len + L2CAP_HDR_SIZE;
+ count = min_t(unsigned int, count, len);
+
+ if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
+ return -EFAULT;
+
+ sent += count;
+ len -= count;
+
+frags:
+ while (*frag)
+ frag = &(*frag)->next;
+
+ while (len) {
+ struct sk_buff *tmp;
+
+ count = chan->omtu - skb->len + L2CAP_HDR_SIZE;
+ count = min_t(unsigned int, count, len);
+ count = min_t(unsigned int, conn->mtu, count);
+
+ tmp = chan->ops->alloc_skb(chan, count,
+ msg->msg_flags & MSG_DONTWAIT);
+ if (IS_ERR(tmp))
+ return PTR_ERR(tmp);
+
+ *frag = tmp;
+
+ if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov,
+ count))
+ return -EFAULT;
+
+ (*frag)->priority = skb->priority;
+
+ sent += count;
+ len -= count;
+
+ skb->len += (*frag)->len;
+ skb->data_len += (*frag)->len;
+
+ frag = &(*frag)->next;
+ }
+
+ lh = (struct l2cap_hdr *) skb->data;
+ lh->len = cpu_to_le16(skb->len - L2CAP_HDR_SIZE);
+
+ return sent;
+}
+
int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
u32 priority)
{
struct sk_buff *skb;
- int err;
+ int err, rem;
struct sk_buff_head seg_queue;
/* Connectionless channel */
@@ -2067,16 +2140,39 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
switch (chan->mode) {
case L2CAP_MODE_BASIC:
+ skb = chan->skb_more;
+
/* Check outgoing MTU */
- if (len > chan->omtu)
+ if (skb)
+ rem = chan->omtu - skb->len + L2CAP_HDR_SIZE;
+ else
+ rem = chan->omtu;
+
+ if (len > rem)
return -EMSGSIZE;
- /* Create a basic PDU */
- skb = l2cap_create_basic_pdu(chan, msg, len, priority);
- if (IS_ERR(skb))
- return PTR_ERR(skb);
+ if (chan->skb_more) {
+ err = l2cap_append_pdu(chan, msg, len);
+ if (err < 0) {
+ kfree_skb(chan->skb_more);
+ chan->skb_more = NULL;
+ return err;
+ }
+ } else {
+ skb = l2cap_create_basic_pdu(chan, msg, len, priority);
+ if (IS_ERR(skb))
+ return PTR_ERR(skb);
+ }
+
+ if (msg->msg_flags & MSG_MORE) {
+ chan->skb_more = skb;
+ return len;
+ }
l2cap_do_send(chan, skb);
+
+ chan->skb_more = NULL;
+
err = len;
break;
--
1.7.10.1
On Wed, May 16, 2012 at 12:53 AM, Gustavo Padovan <[email protected]> wrote:
> MSG_MORE enables us to save buffer space in userspace, the packet are
s/are/is/ ?
--
João Paulo Rechi Vita
Openbossa Labs - INdT
Hi Gustavo -
On Wed, 16 May 2012, Gustavo Padovan wrote:
> MSG_MORE enables us to save buffer space in userspace, the packet are
> built directly in the kernel and sent only when a msg with the MSG_MORE
> flag not set arrives. If a send() tries to add more chan->omtu bytes
> -EMSGSIZE is returned.
>
> Only MSG_MORE for Basic Mode is supported right now. chan->skb_more keeps
> a pointer to the L2CAP packet that is being build through many calls to
> send().
>
> Signed-off-by: Gustavo Padovan <[email protected]>
> ---
> include/net/bluetooth/l2cap.h | 2 +
> net/bluetooth/l2cap_core.c | 112 ++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 106 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 1c7d1cd..5f2845d 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -520,6 +520,8 @@ struct l2cap_chan {
> struct list_head list;
> struct list_head global_l;
>
> + struct sk_buff *skb_more;
> +
> void *data;
> struct l2cap_ops *ops;
> struct mutex lock;
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index e5a4fd9..d42cc11 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -508,6 +508,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
> test_bit(CONF_INPUT_DONE, &chan->conf_state)))
> return;
>
> + kfree_skb(chan->skb_more);
> +
Since l2cap_chan_del doesn't actually free the l2cap_chan struct,
there might still be valid pointers to chan after l2cap_chan_del runs.
I'd suggest setting chan->skb_more to NULL just to be safe.
> skb_queue_purge(&chan->tx_q);
>
> if (chan->mode == L2CAP_MODE_ERTM) {
> @@ -1827,6 +1829,8 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
> struct sk_buff **frag;
> int sent = 0;
>
> + count = min_t(unsigned int, count, len);
> +
> if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
> return -EFAULT;
>
> @@ -1903,9 +1907,13 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
> int err, count;
> struct l2cap_hdr *lh;
>
> - BT_DBG("chan %p len %d", chan, (int)len);
> + BT_DBG("chan %p len %d aclmtu %d" , chan, (int)len, conn->mtu);
>
> - count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len);
> + if (msg->msg_flags & MSG_MORE)
> + count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
> + chan->omtu);
> + else
> + count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len);
>
> skb = chan->ops->alloc_skb(chan, count + L2CAP_HDR_SIZE,
> msg->msg_flags & MSG_DONTWAIT);
> @@ -2048,11 +2056,76 @@ static int l2cap_segment_sdu(struct l2cap_chan *chan,
> return err;
> }
>
> +static int l2cap_append_pdu(struct l2cap_chan *chan, struct msghdr *msg,
> + size_t len)
> +{
> + struct l2cap_conn *conn = chan->conn;
> + struct sk_buff **frag, *skb = chan->skb_more;
> + int sent = 0;
> + unsigned int count;
> + struct l2cap_hdr *lh;
> +
> + BT_DBG("chan %p len %ld", chan, len);
> +
> + frag = &skb_shinfo(skb)->frag_list;
> + if (*frag)
> + goto frags;
> +
> + count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE),
> + chan->omtu);
> + count = count - skb->len + L2CAP_HDR_SIZE;
> + count = min_t(unsigned int, count, len);
> +
> + if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
> + return -EFAULT;
> +
> + sent += count;
> + len -= count;
> +
> +frags:
> + while (*frag)
> + frag = &(*frag)->next;
> +
> + while (len) {
> + struct sk_buff *tmp;
> +
> + count = chan->omtu - skb->len + L2CAP_HDR_SIZE;
> + count = min_t(unsigned int, count, len);
I don't think you want to use 'len' as a limit. The fragment should
be as big as possible (conn->mtu). Only if it is the last fragment
(based on chan->omtu) should it be shorter than conn->mtu.
> + count = min_t(unsigned int, conn->mtu, count);
> +
> + tmp = chan->ops->alloc_skb(chan, count,
> + msg->msg_flags & MSG_DONTWAIT);
It looks like the code skipped by the goto will fill up the first skb
each time you get a MSG_MORE call, but once you have fragments, you
allocate a new skb for every MSG_MORE call rather than fill up the
last fragment in the list. Think of the pathological case where
someone writes a single byte on each call (which would make a pretty
good test case, by the way).
What if you moved the "while (*frag)" up where your goto is, and got
rid of the goto? That way you would always append to the final skb
(whether it is the first one or a fragment), then allocate and
populate additional skbs only when needed.
> + if (IS_ERR(tmp))
> + return PTR_ERR(tmp);
> +
> + *frag = tmp;
> +
> + if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov,
> + count))
> + return -EFAULT;
> +
> + (*frag)->priority = skb->priority;
> +
> + sent += count;
> + len -= count;
> +
> + skb->len += (*frag)->len;
> + skb->data_len += (*frag)->len;
> +
> + frag = &(*frag)->next;
> + }
> +
> + lh = (struct l2cap_hdr *) skb->data;
> + lh->len = cpu_to_le16(skb->len - L2CAP_HDR_SIZE);
> +
> + return sent;
> +}
> +
> int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
> u32 priority)
> {
> struct sk_buff *skb;
> - int err;
> + int err, rem;
> struct sk_buff_head seg_queue;
>
> /* Connectionless channel */
> @@ -2067,16 +2140,39 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
>
> switch (chan->mode) {
> case L2CAP_MODE_BASIC:
> + skb = chan->skb_more;
> +
> /* Check outgoing MTU */
> - if (len > chan->omtu)
> + if (skb)
> + rem = chan->omtu - skb->len + L2CAP_HDR_SIZE;
> + else
> + rem = chan->omtu;
> +
> + if (len > rem)
> return -EMSGSIZE;
>
> - /* Create a basic PDU */
> - skb = l2cap_create_basic_pdu(chan, msg, len, priority);
> - if (IS_ERR(skb))
> - return PTR_ERR(skb);
> + if (chan->skb_more) {
> + err = l2cap_append_pdu(chan, msg, len);
> + if (err < 0) {
> + kfree_skb(chan->skb_more);
> + chan->skb_more = NULL;
> + return err;
> + }
> + } else {
> + skb = l2cap_create_basic_pdu(chan, msg, len, priority);
> + if (IS_ERR(skb))
> + return PTR_ERR(skb);
> + }
> +
> + if (msg->msg_flags & MSG_MORE) {
> + chan->skb_more = skb;
> + return len;
> + }
>
> l2cap_do_send(chan, skb);
> +
> + chan->skb_more = NULL;
> +
> err = len;
> break;
>
> --
> 1.7.10.1
I find this version much easier to follow! Thanks,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum