Re: [PATCH 3/4] Bluetooth: Add MSG_MORE support to L2CAP sockets

[Mat Martineau <mathewm@codeaurora.org>]

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@padovan.org>
> ---
> 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