[Olsr-dev] [PATCH v1 1/1] lq_packet: only report a neighbour once in a hello message
Teco Boot
(spam-protected)
Fri Jan 13 09:56:49 CET 2017
I don't think it is needed, and in fact wrong, to use the "better" link type.
The unspec is the link table entry that is for link info of other interfaces.
In fact it is questionable to send such link info on this interface. This
because the index is incomplete, the receiver cannot insert or update the
correct link entry in its table.
Note: tcpdump info below is wrong, the LQ 0.00% is a decode of the wrong field.
Actual value can be seen in hex, with -X.
What should the receiver do with such a hello message? For sure, the link
table entry for "this interface", where the link-type != Unspec, shall be
updated. For the link-type = Unspec, the LQ of a matching entry could be
updated with this fresh info. But which entry is matching? In the topology
below, there is only one "other interface". So best guessing would pick that
one. Still not sure, there could be a new third one. In cases with multiple
other interfaces, guessing is wrong, receiver cannot know which other interface
link entry to update.
See my other post, a solution would be defining a new hello message, with full
link index: both interface addresses of the link. Let's not go that way.
I suggest removing the "select best" logic, it makes no sense. The duplicate
with link-type != Unspec shall be added, so as result two entries are send.
This makes behavior consistent if first entry is link-type = Unspec or
link-type != Unspec.
For deciding the approach on receiver, this needs more thoughts. Either
investigate the "best guess on what link entry to update" or ignore this unspec
info. Fot the latter, this link-type = Unspec entry can be removed from the
hello message, as it hasn't any purpose. Or I miss something.
Again, I did not fully analyse the code nor this patch. I am just worried we
take the wrong path fixing the issue.
Teco
> Op 13 jan. 2017, om 09:05 heeft Ferry Huberts <(spam-protected)> het volgende geschreven:
>
> From: "Iwan G. Flameling" <(spam-protected)>
>
> A while ago we noticed that neighbours of nodes with multiple interfaces
> on the same medium report infinite costs on their links to those nodes.
>
> This problem is 100% reproducible.
>
> Below is the setup in which we noticed the problem.
>
> wlan0 wlan0
> 172.31.175.97/16 172.31.175.61/16
> (((*))) ------------------------------------------------- (((*)))
> | |
> | |
> | |
> ____|___ 172.29.175.97/15 ________ 172.29.175.61/15 ____|____
> | |-eth1.2580---------| |--------eth1.2580-| |
> | Node 97 | | Switch | | Node 61 |
> |_________|-eth2.2580---------|________| |_________|
> 172.28.175.97/15
>
> In this setup node 97 will report normal link costs for its (wired) links
> to node 61 (see the first table below), while node 61 will report infinite
> link costs for both its (wired) links to node 97 (see the second table
> below).
>
> Table: Links (node 97)
> Local IP Remote IP Hyst. LQ NLQ Cost
> 172.29.175.97 172.29.175.61 0.000 1.000 1.000 0.100
> 172.28.175.97 172.29.175.61 0.000 1.000 1.000 0.100
> 172.31.175.97 172.31.175.61 0.000 1.000 1.000 1.000
>
> Table: Links (node 61)
> Local IP Remote IP Hyst. LQ NLQ Cost
> 172.29.175.61 172.29.175.97 0.000 1.000 0.000 INFINITE
> 172.29.175.61 172.28.175.97 0.000 1.000 0.000 INFINITE
> 172.31.175.61 172.31.175.97 0.000 1.000 1.000 1.000
>
> Checking the HELLO messages that are received on node 61 from node 97,
> we see the following:
>
> [node 61] # tcpdump -vni eth1.2580 udp port 698
> tcpdump: listening on eth1.2580, link-type EN10MB (Ethernet), capture size 262144 bytes
> 06:21:23.528204 IP (tos 0xc0, ttl 1, id 42455, offset 0, flags [DF], proto UDP (17), length 80)
> 172.28.175.97.698 > 255.255.255.255.698: OLSRv4, seq 0xf7c0, length 52
> Hello-LQ Message (0xc9), originator 172.31.175.97, ttl 1, hop 0
> vtime 3.000s, msg-seq 0x533d, length 48
> hello-time 1.000s, MPR willingness 3
> link-type Symmetric, neighbor-type Symmetric, len 12
> neighbor 172.29.175.61, link-quality 0.00%, neighbor-link-quality 0.00%
> link-type Unspecified, neighbor-type Symmetric, len 20
> neighbor 172.31.175.61, link-quality 0.00%, neighbor-link-quality 0.00%
> neighbor 172.29.175.61, link-quality 0.00%, neighbor-link-quality 0.00%
>
> Node 61 receives HELLO messages from node 97 that report (amongst others):
> 1- a SYMMETRIC link-type to node 61 (172.29.175.61)
> 2- an UNSPECIFIED link-type to node 61 (172.29.175.61)
>
> Clearly, this is 'confusing' and the root cause of why node 61 reports
> infinite costs for the links, as shown above.
>
> We pose that in a HELLO message the same neighbour should NEVER be
> reported with conflicting information.
>
> Proposed solution:
>
> 1- NEVER report a neighbour more than once in a HELLO message
> 2- Use the 'best' values for a neighbour that is reported in a HELLO
> message
>
> This requires:
>
> 1- De-duplication of neighbours when constructing a HELLO message.
>
> 2- When a neighbour is already present in the HELLO message
> that is under construction then he 'best' values must be determined
> and those in the neighbour that is already present should only be
> overwritten if those are worse.
>
> This patch implements that solution.
>
> Signed-off-by: Iwan G. Flameling <(spam-protected)>
> Signed-off-by: Ferry Huberts <(spam-protected)>
> ---
> src/lq_packet.c | 52 ++++++++++++----
> src/lq_plugin.c | 5 +-
> src/lq_plugin.h | 4 +-
> src/lq_plugin_default_ff.c | 13 +++-
> src/lq_plugin_default_ffeth.c | 13 +++-
> src/lq_plugin_default_float.c | 19 +++++-
> src/lq_plugin_default_fpm.c | 19 +++++-
> src/olsr_protocol.h | 136 ++++++++++++++++++++++++++++++++++++++++++
> 8 files changed, 236 insertions(+), 25 deletions(-)
>
> diff --git a/src/lq_packet.c b/src/lq_packet.c
> index d44e654..2e8e027 100644
> --- a/src/lq_packet.c
> +++ b/src/lq_packet.c
> @@ -68,6 +68,21 @@ bool lq_tc_pending = false;
> static uint32_t msg_buffer_aligned[(MAXMESSAGESIZE - OLSR_HEADERSIZE) / sizeof(uint32_t) + 1];
> static unsigned char *const msg_buffer = (unsigned char *)msg_buffer_aligned;
>
> +static struct lq_hello_neighbor * neigh_find(struct link_entry *link, struct lq_hello_message *lq_hello) {
> + struct lq_hello_neighbor *neigh;
> +
> + assert(link);
> + assert(lq_hello);
> +
> + for (neigh = lq_hello->neigh; neigh; neigh = neigh->next) {
> + if (ipequal(&link->neighbor_iface_addr, &neigh->addr)) {
> + return neigh;
> + }
> + }
> +
> + return NULL ;
> +}
> +
> static void
> create_lq_hello(struct lq_hello_message *lq_hello, struct interface_olsr *outif)
> {
> @@ -92,46 +107,57 @@ create_lq_hello(struct lq_hello_message *lq_hello, struct interface_olsr *outif)
> // loop through the link set
>
> OLSR_FOR_ALL_LINK_ENTRIES(walker) {
> + uint8_t link_type;
> + uint8_t neigh_type;
>
> // allocate a neighbour entry
> - struct lq_hello_neighbor *neigh = olsr_malloc_lq_hello_neighbor("Build LQ_HELLO");
> + struct lq_hello_neighbor *neigh = neigh_find(walker, lq_hello);
> + bool neigh_is_new = (neigh == NULL );
> + neigh = neigh_is_new ? olsr_malloc_lq_hello_neighbor("Build LQ_HELLO") : neigh;
>
> // a) this neighbor interface IS NOT visible via the output interface
> if (!ipequal(&walker->local_iface_addr, &outif->ip_addr))
> - neigh->link_type = UNSPEC_LINK;
> + link_type = UNSPEC_LINK;
>
> // b) this neighbor interface IS visible via the output interface
>
> else
> - neigh->link_type = lookup_link_status(walker);
> + link_type = lookup_link_status(walker);
> +
> + if (neigh_is_new || link_type_compare(neigh->link_type, link_type))
> + neigh->link_type = link_type;
>
> // set the entry's link quality
> - olsr_copy_hello_lq(neigh, walker);
> + olsr_copy_hello_lq(neigh, walker, neigh_is_new);
>
> // set the entry's neighbour type
>
> if (walker->neighbor->is_mpr)
> - neigh->neigh_type = MPR_NEIGH;
> + neigh_type = MPR_NEIGH;
>
> else if (walker->neighbor->status == SYM)
> - neigh->neigh_type = SYM_NEIGH;
> + neigh_type = SYM_NEIGH;
>
> else if (walker->neighbor->status == NOT_SYM)
> - neigh->neigh_type = NOT_NEIGH;
> + neigh_type = NOT_NEIGH;
>
> else {
> OLSR_PRINTF(0, "Error: neigh_type undefined");
> - neigh->neigh_type = NOT_NEIGH;
> + neigh_type = NOT_NEIGH;
> }
>
> - // set the entry's neighbour interface address
> + if (neigh_is_new || neigh_type_compare(neigh->neigh_type, neigh_type))
> + neigh->neigh_type = neigh_type;
>
> - neigh->addr = walker->neighbor_iface_addr;
> + if (neigh_is_new) {
> + // set the entry's neighbour interface address
>
> - // queue the neighbour entry
> - neigh->next = lq_hello->neigh;
> - lq_hello->neigh = neigh;
> + neigh->addr = walker->neighbor_iface_addr;
>
> + // queue the neighbour entry
> + neigh->next = lq_hello->neigh;
> + lq_hello->neigh = neigh;
> + }
> }
> OLSR_FOR_ALL_LINK_ENTRIES_END(walker);
> }
> diff --git a/src/lq_plugin.c b/src/lq_plugin.c
> index a917c84..01c6e4e 100644
> --- a/src/lq_plugin.c
> +++ b/src/lq_plugin.c
> @@ -351,13 +351,14 @@ get_linkcost_scaled(olsr_linkcost cost, bool route)
> *
> * @param target pointer to target lq_hello_neighbor
> * @param source pointer to source link_entry
> + * @param force true to force the copy, otherwise only copy when source is 'better' than target
> */
> void
> -olsr_copy_hello_lq(struct lq_hello_neighbor *target, struct link_entry *source)
> +olsr_copy_hello_lq(struct lq_hello_neighbor *target, struct link_entry *source, bool force)
> {
> assert((const char *)target + sizeof(*target) >= (const char *)target->linkquality);
> assert((const char *)source + sizeof(*source) >= (const char *)source->linkquality);
> - active_lq_handler->copy_link_lq_into_neigh(target->linkquality, source->linkquality);
> + active_lq_handler->copy_link_lq_into_neigh(target->linkquality, source->linkquality, force);
> }
>
> /**
> diff --git a/src/lq_plugin.h b/src/lq_plugin.h
> index 747a70c..26baba4 100644
> --- a/src/lq_plugin.h
> +++ b/src/lq_plugin.h
> @@ -76,7 +76,7 @@ struct lq_handler {
> void (*packet_loss_handler) (struct link_entry * entry, void *lq, bool lost);
>
> void (*memorize_foreign_hello) (void *local, void *foreign);
> - void (*copy_link_lq_into_neigh) (void *target, void *source);
> + void (*copy_link_lq_into_neigh) (void *target, void *source, bool force);
> void (*copy_link_lq_into_tc) (void *target, void *source);
> void (*clear_hello) (void *target);
> void (*clear_tc) (void *target);
> @@ -134,7 +134,7 @@ const char *get_linkcost_text(olsr_linkcost cost, bool route, struct lqtextbuffe
> double get_linkcost_scaled(olsr_linkcost cost, bool route);
>
> void olsr_clear_hello_lq(struct link_entry */*link*/);
> -void olsr_copy_hello_lq(struct lq_hello_neighbor *target, struct link_entry *source);
> +void olsr_copy_hello_lq(struct lq_hello_neighbor *target, struct link_entry *source, bool force);
> void olsr_copylq_link_entry_2_tc_mpr_addr(struct tc_mpr_addr *target, struct link_entry *source);
> void olsr_copylq_link_entry_2_tc_edge_entry(struct tc_edge_entry *target, struct link_entry *source);
> void olsr_clear_tc_lq(struct tc_mpr_addr *target);
> diff --git a/src/lq_plugin_default_ff.c b/src/lq_plugin_default_ff.c
> index 3d6ba6a..fa617e3 100644
> --- a/src/lq_plugin_default_ff.c
> +++ b/src/lq_plugin_default_ff.c
> @@ -70,7 +70,7 @@ static void default_lq_deserialize_hello_lq_pair_ff(const uint8_t ** curr, void
> static int default_lq_serialize_tc_lq_pair_ff(unsigned char *buff, void *lq);
> static void default_lq_deserialize_tc_lq_pair_ff(const uint8_t ** curr, void *lq);
>
> -static void default_lq_copy_link2neigh_ff(void *t, void *s);
> +static void default_lq_copy_link2neigh_ff(void *t, void *s, bool force);
> static void default_lq_copy_link2tc_ff(void *target, void *source);
> static void default_lq_clear_ff(void *target);
> static void default_lq_clear_ff_hello(void *target);
> @@ -371,11 +371,18 @@ default_lq_memorize_foreign_hello_ff(void *ptrLocal, void *ptrForeign)
> }
>
> static void
> -default_lq_copy_link2neigh_ff(void *t, void *s)
> +default_lq_copy_link2neigh_ff(void *t, void *s, bool force)
> {
> struct default_lq_ff *target = t;
> struct default_lq_ff_hello *source = s;
> - *target = source->smoothed_lq;
> +
> + if (force) {
> + *target = source->smoothed_lq;
> + return;
> + }
> +
> + target->valueLq = MAX(target->valueLq, source->smoothed_lq.valueLq);
> + target->valueNlq = MAX(target->valueNlq, source->smoothed_lq.valueNlq);
> }
>
> static void
> diff --git a/src/lq_plugin_default_ffeth.c b/src/lq_plugin_default_ffeth.c
> index e78dcd7..0e9408e 100644
> --- a/src/lq_plugin_default_ffeth.c
> +++ b/src/lq_plugin_default_ffeth.c
> @@ -72,7 +72,7 @@ static void default_lq_deserialize_hello_lq_pair_ffeth(const uint8_t ** curr, vo
> static int default_lq_serialize_tc_lq_pair_ffeth(unsigned char *buff, void *lq);
> static void default_lq_deserialize_tc_lq_pair_ffeth(const uint8_t ** curr, void *lq);
>
> -static void default_lq_copy_link2neigh_ffeth(void *t, void *s);
> +static void default_lq_copy_link2neigh_ffeth(void *t, void *s, bool force);
> static void default_lq_copy_link2tc_ffeth(void *target, void *source);
> static void default_lq_clear_ffeth(void *target);
> static void default_lq_clear_ffeth_hello(void *target);
> @@ -410,11 +410,18 @@ default_lq_memorize_foreign_hello_ffeth(void *ptrLocal, void *ptrForeign)
> }
>
> static void
> -default_lq_copy_link2neigh_ffeth(void *t, void *s)
> +default_lq_copy_link2neigh_ffeth(void *t, void *s, bool force)
> {
> struct default_lq_ffeth *target = t;
> struct default_lq_ffeth_hello *source = s;
> - *target = source->smoothed_lq;
> +
> + if (force) {
> + *target = source->smoothed_lq;
> + return;
> + }
> +
> + target->valueLq = MAX(target->valueLq, source->smoothed_lq.valueLq);
> + target->valueNlq = MAX(target->valueNlq, source->smoothed_lq.valueNlq);
> }
>
> static void
> diff --git a/src/lq_plugin_default_float.c b/src/lq_plugin_default_float.c
> index 2e07686..f7e2031 100644
> --- a/src/lq_plugin_default_float.c
> +++ b/src/lq_plugin_default_float.c
> @@ -59,6 +59,7 @@ static int default_lq_serialize_hello_lq_pair_float(unsigned char *buff, void *l
> static void default_lq_deserialize_hello_lq_pair_float(const uint8_t ** curr, void *lq);
> static int default_lq_serialize_tc_lq_pair_float(unsigned char *buff, void *lq);
> static void default_lq_deserialize_tc_lq_pair_float(const uint8_t ** curr, void *lq);
> +static void default_lq_copy_link2neigh_float(void *t, void *s, bool force);
> static void default_lq_copy_link2tc_float(void *target, void *source);
> static void default_lq_clear_float(void *target);
> static const char *default_lq_print_float(void *ptr, char separator, struct lqtextbuffer *buffer);
> @@ -74,7 +75,7 @@ struct lq_handler lq_etx_float_handler = {
>
> &default_lq_packet_loss_worker_float,
> &default_lq_memorize_foreign_hello_float,
> - &default_lq_copy_link2tc_float,
> + &default_lq_copy_link2neigh_float,
> &default_lq_copy_link2tc_float,
> &default_lq_clear_float,
> &default_lq_clear_float,
> @@ -207,6 +208,22 @@ default_lq_memorize_foreign_hello_float(void *ptrLocal, void *ptrForeign)
> }
>
> static void
> +default_lq_copy_link2neigh_float(void *t, void *s, bool force)
> +{
> + struct default_lq_float *target = t;
> + struct default_lq_float *source = s;
> +
> + if (force) {
> + memcpy(target, source, sizeof(struct default_lq_float));
> + return;
> + }
> +
> + target->lq = MAX(target->lq, source->lq);
> + target->nlq = MAX(target->nlq, source->nlq);
> + target->quickstart = MAX(target->quickstart, source->quickstart);
> +}
> +
> +static void
> default_lq_copy_link2tc_float(void *target, void *source)
> {
> memcpy(target, source, sizeof(struct default_lq_float));
> diff --git a/src/lq_plugin_default_fpm.c b/src/lq_plugin_default_fpm.c
> index fbfa0cf..2947f92 100644
> --- a/src/lq_plugin_default_fpm.c
> +++ b/src/lq_plugin_default_fpm.c
> @@ -60,6 +60,7 @@ static int default_lq_serialize_hello_lq_pair_fpm(unsigned char *buff, void *lq)
> static void default_lq_deserialize_hello_lq_pair_fpm(const uint8_t ** curr, void *lq);
> static int default_lq_serialize_tc_lq_pair_fpm(unsigned char *buff, void *lq);
> static void default_lq_deserialize_tc_lq_pair_fpm(const uint8_t ** curr, void *lq);
> +static void default_lq_copy_link2neigh_fpm(void *t, void *s, bool force);
> static void default_lq_copy_link2tc_fpm(void *target, void *source);
> static void default_lq_clear_fpm(void *target);
> static const char *default_lq_print_fpm(void *ptr, char separator, struct lqtextbuffer *buffer);
> @@ -75,7 +76,7 @@ struct lq_handler lq_etx_fpm_handler = {
>
> &default_lq_packet_loss_worker_fpm,
> &default_lq_memorize_foreign_hello_fpm,
> - &default_lq_copy_link2tc_fpm,
> + &default_lq_copy_link2neigh_fpm,
> &default_lq_copy_link2tc_fpm,
> &default_lq_clear_fpm,
> &default_lq_clear_fpm,
> @@ -224,6 +225,22 @@ default_lq_copy_link2tc_fpm(void *target, void *source)
> }
>
> static void
> +default_lq_copy_link2neigh_fpm(void *t, void *s, bool force)
> +{
> + struct default_lq_fpm *target = t;
> + struct default_lq_fpm *source = s;
> +
> + if (force) {
> + memcpy(target, source, sizeof(struct default_lq_fpm));
> + return;
> + }
> +
> + target->valueLq = MAX(target->valueLq, source->valueLq);
> + target->valueNlq = MAX(target->valueNlq, source->valueNlq);
> + target->quickstart = MAX(target->quickstart, source->quickstart);
> +}
> +
> +static void
> default_lq_clear_fpm(void *target)
> {
> memset(target, 0, sizeof(struct default_lq_fpm));
> diff --git a/src/olsr_protocol.h b/src/olsr_protocol.h
> index a995db0..ed3e134 100644
> --- a/src/olsr_protocol.h
> +++ b/src/olsr_protocol.h
> @@ -111,6 +111,83 @@ struct olsr;
> #define HIDE_LINK 4
> #define MAX_LINK 4
>
> +/**
> + * Compare two link types.
> + *
> + * <pre>
> + * +-------------+------+------+--------+------+-----+
> + * | | link_type_new |
> + * | +------+------+--------+------+-----+
> + * | link_type | HIDE | LOST | UNSPEC | ASYM | SYM |
> + * |-------------+------+------+--------+------+-----+
> + * | HIDE | F | T | T | T | T |
> + * | LOST | F | F | T | T | T |
> + * | UNSPEC | F | F | F | T | T |
> + * | ASYM | F | F | F | F | T |
> + * | SYM | F | F | F | F | F |
> + * +-------------+------+------+--------+------+-----+
> + * </pre>
> + *
> + * @param link_type a link type
> + * @param link_type_new a (new) link type
> + * @return true when link_type_new is 'better' than link_type
> + */
> +static INLINE bool link_type_compare(int link_type, int link_type_new) {
> + switch (link_type_new) {
> + case SYM_LINK:
> + switch (link_type) {
> + case SYM_LINK:
> + return false;
> +
> + default:
> + return true;
> + }
> + break;
> +
> + case ASYM_LINK:
> + switch (link_type) {
> + case SYM_LINK:
> + case ASYM_LINK:
> + return false;
> +
> + default:
> + return true;
> + }
> + break;
> +
> + case UNSPEC_LINK:
> + switch (link_type) {
> + case SYM_LINK:
> + case ASYM_LINK:
> + case UNSPEC_LINK:
> + return false;
> +
> + default:
> + return true;
> + }
> + break;
> +
> + case LOST_LINK:
> + switch (link_type) {
> + case SYM_LINK:
> + case ASYM_LINK:
> + case UNSPEC_LINK:
> + case LOST_LINK:
> + return false;
> +
> + default:
> + return true;
> + }
> + break;
> +
> + case HIDE_LINK:
> + return false;
> +
> + default:
> + return true;
> + }
> +}
> +
> static INLINE const char * linkTypeToString(int type) {
> switch (type) {
> case ASYM_LINK:
> @@ -140,6 +217,65 @@ static INLINE const char * linkTypeToString(int type) {
> #define MPR_NEIGH 2
> #define MAX_NEIGH 2
>
> +/**
> + * Compare two neighbour types.
> + *
> + * <pre>
> + * +-------+------+-----+-----+
> + * | | neigh_new |
> + * | +------+-----+-----+
> + * | neigh | NOT | SYM | MPR |
> + * |-------+------+-----+-----+
> + * | NOT | F | T | T |
> + * | SYM | F | F | T |
> + * | MPR | F | F | F |
> + * +-------+------+-----+-----+
> + * </pre>
> + *
> + * @param neigh a neighbour type
> + * @param neigh_new a (new) neighbour type
> + * @return true when neigh_new is 'better' than neigh
> + */
> +static INLINE bool neigh_type_compare(int neigh, int neigh_new) {
> + switch (neigh_new) {
> + case MPR_NEIGH:
> + switch (neigh) {
> + case MPR_NEIGH:
> + return false;
> +
> + default:
> + return true;
> + }
> + break;
> +
> + case SYM_NEIGH:
> + switch (neigh) {
> + case MPR_NEIGH:
> + case SYM_NEIGH:
> + return false;
> +
> + default:
> + return true;
> + }
> + break;
> +
> + case NOT_NEIGH:
> + switch (neigh) {
> + case MPR_NEIGH:
> + case SYM_NEIGH:
> + case NOT_NEIGH:
> + return false;
> +
> + default:
> + return true;
> + }
> + break;
> +
> + default:
> + return true;
> + }
> +}
> +
> /*
> *Neighbor status
> */
> --
> 2.9.3
>
More information about the Olsr-dev
mailing list