[Olsr-dev] [PATCH][BUILD] check_neighbor_link is wrong
Ferry Huberts
(spam-protected)
Tue Dec 11 09:45:23 CET 2012
please send the patch inline and not as an attachment.
On 10/12/12 20:34, Giovanni Di Stasi wrote:
> 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
>
>
>
--
Ferry Huberts
More information about the Olsr-dev
mailing list