Hi Gustavo -
On Fri, 11 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 when their size reaches the Output
MTU value.
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().
Could you explain more about how you expect it to work?
I would assume the application would do a series of sends:
send(fd, buf, len, MSG_MORE);
...
send(fd, buf, len, MSG_MORE);
...
send(fd, buf, len, MSG_MORE);
...
send(fd, buf, len, 0);
and the SDU would be sent the first time there is no MSG_MORE flag.
If the MTU is exceeded, the SDU is not sent and an error is returned.
What should happen if a send() with MSG_MORE completely fills an SDU
(length of data sent is equal to MTU)? Does it make sense to treat it
like a normal send, or return an error so that application knows that
later calls with MSG_MORE will not append? Or does the full SDU not
get sent, and a zero-length send() with no MSG_MORE would trigger the
transmission?
Signed-off-by: Gustavo Padovan <gustavo@xxxxxxxxxxx>
---
include/net/bluetooth/l2cap.h | 2 +
net/bluetooth/l2cap_core.c | 116 ++++++++++++++++++++++++++++++++++++++---
2 files changed, 110 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..73bf8a8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1827,6 +1827,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 +1905,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,6 +2054,75 @@ 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;
I think this would be more readable without the goto - just use a
normal if statement with a code block. There's only one nested if
statement.
+
+ 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) {
+ count = chan->omtu - skb->len + L2CAP_HDR_SIZE;
+ count = min_t(unsigned int, count, len);
+ count = min_t(unsigned int, conn->mtu, count);
+
+ *frag = chan->ops->alloc_skb(chan, count,
+ msg->msg_flags & MSG_DONTWAIT);
+ if (IS_ERR(*frag))
+ return PTR_ERR(*frag);
+
+ 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;
+
+ if (skb->len == chan->omtu + L2CAP_HDR_SIZE)
+ break;
Don't you want to return -EMSGSIZE if the data doesn't fit in one SDU?
+ }
+
+ lh = (struct l2cap_hdr *) skb->data;
+ lh->len = cpu_to_le16(skb->len - L2CAP_HDR_SIZE);
+
+ if (skb->len == chan->omtu + L2CAP_HDR_SIZE) {
+ l2cap_do_send(chan, skb);
I don't think it's good to put a send in here. Let the calling
function do the send, so it's in one place.
+ chan->skb_more = NULL;
+ }
+
+ return sent;
+}
+
int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
u32 priority)
{
@@ -2068,16 +2143,41 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
switch (chan->mode) {
case L2CAP_MODE_BASIC:
/* Check outgoing MTU */
- if (len > chan->omtu)
+ if (len > chan->omtu) {
+ kfree_skb(chan->skb_more);
Set skb_more to NULL after freeing.
return -EMSGSIZE;
+ }
- /* Create a basic PDU */
- skb = l2cap_create_basic_pdu(chan, msg, len, priority);
- if (IS_ERR(skb))
+ err = len;
+ if (chan->skb_more) {
+ int sent = l2cap_append_pdu(chan, msg, len);
+
+ if (sent < 0) {
+ kfree_skb(chan->skb_more);
+ return sent;
+ }
+
+ len -= sent;
+ }
+
+ if (len)
+ skb = l2cap_create_basic_pdu(chan, msg, len, priority);
Shouldn't this be the 'else' clause for the above if statement? You
should either call l2cap_append_pdu or l2cap_create_basic_pdu, but
never both. Better to structure the logic so that they are obviously
mutually exclusive.
+ else
+ skb = chan->skb_more;
+
+ if (IS_ERR(skb)) {
+ kfree_skb(chan->skb_more);
Set skb_more to NULL after freeing.
return PTR_ERR(skb);
+ }
+
+ if (!skb)
+ return err;
+
+ if (msg->msg_flags & MSG_MORE && skb->len < chan->omtu)
+ chan->skb_more = skb;
+ else
+ l2cap_do_send(chan, skb);
I think l2cap_do_send() should be called if and only if MSG_MORE is
not set, unless there is an MTU problem.
Also, do you need to account for L2CAP_HDR_SIZE when checking
skb->len?
- l2cap_do_send(chan, skb);
- err = len;
break;
case L2CAP_MODE_ERTM:
--
1.7.10.1
Overall, I would suggest that l2cap_chan_send should be kept simple
and most of the MSG_MORE code should be in a function called by
l2cap_chan_send.
--
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
