[B.A.T.M.A.N.] [PATCH] batman-adv: Only copy skb data for multiple broadcasts

[B.A.T.M.A.N.] [PATCH] batman-adv: Only copy skb data for multiple broadcasts

[Sven Eckelmann]

batman-adv tries to resend broadcasts on all interfaces up to three
times. For each round and each interface it must provide a skb which
gets consumed by the sending function.

It is unnecessary to copy the data of each broadcast because the actual
data is either not shared or already copied by add_bcast_packet_to_list.
So it is enough to just copy the skb control data

Signed-off-by: Sven Eckelmann <sven.eckelmann@gmx.de>
---
 batman-adv/send.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/batman-adv/send.c b/batman-adv/send.c
index cd6be2b..934bd8a 100644
--- a/batman-adv/send.c
+++ b/batman-adv/send.c
@@ -476,7 +476,7 @@ static void send_outstanding_bcast_packet(struct work_struct *work)
 	rcu_read_lock();
 	list_for_each_entry_rcu(batman_if, &if_list, list) {
 		/* send a copy of the saved skb */
-		skb1 = skb_copy(forw_packet->skb, GFP_ATOMIC);
+		skb1 = skb_clone(forw_packet->skb, GFP_ATOMIC);
 		if (skb1)
 			send_skb_packet(skb1,
 				batman_if, broadcast_addr);
-- 
1.7.1

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Only copy skb data for multiple broadcasts

[Marek Lindner]

On Saturday 24 July 2010 00:14:29 Sven Eckelmann wrote:
> batman-adv tries to resend broadcasts on all interfaces up to three
> times. For each round and each interface it must provide a skb which
> gets consumed by the sending function.
> 
> It is unnecessary to copy the data of each broadcast because the actual
> data is either not shared or already copied by add_bcast_packet_to_list.
> So it is enough to just copy the skb control data

I think the reason to call skb_copy() is the following dev_queue_xmit() call 
which will consume the given skb. If we consider a case of having 3 interfaces 
all 3 cloned skbs point to the same data while going out via different 
interfaces ? I wonder whether that can work ?!

Cheers,
Marek

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Only copy skb data for multiple broadcasts

[Sven Eckelmann]

On Sunday 01 August 2010 17:28:53 Marek Lindner wrote:
> On Saturday 24 July 2010 00:14:29 Sven Eckelmann wrote:
> > batman-adv tries to resend broadcasts on all interfaces up to three
> > times. For each round and each interface it must provide a skb which
> > gets consumed by the sending function.
> > 
> > It is unnecessary to copy the data of each broadcast because the actual
> > data is either not shared or already copied by add_bcast_packet_to_list.
> > So it is enough to just copy the skb control data
> 
> I think the reason to call skb_copy() is the following dev_queue_xmit()
> call which will consume the given skb. If we consider a case of having 3
> interfaces all 3 cloned skbs point to the same data while going out via
> different interfaces ? I wonder whether that can work ?!

Can we try to explain what we think will happen? I would assume following:
 * payload is there in skb
 * skb will be cloned to skb1 (not copied, so the actual data is shared)
 * send_skb_packet will be called and a ethernet header with the actual
   address is added to the data of skb and skb1 - only skb1 will know about
   that new stuff and point to the new data
 * skb1 will be consumed by dev_queue_xmit (not the data, because the refcount
   will not become 0)
 * next if will be processed
 * skb (without the knowledge about the ethernet header stuff) will be cloned
   to skb1
 * send_skb_packet will be called with skb1 and new, personalized ethernet
   header is added
 * skb1 will be consumed by dev_queue_xmit
 * and so on

So the data will be touched, but only the part which is not "know" by the 
initial/original skb. It is not accessed at the same time by other 
cpus/threads/whatever. The skb control data is only cloned because it will be 
consumed and changed in a way which would affect other batman-ifs.

So I would assume that this works with skb_clone instead of skb_copy.

Best regards,
	Sven

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Only copy skb data for multiple broadcasts

[Sven Eckelmann]

[-- Attachment #1: Type: Text/Plain, Size: 1600 bytes --]

Sven Eckelmann wrote:
> On Sunday 01 August 2010 17:28:53 Marek Lindner wrote:
> > On Saturday 24 July 2010 00:14:29 Sven Eckelmann wrote:
> > > batman-adv tries to resend broadcasts on all interfaces up to three
> > > times. For each round and each interface it must provide a skb which
> > > gets consumed by the sending function.
> > > 
> > > It is unnecessary to copy the data of each broadcast because the actual
> > > data is either not shared or already copied by
> > > add_bcast_packet_to_list. So it is enough to just copy the skb control
> > > data
> > 
> > I think the reason to call skb_copy() is the following dev_queue_xmit()
> > call which will consume the given skb. If we consider a case of having 3
> > interfaces all 3 cloned skbs point to the same data while going out via
> > different interfaces ? I wonder whether that can work ?!

We discussed that off the list. This part is addressed in
 [PATCH 1/7] batman-adv: Keep header writable and unshared

the problem we found was that our data is maybe still be queued somewhere 
after the dev_queue_xmit. This means that the header part is still be shared 
and we may not be able to change it in our new clone without side effects to 
the other skb. To prevent that we use skb_cow_head to ensure that our new 
added header doesn't interfere with skbs using the same data buffer.

This should reduce the overhead created by the skb_copy in many situations, 
but still allow us to freely write into the new pushed header area when the 
skb would otherwise be in a shared/cloned state.

Regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]