| Message ID | 20240726190716.107346-1-naveen.yerramneni@nutanix.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] northd: Add change handler for FDB updates. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
References: <20240726190716.107346-1-naveen.yerramneni@nutanix.com>
Bleep bloop. Greetings Naveen Yerramneni, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
ERROR: Improper whitespace around control block
#37 FILE: northd/en-northd.c:272:
SBREC_FDB_TABLE_FOR_EACH_TRACKED(fdb_e, sbrec_fdb_table) {
Lines checked: 136, Warnings: 0, Errors: 1
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On Fri, Jul 26, 2024 at 9:08 PM Naveen Yerramneni < naveen.yerramneni@nutanix.com> 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 <naveen.yerramneni@nutanix.com> > --- > 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. > + 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? 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); } > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales
> On 29 Jul 2024, at 11:22 AM, Ales Musil <amusil@redhat.com> wrote: > > CAUTION: External Email > > > On Fri, Jul 26, 2024 at 9:08 PM Naveen Yerramneni <naveen.yerramneni@nutanix.com> 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 <naveen.yerramneni@nutanix.com> > --- > 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 > dev@openvswitch.org > 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]amusil@redhat.com [red.ht]
On Mon, Jul 29, 2024 at 8:16 AM Naveen Yerramneni < naveen.yerramneni@nutanix.com> wrote: > > > > On 29 Jul 2024, at 11:22 AM, Ales Musil <amusil@redhat.com> wrote: > > > > CAUTION: External Email > > > > > > On Fri, Jul 26, 2024 at 9:08 PM Naveen Yerramneni < > naveen.yerramneni@nutanix.com> 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 <naveen.yerramneni@nutanix.com> > > --- > > 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. > Ah right, nevertheless the entries to delete should be probably stored in some structure (array maybe) to iterate over them at the end WDYT? > > > + 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. > > Also it should be made explicit by the function name that it is removing more than one row now e.g. delete_fdb_entries(). > > 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 > > dev@openvswitch.org > > 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]amusil@redhat.com [red.ht] > > >
> On 29 Jul 2024, at 12:10 PM, Ales Musil <amusil@redhat.com> wrote: > > CAUTION: External Email > > > On Mon, Jul 29, 2024 at 8:16 AM Naveen Yerramneni <naveen.yerramneni@nutanix.com> wrote: > > > > On 29 Jul 2024, at 11:22 AM, Ales Musil <amusil@redhat.com> wrote: > > > > CAUTION: External Email > > > > > > On Fri, Jul 26, 2024 at 9:08 PM Naveen Yerramneni <naveen.yerramneni@nutanix.com> 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 <naveen.yerramneni@nutanix.com> > > --- > > 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. > > > Ah right, nevertheless the entries to delete should be probably stored in > some structure (array maybe) to iterate over them at the end WDYT? > I preferred this way since the no.of entries to be deleted are variable and they could be large as well incase if there are multiple MACs learnt on single port. If you suggest to delete all entries at the end by storing them into some structure then, I think we need to maintain some list since the size is variable. > > > > + 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. > > > Also it should be made explicit by the function name that it is removing more than one > row now e.g. delete_fdb_entries(). Ack. > > 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev [mail.openvswitch.org] [mail.openvswitch.org [mail.openvswitch.org]] > > > > > > Thanks, > > Ales > > > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA [redhat.com [redhat.com]]amusil@redhat.com [red.ht [red.ht]] > > > > > -- > Ales Musil > Senior Software Engineer - OVN Core > Red Hat EMEA [redhat.com]amusil@redhat.com [red.ht]
On Mon, Jul 29, 2024 at 9:01 AM Naveen Yerramneni < naveen.yerramneni@nutanix.com> wrote: > > > > On 29 Jul 2024, at 12:10 PM, Ales Musil <amusil@redhat.com> wrote: > > > > CAUTION: External Email > > > > > > On Mon, Jul 29, 2024 at 8:16 AM Naveen Yerramneni < > naveen.yerramneni@nutanix.com> wrote: > > > > > > > On 29 Jul 2024, at 11:22 AM, Ales Musil <amusil@redhat.com> wrote: > > > > > > CAUTION: External Email > > > > > > > > > On Fri, Jul 26, 2024 at 9:08 PM Naveen Yerramneni < > naveen.yerramneni@nutanix.com> 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 <naveen.yerramneni@nutanix.com> > > > --- > > > 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. > > > > > > Ah right, nevertheless the entries to delete should be probably stored in > > some structure (array maybe) to iterate over them at the end WDYT? > > > > I preferred this way since the no.of entries to be deleted are variable > and they > could be large as well incase if there are multiple MACs learnt on single > port. > > If you suggest to delete all entries at the end by storing them into some > structure then, > I think we need to maintain some list since the size is variable. > Let's keep it as is then, it can always be changed later if needed. > > > > > > > + 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. > > > > > > Also it should be made explicit by the function name that it is removing > more than one > > row now e.g. delete_fdb_entries(). > > Ack. > > > > 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 > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev [ > mail.openvswitch.org] [mail.openvswitch.org [mail.openvswitch.org]] > > > > > > > > > Thanks, > > > Ales > > > > > > -- > > > Ales Musil > > > Senior Software Engineer - OVN Core > > > Red Hat EMEA [redhat.com [redhat.com]]amusil@redhat.com [red.ht [ > red.ht]] > > > > > > > > > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA [redhat.com]amusil@redhat.com [red.ht] > > >
> On 29 Jul 2024, at 1:47 PM, Ales Musil <amusil@redhat.com> wrote: > > CAUTION: External Email > > > On Mon, Jul 29, 2024 at 9:01 AM Naveen Yerramneni <naveen.yerramneni@nutanix.com> wrote: > > > > On 29 Jul 2024, at 12:10 PM, Ales Musil <amusil@redhat.com> wrote: > > > > CAUTION: External Email > > > > > > On Mon, Jul 29, 2024 at 8:16 AM Naveen Yerramneni <naveen.yerramneni@nutanix.com> wrote: > > > > > > > On 29 Jul 2024, at 11:22 AM, Ales Musil <amusil@redhat.com> wrote: > > > > > > CAUTION: External Email > > > > > > > > > On Fri, Jul 26, 2024 at 9:08 PM Naveen Yerramneni <naveen.yerramneni@nutanix.com> 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 <naveen.yerramneni@nutanix.com> > > > --- > > > 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. > > > > > > Ah right, nevertheless the entries to delete should be probably stored in > > some structure (array maybe) to iterate over them at the end WDYT? > > > > I preferred this way since the no.of entries to be deleted are variable and they > could be large as well incase if there are multiple MACs learnt on single port. > > If you suggest to delete all entries at the end by storing them into some structure then, > I think we need to maintain some list since the size is variable. > > > Let's keep it as is then, it can always be changed later if needed. > Hi Ales, Addressed all the review comments and sent v3. Thanks, Naveen > > > > > > > > + 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. > > > > > > Also it should be made explicit by the function name that it is removing more than one > > row now e.g. delete_fdb_entries(). > > Ack. > > > > 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 > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev [mail.openvswitch.org] [mail.openvswitch.org [mail.openvswitch.org]] [mail.openvswitch.org [mail.openvswitch.org] [mail.openvswitch.org [mail.openvswitch.org]]] > > > > > > > > > Thanks, > > > Ales > > > > > > -- > > > Ales Musil > > > Senior Software Engineer - OVN Core > > > Red Hat EMEA [redhat.com [redhat.com] [redhat.com [redhat.com]]]amusil@redhat.com [red.ht [red.ht] [red.ht [red.ht]]] > > > > > > > > > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA [redhat.com [redhat.com]]amusil@redhat.com [red.ht [red.ht]] > > > > > -- > Ales Musil > Senior Software Engineer - OVN Core > Red Hat EMEA [redhat.com]amusil@redhat.com [red.ht]
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; + 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; + } + } + } + + 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))) { 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 {
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 <naveen.yerramneni@nutanix.com> --- v2: - Addressed review comments from Ales. - Fixed incremental processing for port deletion to delete all fdb entries associated to a port. --- 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(-)