> On 29 Jul 2024, at 11:22 AM, Ales Musil <[email protected]> wrote:
>
> CAUTION: External Email
>
>
> On Fri, Jul 26, 2024 at 9:08 PM Naveen Yerramneni
> <[email protected]> wrote:
> This change avoids northd full recompute for FDB updates.
> Also, fixed incremental processing for port deletion to
> delete all fdb entries associated to a port.
>
> Signed-off-by: Naveen Yerramneni <[email protected]>
> ---
> v2:
>
> - Addressed review comments from Ales.
>
> - Fixed incremental processing for port deletion to
>
> delete all fdb entries associated to a port.
>
> ---
>
> Hi Naveen,
>
> thank you for the v2, I have a couple of comments down below.
> northd/en-northd.c | 37 +++++++++++++++++++++++++++++++++++++
> northd/en-northd.h | 1 +
> northd/inc-proc-northd.c | 2 +-
> northd/northd.c | 11 +++++------
> northd/northd.h | 3 +++
> 5 files changed, 47 insertions(+), 7 deletions(-)
>
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 4479b4aff..badc49d43 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -259,3 +259,40 @@ en_northd_clear_tracked_data(void *data_)
> struct northd_data *data = data_;
> destroy_northd_data_tracked_changes(data);
> }
> +
> +bool
> +sb_fdb_change_handler(struct engine_node *node, void *data)
> +{
> + struct northd_data *nd = data;
> + const struct sbrec_fdb_table *sbrec_fdb_table =
> + EN_OVSDB_GET(engine_get_input("SB_fdb", node));
> +
> + /* check if changed rows are stale and delete them */
> + const struct sbrec_fdb *fdb_e, *fdb_prev_del = NULL;
>
> I'm kinda confused as to why the fdb_prev_del is needed at all.
I couldn’t find SAFE variant for SBREC_FDB_TABLE_FOR_EACH_TRACKED
similar to SBREC_FDB_TABLE_FOR_EACH_SAFE hence I used fdb_prev_del
to store and delete the previous stale entry during the iteration.
> + SBREC_FDB_TABLE_FOR_EACH_TRACKED(fdb_e, sbrec_fdb_table) {
> + if (sbrec_fdb_is_deleted(fdb_e)) {
> + continue;
> + }
> +
> + if (fdb_prev_del) {
> + sbrec_fdb_delete(fdb_prev_del);
> + }
> +
> + fdb_prev_del = fdb_e;
> + struct ovn_datapath *od
> + = ovn_datapath_find_by_key(&nd->ls_datapaths.datapaths,
> + fdb_e->dp_key);
> + if (od) {
> + if (ovn_tnlid_present(&od->port_tnlids, fdb_e->port_key)) {
> + fdb_prev_del = NULL;
> + }
> + }
>
> Why not the following?
> if (!(od && ovn_tnlid_present(...)) {
> sbrec_fdb_delete(fdb_e);
> }
>
>
> + }
> +
> + if (fdb_prev_del) {
> + sbrec_fdb_delete(fdb_prev_del);
> + }
> +
> + engine_set_node_state(node, EN_UPDATED);
> + return true;
> +}
> diff --git a/northd/en-northd.h b/northd/en-northd.h
> index 9b7bda32a..9c722e401 100644
> --- a/northd/en-northd.h
> +++ b/northd/en-northd.h
> @@ -19,5 +19,6 @@ bool northd_nb_logical_switch_handler(struct engine_node *,
> void *data);
> bool northd_nb_logical_router_handler(struct engine_node *, void *data);
> bool northd_sb_port_binding_handler(struct engine_node *, void *data);
> bool northd_lb_data_handler(struct engine_node *, void *data);
> +bool sb_fdb_change_handler(struct engine_node *node, void *data);
>
> #endif /* EN_NORTHD_H */
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index d56e9783a..213e6d88a 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -189,7 +189,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> engine_add_input(&en_northd, &en_sb_ha_chassis_group, NULL);
> engine_add_input(&en_northd, &en_sb_ip_multicast, NULL);
> engine_add_input(&en_northd, &en_sb_service_monitor, NULL);
> - engine_add_input(&en_northd, &en_sb_fdb, NULL);
> + engine_add_input(&en_northd, &en_sb_fdb, sb_fdb_change_handler);
> engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL);
> engine_add_input(&en_northd, &en_sb_chassis_template_var, NULL);
> engine_add_input(&en_northd, &en_global_config,
> diff --git a/northd/northd.c b/northd/northd.c
> index 6898daa00..8ad3ad285 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -496,7 +496,7 @@ ovn_datapath_find(const struct hmap *datapaths,
> return ovn_datapath_find_(datapaths, uuid);
> }
>
> -static struct ovn_datapath *
> +struct ovn_datapath *
> ovn_datapath_find_by_key(struct hmap *datapaths, uint32_t dp_key)
> {
> struct ovn_datapath *od;
> @@ -3300,13 +3300,12 @@ delete_fdb_entry(struct ovsdb_idl_index
> *sbrec_fdb_by_dp_and_port,
> sbrec_fdb_index_set_dp_key(target, dp_key);
> sbrec_fdb_index_set_port_key(target, port_key);
>
> - struct sbrec_fdb *fdb_e = sbrec_fdb_index_find(sbrec_fdb_by_dp_and_port,
> - target);
> - sbrec_fdb_index_destroy_row(target);
> -
> - if (fdb_e) {
> + struct sbrec_fdb *fdb_e;
> + while ((fdb_e = sbrec_fdb_index_find(sbrec_fdb_by_dp_and_port,
> + target))) {
>
> Why is this change needed and wasn't before? What has changed?
>
In previous code, assume there are 2 entries associated to a port. When port
got deleted,
one of the 2 entries gets removed from FDB table, this triggers FDB update and
since there
was no change handler full recompute used to happen. As part of full recompute,
the remaining
stale entries used to get removed.
Now, we have change handler in which we are iterating only tracked flows hence
unmodified stale
entries still remain in the FDB table without this change.
> Also the proper way to iterate over index is to use the FOR_EACH_EQUAL macro:
>
> struct sbrec_fdb *fdb_e;
> SBREC_FDB_FOR_EACH_EQUAL (fdb_e, target, sbrec_fdb_by_dp_and_port) {
> sbrec_fdb_delete(fdb_e);
> }
Ack, will address in v3.
> sbrec_fdb_delete(fdb_e);
> }
> + sbrec_fdb_index_destroy_row(target);
> }
>
> struct service_monitor_info {
> diff --git a/northd/northd.h b/northd/northd.h
> index d4a8d75ab..26fe24df3 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -87,6 +87,9 @@ ods_size(const struct ovn_datapaths *datapaths)
> return hmap_count(&datapaths->datapaths);
> }
>
> +struct ovn_datapath *
> +ovn_datapath_find_by_key(struct hmap *datapaths, uint32_t dp_key);
> +
> bool od_has_lb_vip(const struct ovn_datapath *od);
>
> struct tracked_ovn_ports {
> --
> 2.36.6
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev [mail.openvswitch.org]
>
>
> Thanks,
> Ales
>
> --
> Ales Musil
> Senior Software Engineer - OVN Core
> Red Hat EMEA [redhat.com][email protected] [red.ht]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
