* [RFC] Bluetooth: Refactor l2cap_sock_sendmsg() to copy user buffer
@ 2014-06-04 8:24 Jukka Rissanen
2014-06-05 11:25 ` Marcel Holtmann
0 siblings, 1 reply; 2+ messages in thread
From: Jukka Rissanen @ 2014-06-04 8:24 UTC (permalink / raw)
To: linux-bluetooth
The l2cap_chan_send() is changed to use kernel memory directly,
so this function must read the user buffer before sending the
message.
The change is done as the 6LoWPAN also uses l2cap_chan_send()
and in order to minimize the amount of code changes, we must
copy the user buffer in sock handling code.
Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
---
Hi,
this is a proposal related to 6LoWPAN patches I sent earlier.
In this patch we do not copy the memory buffer for L2CAP sock
users thus avoiding one extra memory copy.
Cheers,
Jukka
include/net/bluetooth/l2cap.h | 11 +++-
net/bluetooth/a2mp.c | 12 +----
net/bluetooth/l2cap_core.c | 115 +++++++++++++++++++++++++++++-------------
net/bluetooth/l2cap_sock.c | 49 +++++++++++++++++-
4 files changed, 137 insertions(+), 50 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 4abdcb2..c6f7358 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -872,8 +872,15 @@ struct l2cap_chan *l2cap_chan_create(void);
void l2cap_chan_close(struct l2cap_chan *chan, int reason);
int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
bdaddr_t *dst, u8 dst_type);
-int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
- u32 priority);
+int l2cap_chan_send(struct l2cap_chan *chan, const void *buf, size_t len,
+ u32 priority, unsigned int flags);
+typedef int (*skb_copy_cb)(struct l2cap_chan *chan, const struct msghdr *msg,
+ int len, unsigned int flags, int count,
+ struct sk_buff *skb);
+int l2cap_chan_send_fromiovec(struct l2cap_chan *chan,
+ const struct msghdr *msg, size_t len,
+ u32 priority, skb_copy_cb fromiovec);
+
void l2cap_chan_busy(struct l2cap_chan *chan, int busy);
int l2cap_chan_check_security(struct l2cap_chan *chan);
void l2cap_chan_set_defaults(struct l2cap_chan *chan);
diff --git a/net/bluetooth/a2mp.c b/net/bluetooth/a2mp.c
index 9514cc9..6b99b1b 100644
--- a/net/bluetooth/a2mp.c
+++ b/net/bluetooth/a2mp.c
@@ -48,22 +48,12 @@ void a2mp_send(struct amp_mgr *mgr, u8 code, u8 ident, u16 len, void *data)
struct l2cap_chan *chan = mgr->a2mp_chan;
struct a2mp_cmd *cmd;
u16 total_len = len + sizeof(*cmd);
- struct kvec iv;
- struct msghdr msg;
cmd = __a2mp_build(code, ident, len, data);
if (!cmd)
return;
- iv.iov_base = cmd;
- iv.iov_len = total_len;
-
- memset(&msg, 0, sizeof(msg));
-
- msg.msg_iov = (struct iovec *) &iv;
- msg.msg_iovlen = 1;
-
- l2cap_chan_send(chan, &msg, total_len, 0);
+ l2cap_chan_send(chan, cmd, total_len, 0, 0);
kfree(cmd);
}
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 7468482..c10c2ee4 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2104,19 +2104,24 @@ static void l2cap_send_ack(struct l2cap_chan *chan)
}
}
-static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
- struct msghdr *msg, int len,
- int count, struct sk_buff *skb)
+static inline int l2cap_copy_into_skbuff(struct l2cap_chan *chan,
+ const void *buf, int len,
+ unsigned int flags, int count,
+ struct sk_buff *skb,
+ skb_copy_cb fromiovec)
{
struct l2cap_conn *conn = chan->conn;
struct sk_buff **frag;
int sent = 0;
- if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
- return -EFAULT;
+ if (fromiovec)
+ return fromiovec(chan, buf, len, flags, count, skb);
+
+ memcpy(skb_put(skb, count), buf, count);
sent += count;
len -= count;
+ buf += count;
/* Continuation fragments (no L2CAP header) */
frag = &skb_shinfo(skb)->frag_list;
@@ -2126,19 +2131,19 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
count = min_t(unsigned int, conn->mtu, len);
tmp = chan->ops->alloc_skb(chan, count,
- msg->msg_flags & MSG_DONTWAIT);
+ 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;
+ memcpy(skb_put(*frag, count), buf, count);
(*frag)->priority = skb->priority;
sent += count;
len -= count;
+ buf += count;
skb->len += (*frag)->len;
skb->data_len += (*frag)->len;
@@ -2150,8 +2155,10 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
}
static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan,
- struct msghdr *msg, size_t len,
- u32 priority)
+ const void *buf, size_t len,
+ u32 priority,
+ unsigned int flags,
+ skb_copy_cb fromiovec)
{
struct l2cap_conn *conn = chan->conn;
struct sk_buff *skb;
@@ -2164,7 +2171,7 @@ static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan,
count = min_t(unsigned int, (conn->mtu - hlen), len);
skb = chan->ops->alloc_skb(chan, count + hlen,
- msg->msg_flags & MSG_DONTWAIT);
+ flags & MSG_DONTWAIT);
if (IS_ERR(skb))
return skb;
@@ -2176,7 +2183,8 @@ static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan,
lh->len = cpu_to_le16(len + L2CAP_PSMLEN_SIZE);
put_unaligned(chan->psm, (__le16 *) skb_put(skb, L2CAP_PSMLEN_SIZE));
- err = l2cap_skbuff_fromiovec(chan, msg, len, count, skb);
+ err = l2cap_copy_into_skbuff(chan, buf, len, flags, count, skb,
+ fromiovec);
if (unlikely(err < 0)) {
kfree_skb(skb);
return ERR_PTR(err);
@@ -2185,8 +2193,9 @@ static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan,
}
static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
- struct msghdr *msg, size_t len,
- u32 priority)
+ const void *buf, size_t len,
+ u32 priority, unsigned int flags,
+ skb_copy_cb fromiovec)
{
struct l2cap_conn *conn = chan->conn;
struct sk_buff *skb;
@@ -2198,7 +2207,7 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
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);
+ flags & MSG_DONTWAIT);
if (IS_ERR(skb))
return skb;
@@ -2209,7 +2218,8 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
lh->cid = cpu_to_le16(chan->dcid);
lh->len = cpu_to_le16(len);
- err = l2cap_skbuff_fromiovec(chan, msg, len, count, skb);
+ err = l2cap_copy_into_skbuff(chan, buf, len, flags, count, skb,
+ fromiovec);
if (unlikely(err < 0)) {
kfree_skb(skb);
return ERR_PTR(err);
@@ -2218,8 +2228,9 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
}
static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
- struct msghdr *msg, size_t len,
- u16 sdulen)
+ const void *buf, size_t len,
+ u16 sdulen, unsigned int flags,
+ skb_copy_cb fromiovec)
{
struct l2cap_conn *conn = chan->conn;
struct sk_buff *skb;
@@ -2242,7 +2253,7 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
count = min_t(unsigned int, (conn->mtu - hlen), len);
skb = chan->ops->alloc_skb(chan, count + hlen,
- msg->msg_flags & MSG_DONTWAIT);
+ flags & MSG_DONTWAIT);
if (IS_ERR(skb))
return skb;
@@ -2260,7 +2271,8 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
if (sdulen)
put_unaligned_le16(sdulen, skb_put(skb, L2CAP_SDULEN_SIZE));
- err = l2cap_skbuff_fromiovec(chan, msg, len, count, skb);
+ err = l2cap_copy_into_skbuff(chan, buf, len, flags, count, skb,
+ fromiovec);
if (unlikely(err < 0)) {
kfree_skb(skb);
return ERR_PTR(err);
@@ -2273,14 +2285,16 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
static int l2cap_segment_sdu(struct l2cap_chan *chan,
struct sk_buff_head *seg_queue,
- struct msghdr *msg, size_t len)
+ const void *buf, size_t len,
+ unsigned int flags,
+ skb_copy_cb fromiovec)
{
struct sk_buff *skb;
u16 sdu_len;
size_t pdu_len;
u8 sar;
- BT_DBG("chan %p, msg %p, len %zu", chan, msg, len);
+ BT_DBG("chan %p, buf %p, len %zu", chan, buf, len);
/* It is critical that ERTM PDUs fit in a single HCI fragment,
* so fragmented skbs are not used. The HCI layer's handling
@@ -2314,7 +2328,8 @@ static int l2cap_segment_sdu(struct l2cap_chan *chan,
}
while (len > 0) {
- skb = l2cap_create_iframe_pdu(chan, msg, pdu_len, sdu_len);
+ skb = l2cap_create_iframe_pdu(chan, buf, pdu_len, sdu_len,
+ flags, fromiovec);
if (IS_ERR(skb)) {
__skb_queue_purge(seg_queue);
@@ -2342,8 +2357,10 @@ static int l2cap_segment_sdu(struct l2cap_chan *chan,
}
static struct sk_buff *l2cap_create_le_flowctl_pdu(struct l2cap_chan *chan,
- struct msghdr *msg,
- size_t len, u16 sdulen)
+ const void *buf, size_t len,
+ u16 sdulen,
+ unsigned int flags,
+ skb_copy_cb fromiovec)
{
struct l2cap_conn *conn = chan->conn;
struct sk_buff *skb;
@@ -2363,7 +2380,7 @@ static struct sk_buff *l2cap_create_le_flowctl_pdu(struct l2cap_chan *chan,
count = min_t(unsigned int, (conn->mtu - hlen), len);
skb = chan->ops->alloc_skb(chan, count + hlen,
- msg->msg_flags & MSG_DONTWAIT);
+ flags & MSG_DONTWAIT);
if (IS_ERR(skb))
return skb;
@@ -2375,7 +2392,8 @@ static struct sk_buff *l2cap_create_le_flowctl_pdu(struct l2cap_chan *chan,
if (sdulen)
put_unaligned_le16(sdulen, skb_put(skb, L2CAP_SDULEN_SIZE));
- err = l2cap_skbuff_fromiovec(chan, msg, len, count, skb);
+ err = l2cap_copy_into_skbuff(chan, buf, len, flags, count, skb,
+ fromiovec);
if (unlikely(err < 0)) {
kfree_skb(skb);
return ERR_PTR(err);
@@ -2386,13 +2404,15 @@ static struct sk_buff *l2cap_create_le_flowctl_pdu(struct l2cap_chan *chan,
static int l2cap_segment_le_sdu(struct l2cap_chan *chan,
struct sk_buff_head *seg_queue,
- struct msghdr *msg, size_t len)
+ const void *buf, size_t len,
+ unsigned int flags,
+ skb_copy_cb fromiovec)
{
struct sk_buff *skb;
size_t pdu_len;
u16 sdu_len;
- BT_DBG("chan %p, msg %p, len %zu", chan, msg, len);
+ BT_DBG("chan %p, buf %p, len %zu", chan, buf, len);
pdu_len = chan->conn->mtu - L2CAP_HDR_SIZE;
@@ -2405,7 +2425,8 @@ static int l2cap_segment_le_sdu(struct l2cap_chan *chan,
if (len <= pdu_len)
pdu_len = len;
- skb = l2cap_create_le_flowctl_pdu(chan, msg, pdu_len, sdu_len);
+ skb = l2cap_create_le_flowctl_pdu(chan, buf, pdu_len, sdu_len,
+ flags, fromiovec);
if (IS_ERR(skb)) {
__skb_queue_purge(seg_queue);
return PTR_ERR(skb);
@@ -2414,6 +2435,7 @@ static int l2cap_segment_le_sdu(struct l2cap_chan *chan,
__skb_queue_tail(seg_queue, skb);
len -= pdu_len;
+ buf += pdu_len;
if (sdu_len) {
sdu_len = 0;
@@ -2424,8 +2446,10 @@ static int l2cap_segment_le_sdu(struct l2cap_chan *chan,
return 0;
}
-int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
- u32 priority)
+static int l2cap_chan_send_internal(struct l2cap_chan *chan, const void *buf,
+ size_t len, u32 priority,
+ unsigned int flags,
+ skb_copy_cb fromiovec)
{
struct sk_buff *skb;
int err;
@@ -2436,7 +2460,8 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
/* Connectionless channel */
if (chan->chan_type == L2CAP_CHAN_CONN_LESS) {
- skb = l2cap_create_connless_pdu(chan, msg, len, priority);
+ skb = l2cap_create_connless_pdu(chan, buf, len, priority,
+ flags, fromiovec);
if (IS_ERR(skb))
return PTR_ERR(skb);
@@ -2463,7 +2488,8 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
__skb_queue_head_init(&seg_queue);
- err = l2cap_segment_le_sdu(chan, &seg_queue, msg, len);
+ err = l2cap_segment_le_sdu(chan, &seg_queue, buf, len, flags,
+ fromiovec);
if (chan->state != BT_CONNECTED) {
__skb_queue_purge(&seg_queue);
@@ -2493,7 +2519,8 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
return -EMSGSIZE;
/* Create a basic PDU */
- skb = l2cap_create_basic_pdu(chan, msg, len, priority);
+ skb = l2cap_create_basic_pdu(chan, buf, len, priority, flags,
+ fromiovec);
if (IS_ERR(skb))
return PTR_ERR(skb);
@@ -2523,7 +2550,8 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
* since it's possible to block while waiting for memory
* allocation.
*/
- err = l2cap_segment_sdu(chan, &seg_queue, msg, len);
+ err = l2cap_segment_sdu(chan, &seg_queue, buf, len, flags,
+ fromiovec);
/* The channel could have been closed while segmenting,
* check that it is still connected.
@@ -2557,6 +2585,21 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
return err;
}
+int l2cap_chan_send_fromiovec(struct l2cap_chan *chan,
+ const struct msghdr *msg, size_t len,
+ u32 priority, skb_copy_cb fromiovec)
+{
+ return l2cap_chan_send_internal(chan, msg, len, priority,
+ msg->msg_flags, fromiovec);
+}
+
+int l2cap_chan_send(struct l2cap_chan *chan, const void *buf, size_t len,
+ u32 priority, unsigned int flags)
+{
+ return l2cap_chan_send_internal(chan, buf, len, priority, flags, NULL);
+}
+EXPORT_SYMBOL_GPL(l2cap_chan_send);
+
static void l2cap_send_srej(struct l2cap_chan *chan, u16 txseq)
{
struct l2cap_ctrl control;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index f59e00c..4c47497 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -943,6 +943,52 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
return err;
}
+static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
+ const struct msghdr *msg, int len,
+ unsigned int flags,
+ int count, struct sk_buff *skb)
+{
+ struct l2cap_conn *conn = chan->conn;
+ struct sk_buff **frag;
+ int sent = 0;
+
+ if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
+ return -EFAULT;
+
+ sent += count;
+ len -= count;
+
+ /* Continuation fragments (no L2CAP header) */
+ frag = &skb_shinfo(skb)->frag_list;
+ while (len) {
+ struct sk_buff *tmp;
+
+ count = min_t(unsigned int, conn->mtu, len);
+
+ tmp = chan->ops->alloc_skb(chan, count,
+ 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;
+ }
+
+ return sent;
+}
+
static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
struct msghdr *msg, size_t len)
{
@@ -969,7 +1015,8 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
return err;
l2cap_chan_lock(chan);
- err = l2cap_chan_send(chan, msg, len, sk->sk_priority);
+ err = l2cap_chan_send_fromiovec(chan, msg, len, sk->sk_priority,
+ l2cap_skbuff_fromiovec);
l2cap_chan_unlock(chan);
return err;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [RFC] Bluetooth: Refactor l2cap_sock_sendmsg() to copy user buffer
2014-06-04 8:24 [RFC] Bluetooth: Refactor l2cap_sock_sendmsg() to copy user buffer Jukka Rissanen
@ 2014-06-05 11:25 ` Marcel Holtmann
0 siblings, 0 replies; 2+ messages in thread
From: Marcel Holtmann @ 2014-06-05 11:25 UTC (permalink / raw)
To: Jukka Rissanen; +Cc: linux-bluetooth
Hi Jukka,
> The l2cap_chan_send() is changed to use kernel memory directly,
> so this function must read the user buffer before sending the
> message.
>
> The change is done as the 6LoWPAN also uses l2cap_chan_send()
> and in order to minimize the amount of code changes, we must
> copy the user buffer in sock handling code.
>
> Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
> ---
> Hi,
>
> this is a proposal related to 6LoWPAN patches I sent earlier.
> In this patch we do not copy the memory buffer for L2CAP sock
> users thus avoiding one extra memory copy.
>
>
> Cheers,
> Jukka
>
>
> include/net/bluetooth/l2cap.h | 11 +++-
> net/bluetooth/a2mp.c | 12 +----
> net/bluetooth/l2cap_core.c | 115 +++++++++++++++++++++++++++++-------------
> net/bluetooth/l2cap_sock.c | 49 +++++++++++++++++-
> 4 files changed, 137 insertions(+), 50 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 4abdcb2..c6f7358 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -872,8 +872,15 @@ struct l2cap_chan *l2cap_chan_create(void);
> void l2cap_chan_close(struct l2cap_chan *chan, int reason);
> int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
> bdaddr_t *dst, u8 dst_type);
> -int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
> - u32 priority);
> +int l2cap_chan_send(struct l2cap_chan *chan, const void *buf, size_t len,
> + u32 priority, unsigned int flags);
> +typedef int (*skb_copy_cb)(struct l2cap_chan *chan, const struct msghdr *msg,
> + int len, unsigned int flags, int count,
> + struct sk_buff *skb);
> +int l2cap_chan_send_fromiovec(struct l2cap_chan *chan,
> + const struct msghdr *msg, size_t len,
> + u32 priority, skb_copy_cb fromiovec);
this is not a direction I like to go towards. The caller should not be required to provide a callback. I spent some time looking into this and it seems it is not trivial.
Our current code is super optimized to go from an iovec of the socket data into skb->frag with ACL frames. Essentially we do not have any interim layer for L2CAP. The packets are directly converted into SKBs for the HCI layer. We need to keep this optimized handling. That it depends on copy_from_user is a bit sad. And I also realized that A2MP is buggy at the moment and we need to fix that.
I do not have a solution right now. All ideas I came up with break with I-Frames from L2CAP ERTM mode. The problem is its variable header length that is killing us when we want to keep this optimized behavior. Meaning we can not pre-calculate the ACL fragments in the caller.
Regards
Marcel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-06-05 11:25 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-04 8:24 [RFC] Bluetooth: Refactor l2cap_sock_sendmsg() to copy user buffer Jukka Rissanen
2014-06-05 11:25 ` Marcel Holtmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.