[B.A.T.M.A.N.] [PATCH 1/2] batman-adv: move and restructure batadv_v_ogm_forward

[B.A.T.M.A.N.] [PATCH 1/2] batman-adv: move and restructure batadv_v_ogm_forward

[Simon Wunderlich]

From: Simon Wunderlich <simon@open-mesh.com>

To match our code better to the protocol description of B.A.T.M.A.N. V,
move batadv_v_ogm_forward() out into batadv_v_ogm_process_per_outif()
and move all checks directly deciding whether the OGM should be
forwarded into batadv_v_ogm_forward().

Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
---
 net/batman-adv/bat_v_ogm.c | 102 +++++++++++++++++++++++++--------------------
 1 file changed, 57 insertions(+), 45 deletions(-)

diff --git a/net/batman-adv/bat_v_ogm.c b/net/batman-adv/bat_v_ogm.c
index d9bcbe6..d6e89a7 100644
--- a/net/batman-adv/bat_v_ogm.c
+++ b/net/batman-adv/bat_v_ogm.c
@@ -347,10 +347,11 @@ static u32 batadv_v_forward_penalty(struct batadv_priv *bat_priv,
 }
 
 /**
- * batadv_v_ogm_forward - forward an OGM to the given outgoing interface
+ * batadv_v_ogm_forward - check conditions and forward an OGM to the given
+ *  outgoing interface
  * @bat_priv: the bat priv with all the soft interface information
  * @ogm_received: previously received OGM to be forwarded
- * @throughput: throughput to announce, may vary per outgoing interface
+ * @neigh_node: the neigh_node through with the OGM has been received
  * @if_incoming: the interface on which this OGM was received on
  * @if_outgoing: the interface to which the OGM has to be forwarded to
  *
@@ -359,28 +360,55 @@ static u32 batadv_v_forward_penalty(struct batadv_priv *bat_priv,
  */
 static void batadv_v_ogm_forward(struct batadv_priv *bat_priv,
 				 const struct batadv_ogm2_packet *ogm_received,
-				 u32 throughput,
+				 struct batadv_neigh_node *neigh_node,
 				 struct batadv_hard_iface *if_incoming,
 				 struct batadv_hard_iface *if_outgoing)
 {
+	struct batadv_neigh_ifinfo *neigh_ifinfo = NULL;
+	struct batadv_orig_ifinfo *orig_ifinfo = NULL;
+	struct batadv_neigh_node *router = NULL;
 	struct batadv_ogm2_packet *ogm_forward;
 	unsigned char *skb_buff;
 	struct sk_buff *skb;
 	size_t packet_len;
 	u16 tvlv_len;
 
+	/* only forward for specific interfaces, not for the default one. */
+	if (if_outgoing != BATADV_IF_DEFAULT)
+		goto out;
+
+	orig_ifinfo = batadv_orig_ifinfo_new(orig_node, if_outgoing);
+	if (!orig_ifinfo)
+		goto out;
+	/* acquire possibly updated router */
+	router = batadv_orig_router_get(orig_node, if_outgoing);
+
+	/* strict rule: forward packets coming from the best next hop only */
+	if (neigh_node != router)
+		goto out;
+
+	/* don't forward the same seqno twice on one interface */
+	if (orig_ifinfo->last_seqno_forwarded == ntohl(ogm2->seqno))
+		goto out;
+
+	orig_ifinfo->last_seqno_forwarded = ntohl(ogm2->seqno);
+
 	if (ogm_received->ttl <= 1) {
 		batadv_dbg(BATADV_DBG_BATMAN, bat_priv, "ttl exceeded\n");
-		return;
+		goto out;
 	}
 
+	neigh_ifinfo = batadv_neigh_ifinfo_get(neigh_node, if_outgoing);
+	if (!neigh_ifinfo)
+		goto out;
+
 	tvlv_len = ntohs(ogm_received->tvlv_len);
 
 	packet_len = BATADV_OGM2_HLEN + tvlv_len;
 	skb = netdev_alloc_skb_ip_align(if_outgoing->net_dev,
 					ETH_HLEN + packet_len);
 	if (!skb)
-		return;
+		goto out;
 
 	skb_reserve(skb, ETH_HLEN);
 	skb_buff = skb_put(skb, packet_len);
@@ -388,7 +416,7 @@ static void batadv_v_ogm_forward(struct batadv_priv *bat_priv,
 
 	/* apply forward penalty */
 	ogm_forward = (struct batadv_ogm2_packet *)skb_buff;
-	ogm_forward->throughput = htonl(throughput);
+	ogm_forward->throughput = htonl(neigh_ifinfo->bat_v.throughput);
 	ogm_forward->ttl--;
 
 	batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
@@ -397,6 +425,14 @@ static void batadv_v_ogm_forward(struct batadv_priv *bat_priv,
 		   if_incoming->net_dev->name);
 
 	batadv_v_ogm_send_to_if(skb, if_outgoing);
+
+out:
+	if (orig_ifinfo)
+		batadv_orig_ifinfo_put(orig_ifinfo);
+	if (router)
+		batadv_neigh_node_put(router);
+	if (neigh_ifinfo)
+		batadv_neigh_ifinfo_put(neigh_ifinfo);
 }
 
 /**
@@ -493,8 +529,10 @@ out:
  * @neigh_node: the neigh_node through with the OGM has been received
  * @if_incoming: the interface where this packet was received
  * @if_outgoing: the interface for which the packet should be considered
+ *
+ * Return: true if the packet should be forwarded, false otherwise
  */
-static void batadv_v_ogm_route_update(struct batadv_priv *bat_priv,
+static bool batadv_v_ogm_route_update(struct batadv_priv *bat_priv,
 				      const struct ethhdr *ethhdr,
 				      const struct batadv_ogm2_packet *ogm2,
 				      struct batadv_orig_node *orig_node,
@@ -503,14 +541,9 @@ static void batadv_v_ogm_route_update(struct batadv_priv *bat_priv,
 				      struct batadv_hard_iface *if_outgoing)
 {
 	struct batadv_neigh_node *router = NULL;
-	struct batadv_neigh_ifinfo *neigh_ifinfo = NULL;
 	struct batadv_orig_node *orig_neigh_node = NULL;
-	struct batadv_orig_ifinfo *orig_ifinfo = NULL;
 	struct batadv_neigh_node *orig_neigh_router = NULL;
-
-	neigh_ifinfo = batadv_neigh_ifinfo_get(neigh_node, if_outgoing);
-	if (!neigh_ifinfo)
-		goto out;
+	bool forward = false;
 
 	orig_neigh_node = batadv_v_ogm_orig_get(bat_priv, ethhdr->h_source);
 	if (!orig_neigh_node)
@@ -529,47 +562,20 @@ static void batadv_v_ogm_route_update(struct batadv_priv *bat_priv,
 		goto out;
 	}
 
-	if (router)
-		batadv_neigh_node_put(router);
-
 	/* Update routes, and check if the OGM is from the best next hop */
 	batadv_v_ogm_orig_update(bat_priv, orig_node, neigh_node, ogm2,
 				 if_outgoing);
 
-	orig_ifinfo = batadv_orig_ifinfo_new(orig_node, if_outgoing);
-	if (!orig_ifinfo)
-		goto out;
-
-	/* don't forward the same seqno twice on one interface */
-	if (orig_ifinfo->last_seqno_forwarded == ntohl(ogm2->seqno))
-		goto out;
-
-	/* acquire possibly updated router */
-	router = batadv_orig_router_get(orig_node, if_outgoing);
-
-	/* strict rule: forward packets coming from the best next hop only */
-	if (neigh_node != router)
-		goto out;
-
-	/* only forward for specific interface, not for the default one. */
-	if (if_outgoing != BATADV_IF_DEFAULT) {
-		orig_ifinfo->last_seqno_forwarded = ntohl(ogm2->seqno);
-		batadv_v_ogm_forward(bat_priv, ogm2,
-				     neigh_ifinfo->bat_v.throughput,
-				     if_incoming, if_outgoing);
-	}
-
+	forward = true;
 out:
-	if (orig_ifinfo)
-		batadv_orig_ifinfo_put(orig_ifinfo);
 	if (router)
 		batadv_neigh_node_put(router);
 	if (orig_neigh_router)
 		batadv_neigh_node_put(orig_neigh_router);
 	if (orig_neigh_node)
 		batadv_orig_node_put(orig_neigh_node);
-	if (neigh_ifinfo)
-		batadv_neigh_ifinfo_put(neigh_ifinfo);
+
+	return forward;
 }
 
 /**
@@ -610,8 +616,14 @@ batadv_v_ogm_process_per_outif(struct batadv_priv *bat_priv,
 					       ntohs(ogm2->tvlv_len));
 
 	/* if the metric update went through, update routes if needed */
-	batadv_v_ogm_route_update(bat_priv, ethhdr, ogm2, orig_node,
-				  neigh_node, if_incoming, if_outgoing);
+	forward = batadv_v_ogm_route_update(bat_priv, ethhdr, ogm2, orig_node,
+					    neigh_node, if_incoming,
+					    if_outgoing);
+
+	/* if the routes have been processed correctly, check and forward */
+	if (forward)
+		bat_v_ogm_forward(bat_priv, ogm2, neigh_node, if_incoming,
+				  if_outgoing);
 }
 
 /**
-- 
2.7.0.rc3

[B.A.T.M.A.N.] [PATCH 2/2] batman-adv: Merge batadv_v_ogm_orig_update into batadv_v_ogm_route_update

[Simon Wunderlich]

From: Simon Wunderlich <simon@open-mesh.com>

Since batadv_v_ogm_orig_update() was only called from one place and the
calling function became very short, merge these two functions together.

This should also reflect the protocol description of B.A.T.M.A.N. V
better.

Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
---
 net/batman-adv/bat_v_ogm.c | 117 ++++++++++++++++++---------------------------
 1 file changed, 46 insertions(+), 71 deletions(-)

diff --git a/net/batman-adv/bat_v_ogm.c b/net/batman-adv/bat_v_ogm.c
index d6e89a7..c7eb6fb 100644
--- a/net/batman-adv/bat_v_ogm.c
+++ b/net/batman-adv/bat_v_ogm.c
@@ -234,73 +234,6 @@ void batadv_v_ogm_primary_iface_set(struct batadv_hard_iface *primary_iface)
 }
 
 /**
- * batadv_v_ogm_orig_update - update the originator status based on the received
- *  OGM
- * @bat_priv: the bat priv with all the soft interface information
- * @orig_node: the originator to update
- * @neigh_node: the neighbour the OGM has been received from (to update)
- * @ogm2: the received OGM
- * @if_outgoing: the interface where this OGM is going to be forwarded through
- */
-static void
-batadv_v_ogm_orig_update(struct batadv_priv *bat_priv,
-			 struct batadv_orig_node *orig_node,
-			 struct batadv_neigh_node *neigh_node,
-			 const struct batadv_ogm2_packet *ogm2,
-			 struct batadv_hard_iface *if_outgoing)
-{
-	struct batadv_neigh_ifinfo *router_ifinfo = NULL, *neigh_ifinfo = NULL;
-	struct batadv_neigh_node *router = NULL;
-	s32 neigh_seq_diff;
-	u32 neigh_last_seqno;
-	u32 router_last_seqno;
-	u32 router_throughput, neigh_throughput;
-
-	batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
-		   "Searching and updating originator entry of received packet\n");
-
-	/* if this neighbor already is our next hop there is nothing
-	 * to change
-	 */
-	router = batadv_orig_router_get(orig_node, if_outgoing);
-	if (router == neigh_node)
-		goto out;
-
-	/* don't consider neighbours with worse throughput.
-	 * also switch route if this seqno is BATADV_V_MAX_ORIGDIFF newer than
-	 * the last received seqno from our best next hop.
-	 */
-	if (router) {
-		router_ifinfo = batadv_neigh_ifinfo_get(router, if_outgoing);
-		neigh_ifinfo = batadv_neigh_ifinfo_get(neigh_node, if_outgoing);
-
-		/* if these are not allocated, something is wrong. */
-		if (!router_ifinfo || !neigh_ifinfo)
-			goto out;
-
-		neigh_last_seqno = neigh_ifinfo->bat_v.last_seqno;
-		router_last_seqno = router_ifinfo->bat_v.last_seqno;
-		neigh_seq_diff = neigh_last_seqno - router_last_seqno;
-		router_throughput = router_ifinfo->bat_v.throughput;
-		neigh_throughput = neigh_ifinfo->bat_v.throughput;
-
-		if ((neigh_seq_diff < BATADV_OGM_MAX_ORIGDIFF) &&
-		    (router_throughput >= neigh_throughput))
-			goto out;
-	}
-
-	batadv_update_route(bat_priv, orig_node, if_outgoing, neigh_node);
-
-out:
-	if (router_ifinfo)
-		batadv_neigh_ifinfo_put(router_ifinfo);
-	if (neigh_ifinfo)
-		batadv_neigh_ifinfo_put(neigh_ifinfo);
-	if (router)
-		batadv_neigh_node_put(router);
-}
-
-/**
  * batadv_v_forward_penalty - apply a penalty to the throughput metric forwarded
  *  with B.A.T.M.A.N. V OGMs
  * @bat_priv: the bat priv with all the soft interface information
@@ -543,6 +476,11 @@ static bool batadv_v_ogm_route_update(struct batadv_priv *bat_priv,
 	struct batadv_neigh_node *router = NULL;
 	struct batadv_orig_node *orig_neigh_node = NULL;
 	struct batadv_neigh_node *orig_neigh_router = NULL;
+	struct batadv_neigh_ifinfo *router_ifinfo = NULL, *neigh_ifinfo = NULL;
+	u32 router_throughput, neigh_throughput;
+	u32 router_last_seqno;
+	u32 neigh_last_seqno;
+	s32 neigh_seq_diff;
 	bool forward = false;
 
 	orig_neigh_node = batadv_v_ogm_orig_get(bat_priv, ethhdr->h_source);
@@ -562,11 +500,44 @@ static bool batadv_v_ogm_route_update(struct batadv_priv *bat_priv,
 		goto out;
 	}
 
-	/* Update routes, and check if the OGM is from the best next hop */
-	batadv_v_ogm_orig_update(bat_priv, orig_node, neigh_node, ogm2,
-				 if_outgoing);
-
+	/* Mark the OGM to be considered for forwarding, and update routes
+	 * if needed.
+	 */
 	forward = true;
+
+	batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
+		   "Searching and updating originator entry of received packet\n");
+
+	/* if this neighbor already is our next hop there is nothing
+	 * to change
+	 */
+	if (router == neigh_node)
+		goto out;
+
+	/* don't consider neighbours with worse throughput.
+	 * also switch route if this seqno is BATADV_V_MAX_ORIGDIFF newer than
+	 * the last received seqno from our best next hop.
+	 */
+	if (router) {
+		router_ifinfo = batadv_neigh_ifinfo_get(router, if_outgoing);
+		neigh_ifinfo = batadv_neigh_ifinfo_get(neigh_node, if_outgoing);
+
+		/* if these are not allocated, something is wrong. */
+		if (!router_ifinfo || !neigh_ifinfo)
+			goto out;
+
+		neigh_last_seqno = neigh_ifinfo->bat_v.last_seqno;
+		router_last_seqno = router_ifinfo->bat_v.last_seqno;
+		neigh_seq_diff = neigh_last_seqno - router_last_seqno;
+		router_throughput = router_ifinfo->bat_v.throughput;
+		neigh_throughput = neigh_ifinfo->bat_v.throughput;
+
+		if ((neigh_seq_diff < BATADV_OGM_MAX_ORIGDIFF) &&
+		    (router_throughput >= neigh_throughput))
+			goto out;
+	}
+
+	batadv_update_route(bat_priv, orig_node, if_outgoing, neigh_node);
 out:
 	if (router)
 		batadv_neigh_node_put(router);
@@ -574,6 +545,10 @@ out:
 		batadv_neigh_node_put(orig_neigh_router);
 	if (orig_neigh_node)
 		batadv_orig_node_put(orig_neigh_node);
+	if (router_ifinfo)
+		batadv_neigh_ifinfo_put(router_ifinfo);
+	if (neigh_ifinfo)
+		batadv_neigh_ifinfo_put(neigh_ifinfo);
 
 	return forward;
 }
-- 
2.7.0.rc3

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: move and restructure batadv_v_ogm_forward

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1733 bytes --]



On 20/01/16 21:32, Simon Wunderlich wrote:
>  static void batadv_v_ogm_forward(struct batadv_priv *bat_priv,
>  				 const struct batadv_ogm2_packet *ogm_received,
> -				 u32 throughput,
> +				 struct batadv_neigh_node *neigh_node,
>  				 struct batadv_hard_iface *if_incoming,
>  				 struct batadv_hard_iface *if_outgoing)
>  {
> +	struct batadv_neigh_ifinfo *neigh_ifinfo = NULL;
> +	struct batadv_orig_ifinfo *orig_ifinfo = NULL;
> +	struct batadv_neigh_node *router = NULL;
>  	struct batadv_ogm2_packet *ogm_forward;
>  	unsigned char *skb_buff;
>  	struct sk_buff *skb;
>  	size_t packet_len;
>  	u16 tvlv_len;
>  
> +	/* only forward for specific interfaces, not for the default one. */
> +	if (if_outgoing != BATADV_IF_DEFAULT)
> +		goto out;
> +

personally I'd prefer the forward function to do what it says: forward
the OGM. The checks whether or not we should do this should stay in
ogm_process_per_outif().

> +	orig_ifinfo = batadv_orig_ifinfo_new(orig_node, if_outgoing);
> +	if (!orig_ifinfo)
> +		goto out;
> +	/* acquire possibly updated router */
> +	router = batadv_orig_router_get(orig_node, if_outgoing);
> +
> +	/* strict rule: forward packets coming from the best next hop only */
> +	if (neigh_node != router)
> +		goto out;
> +

this is changing the behaviour.
here now we get a router which potentially was elected during the
previous update_route() call while processing this very OGM. We are
still discussing if we want to do this or not, but this patch should be
just a style change, while this is not.

This check should be performed against the router which was selected
before performing any route update.


Cheers,

-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: move and restructure batadv_v_ogm_forward

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 2667 bytes --]

On Wednesday 20 January 2016 22:34:25 Antonio Quartulli wrote:
> On 20/01/16 21:32, Simon Wunderlich wrote:
> >  static void batadv_v_ogm_forward(struct batadv_priv *bat_priv,
> >  
> >  				 const struct batadv_ogm2_packet *ogm_received,
> > 
> > -				 u32 throughput,
> > +				 struct batadv_neigh_node *neigh_node,
> > 
> >  				 struct batadv_hard_iface *if_incoming,
> >  				 struct batadv_hard_iface *if_outgoing)
> >  
> >  {
> > 
> > +	struct batadv_neigh_ifinfo *neigh_ifinfo = NULL;
> > +	struct batadv_orig_ifinfo *orig_ifinfo = NULL;
> > +	struct batadv_neigh_node *router = NULL;
> > 
> >  	struct batadv_ogm2_packet *ogm_forward;
> >  	unsigned char *skb_buff;
> >  	struct sk_buff *skb;
> >  	size_t packet_len;
> >  	u16 tvlv_len;
> > 
> > +	/* only forward for specific interfaces, not for the default one. */
> > +	if (if_outgoing != BATADV_IF_DEFAULT)
> > +		goto out;
> > +
> 
> personally I'd prefer the forward function to do what it says: forward
> the OGM. The checks whether or not we should do this should stay in
> ogm_process_per_outif().
> 

Hm, yeah we discussed to keep these changes in route_update, but actually 
those checks decide whether we forward or not (i.e. directly involving 
forwarding). Therefore I thought they would fit better in the forward function 
and also move the corresponding description [1] in the OGMv2 wiki page from 
route update to forward. We don't have a matching "general" description in the 
OGMv2 wiki page which corresponds to ogm_process_per_outif() after all.

Any other opinions?

[1] From the OGMv2 wiki page: If the OGMv2 has been received by the (now) 
selected router, the OGM is forwarded on the considered outgoing interface 
(except for the default interface). However, the OGMv2 is not forwarded if 
another OGMv2 has been forwarded with the same sequence number.
> > +	orig_ifinfo = batadv_orig_ifinfo_new(orig_node, if_outgoing);
> > +	if (!orig_ifinfo)
> > +		goto out;
> > +	/* acquire possibly updated router */
> > +	router = batadv_orig_router_get(orig_node, if_outgoing);
> > +
> > +	/* strict rule: forward packets coming from the best next hop only */
> > +	if (neigh_node != router)
> > +		goto out;
> > +
> 
> this is changing the behaviour.
> here now we get a router which potentially was elected during the
> previous update_route() call while processing this very OGM. We are
> still discussing if we want to do this or not, but this patch should be
> just a style change, while this is not.

No, this is already in the code which is merged into master - we already 
acquire the updated router (see bat_v_ogm.c:547, function 
batadv_v_ogm_route_update()).

Cheers,
    Simon

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

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: move and restructure batadv_v_ogm_forward

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]



On 20/01/16 23:18, Simon Wunderlich wrote:
>> this is changing the behaviour.
>> here now we get a router which potentially was elected during the
>> previous update_route() call while processing this very OGM. We are
>> still discussing if we want to do this or not, but this patch should be
>> just a style change, while this is not.
> 
> No, this is already in the code which is merged into master - we already 
> acquire the updated router (see bat_v_ogm.c:547, function 
> batadv_v_ogm_route_update()).

uhuhuh?! Actually you are right!
This means we currently send one OGM every time we make an election,
thus we might send multiple OGMs with the sequence numnber, despite this
is still under debate.

As far as I remember did not want to follow this approach at the moment?
Am I missing something?


Cheers,

-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: move and restructure batadv_v_ogm_forward

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]



On 20/01/16 23:26, Antonio Quartulli wrote:
> 
> 
> On 20/01/16 23:18, Simon Wunderlich wrote:
>>> this is changing the behaviour.
>>> here now we get a router which potentially was elected during the
>>> previous update_route() call while processing this very OGM. We are
>>> still discussing if we want to do this or not, but this patch should be
>>> just a style change, while this is not.
>>
>> No, this is already in the code which is merged into master - we already 
>> acquire the updated router (see bat_v_ogm.c:547, function 
>> batadv_v_ogm_route_update()).
> 
> uhuhuh?! Actually you are right!
> This means we currently send one OGM every time we make an election,
> thus we might send multiple OGMs with the sequence numnber, despite this
> is still under debate.
> 
> As far as I remember did not want to follow this approach at the moment?
> Am I missing something?
> 

I was missing this:

+       /* don't forward the same seqno twice on one interface */
+       if (orig_ifinfo->last_seqno_forwarded == ntohl(ogm2->seqno))
+               goto out;
+

thanks Marek for pointing this out for me :)

it's all good then!

Cheers,

-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: move and restructure batadv_v_ogm_forward

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 1304 bytes --]

On Wednesday 20 January 2016 23:26:15 Antonio Quartulli wrote:
> On 20/01/16 23:18, Simon Wunderlich wrote:
> >> this is changing the behaviour.
> >> here now we get a router which potentially was elected during the
> >> previous update_route() call while processing this very OGM. We are
> >> still discussing if we want to do this or not, but this patch should be
> >> just a style change, while this is not.
> > 
> > No, this is already in the code which is merged into master - we already
> > acquire the updated router (see bat_v_ogm.c:547, function
> > batadv_v_ogm_route_update()).
> 
> uhuhuh?! Actually you are right!
> This means we currently send one OGM every time we make an election,
> thus we might send multiple OGMs with the sequence numnber, despite this
> is still under debate.

No, we only send once per sequence number, see below.
> 
> As far as I remember did not want to follow this approach at the moment?
> Am I missing something?

Right now we store the last sent sequence number for an originator in 
orig_ifinfo->last_seqno_forwarded when forwarding. This field also gets checked 
and the OGMv2 gets dropped before forwarding when receive it again.

So yes, we send immediately after we make a new election, but only send an OGM 
once per sequence number.

Cheers,
     Simon

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

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: move and restructure batadv_v_ogm_forward

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

On Wednesday 20 January 2016 23:29:21 Antonio Quartulli wrote:
> On 20/01/16 23:26, Antonio Quartulli wrote:
> > On 20/01/16 23:18, Simon Wunderlich wrote:
> >>> this is changing the behaviour.
> >>> here now we get a router which potentially was elected during the
> >>> previous update_route() call while processing this very OGM. We are
> >>> still discussing if we want to do this or not, but this patch should be
> >>> just a style change, while this is not.
> >> 
> >> No, this is already in the code which is merged into master - we already
> >> acquire the updated router (see bat_v_ogm.c:547, function
> >> batadv_v_ogm_route_update()).
> > 
> > uhuhuh?! Actually you are right!
> > This means we currently send one OGM every time we make an election,
> > thus we might send multiple OGMs with the sequence numnber, despite this
> > is still under debate.
> > 
> > As far as I remember did not want to follow this approach at the moment?
> > Am I missing something?
> 
> I was missing this:
> 
> +       /* don't forward the same seqno twice on one interface */
> +       if (orig_ifinfo->last_seqno_forwarded == ntohl(ogm2->seqno))
> +               goto out;
> +
> 
> thanks Marek for pointing this out for me :)
> 
> it's all good then!

Ok cool! I guess we just need a conclusion where we put the conditions, then. 
:)

Cheers,
    Simon

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

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: move and restructure batadv_v_ogm_forward

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 2021 bytes --]

On Wednesday, January 20, 2016 14:32:14 Simon Wunderlich wrote:
>  /**
> - * batadv_v_ogm_forward - forward an OGM to the given outgoing interface
> + * batadv_v_ogm_forward - check conditions and forward an OGM to the given
> + *  outgoing interface
>   * @bat_priv: the bat priv with all the soft interface information
>   * @ogm_received: previously received OGM to be forwarded
> - * @throughput: throughput to announce, may vary per outgoing interface
> + * @neigh_node: the neigh_node through with the OGM has been received
>   * @if_incoming: the interface on which this OGM was received on
>   * @if_outgoing: the interface to which the OGM has to be forwarded to
>   *
> @@ -359,28 +360,55 @@ static u32 batadv_v_forward_penalty(struct batadv_priv
> *bat_priv, */
>  static void batadv_v_ogm_forward(struct batadv_priv *bat_priv,
>  				 const struct batadv_ogm2_packet *ogm_received,
> -				 u32 throughput,
> +				 struct batadv_neigh_node *neigh_node,
>  				 struct batadv_hard_iface *if_incoming,
>  				 struct batadv_hard_iface *if_outgoing)
>  {
> +	struct batadv_neigh_ifinfo *neigh_ifinfo = NULL;
> +	struct batadv_orig_ifinfo *orig_ifinfo = NULL;
> +	struct batadv_neigh_node *router = NULL;
>  	struct batadv_ogm2_packet *ogm_forward;
>  	unsigned char *skb_buff;
>  	struct sk_buff *skb;
>  	size_t packet_len;
>  	u16 tvlv_len;
> 
> +	/* only forward for specific interfaces, not for the default one. */
> +	if (if_outgoing != BATADV_IF_DEFAULT)
> +		goto out;
> +
> +	orig_ifinfo = batadv_orig_ifinfo_new(orig_node, if_outgoing);

The variable 'orig_node' isn't defined anywhere in the context of this 
function. It was passed as an argument to batadv_v_ogm_route_update().

Please recompile with 'make CONFIG_BATMAN_ADV_BATMAN_V=y' and fix all errors 
and warnings.  :-)


> +	if (!orig_ifinfo)
> +		goto out;
> +	/* acquire possibly updated router */
> +	router = batadv_orig_router_get(orig_node, if_outgoing);

A new line between the goto and the comment would be nice to have.

Cheers,
Marek

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