Hi Gustavo -
On Wed, 9 May 2012, Gustavo Padovan wrote:
When we add a fragment to a skb, len, data_len and truesize fields needs
to be updated.
Signed-off-by: Gustavo Padovan <gustavo@xxxxxxxxxxx>
---
net/bluetooth/l2cap_core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 66a1a55..8d7c6ba 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1851,6 +1851,10 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
sent += count;
len -= count;
+ skb->len += (*frag)->len;
+ skb->data_len += (*frag)->len;
+ skb->truesize += (*frag)->truesize;
+
frag = &(*frag)->next;
}
--
1.7.10.1
Good to hear that MSG_MORE support is in progress! Is this change
necessary to support MSG_MORE, or is it only something you noticed
while working on that feature?
I think this patch breaks SO_SNDBUF accounting, which uses
skb->truesize in the sock_wfree() destructor.
For outgoing packets, L2CAP and HCI currently use skb fragments in a
non-standard way. The &skb_shinfo(skb)->frag_list and skb->next
pointers are used to group HCI fragments so they are placed in the HCI
send queue all at once. But they are *not* intended to represent a
"normal" fragmented skb.
Once the list of skbs is passed to hci_send_acl(), hci_queue_acl()
separates all of the linked skbs before being placing them in the HCI
chan->data_q. Since the driver sees packets coming out of
chan->data_q, it only sees unfragmented skbs. If you change
skb->truesize on the first HCI fragment, sk->sk_wmem_alloc will be
adjusted for the bytes used by that first fragment plus the bytes used
by all of the continuation fragments. However, all of the later
continuation fragments will adjust sk->sk_wmem_alloc too!
There are other problems due to the non-standard use of skb fragments
by HCI and L2CAP. One is that it is confusing, and makes changes that
*should* work (like this one) instead cause breakage.
The big problem is that the skb->next pointer is used for both skb
queuing and skb fragment lists. On top of that, skb clones share the
&skb_shinfo(skb)->frag_list pointer - so the continuation fragments
and their 'next' pointers are also shared between clones! When
hci_queue_acl() puts each fragment in the chan->data_q, the skb->next
pointers of the fragments are overwritten. If there is a clone of the
head skb in another queue (like the ERTM tx queue), its fragments get
corrupted. This is why ERTM PDUs must fit in a single HCI fragment,
so that no fragment lists are included in the ERTM tx queue. Some
devices (especially USB) have very short HCI MTUs, which makes ERTM
much less efficient on those devices.
I see these options:
* Add comments to explain non-standard use of skb fragments
* Keep your change above, but also modify hci_queue_acl() to rewrite
the len, data_len, and truesize values of the head skb before queueing
it.
* Try to find a different way to represent HCI fragments that fits
with standard skb usage and interacts better with ERTM tx queues.
What sounds good to you?
--
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
