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); }
/**
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; }
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,
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
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,
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,
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
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
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
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
Marek Lindner -
Simon Wunderlich