[B.A.T.M.A.N.] batman-adv: unicast fragmentation enhancements

[B.A.T.M.A.N.] batman-adv: unicast fragmentation enhancements

[Andreas Langer]

Hi,

things that changed since the last patch:
- add support to fragment packets on every node,
  if outgoing iface mtu is smaller as the packet
- add support to defragment packets on every node,
  if outgoing iface mtu is >= original size
- rename some functions

In this patch every node defragment packets if it can. Do you think it
is better to add an option to enable/disable this ?

regards,
Andreas

[B.A.T.M.A.N.] [PATCH] batman-adv: layer2 unicast packet fragmentation enhancements

[Andreas Langer]

This patch improves the fragmentation by also fragmenting
packets that came in via an interface which does not need
fragmentation. In addition it reassembles fragmented packets
as soon as the interface MTU allows it.

Signed-off-by: Andreas Langer <an.langer at gmx.de>
---
 batman-adv/routing.c |   64 +++++++++---------------
 batman-adv/unicast.c |  130 +++++++++++++++++++++++++++++++++-----------------
 batman-adv/unicast.h |   13 ++----
 3 files changed, 114 insertions(+), 93 deletions(-)

diff --git a/batman-adv/routing.c b/batman-adv/routing.c
index a2c64a4..e975452 100644
--- a/batman-adv/routing.c
+++ b/batman-adv/routing.c
@@ -1142,15 +1142,11 @@ static int route_unicast_packet(struct sk_buff *skb,
 	unsigned long flags;
 	struct unicast_packet *unicast_packet;
 	struct ethhdr *ethhdr = (struct ethhdr *)skb_mac_header(skb);
+	int hdr_lenght = sizeof(struct unicast_packet),
+	    uni_diff = sizeof(struct unicast_frag_packet) - hdr_lenght;
 
 	unicast_packet = (struct unicast_packet *)skb->data;
 
-	/* packet for me */
-	if (is_my_mac(unicast_packet->dest)) {
-		interface_rx(recv_if->soft_iface, skb, hdr_size);
-		return NET_RX_SUCCESS;
-	}
-
 	/* TTL exceeded */
 	if (unicast_packet->ttl < 2) {
 		pr_debug("Warning - can't forward unicast packet from %pM to "
@@ -1184,7 +1180,26 @@ static int route_unicast_packet(struct sk_buff *skb,
 		return NET_RX_DROP;
 
 	unicast_packet = (struct unicast_packet *)skb->data;
-	ethhdr = (struct ethhdr *)skb_mac_header(skb);
+
+	if (unicast_packet->packet_type == BAT_UNICAST &&
+	    atomic_read(&bat_priv->frag_enabled) &&
+	    skb->len > batman_if->net_dev->mtu)
+		return frag_send_skb(skb, bat_priv, batman_if,
+				     dstaddr);
+
+	if (unicast_packet->packet_type == BAT_UNICAST_FRAG &&
+	    2 * skb->len - hdr_size <= batman_if->net_dev->mtu) {
+
+		skb = frag_reassemble_skb(skb, bat_priv);
+		if (!skb)
+			return NET_RX_SUCCESS;
+
+		memmove(skb->data + uni_diff, skb->data, hdr_lenght);
+		skb_pull(skb, uni_diff);
+
+		unicast_packet = (struct unicast_packet *)skb->data;
+		unicast_packet->packet_type = BAT_UNICAST;
+	}
 
 	/* decrement ttl */
 	unicast_packet->ttl--;
@@ -1218,10 +1233,7 @@ int recv_ucast_frag_packet(struct sk_buff *skb, struct batman_if *recv_if)
 {
 	struct bat_priv *bat_priv = netdev_priv(recv_if->soft_iface);
 	struct unicast_frag_packet *unicast_packet;
-	struct orig_node *orig_node;
-	struct frag_packet_list_entry *tmp_frag_entry;
 	int hdr_size = sizeof(struct unicast_frag_packet);
-	unsigned long flags;
 
 	if (check_unicast_packet(skb, hdr_size) < 0)
 		return NET_RX_DROP;
@@ -1231,38 +1243,10 @@ int recv_ucast_frag_packet(struct sk_buff *skb, struct batman_if *recv_if)
 	/* packet for me */
 	if (is_my_mac(unicast_packet->dest)) {
 
-		spin_lock_irqsave(&bat_priv->orig_hash_lock, flags);
-		orig_node = ((struct orig_node *)
-			hash_find(bat_priv->orig_hash, unicast_packet->orig));
-
-		if (!orig_node) {
-			pr_debug("couldn't find orig node for fragmentation\n");
-			spin_unlock_irqrestore(&bat_priv->orig_hash_lock,
-					       flags);
-			return NET_RX_DROP;
-		}
-
-		orig_node->last_frag_packet = jiffies;
-
-		if (list_empty(&orig_node->frag_list))
-			create_frag_buffer(&orig_node->frag_list);
+		skb = frag_reassemble_skb(skb, bat_priv);
 
-		tmp_frag_entry =
-			search_frag_packet(&orig_node->frag_list,
-					   unicast_packet);
-
-		if (!tmp_frag_entry) {
-			create_frag_entry(&orig_node->frag_list, skb);
-			spin_unlock_irqrestore(&bat_priv->orig_hash_lock,
-					       flags);
-			return NET_RX_SUCCESS;
-		}
-
-		skb = merge_frag_packet(&orig_node->frag_list,
-					tmp_frag_entry, skb);
-		spin_unlock_irqrestore(&bat_priv->orig_hash_lock, flags);
 		if (!skb)
-			return NET_RX_DROP;
+			return NET_RX_SUCCESS;
 
 		interface_rx(recv_if->soft_iface, skb, hdr_size);
 		return NET_RX_SUCCESS;
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 84b204b..5903692 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -30,9 +30,9 @@
 #include "hard-interface.h"
 
 
-struct sk_buff *merge_frag_packet(struct list_head *head,
-				  struct frag_packet_list_entry *tfp,
-				  struct sk_buff *skb)
+static struct sk_buff *frag_merge_packet(struct list_head *head,
+					 struct frag_packet_list_entry *tfp,
+					 struct sk_buff *skb)
 {
 	struct unicast_frag_packet *up =
 		(struct unicast_frag_packet *)skb->data;
@@ -63,7 +63,7 @@ struct sk_buff *merge_frag_packet(struct list_head *head,
 	return skb;
 }
 
-void create_frag_entry(struct list_head *head, struct sk_buff *skb)
+static void frag_create_entry(struct list_head *head, struct sk_buff *skb)
 {
 	struct frag_packet_list_entry *tfp;
 	struct unicast_frag_packet *up =
@@ -79,7 +79,7 @@ void create_frag_entry(struct list_head *head, struct sk_buff *skb)
 	return;
 }
 
-void create_frag_buffer(struct list_head *head)
+static void frag_create_buffer(struct list_head *head)
 {
 	int i;
 	struct frag_packet_list_entry *tfp;
@@ -96,7 +96,7 @@ void create_frag_buffer(struct list_head *head)
 	return;
 }
 
-struct frag_packet_list_entry *search_frag_packet(struct list_head *head,
+static struct frag_packet_list_entry *frag_search_packet(struct list_head *head,
 						 struct unicast_frag_packet *up)
 {
 	struct frag_packet_list_entry *tfp;
@@ -149,55 +149,94 @@ void frag_list_free(struct list_head *head)
 	return;
 }
 
-static int unicast_send_frag_skb(struct sk_buff *skb, struct bat_priv *bat_priv,
-			  struct batman_if *batman_if, uint8_t dstaddr[],
-			  struct orig_node *orig_node)
+/* if matching fragment found return reassembled skb
+   otherwise buffer the fragment */
+struct sk_buff *frag_reassemble_skb(struct sk_buff *skb,
+				    struct bat_priv *bat_priv)
 {
-	struct unicast_frag_packet *ucast_frag1, *ucast_frag2;
-	int hdr_len = sizeof(struct unicast_frag_packet);
+	unsigned long flags;
+	struct orig_node *orig_node;
+	struct frag_packet_list_entry *tmp_frag_entry;
+	struct sk_buff *ret = NULL;
+
+	struct unicast_frag_packet *unicast_packet =
+		(struct unicast_frag_packet *)skb->data;
+
+	spin_lock_irqsave(&bat_priv->orig_hash_lock, flags);
+	orig_node = ((struct orig_node *)
+		    hash_find(bat_priv->orig_hash, unicast_packet->orig));
+
+	if (!orig_node) {
+		pr_debug("couldn't find originator in orig_hash\n");
+		spin_unlock_irqrestore(&bat_priv->orig_hash_lock, flags);
+		return NULL;
+	}
+
+	orig_node->last_frag_packet = jiffies;
+
+	if (list_empty(&orig_node->frag_list))
+		frag_create_buffer(&orig_node->frag_list);
+
+	tmp_frag_entry = frag_search_packet(&orig_node->frag_list,
+					    unicast_packet);
+
+	if (!tmp_frag_entry) {
+		frag_create_entry(&orig_node->frag_list, skb);
+		goto out;
+	}
+
+	ret = frag_merge_packet(&orig_node->frag_list, tmp_frag_entry, skb);
+out:
+	spin_unlock_irqrestore(&bat_priv->orig_hash_lock, flags);
+
+	return ret;
+}
+
+int frag_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv,
+		  struct batman_if *batman_if, uint8_t dstaddr[])
+{
+	struct unicast_packet tmp_uc, *unicast_packet;
 	struct sk_buff *frag_skb;
+	struct unicast_frag_packet *frag1, *frag2;
+	int uc_hdr_len = sizeof(struct unicast_packet);
+	int ucf_hdr_len = sizeof(struct unicast_frag_packet);
 	int data_len = skb->len;
 
-	if (!bat_priv->primary_if)
-		goto dropped;
+	unicast_packet = (struct unicast_packet *) skb->data;
 
-	frag_skb = dev_alloc_skb(data_len - (data_len / 2) + hdr_len);
+	memcpy(&tmp_uc, unicast_packet, uc_hdr_len);
+	frag_skb = dev_alloc_skb(data_len - (data_len / 2) + ucf_hdr_len);
 	skb_split(skb, frag_skb, data_len / 2);
 
-	if (my_skb_head_push(frag_skb, hdr_len) < 0 ||
-	    my_skb_head_push(skb, hdr_len) < 0)
-		goto drop_frag;
+	if (my_skb_head_push(skb, ucf_hdr_len - uc_hdr_len) < 0 ||
+	    my_skb_head_push(frag_skb, ucf_hdr_len) < 0) {
+		kfree_skb(frag_skb);
+		return NET_RX_DROP;
+	}
 
-	ucast_frag1 = (struct unicast_frag_packet *)skb->data;
-	ucast_frag2 = (struct unicast_frag_packet *)frag_skb->data;
+	frag1 = (struct unicast_frag_packet *)skb->data;
+	frag2 = (struct unicast_frag_packet *)frag_skb->data;
 
-	ucast_frag1->version = COMPAT_VERSION;
-	ucast_frag1->packet_type = BAT_UNICAST_FRAG;
-	ucast_frag1->ttl = TTL;
-	memcpy(ucast_frag1->orig,
-	       bat_priv->primary_if->net_dev->dev_addr, ETH_ALEN);
-	memcpy(ucast_frag1->dest, orig_node->orig, ETH_ALEN);
+	memcpy(frag1, &tmp_uc, sizeof(struct unicast_packet));
 
-	memcpy(ucast_frag2, ucast_frag1, sizeof(struct unicast_frag_packet));
+	frag1->ttl--;
+	frag1->version = COMPAT_VERSION;
+	frag1->packet_type = BAT_UNICAST_FRAG;
 
-	ucast_frag1->flags |= UNI_FRAG_HEAD;
-	ucast_frag2->flags &= ~UNI_FRAG_HEAD;
+	memcpy(frag1->orig, bat_priv->primary_if->net_dev->dev_addr, ETH_ALEN);
+	memcpy(frag2, frag1, sizeof(struct unicast_frag_packet));
 
-	ucast_frag1->seqno = htons((uint16_t)atomic_inc_return(
-						&batman_if->frag_seqno));
+	frag1->flags |= UNI_FRAG_HEAD;
+	frag2->flags &= ~UNI_FRAG_HEAD;
 
-	ucast_frag2->seqno = htons((uint16_t)atomic_inc_return(
-						&batman_if->frag_seqno));
+	frag1->seqno = htons((uint16_t)atomic_inc_return(
+			     &batman_if->frag_seqno));
+	frag2->seqno = htons((uint16_t)atomic_inc_return(
+			     &batman_if->frag_seqno));
 
 	send_skb_packet(skb, batman_if, dstaddr);
 	send_skb_packet(frag_skb, batman_if, dstaddr);
-	return 0;
-
-drop_frag:
-	kfree_skb(frag_skb);
-dropped:
-	kfree_skb(skb);
-	return 1;
+	return NET_RX_SUCCESS;
 }
 
 int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv)
@@ -240,11 +279,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv)
 	if (batman_if->if_status != IF_ACTIVE)
 		goto dropped;
 
-	if (atomic_read(&bat_priv->frag_enabled) &&
-	    data_len + sizeof(struct unicast_packet) > batman_if->net_dev->mtu)
-		return unicast_send_frag_skb(skb, bat_priv, batman_if,
-					     dstaddr, orig_node);
-
 	if (my_skb_head_push(skb, sizeof(struct unicast_packet)) < 0)
 		goto dropped;
 
@@ -258,6 +292,14 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv)
 	/* copy the destination for faster routing */
 	memcpy(unicast_packet->dest, orig_node->orig, ETH_ALEN);
 
+	if (atomic_read(&bat_priv->frag_enabled) &&
+	    data_len + sizeof(struct unicast_packet) >
+	    batman_if->net_dev->mtu) {
+		/* send frag skb decrease ttl */
+		unicast_packet->ttl++;
+		return frag_send_skb(skb, bat_priv, batman_if,
+				     dstaddr);
+	}
 	send_skb_packet(skb, batman_if, dstaddr);
 	return 0;
 
diff --git a/batman-adv/unicast.h b/batman-adv/unicast.h
index 1d5cbeb..d1a606f 100644
--- a/batman-adv/unicast.h
+++ b/batman-adv/unicast.h
@@ -25,15 +25,10 @@
 #define FRAG_TIMEOUT 10000	/* purge frag list entrys after time in ms */
 #define FRAG_BUFFER_SIZE 6	/* number of list elements in buffer */
 
-struct sk_buff *merge_frag_packet(struct list_head *head,
-	struct frag_packet_list_entry *tfp,
-	struct sk_buff *skb);
-
-void create_frag_entry(struct list_head *head, struct sk_buff *skb);
-void create_frag_buffer(struct list_head *head);
-struct frag_packet_list_entry *search_frag_packet(struct list_head *head,
-	struct unicast_frag_packet *up);
 void frag_list_free(struct list_head *head);
+struct sk_buff *frag_reassemble_skb(struct sk_buff *skb,
+				    struct bat_priv *bat_priv);
+int frag_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv,
+		  struct batman_if *batman_if, uint8_t dstaddr[]);
 int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv);
-
 #endif /* _NET_BATMAN_ADV_UNICAST_H_ */
-- 
1.7.0.4

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: layer2 unicast packet fragmentation enhancements

[Sven Eckelmann]

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

Andreas Langer wrote:
> things that changed since the last patch:
> - add support to fragment packets on every node,
>   if outgoing iface mtu is smaller as the packet
> - add support to defragment packets on every node,
>   if outgoing iface mtu is >= original size
> - rename some functions

Is it possible to split the patch in those parts? It would make it easier to 
read it and understand the patches later.

I haven't checked the patch in detail, but scrolled over it a little bit.

Andreas Langer wrote:
> This patch improves the fragmentation by also fragmenting
> packets that came in via an interface which does not need
> fragmentation. In addition it reassembles fragmented packets
> as soon as the interface MTU allows it.
> 
> Signed-off-by: Andreas Langer <an.langer at gmx.de>
> ---
>  batman-adv/routing.c |   64 +++++++++---------------
>  batman-adv/unicast.c |  130
> +++++++++++++++++++++++++++++++++----------------- batman-adv/unicast.h | 
>  13 ++----
>  3 files changed, 114 insertions(+), 93 deletions(-)
> 
> diff --git a/batman-adv/routing.c b/batman-adv/routing.c
> index a2c64a4..e975452 100644
> --- a/batman-adv/routing.c
> +++ b/batman-adv/routing.c
> @@ -1142,15 +1142,11 @@ static int route_unicast_packet(struct sk_buff
> *skb, unsigned long flags;
>  	struct unicast_packet *unicast_packet;
>  	struct ethhdr *ethhdr = (struct ethhdr *)skb_mac_header(skb);
> +	int hdr_lenght = sizeof(struct unicast_packet),
> +	    uni_diff = sizeof(struct unicast_frag_packet) - hdr_lenght;
> 

Could you change lenght to length?

>  	unicast_packet = (struct unicast_packet *)skb->data;
> 
> -	/* packet for me */
> -	if (is_my_mac(unicast_packet->dest)) {
> -		interface_rx(recv_if->soft_iface, skb, hdr_size);
> -		return NET_RX_SUCCESS;
> -	}
> -

There are different parts of the patch which makes ma a little bit curious - 
for example: why it is possible to drop that check entirely? Could that be an 
extra patch with an explanation why that can be dropped? Is it only valid in 
context of the new fragmentation handling? ...

[...]
> -static int unicast_send_frag_skb(struct sk_buff *skb, struct bat_priv
> *bat_priv, -			  struct batman_if *batman_if, uint8_t dstaddr[],
> -			  struct orig_node *orig_node)
> +/* if matching fragment found return reassembled skb
> +   otherwise buffer the fragment */

Could you change the first three spaces to a " * ". Otherwise checkpatch.pl is 
unhappy again.

thanks,
	Sven

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

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: layer2 unicast packet fragmentation enhancements

[Marek Lindner]

On Saturday 11 September 2010 11:26:37 Sven Eckelmann wrote:
> Is it possible to split the patch in those parts? It would make it easier
> to read it and understand the patches later.

I'm not sure that will do much good. He managed to reorganize the code and 
thereby remove redundancies. The second patch would probably be no bigger than 
3-5 lines of code.


> > -	/* packet for me */
> > -	if (is_my_mac(unicast_packet->dest)) {
> > -		interface_rx(recv_if->soft_iface, skb, hdr_size);
> > -		return NET_RX_SUCCESS;
> > -	}
> > -
> 
> There are different parts of the patch which makes ma a little bit curious
> - for example: why it is possible to drop that check entirely? Could that
> be an extra patch with an explanation why that can be dropped? Is it only
> valid in context of the new fragmentation handling? ...

I ran into the same question as it looks a bit odd here but if you apply the 
patch it all looks good. As I said: it seems he managed to purge quite a bit 
of redundancy (fragmented and non-fragmented packets are almost identical 
after all).

Cheers,
Marek

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: layer2 unicast packet fragmentation enhancements

[Andreas Langer]

Am Sat, 11 Sep 2010 11:26:37 +0200
schrieb Sven Eckelmann <sven.eckelmann@gmx.de>:

> Andreas Langer wrote:
> > things that changed since the last patch:
> > - add support to fragment packets on every node,
> >   if outgoing iface mtu is smaller as the packet
> > - add support to defragment packets on every node,
> >   if outgoing iface mtu is >= original size
> > - rename some functions
> 
> Is it possible to split the patch in those parts? It would make it easier to 
> read it and understand the patches later.

I think it makes no sense, it belongs together. But I can put the renaming in an
extra patch. Would that be ok ?

> 
> I haven't checked the patch in detail, but scrolled over it a little bit.
> 
> Andreas Langer wrote:
> > This patch improves the fragmentation by also fragmenting
> > packets that came in via an interface which does not need
> > fragmentation. In addition it reassembles fragmented packets
> > as soon as the interface MTU allows it.
> > 
> > Signed-off-by: Andreas Langer <an.langer at gmx.de>
> > ---
> >  batman-adv/routing.c |   64 +++++++++---------------
> >  batman-adv/unicast.c |  130
> > +++++++++++++++++++++++++++++++++----------------- batman-adv/unicast.h | 
> >  13 ++----
> >  3 files changed, 114 insertions(+), 93 deletions(-)
> > 
> > diff --git a/batman-adv/routing.c b/batman-adv/routing.c
> > index a2c64a4..e975452 100644
> > --- a/batman-adv/routing.c
> > +++ b/batman-adv/routing.c
> > @@ -1142,15 +1142,11 @@ static int route_unicast_packet(struct sk_buff
> > *skb, unsigned long flags;
> >  	struct unicast_packet *unicast_packet;
> >  	struct ethhdr *ethhdr = (struct ethhdr *)skb_mac_header(skb);
> > +	int hdr_lenght = sizeof(struct unicast_packet),
> > +	    uni_diff = sizeof(struct unicast_frag_packet) - hdr_lenght;
> > 
> 
> Could you change lenght to length?

yes i can.


> 
> >  	unicast_packet = (struct unicast_packet *)skb->data;
> > 
> > -	/* packet for me */
> > -	if (is_my_mac(unicast_packet->dest)) {
> > -		interface_rx(recv_if->soft_iface, skb, hdr_size);
> > -		return NET_RX_SUCCESS;
> > -	}
> > -
> 
> There are different parts of the patch which makes ma a little bit curious - 
> for example: why it is possible to drop that check entirely? Could that be an 
> extra patch with an explanation why that can be dropped? Is it only valid in 
> context of the new fragmentation handling? .

No it's not in context, recv_unicast_packet and recv_ucast_frag_packet check this,
so it makes no sense to call it again in route_unicast_packet. I'm not sure if it 
came from the first fragmentation patch, i put it extra.

what are the other parts which makes you curious ?

> 
> [...]
> > -static int unicast_send_frag_skb(struct sk_buff *skb, struct bat_priv
> > *bat_priv, -			  struct batman_if *batman_if, uint8_t dstaddr[],
> > -			  struct orig_node *orig_node)
> > +/* if matching fragment found return reassembled skb
> > +   otherwise buffer the fragment */
> 
> Could you change the first three spaces to a " * ". Otherwise checkpatch.pl is 
> unhappy again.
> 
> thanks,
> 	Sven

thanks, andreas

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: layer2 unicast packet fragmentation enhancements

[Sven Eckelmann]

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

Marek Lindner wrote:
> On Saturday 11 September 2010 11:26:37 Sven Eckelmann wrote:
> > Is it possible to split the patch in those parts? It would make it easier
> > to read it and understand the patches later.
> 
> I'm not sure that will do much good. He managed to reorganize the code and
> thereby remove redundancies. The second patch would probably be no bigger
> than 3-5 lines of code.

That would not be bad. But refactoring and adding two new features isn't 
something I personally like. Guessing what part could belong to which other 
part just makes it hard to find the real problems. Take for example the 
missing failure checks in the patch.

Something I had in mind would be  a patch which adds frag_send_skb and starts 
to use it, one that removes the duplicated code, another one that does the 
cleanup/renaming stuff, the next adds the fragmentation on routing and finally 
there is the defragmentation (probably skipped some steps) - you could much 
more easily check the actual change in its own context. That would for example 
remove the question why those lines were removed - the patch itself would 
answer that question using its commit message which states: "hey those lines 
aren't needed because we already check that fact here and there and don't need 
that information before".

And if a patch real adds a cool feature only by changing some lines and 
without rewriting the whole code then we must have done something right.

If you think different - ok, leave it that way.

> > > -	/* packet for me */
> > > -	if (is_my_mac(unicast_packet->dest)) {
> > > -		interface_rx(recv_if->soft_iface, skb, hdr_size);
> > > -		return NET_RX_SUCCESS;
> > > -	}
> > > -
> > 
> > There are different parts of the patch which makes ma a little bit
> > curious - for example: why it is possible to drop that check entirely?
> > Could that be an extra patch with an explanation why that can be
> > dropped? Is it only valid in context of the new fragmentation handling?
> > ...
> 
> I ran into the same question as it looks a bit odd here but if you apply
> the patch it all looks good. As I said: it seems he managed to purge quite
> a bit of redundancy (fragmented and non-fragmented packets are almost
> identical after all).
 
Andreas Langer wrote:
> what are the other parts which makes you curious ?

That a failure in frag_reassemble_skb is a good successfully rx, that 
frag_create_buffer is magically always successful even when the called 
functions can fail, that you use skb_pull and directly afterward access the 
data pointer, that no one frees the skb when frag_reassemble_skb cannot find a 
matching originator, ...

Best regards,
	Sven

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

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: layer2 unicast packet fragmentation enhancements

[Andreas Langer]

Am Sun, 12 Sep 2010 22:11:06 +0200
schrieb Sven Eckelmann <sven.eckelmann@gmx.de>:

> Marek Lindner wrote:
> > On Saturday 11 September 2010 11:26:37 Sven Eckelmann wrote:
> > > Is it possible to split the patch in those parts? It would make it easier
> > > to read it and understand the patches later.
> > 
> > I'm not sure that will do much good. He managed to reorganize the code and
> > thereby remove redundancies. The second patch would probably be no bigger
> > than 3-5 lines of code.
> 
> That would not be bad. But refactoring and adding two new features isn't 
> something I personally like. Guessing what part could belong to which other 
> part just makes it hard to find the real problems. Take for example the 
> missing failure checks in the patch.
> 
> Something I had in mind would be  a patch which adds frag_send_skb and starts 
> to use it, one that removes the duplicated code, another one that does the 
> cleanup/renaming stuff, the next adds the fragmentation on routing and finally 
> there is the defragmentation (probably skipped some steps) - you could much 
> more easily check the actual change in its own context. That would for example 
> remove the question why those lines were removed - the patch itself would 
> answer that question using its commit message which states: "hey those lines 
> aren't needed because we already check that fact here and there and don't need 
> that information before".
> 
> And if a patch real adds a cool feature only by changing some lines and 
> without rewriting the whole code then we must have done something right.
> 
> If you think different - ok, leave it that way.

No it's ok, you are right. I will split the patch.
> 
> > > > -	/* packet for me */
> > > > -	if (is_my_mac(unicast_packet->dest)) {
> > > > -		interface_rx(recv_if->soft_iface, skb, hdr_size);
> > > > -		return NET_RX_SUCCESS;
> > > > -	}
> > > > -
> > > 
> > > There are different parts of the patch which makes ma a little bit
> > > curious - for example: why it is possible to drop that check entirely?
> > > Could that be an extra patch with an explanation why that can be
> > > dropped? Is it only valid in context of the new fragmentation handling?
> > > ...
> > 
> > I ran into the same question as it looks a bit odd here but if you apply
> > the patch it all looks good. As I said: it seems he managed to purge quite
> > a bit of redundancy (fragmented and non-fragmented packets are almost
> > identical after all).
>  
> Andreas Langer wrote:
> > what are the other parts which makes you curious ?
> 
> That a failure in frag_reassemble_skb is a good successfully rx, that 
> frag_create_buffer is magically always successful even when the called 
> functions can fail, that you use skb_pull and directly afterward access the 
> data pointer, that no one frees the skb when frag_reassemble_skb cannot find a 
> matching originator, ...

I will look into it.Thanks for your hints.

regards,
andreas

[B.A.T.M.A.N.] batman-adv: unicast fragmentation enhancements

[Andreas Langer]

Hi again,

things that changed since the last patch:
- add support to fragment packets on every node,
  if outgoing iface mtu is smaller as the packet
- add support to defragment packets on every node,
  if outgoing iface mtu is >= original size
- rename some functions

In this patch every node defragment packets if it can. Do you think it
is better to add an option to enable/disable this ?



Sven, any idea why checkpatch (0.31)b do this ?

ERROR: space prohibited after that '-' (ctx:WxW)
#10: FILE: batman-adv/routing.c:1146:
+	    uni_diff = sizeof(struct unicast_frag_packet) - hdr_len;
 	                                                  ^



regards,
Andreas

Re: [B.A.T.M.A.N.] batman-adv: unicast fragmentation enhancements

[Sven Eckelmann]

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

Andreas Langer wrote:
> Sven, any idea why checkpatch (0.31)b do this ?
> 
> ERROR: space prohibited after that '-' (ctx:WxW)
> #10: FILE: batman-adv/routing.c:1146:
> +	    uni_diff = sizeof(struct unicast_frag_packet) - hdr_len;

Because... what the heck? Usually it says that you should use consistent 
spaces around '-'. So better use spaces in patch 1 and 6 and don't remove the 
space before hdr_len as you did now.

My version from linux-next (today) is 0.30 and doesn't have a problem with 
spaces around those '-' (but with your version of the patches). The 
checkpatch.pl-testing also don't have the problem.

I would guess that it was a regression in checkpatch.pl. Maybe Andy Whitcroft 
knows more about it

Best regards,
	Sven

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

Re: [B.A.T.M.A.N.] batman-adv: unicast fragmentation enhancements

[Andy Whitcroft]

On Fri, Sep 17, 2010 at 12:27:13AM +0200, Sven Eckelmann wrote:
> Andreas Langer wrote:
> > Sven, any idea why checkpatch (0.31)b do this ?
> > 
> > ERROR: space prohibited after that '-' (ctx:WxW)
> > #10: FILE: batman-adv/routing.c:1146:
> > +	    uni_diff = sizeof(struct unicast_frag_packet) - hdr_len;
> 
> Because... what the heck? Usually it says that you should use consistent 
> spaces around '-'. So better use spaces in patch 1 and 6 and don't remove the 
> space before hdr_len as you did now.

Because it is wrong.  I believe that I've fixed that one up in my
development tree.  Could you confirm it is correctly left alone with the
version below:

  http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

> My version from linux-next (today) is 0.30 and doesn't have a problem with 
> spaces around those '-' (but with your version of the patches). The 
> checkpatch.pl-testing also don't have the problem.
> 
> I would guess that it was a regression in checkpatch.pl. Maybe Andy Whitcroft 
> knows more about it

-apw

Re: [B.A.T.M.A.N.] batman-adv: unicast fragmentation enhancements

[Sven Eckelmann]

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

Andy Whitcroft wrote:
> On Fri, Sep 17, 2010 at 12:27:13AM +0200, Sven Eckelmann wrote:
> > Andreas Langer wrote:
> > > Sven, any idea why checkpatch (0.31)b do this ?
> > > 
> > > ERROR: space prohibited after that '-' (ctx:WxW)
> > > #10: FILE: batman-adv/routing.c:1146:
> > > +	    uni_diff = sizeof(struct unicast_frag_packet) - hdr_len;
> > 
> > Because... what the heck? Usually it says that you should use consistent
> > spaces around '-'. So better use spaces in patch 1 and 6 and don't remove
> > the space before hdr_len as you did now.
> 
> Because it is wrong.  I believe that I've fixed that one up in my
> development tree.  Could you confirm it is correctly left alone with the
> version below:
> 
>  
> http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl
> -testing

Yes, the testing version doesn't show that behavior.

thanks,
	Sven

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

Re: [B.A.T.M.A.N.] batman-adv: unicast fragmentation enhancements

[Marek Lindner]

Hi,

> things that changed since the last patch:
> - add support to fragment packets on every node,
>   if outgoing iface mtu is smaller as the packet
> - add support to defragment packets on every node,
>   if outgoing iface mtu is >= original size
> - rename some functions

I just merged your patchset (revision 1810-1815) into the trunk since 
everybody seemed to be happy with it. :-)

Thanks for your work on the fragmentation,
Marek