On 2/27/25 21:58, Lorenzo Bianconi wrote:
> Introduce the capability to actively refresh mac_binding entries available
> in OFTABLE_MAC_BINDING table when they are inactive for more than the
> configured threshold. Add the OFTABLE_MAC_BINDING table stats
> monitoring. This feature avoids possible unnecessary mac_entry removals
> if the destination is still alive but inactive for more than the configured
> value.
> Reduce threshold dump_period and cooldown_period to 3/16 of threshold
> value in order to not miss any re-arping time slot.
>
> Acked-by: Ales Musil <[email protected]>
> Acked-by: Mark Michelson <[email protected]>
> Reported-at: https://issues.redhat.com/browse/FDP-1135
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
> controller/mac-cache.c | 123 ++++++++++++++++++--
> controller/mac-cache.h | 26 ++++-
> controller/ovn-controller.c | 40 +++++--
> controller/pinctrl.c | 8 +-
> controller/pinctrl.h | 7 ++
> controller/statctrl.c | 25 +++-
> tests/ovn.at | 225 ++++++++++++++++++++++++++++++++++++
> tests/system-ovn.at | 108 +++++++++++++++++
> 8 files changed, 530 insertions(+), 32 deletions(-)
Not a full review, but a few comments below.
>
> diff --git a/controller/mac-cache.c b/controller/mac-cache.c
> index 9d05c1950..9eff72207 100644
> --- a/controller/mac-cache.c
> +++ b/controller/mac-cache.c
> @@ -16,6 +16,7 @@
> #include <config.h>
> #include <stdbool.h>
>
> +#include "lflow.h"
> #include "lib/mac-binding-index.h"
> #include "local_data.h"
> #include "lport.h"
> @@ -24,6 +25,7 @@
> #include "openvswitch/vlog.h"
> #include "ovn/logical-fields.h"
> #include "ovn-sb-idl.h"
> +#include "pinctrl.h"
>
> VLOG_DEFINE_THIS_MODULE(mac_cache);
>
> @@ -90,16 +92,15 @@ mac_cache_threshold_add(struct mac_cache_data *data,
> threshold = xmalloc(sizeof *threshold);
> threshold->dp_key = dp->tunnel_key;
> threshold->value = value;
> - threshold->dump_period = value / 2;
> - threshold->cooldown_period = value / 4;
> + threshold->dump_period = (3 * value) / 16;
> + threshold->cooldown_period = (3 * value) / 16;
>
> /* (cooldown_period + dump_period) is the maximum time the timestamp may
> - * be not updated. So, the sum of those times must be lower than the
> - * threshold, otherwise we may fail to update an active MAC binding in
> - * time and risk it being removed. Giving it an extra 1/10 of the time
> - * for all the processing that needs to happen. */
> + * be not updated. So, the sum of those times must be lower than 4/10
> + * of the threshold, otherwise we may fail to update an active MAC
> binding
> + * in time and risk it being removed. */
Hmm. This comment doesn't actually explain why it must be lower than 4/10.
We need to give more information on the mechanism of how the update can be
missed.
This is important because this check depends on two separate stats handlers and
it's not obvious that they have to have their timings interdependent in this
way.
Maybe something like this:
/* (cooldown_period + dump_period) is the maximum time the timestamp may
* be not updated for an entry with IP + MAC combination from which we see
* incoming traffic. For the entry that is used only in Tx direction
* (e.g., an entry for a default gateway of the chassis) this time is
* doubled, because an ARP/ND probe will need to be sent first and the
* (cooldown_period + dump_period) will be the maximum time between such
* probes. Hence, 2 * (cooldown_period + dump_period) should be less than
* a threshold, otherwise we may fail to update an active MAC binding in
* time and risk it being removed. Giving it an extra 1/10 of the time
* for all the processing that needs to happen. */
What do you think?
> ovs_assert(threshold->cooldown_period + threshold->dump_period
> - < (9 * value) / 10);
> + < (4 * value) / 10);
Given the comment above, it might be better to multiply the first line by 2
instead of replacing 9 with a 4.
>
> hmap_insert(&data->thresholds, &threshold->hmap_node, dp->tunnel_key);
> }
> @@ -160,17 +161,25 @@ mac_cache_thresholds_clear(struct mac_cache_data *data)
> /* MAC binding. */
> void
> mac_binding_add(struct hmap *map, struct mac_binding_data mb_data,
> - const struct sbrec_mac_binding *smb, long long timestamp)
> + const struct sbrec_mac_binding *smb,
> + const struct sbrec_port_binding *pb,
> + long long timestamp)
> {
> struct mac_binding *mb = mac_binding_find(map, &mb_data);
> if (!mb) {
> - mb = xmalloc(sizeof *mb);
> + mb = xzalloc(sizeof *mb);
> hmap_insert(map, &mb->hmap_node, mac_binding_data_hash(&mb_data));
> }
>
> mb->data = mb_data;
> mb->sbrec = smb;
> mb->timestamp = timestamp;
> + if (pb) {
> + destroy_lport_addresses(&mb->laddr);
> + if (extract_lsp_addresses(pb->mac[0], &mb->laddr)) {
> + mb->tunnel_key = pb->tunnel_key;
> + }
> + }
> mac_binding_update_log("Added", &mb_data, false, NULL, 0, 0);
> }
>
> @@ -179,6 +188,7 @@ mac_binding_remove(struct hmap *map, struct mac_binding
> *mb)
> {
> mac_binding_update_log("Removed", &mb->data, false, NULL, 0, 0);
> hmap_remove(map, &mb->hmap_node);
> + destroy_lport_addresses(&mb->laddr);
> free(mb);
> }
>
> @@ -237,6 +247,7 @@ mac_bindings_clear(struct hmap *map)
> {
> struct mac_binding *mb;
> HMAP_FOR_EACH_POP (mb, hmap_node, map) {
> + destroy_lport_addresses(&mb->laddr);
> free(mb);
> }
> }
> @@ -399,7 +410,8 @@ mac_binding_update_log(const char *action,
> }
>
> void
> -mac_binding_stats_run(struct ovs_list *stats_list, uint64_t *req_delay,
> +mac_binding_stats_run(struct rconn *swconn OVS_UNUSED,
> + struct ovs_list *stats_list, uint64_t *req_delay,
> void *data)
> {
> struct mac_cache_data *cache_data = data;
> @@ -495,8 +507,9 @@ fdb_update_log(const char *action,
> }
>
> void
> -fdb_stats_run(struct ovs_list *stats_list, uint64_t *req_delay,
> - void *data)
> +fdb_stats_run(struct rconn *swconn OVS_UNUSED,
> + struct ovs_list *stats_list,
> + uint64_t *req_delay, void *data)
> {
> struct mac_cache_data *cache_data = data;
> long long timewall_now = time_wall_msec();
> @@ -847,3 +860,89 @@ buffered_packets_db_lookup(struct buffered_packets *bp,
> struct ds *ip,
>
> eth_addr_from_string(smb->mac, mac);
> }
> +
> +void
> +mac_binding_probe_stats_process_flow_stats(
> + struct ovs_list *stats_list,
> + struct ofputil_flow_stats *ofp_stats)
> +{
> + struct mac_cache_stats *stats = xmalloc(sizeof *stats);
> +
> + stats->idle_age_ms = ofp_stats->idle_age * 1000;
> + stats->data.mb = (struct mac_binding_data) {
> + .cookie = ntohll(ofp_stats->cookie),
> + /* The port_key must be zero to match mac_binding_data_from_sbrec. */
> + .port_key = 0,
> + .dp_key = ntohll(ofp_stats->match.flow.metadata),
> + };
> +
> + if (ofp_stats->match.flow.regs[0]) {
> + stats->data.mb.ip =
> + in6_addr_mapped_ipv4(htonl(ofp_stats->match.flow.regs[0]));
> + } else {
> + ovs_be128 ip6 = hton128(flow_get_xxreg(&ofp_stats->match.flow, 1));
> + memcpy(&stats->data.mb.ip, &ip6, sizeof stats->data.mb.ip);
> + }
> +
> + ovs_list_push_back(stats_list, &stats->list_node);
> +}
> +
> +void
> +mac_binding_probe_stats_run(
> + struct rconn *swconn,
> + struct ovs_list *stats_list,
> + uint64_t *req_delay, void *data)
> +{
> + long long timewall_now = time_wall_msec();
> + struct mac_cache_data *cache_data = data;
> +
> + struct mac_cache_stats *stats;
> + LIST_FOR_EACH_POP (stats, list_node, stats_list) {
> + struct mac_binding *mb = mac_binding_find(&cache_data->mac_bindings,
> + &stats->data.mb);
> + if (!mb) {
> + mac_binding_update_log("Probe: not found in the cache:",
> + &stats->data.mb, false, NULL, 0, 0);
> + free(stats);
> + continue;
> + }
> +
> + struct mac_cache_threshold *threshold =
> + mac_cache_threshold_find(cache_data, mb->data.dp_key);
> + uint64_t since_updated_ms = timewall_now - mb->sbrec->timestamp;
> + const struct sbrec_mac_binding *sbrec = mb->sbrec;
> + mac_binding_update_log("Probe: trying to refresh", &mb->data, true,
> + threshold, stats->idle_age_ms,
> + since_updated_ms);
The log here is a little misleading. It makes me think that we're sending
the ARP request, while in practice we do not. This should be moved below
to the actual ARP/ND send and maybe re-worded to highlight that fact.
Ideally, all the cases below should have their own log messages explaining
why we're not sending ARP/ND for this MAC binding.
E.g.
"Sending ARP/ND request for active"
"Not sending ARP/ND request for non-active"
"Not sending ARP/ND request for recently updated"
"Not sending ARP/ND request (no local address) for active"
Can the last one even happen? I'm not sure. BTW, what happens if the address
of the local interface changes? Will the cache be updated? Or do we expect
the entry to just expire?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
