Skip to content

Commit

Permalink
fib: only update glean for interface if necessary
Browse files Browse the repository at this point in the history 
Type: improvement

If an interface address is added, the glean adjacency for it's covering
prefix is updated with that address. In the case of multiple addresses
within the same prefix being added, the most recently added one will end
up being used as the sender protocol address for ARP requests.

Similar behavior occurs when an interface address is deleted. The glean
adjacency is updated to some appropriate entry under it's covering
prefix. If there were multiple interface addresses configured, we may
update the address on the adjacency even though the address currently in
use is not the one being deleted.

Add a new value PROVIDES_GLEAN to fib_entry_src_flag_t. The flag
identifies whether a source interface entry is being used as the address
for the glean adjacency for the covering prefix.

Update logic so that the glean is only updated on adding an interface
address if there is not already a sibling entry in use which has the
flag set. Also, only update the glean on deleting an interface address
if the address being deleted has the flag set.

Also update unit test which validates expected behavior in the case
where multiple addresses within a prefix are configured on an interface.

Signed-off-by: Matthew Smith <[email protected]>
Change-Id: I7d918b8dd703735b20ec76e0a60af6d7e571b766
  • Loading branch information
@mgsmith1000
mgsmith1000 authored and Neale Ranns committed Oct 25, 2023
1 parent 52aaa9b commit 9e5694b
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 17 deletions.
8 changes: 7 additions & 1 deletion src/vnet/fib/fib_entry.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,14 @@ typedef enum fib_entry_src_attribute_t_ {
* the source is inherited from its cover
*/
FIB_ENTRY_SRC_ATTRIBUTE_INHERITED,
/**
* the source is currently used as glean src address
*/
FIB_ENTRY_SRC_ATTRIBUTE_PROVIDES_GLEAN,
/**
* Marker. add new entries before this one.
*/
FIB_ENTRY_SRC_ATTRIBUTE_LAST = FIB_ENTRY_SRC_ATTRIBUTE_INHERITED,
FIB_ENTRY_SRC_ATTRIBUTE_LAST = FIB_ENTRY_SRC_ATTRIBUTE_PROVIDES_GLEAN,
} fib_entry_src_attribute_t;


Expand All @@ -166,6 +170,7 @@ typedef enum fib_entry_src_attribute_t_ {
[FIB_ENTRY_SRC_ATTRIBUTE_ACTIVE] = "active", \
[FIB_ENTRY_SRC_ATTRIBUTE_STALE] = "stale", \
[FIB_ENTRY_SRC_ATTRIBUTE_INHERITED] = "inherited", \
[FIB_ENTRY_SRC_ATTRIBUTE_PROVIDES_GLEAN] = "provides-glean", \
}

#define FOR_EACH_FIB_SRC_ATTRIBUTE(_item) \
Expand All @@ -180,6 +185,7 @@ typedef enum fib_entry_src_flag_t_ {
FIB_ENTRY_SRC_FLAG_ACTIVE = (1 << FIB_ENTRY_SRC_ATTRIBUTE_ACTIVE),
FIB_ENTRY_SRC_FLAG_STALE = (1 << FIB_ENTRY_SRC_ATTRIBUTE_STALE),
FIB_ENTRY_SRC_FLAG_INHERITED = (1 << FIB_ENTRY_SRC_ATTRIBUTE_INHERITED),
FIB_ENTRY_SRC_FLAG_PROVIDES_GLEAN = (1 << FIB_ENTRY_SRC_ATTRIBUTE_PROVIDES_GLEAN),
} __attribute__ ((packed)) fib_entry_src_flag_t;

extern u8 * format_fib_entry_src_flags(u8 *s, va_list *args);
Expand Down
81 changes: 74 additions & 7 deletions src/vnet/fib/fib_entry_src_interface.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,16 @@ fib_entry_src_interface_update_glean (fib_entry_t *cover,
if (fib_prefix_is_cover(&adj->sub_type.glean.rx_pfx,
&local->fe_prefix))
{
adj->sub_type.glean.rx_pfx.fp_addr = local->fe_prefix.fp_addr;
return (1);
fib_entry_src_t *local_src;

local_src = fib_entry_src_find (local, FIB_SOURCE_INTERFACE);
if (local_src != NULL)
{
adj->sub_type.glean.rx_pfx.fp_addr =
local->fe_prefix.fp_addr;
local_src->fes_flags |= FIB_ENTRY_SRC_FLAG_PROVIDES_GLEAN;
return (1);
}
}
}
}
Expand Down Expand Up @@ -116,6 +124,52 @@ fib_entry_src_interface_path_swap (fib_entry_src_t *src,
src->fes_pl = fib_path_list_create(pl_flags, paths);
}

typedef struct fesi_find_glean_ctx_t_ {
fib_node_index_t glean_node_index;
} fesi_find_glean_ctx_t;

static walk_rc_t
fib_entry_src_interface_find_glean_walk (fib_entry_t *cover,
fib_node_index_t covered,
void *ctx)
{
fesi_find_glean_ctx_t *find_glean_ctx = ctx;
fib_entry_t *covered_entry;
fib_entry_src_t *covered_src;

covered_entry = fib_entry_get (covered);
covered_src = fib_entry_src_find (covered_entry, FIB_SOURCE_INTERFACE);
if ((covered_src != NULL) &&
(covered_src->fes_flags & FIB_ENTRY_SRC_FLAG_PROVIDES_GLEAN))
{
find_glean_ctx->glean_node_index = covered;
return WALK_STOP;
}

return WALK_CONTINUE;
}

static fib_entry_t *
fib_entry_src_interface_find_glean (fib_entry_t *cover)
{
fib_entry_src_t *src;

src = fib_entry_src_find (cover, FIB_SOURCE_INTERFACE);
if (src == NULL)
/* the cover is not an interface source */
return NULL;

fesi_find_glean_ctx_t ctx = {
.glean_node_index = ~0,
};

fib_entry_cover_walk (cover, fib_entry_src_interface_find_glean_walk,
&ctx);

return (ctx.glean_node_index == ~0) ? NULL :
fib_entry_get (ctx.glean_node_index);
}

/*
* Source activate.
* Called when the source is teh new longer best source on the entry
Expand All @@ -128,6 +182,8 @@ fib_entry_src_interface_activate (fib_entry_src_t *src,

if (FIB_ENTRY_FLAG_LOCAL & src->fes_entry_flags)
{
u8 update_glean;

/*
* Track the covering attached/connected cover. This is so that
* during an attached export of the cover, this local prefix is
Expand All @@ -141,10 +197,17 @@ fib_entry_src_interface_activate (fib_entry_src_t *src,

cover = fib_entry_get(src->u.interface.fesi_cover);

/*
* Before adding as a child of the cover, check whether an existing
* child has already been used to populate the glean adjacency. If so,
* we don't need to update the adjacency.
*/
update_glean = (fib_entry_src_interface_find_glean (cover) == NULL);
src->u.interface.fesi_sibling =
fib_entry_cover_track(cover, fib_entry_get_index(fib_entry));

fib_entry_src_interface_update_glean(cover, fib_entry);
if (update_glean)
fib_entry_src_interface_update_glean(cover, fib_entry);
}

return (!0);
Expand All @@ -167,15 +230,19 @@ fib_entry_src_interface_deactivate (fib_entry_src_t *src,
if (FIB_NODE_INDEX_INVALID != src->u.interface.fesi_cover)
{
cover = fib_entry_get(src->u.interface.fesi_cover);

fib_entry_cover_untrack(cover, src->u.interface.fesi_sibling);

src->u.interface.fesi_cover = FIB_NODE_INDEX_INVALID;
src->u.interface.fesi_sibling = ~0;

fib_entry_cover_walk(cover,
fib_entry_src_interface_update_glean_walk,
NULL);
/* If this was the glean address, find a new one */
if (src->fes_flags & FIB_ENTRY_SRC_FLAG_PROVIDES_GLEAN)
{
fib_entry_cover_walk(cover,
fib_entry_src_interface_update_glean_walk,
NULL);
src->fes_flags &= ~FIB_ENTRY_SRC_FLAG_PROVIDES_GLEAN;
}
}
}

Expand Down
21 changes: 12 additions & 9 deletions test/test_neighbor.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2126,27 +2126,30 @@ def test_glean_src_select(self):

#
# add a local address in the same subnet
# the source addresses are equivalent. VPP happens to
# choose the last one that was added
# the source addresses are equivalent.
# VPP leaves the glean address being used for a prefix
# in place until that address is deleted.
#
conn3 = VppIpInterfaceAddress(self, self.pg1, "10.0.1.2", 24).add_vpp_config()

rxs = self.send_and_expect(self.pg0, [p2], self.pg1)
for rx in rxs:
self.verify_arp_req(rx, self.pg1.local_mac, "10.0.1.2", "10.0.1.128")
self.verify_arp_req(rx, self.pg1.local_mac, "10.0.1.1", "10.0.1.128")

#
# remove
# remove first address, which is currently in use
# the second address should be used now
#
conn3.remove_vpp_config()
conn2.remove_vpp_config()
rxs = self.send_and_expect(self.pg0, [p2], self.pg1)
for rx in rxs:
self.verify_arp_req(rx, self.pg1.local_mac, "10.0.1.1", "10.0.1.128")
self.verify_arp_req(rx, self.pg1.local_mac, "10.0.1.2", "10.0.1.128")

#
# add back, this time remove the first one
# add first address back. Second address should continue
# being used.
#
conn3 = VppIpInterfaceAddress(self, self.pg1, "10.0.1.2", 24).add_vpp_config()

conn2 = VppIpInterfaceAddress(self, self.pg1, "10.0.1.1", 24).add_vpp_config()
rxs = self.send_and_expect(self.pg0, [p2], self.pg1)
for rx in rxs:
self.verify_arp_req(rx, self.pg1.local_mac, "10.0.1.2", "10.0.1.128")
Expand Down

0 comments on commit 9e5694b

Please sign in to comment.