[Olsr-dev] [PATCH][BUILD] check_neighbor_link is wrong
Giovanni Di Stasi
(spam-protected)
Mon Dec 10 20:34:49 CET 2012
Il 10/12/2012 10:12, Henning Rogge ha scritto:
> On 12/09/2012 03:00 PM, Saverio Proto wrote:
>> What about this patch ?
>> I did not see any discussion... did it went silently discarded ?
>>
>> In Ninux we have mostly single radio routers so it is hard for me to reproduce.
>>
>> I understood the patch helps when two routers are connected to each
>> other with two links, where at least one router has two olsrd
>> interfaces.
> Had no large timeslot to look at it until today.
>
> First review below.
>
>> 2012/11/12 Giovanni Di Stasi <(spam-protected)>:
>>> Check_neighbor_link function searches a link based only on the receiver interface address. This is not correct as two links may exist toward the same neighbor interface. In that case the function considers only the first link, neglecting the others (which may be better). The patch makes sure that the right link is considered by comparing both the local_address and the remote_address.
>>>
>>> Consider as an example this scenario: two nodes, each equipped with two wireless interfaces; first node has its interfaces on channels 1 and 2, while the second on channels 1 and 5. After a while, node 1 switches the first interface from channel 1 to 5. At that point two links towards node 2 exist (in the link set) towards the same neighbor interface (the old on channel 1 and the new on channel 5). At that point, check_neighbor_link only considers, as previously steted, the old link and the neighbor is soon considered not reachable (because that link becomes lost). Eventually, the old link is deleted from the link set and the new link is considered, making the neighbor reachable again after a long interruption. The downtime with the patch is reduced from about 35 seconds to 6-7 seconds (with hysteresis and default configuration).
>>>
>>> The patch also fixes get_best_link_to_neighbor (in case of RFC behaviour). Indeed, as in the previous case, code was just picking the first link without considering the others. So, if the first link is asymmetric and the second is symmetric (and with the same metric), the first one is taken as backup and the good link is NULL.
>>>
>>> Signed-off-by: Giovanni Di Stasi <(spam-protected)>
>>> ---
>>> src/hna_set.c | 4 ++--
>>> src/link_set.c | 9 ++++-----
>>> src/link_set.h | 2 +-
>>> src/mid_set.c | 4 ++--
>>> src/tc_set.c | 4 ++--
>>> 5 files changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/src/hna_set.c b/src/hna_set.c
>>> index 13d0dc3..469348b 100644
>>> --- a/src/hna_set.c
>>> +++ b/src/hna_set.c
>>> @@ -362,7 +362,7 @@ olsr_print_hna_set(void)
>>> */
>>>
>>> bool
>>> -olsr_input_hna(union olsr_message *m, struct interface *in_if __attribute__ ((unused)), union olsr_ip_addr *from_addr)
>>> +olsr_input_hna(union olsr_message *m, struct interface *in_if, union olsr_ip_addr *from_addr)
>>> {
>>>
>>> uint8_t olsr_msgtype;
>>> @@ -425,7 +425,7 @@ olsr_input_hna(union olsr_message *m, struct interface *in_if __attribute__ ((un
>>> * is not in the symmetric 1-hop neighborhood of this node, the
>>> * message MUST be discarded.
>>> */
>>> - if (check_neighbor_link(from_addr) != SYM_LINK) {
>>> + if (check_neighbor_link(&in_if->ip_addr, from_addr) != SYM_LINK) {
>>> OLSR_PRINTF(2, "Received HNA from NON SYM neighbor %s\n", olsr_ip_to_string(&buf, from_addr));
>>> return false;
>>> }
>>> diff --git a/src/link_set.c b/src/link_set.c
>>> index 399a5ee..cf3a76d 100644
>>> --- a/src/link_set.c
>>> +++ b/src/link_set.c
>>> @@ -184,7 +184,7 @@ get_neighbor_status(const union olsr_ip_addr *address)
>>> if (!(main_addr = mid_lookup_main_addr(address)))
>>> main_addr = address;
>>>
>>> - /* Loop trough local interfaces to check all possebilities */
>>> + /* Loop trough local interfaces to check all possibilities */
>>> for (ifs = ifnet; ifs != NULL; ifs = ifs->int_next) {
>>> struct mid_address *aliases;
>>> struct link_entry *lnk = lookup_link_entry(main_addr, NULL, ifs);
>>> @@ -257,9 +257,8 @@ get_best_link_to_neighbor(const union olsr_ip_addr *remote)
>>>
>>> /*
>>> * is this interface better than anything we had before ?
>>> - * use the requested remote interface address as a tie-breaker
>>> */
>>> - if ((tmp_if->int_metric < curr_metric) || ((tmp_if->int_metric == curr_metric) && ipequal(&walker->local_iface_addr, remote))) {
>>> + if ((tmp_if->int_metric < curr_metric) || ((tmp_if->int_metric == curr_metric) && ! good_link)) {
> I am skeptical about this change, it could lead to instable route
> selection because the selection choice might depend on the order in the
> interface list.
>
> Is there any reason to remove the tie-breaker?
>
> (this code is only important for hopcount metric, but I am curious)
Hi Henning,
you are right regarding the tie-breaker. I have prepared a new version of the patch (attached, please consider that from now one) that uses a tie-breaker based on the interface index of the link and also improves the link selection process for both hop-count and link quality (by selecting the best symmetric link as good_link and the best asymmetric link as backup link).
For the record, the old version of the tie-breaker looked quite strange to me: is ipequal(&walker->local_iface_addr, remote) always false or am I missing something?
>
>>> /* memorize the interface's metric */
>>> curr_metric = tmp_if->int_metric;
>>> @@ -627,12 +626,12 @@ add_link_entry(const union olsr_ip_addr *local, const union olsr_ip_addr *remote
>>> * @return 1 of the link is symmertic 0 if not
>>> */
>>> int
>>> -check_neighbor_link(const union olsr_ip_addr *int_addr)
>>> +check_neighbor_link(const union olsr_ip_addr * local, const union olsr_ip_addr *remote)
>>> {
>>> struct link_entry *link;
>>>
>>> OLSR_FOR_ALL_LINK_ENTRIES(link) {
>>> - if (ipequal(int_addr, &link->neighbor_iface_addr)) {
>>> + if (ipequal(remote, &link->neighbor_iface_addr) && ipequal(local, &link->local_iface_addr)) {
>>> return lookup_link_status(link);
>>> }
>>> }
> I can see the reason behind this change, but does it solve the problem
> completely?
>
> I see only two cases where this "mix up links between nodes" will happen:
> a) a node use the same IPs for multiple interfaces
> b) a node can see multiple interfaces of a different node
>
> The patch solves parts of case b) as long as no VLANs are involved. can
> we use the interface index instead of the local IP?
The intent of the patch was to fix case b) that may easily arise if you change channels on the interfaces.
I didn't consider the VLAN case, but from our point of view using the interface index is perfectly fine.
Best regards,
Giovanni
-------------- next part --------------
diff --git a/src/hna_set.c b/src/hna_set.c
index df8a82d..711e85a 100644
--- a/src/hna_set.c
+++ b/src/hna_set.c
@@ -362,7 +362,7 @@ olsr_print_hna_set(void)
*/
bool
-olsr_input_hna(union olsr_message *m, struct interface *in_if __attribute__ ((unused)), union olsr_ip_addr *from_addr)
+olsr_input_hna(union olsr_message *m, struct interface *in_if, union olsr_ip_addr *from_addr)
{
uint8_t olsr_msgtype;
@@ -425,7 +425,7 @@ olsr_input_hna(union olsr_message *m, struct interface *in_if __attribute__ ((un
* is not in the symmetric 1-hop neighborhood of this node, the
* message MUST be discarded.
*/
- if (check_neighbor_link(from_addr) != SYM_LINK) {
+ if (check_neighbor_link(&in_if->ip_addr, from_addr) != SYM_LINK) {
OLSR_PRINTF(2, "Received HNA from NON SYM neighbor %s\n", olsr_ip_to_string(&buf, from_addr));
return false;
}
diff --git a/src/link_set.c b/src/link_set.c
index aa6a9ea..cba1855 100644
--- a/src/link_set.c
+++ b/src/link_set.c
@@ -184,7 +184,7 @@ get_neighbor_status(const union olsr_ip_addr *address)
if (!(main_addr = mid_lookup_main_addr(address)))
main_addr = address;
- /* Loop trough local interfaces to check all possebilities */
+ /* Loop trough local interfaces to check all possbilities */
for (ifs = ifnet; ifs != NULL; ifs = ifs->int_next) {
struct mid_address *aliases;
struct link_entry *lnk = lookup_link_entry(main_addr, NULL, ifs);
@@ -216,10 +216,14 @@ get_best_link_to_neighbor(const union olsr_ip_addr *remote)
const union olsr_ip_addr *main_addr;
struct link_entry *walker, *good_link, *backup_link;
struct interface *tmp_if;
- int curr_metric = MAX_IF_METRIC;
- olsr_linkcost curr_lcost = LINK_COST_BROKEN;
+ int curr_metric_good = MAX_IF_METRIC;
+ int curr_metric_backup = MAX_IF_METRIC;
+ olsr_linkcost curr_lcost_good = LINK_COST_BROKEN;
+ olsr_linkcost curr_lcost_backup = LINK_COST_BROKEN;
+
olsr_linkcost tmp_lc;
+
/* main address lookup */
main_addr = mid_lookup_main_addr(remote);
@@ -234,13 +238,12 @@ get_best_link_to_neighbor(const union olsr_ip_addr *remote)
/* loop through all links that we have */
OLSR_FOR_ALL_LINK_ENTRIES(walker) {
-
+
/* if this is not a link to the neighour in question, skip */
if (!ipequal(&walker->neighbor->neighbor_main_addr, main_addr))
continue;
-
+
if (olsr_cnf->lq_level == 0) {
-
/*
* handle the non-LQ, RFC-compliant case
*/
@@ -257,20 +260,22 @@ get_best_link_to_neighbor(const union olsr_ip_addr *remote)
/*
* is this interface better than anything we had before ?
- * use the requested remote interface address as a tie-breaker
+ * use the link interface index as a tie-breaker
*/
- if ((tmp_if->int_metric < curr_metric) || ((tmp_if->int_metric == curr_metric) && ipequal(&walker->local_iface_addr, remote))) {
-
- /* memorize the interface's metric */
- curr_metric = tmp_if->int_metric;
-
- /* prefer symmetric links over asymmetric links */
- if (lookup_link_status(walker) == SYM_LINK) {
- good_link = walker;
- } else {
- backup_link = walker;
- }
+ if (lookup_link_status(walker) == SYM_LINK) {
+ if (tmp_if->int_metric < curr_metric_good ||
+ (tmp_if->int_metric == curr_metric_good && good_link && tmp_if->if_index < good_link->inter->if_index)){
+ good_link = walker;
+ curr_metric_good = tmp_if->int_metric;
+ }
+ } else {
+ if (tmp_if->int_metric < curr_metric_backup ||
+ (tmp_if->int_metric == curr_metric_backup && backup_link && tmp_if->if_index < backup_link->inter->if_index)){
+ backup_link = walker;
+ curr_metric_backup = tmp_if->int_metric;
+ }
}
+
} else {
/*
@@ -280,22 +285,31 @@ get_best_link_to_neighbor(const union olsr_ip_addr *remote)
/* get the link cost */
tmp_lc = walker->linkcost;
+ /* find the interface for the link */
+ tmp_if = walker->if_name ? if_ifwithname(walker->if_name) : if_ifwithaddr(&walker->local_iface_addr);
+
+ if (!tmp_if) {
+ continue;
+ }
+
/*
* is this link better than anything we had before ?
- * use the requested remote interface address as a tie-breaker.
+ * use the link interface index as a tie-breaker
*/
- if ((tmp_lc < curr_lcost) || ((tmp_lc == curr_lcost) && ipequal(&walker->local_iface_addr, remote))) {
-
- /* memorize the link quality */
- curr_lcost = tmp_lc;
-
- /* prefer symmetric links over asymmetric links */
- if (lookup_link_status(walker) == SYM_LINK) {
- good_link = walker;
- } else {
- backup_link = walker;
- }
+ if (lookup_link_status(walker) == SYM_LINK) {
+ if (tmp_lc < curr_lcost_good ||
+ (tmp_lc == curr_lcost_good && good_link && tmp_if->if_index < good_link->inter->if_index)){
+ good_link = walker;
+ curr_lcost_good=tmp_lc;
+ }
+ } else {
+ if (tmp_lc < curr_lcost_backup ||
+ (tmp_lc == curr_lcost_backup && backup_link && tmp_if->if_index < backup_link->inter->if_index)){
+ backup_link = walker;
+ curr_lcost_backup=tmp_lc;
+ }
}
+
}
}
OLSR_FOR_ALL_LINK_ENTRIES_END(walker);
@@ -327,7 +341,7 @@ set_loss_link_multiplier(struct link_entry *entry)
}
}
assert(cfg_inter);
-
+
/* create a null address for comparison */
memset(&null_addr, 0, sizeof(union olsr_ip_addr));
@@ -627,12 +641,12 @@ add_link_entry(const union olsr_ip_addr *local, const union olsr_ip_addr *remote
* @return 1 of the link is symmertic 0 if not
*/
int
-check_neighbor_link(const union olsr_ip_addr *int_addr)
+check_neighbor_link(const union olsr_ip_addr * local, const union olsr_ip_addr *remote)
{
struct link_entry *link;
OLSR_FOR_ALL_LINK_ENTRIES(link) {
- if (ipequal(int_addr, &link->neighbor_iface_addr)) {
+ if (ipequal(remote, &link->neighbor_iface_addr) && ipequal(local, &link->local_iface_addr)) {
return lookup_link_status(link);
}
}
@@ -720,7 +734,7 @@ update_link_entry(const union olsr_ip_addr *local, const union olsr_ip_addr *rem
}
/* L_time = max(L_time, L_ASYM_time) */
- if (entry->link_timer == NULL || (entry->link_timer->timer_clock < entry->ASYM_time)) {
+ if (entry->link_timer && (entry->link_timer->timer_clock < entry->ASYM_time)) {
olsr_set_link_timer(entry, TIME_DUE(entry->ASYM_time));
}
diff --git a/src/link_set.h b/src/link_set.h
index 9ae27f5..f7645e2 100644
--- a/src/link_set.h
+++ b/src/link_set.h
@@ -133,7 +133,7 @@ struct link_entry *lookup_link_entry(const union olsr_ip_addr *, const union ols
struct link_entry *update_link_entry(const union olsr_ip_addr *, const union olsr_ip_addr *, const struct hello_message *,
const struct interface *);
-int check_neighbor_link(const union olsr_ip_addr *);
+int check_neighbor_link(const union olsr_ip_addr *, const union olsr_ip_addr *);
int replace_neighbor_link_set(const struct neighbor_entry *, struct neighbor_entry *);
int lookup_link_status(const struct link_entry *);
void olsr_update_packet_loss_hello_int(struct link_entry *, olsr_reltime);
diff --git a/src/mid_set.c b/src/mid_set.c
index c25e0bf..13d8b18 100644
--- a/src/mid_set.c
+++ b/src/mid_set.c
@@ -561,7 +561,7 @@ olsr_print_mid_set(void)
*/
bool
-olsr_input_mid(union olsr_message *m, struct interface *in_if __attribute__ ((unused)), union olsr_ip_addr *from_addr)
+olsr_input_mid(union olsr_message *m, struct interface *in_if, union olsr_ip_addr *from_addr)
{
struct ipaddr_str buf;
struct mid_alias *tmp_adr;
@@ -584,7 +584,7 @@ olsr_input_mid(union olsr_message *m, struct interface *in_if __attribute__ ((un
* message MUST be discarded.
*/
- if (check_neighbor_link(from_addr) != SYM_LINK) {
+ if (check_neighbor_link(&in_if->ip_addr, from_addr) != SYM_LINK) {
OLSR_PRINTF(2, "Received MID from NON SYM neighbor %s\n", olsr_ip_to_string(&buf, from_addr));
olsr_free_mid_packet(&message);
return false;
diff --git a/src/tc_set.c b/src/tc_set.c
index e1480d6..25e98d8 100644
--- a/src/tc_set.c
+++ b/src/tc_set.c
@@ -768,7 +768,7 @@ olsr_calculate_tc_border(uint8_t lower_border, union olsr_ip_addr *lower_border_
* hence the spot we are looking at.
*/
bool
-olsr_input_tc(union olsr_message * msg, struct interface * input_if __attribute__ ((unused)), union olsr_ip_addr * from_addr)
+olsr_input_tc(union olsr_message * msg, struct interface * input_if, union olsr_ip_addr * from_addr)
{
struct ipaddr_str buf;
uint16_t size, msg_seq, ansn;
@@ -798,7 +798,7 @@ olsr_input_tc(union olsr_message * msg, struct interface * input_if __attribute_
* is not in the symmetric 1-hop neighborhood of this node, the
* message MUST be discarded.
*/
- if (check_neighbor_link(from_addr) != SYM_LINK) {
+ if (check_neighbor_link(&input_if->ip_addr, from_addr) != SYM_LINK) {
OLSR_PRINTF(2, "Received TC from NON SYM neighbor %s\n", olsr_ip_to_string(&buf, from_addr));
return false;
}
More information about the Olsr-dev
mailing list