[net-next v2 1/1] tipc: avoid unnecessary copying of bundled messages

[net-next v2 1/1] tipc: avoid unnecessary copying of bundled messages

[Jon Maloy]

A received sk buffer may contain dozens of smaller 'bundled' messages
which after extraction go each in their own direction.

Unfortunately, when we extract those messages using skb_clone() each
of the extracted buffers inherit the truesize value of the original
buffer. Apart from causing massive overaccounting of the base buffer's
memory, this often causes tipc_msg_validate() to come to the false
conclusion that the ratio truesize/datasize > 4, and perform an
unnecessary copying of the extracted buffer.

We now fix this problem by explicitly correcting the truesize value of
the buffer clones to be the truesize of the clone itself plus a
calculated fraction of the base buffer's overhead. This change
eliminates the overaccounting and at least mitigates the occurrence
of unnecessary buffer copying.

Reported-by: Hoang Le <hoang.h.le@dektek.com.au>
Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/msg.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 4e1c6f6..ce0bfc4 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -416,8 +416,8 @@ bool tipc_msg_bundle(struct sk_buff *skb, struct tipc_msg *msg, u32 mtu)
  */
 bool tipc_msg_extract(struct sk_buff *skb, struct sk_buff **iskb, int *pos)
 {
+	int imsz, offset, clone_cnt, skb_overhead;
 	struct tipc_msg *msg;
-	int imsz, offset;
 
 	*iskb = NULL;
 	if (unlikely(skb_linearize(skb)))
@@ -434,6 +434,11 @@ bool tipc_msg_extract(struct sk_buff *skb, struct sk_buff **iskb, int *pos)
 	skb_pull(*iskb, offset);
 	imsz = msg_size(buf_msg(*iskb));
 	skb_trim(*iskb, imsz);
+
+	/* Scale extracted buffer's truesize to avoid double accounting */
+	clone_cnt = max_t(u32, 1, msg_msgcnt(msg));
+	skb_overhead = skb->truesize - skb->len;
+	(*iskb)->truesize = SKB_TRUESIZE(imsz) + skb_overhead / clone_cnt;
 	if (unlikely(!tipc_msg_validate(iskb)))
 		goto none;
 	*pos += align(imsz);
-- 
2.1.4

Re: [net-next v2 1/1] tipc: avoid unnecessary copying of bundled messages

[David Miller]

From: Jon Maloy <jon.maloy@ericsson.com>
Date: Thu, 15 Feb 2018 14:14:37 +0100

> A received sk buffer may contain dozens of smaller 'bundled' messages
> which after extraction go each in their own direction.
> 
> Unfortunately, when we extract those messages using skb_clone() each
> of the extracted buffers inherit the truesize value of the original
> buffer. Apart from causing massive overaccounting of the base buffer's
> memory, this often causes tipc_msg_validate() to come to the false
> conclusion that the ratio truesize/datasize > 4, and perform an
> unnecessary copying of the extracted buffer.
> 
> We now fix this problem by explicitly correcting the truesize value of
> the buffer clones to be the truesize of the clone itself plus a
> calculated fraction of the base buffer's overhead. This change
> eliminates the overaccounting and at least mitigates the occurrence
> of unnecessary buffer copying.
> 
> Reported-by: Hoang Le <hoang.h.le@dektek.com.au>
> Acked-by: Ying Xue <ying.xue@windriver.com>
> Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>

As I explained in my previous two emails, I don't think this method of
accounting is appropriate.

All of your clones must use the same skb->truesize as the original SKB
because each and every one of them keeps the full buffer from being
liberated until they are released.

RE: [net-next v2 1/1] tipc: avoid unnecessary copying of bundled messages

[Jon Maloy]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Friday, February 16, 2018 21:33
> To: Jon Maloy <jon.maloy@ericsson.com>
> Cc: netdev@vger.kernel.org; Mohan Krishna Ghanta Krishnamurthy
> <mohan.krishna.ghanta.krishnamurthy@ericsson.com>; Tung Quang Nguyen
> <tung.q.nguyen@dektech.com.au>; Hoang Huu Le
> <hoang.h.le@dektech.com.au>; Canh Duc Luu
> <canh.d.luu@dektech.com.au>; Ying Xue <ying.xue@windriver.com>; tipc-
> discussion@lists.sourceforge.net
> Subject: Re: [net-next v2 1/1] tipc: avoid unnecessary copying of bundled
> messages
> 
> From: Jon Maloy <jon.maloy@ericsson.com>
> Date: Thu, 15 Feb 2018 14:14:37 +0100
> 
> > A received sk buffer may contain dozens of smaller 'bundled' messages
> > which after extraction go each in their own direction.
> >
> > Unfortunately, when we extract those messages using skb_clone() each
> > of the extracted buffers inherit the truesize value of the original
> > buffer. Apart from causing massive overaccounting of the base buffer's
> > memory, this often causes tipc_msg_validate() to come to the false
> > conclusion that the ratio truesize/datasize > 4, and perform an
> > unnecessary copying of the extracted buffer.
> >
> > We now fix this problem by explicitly correcting the truesize value of
> > the buffer clones to be the truesize of the clone itself plus a
> > calculated fraction of the base buffer's overhead. This change
> > eliminates the overaccounting and at least mitigates the occurrence of
> > unnecessary buffer copying.
> >
> > Reported-by: Hoang Le <hoang.h.le@dektek.com.au>
> > Acked-by: Ying Xue <ying.xue@windriver.com>
> > Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
> 
> As I explained in my previous two emails, I don't think this method of
> accounting is appropriate.
> 
> All of your clones must use the same skb->truesize as the original SKB
> because each and every one of them keeps the full buffer from being
> liberated until they are released.

I understand what you are saying, although I am not happy with its consequences in this case. I guess I will just leave it the way it is until I can come up with something  smarter.
///jon

RE: [net-next v2 1/1] tipc: avoid unnecessary copying of bundled messages

[David Laight]

From: Jon Maloy <jon.maloy@ericsson.com>
Date: Thu, 15 Feb 2018 14:14:37 +0100

> A received sk buffer may contain dozens of smaller 'bundled' messages
> which after extraction go each in their own direction.
>
> Unfortunately, when we extract those messages using skb_clone() each
> of the extracted buffers inherit the truesize value of the original
> buffer. Apart from causing massive overaccounting of the base buffer's
> memory, this often causes tipc_msg_validate() to come to the false
> conclusion that the ratio truesize/datasize > 4, and perform an
> unnecessary copying of the extracted buffer.
>
> We now fix this problem by explicitly correcting the truesize value of
> the buffer clones to be the truesize of the clone itself plus a
> calculated fraction of the base buffer's overhead. This change
> eliminates the overaccounting and at least mitigates the occurrence
> of unnecessary buffer copying.

Have you actually checked that copying the data when you extract the
messages isn't faster than cloning and trying to avoid the copy?
Copying at the point is probably cheaper because it leads to
a simpler message structure.

	David

Re: [net-next v2 1/1] tipc: avoid unnecessary copying of bundled messages

[Jon Maloy]

> -----Original Message-----
> From: David Laight [mailto:David.Laight@ACULAB.COM]
> Sent: Monday, February 19, 2018 14:30
> To: Jon Maloy <jon.maloy@ericsson.com>
> Cc: netdev@vger.kernel.org; Mohan Krishna Ghanta Krishnamurthy
> <mohan.krishna.ghanta.krishnamurthy@ericsson.com>; Tung Quang Nguyen
> <tung.q.nguyen@dektech.com.au>; Hoang Huu Le
> <hoang.h.le@dektech.com.au>; Canh Duc Luu
> <canh.d.luu@dektech.com.au>; Ying Xue <ying.xue@windriver.com>; tipc-
> discussion@lists.sourceforge.net
> Subject: RE: [net-next v2 1/1] tipc: avoid unnecessary copying of bundled
> messages
> 
> From: Jon Maloy <jon.maloy@ericsson.com>
> Date: Thu, 15 Feb 2018 14:14:37 +0100
> 
> > A received sk buffer may contain dozens of smaller 'bundled' messages
> > which after extraction go each in their own direction.
> >
> > Unfortunately, when we extract those messages using skb_clone() each
> > of the extracted buffers inherit the truesize value of the original
> > buffer. Apart from causing massive overaccounting of the base buffer's
> > memory, this often causes tipc_msg_validate() to come to the false
> > conclusion that the ratio truesize/datasize > 4, and perform an
> > unnecessary copying of the extracted buffer.
> >
> > We now fix this problem by explicitly correcting the truesize value of
> > the buffer clones to be the truesize of the clone itself plus a
> > calculated fraction of the base buffer's overhead. This change
> > eliminates the overaccounting and at least mitigates the occurrence of
> > unnecessary buffer copying.
> 
> Have you actually checked that copying the data when you extract the
> messages isn't faster than cloning and trying to avoid the copy?
> Copying at the point is probably cheaper because it leads to a simpler
> message structure.

Yes, that is probably what I'll end up doing, if copying is unavoidable anyway.

///jon

> 
> 	David


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot