To improve ovn-northd performance when processing changes to IGMP groups this patch implements a handler to process IGMP group changes instead of relying on a full database recompute.
I tested using a script that is available here: https://gist.github.com/JacobTanenbaum/da70c75178bec2b8e1cf5487c74466e3 when running this script inside the OVN sandbox it creates sw0 and 5000 ports on sw0. It then adds one IGMP group with 1 port in it and waits for the databases to sync using upstream main Time spent on processing nb_cfg 2: ovn-northd delay before processing: 5ms ovn-northd completion: 5ms this is a typical output after adding the IGMP group waiting for the Sb to sync while using a branch with this patch: Time spent on processing nb_cfg 2: ovn-northd delay before processing: 1ms ovn-northd completion: 1ms this is the time that it takes to wait for the database to sync. Additionally the time it takes to add an IGMP group increases the more ports are on a switch from the current head of main while on a branch with this patch the time to add an IGMP group remains the same regardless of the size of the database (up to the 100000 ports on the logical switch that I have tested). Signed-off-by: Jacob Tanenbaum <[email protected]> Suggested-by: Dumitru Ceara <[email protected]> Reported-at: https://issues.redhat.com/browse/FDP-756 --- v4: split into two patches removed checking if uuid in uuidset added more specific checks to testing changed else if to else for updates to igmp_groups separated the deletion code into it's own function v3: Removed incorrect call to ovn_igmp_group_destroy() v2: Rebase on top of latest main. Fix memory leaks detected by CI. Capitalize first letter of the subject line Fix typos in commit message diff --git a/northd/en-lflow.c b/northd/en-lflow.c index fa1f0236d..6c3e20695 100644 --- a/northd/en-lflow.c +++ b/northd/en-lflow.c @@ -233,3 +233,20 @@ void en_lflow_cleanup(void *data_) struct lflow_data *data = data_; lflow_table_destroy(data->lflow_table); } + +bool +igmp_group_change_handler(struct engine_node *node, void *data) +{ + const struct sbrec_igmp_group_table *igmp_data = + EN_OVSDB_GET(engine_get_input("SB_igmp_group", node)); + + const struct engine_context *eng_ctx = engine_get_context(); + struct lflow_input lflow_input; + lflow_get_input_data(node, &lflow_input); + + struct lflow_data *lflow_data = data; + return handle_igmp_change(eng_ctx->ovnsb_idl_txn, + igmp_data, + &lflow_input, + lflow_data->lflow_table); +} diff --git a/northd/en-lflow.h b/northd/en-lflow.h index 32cae6176..7c8a4ae1a 100644 --- a/northd/en-lflow.h +++ b/northd/en-lflow.h @@ -22,5 +22,6 @@ bool lflow_northd_handler(struct engine_node *, void *data); bool lflow_port_group_handler(struct engine_node *, void *data); bool lflow_lr_stateful_handler(struct engine_node *, void *data); bool lflow_ls_stateful_handler(struct engine_node *node, void *data); +bool igmp_group_change_handler(struct engine_node *node, void *data); #endif /* EN_LFLOW_H */ diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 6e0aa04c4..2f0a1d7cf 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -271,7 +271,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_lflow, &en_sync_meters, NULL); engine_add_input(&en_lflow, &en_sb_logical_flow, NULL); engine_add_input(&en_lflow, &en_sb_multicast_group, NULL); - engine_add_input(&en_lflow, &en_sb_igmp_group, NULL); + engine_add_input(&en_lflow, &en_sb_igmp_group, igmp_group_change_handler); engine_add_input(&en_lflow, &en_sb_logical_dp_group, NULL); engine_add_input(&en_lflow, &en_bfd_sync, NULL); engine_add_input(&en_lflow, &en_route_policies, NULL); diff --git a/northd/northd.c b/northd/northd.c index 0dfeee0b9..0dd527414 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -462,6 +462,7 @@ ovn_datapath_create(struct hmap *datapaths, const struct uuid *key, od->lr_group = NULL; hmap_init(&od->ports); sset_init(&od->router_ips); + od->igmp_lflow_ref = lflow_ref_create(); return od; } @@ -491,6 +492,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od) destroy_mcast_info_for_datapath(od); destroy_ports_for_datapath(od); sset_destroy(&od->router_ips); + lflow_ref_destroy(od->igmp_lflow_ref); free(od); } } @@ -10163,7 +10165,7 @@ build_lswitch_destination_lookup_bmcast(struct ovn_datapath *od, ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 80, "ip4.mcast || ip6.mcast", - ds_cstr(actions), lflow_ref); + ds_cstr(actions), od->igmp_lflow_ref); } } @@ -10258,7 +10260,8 @@ build_lswitch_ip_mcast_igmp_mld(struct ovn_igmp_group *igmp_group, igmp_group->mcgroup.name); ovn_lflow_add(lflows, igmp_group->datapath, S_SWITCH_IN_L2_LKUP, - 90, ds_cstr(match), ds_cstr(actions), NULL); + 90, ds_cstr(match), ds_cstr(actions), + igmp_group->datapath->igmp_lflow_ref); } } @@ -17268,7 +17271,7 @@ build_lswitch_and_lrouter_iterate_by_lr(struct ovn_datapath *od, lsi->parsed_routes, lsi->route_tables, lsi->bfd_ports, NULL); build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match, - &lsi->actions, NULL); + &lsi->actions, od->igmp_lflow_ref); build_ingress_policy_flows_for_lrouter(od, lsi->lflows, lsi->lr_ports, lsi->route_policies, NULL); build_arp_resolve_flows_for_lrouter(od, lsi->lflows, NULL); @@ -17798,6 +17801,14 @@ build_mcast_groups(const struct sbrec_igmp_group_table *sbrec_igmp_group_table, const struct hmap *lr_ports, struct hmap *mcast_groups, struct hmap *igmp_groups); +static void +delete_igmp_group(const struct sbrec_igmp_group *sb_igmp, + struct ovn_datapath *od, + struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp, + struct lflow_table *lflows, + struct uuidset *datapaths_to_sync, + struct ds *actions, + struct ds *match); static void build_igmp_group_for_sb(const struct sbrec_igmp_group *sb_igmp, @@ -17814,6 +17825,14 @@ static void build_mcast_group_from_igmp_group(struct ovn_igmp_group *igmp_group, struct hmap *mcast_groups, struct hmap *igmp_groups); +static void +process_peer_switches_for_igmp(struct ovn_igmp_group *igmp_group, + struct uuidset *datapaths_to_sync, + struct hmap *mcast_groups, + struct lflow_table *lflows, + struct ds *actions, + const struct shash *meter_groups); + static struct sbrec_multicast_group * create_sb_multicast_group(struct ovsdb_idl_txn *ovnsb_txn, const struct sbrec_datapath_binding *dp, @@ -19326,6 +19345,217 @@ northd_get_datapath_for_port(const struct hmap *ls_ports, return op ? op->od : NULL; } +/* handle changes to the sbrec_igmp_group_table in the incremental + * processor + */ +bool +handle_igmp_change(struct ovsdb_idl_txn *ovnsb_txn, + const struct sbrec_igmp_group_table *sbrec_igmp_group_table, + struct lflow_input *input_data, + struct lflow_table *lflows) +{ + struct hmap igmp_groups = HMAP_INITIALIZER(&igmp_groups); + struct hmap mcast_groups = HMAP_INITIALIZER(&mcast_groups); + struct ds match = DS_EMPTY_INITIALIZER; + struct ds actions = DS_EMPTY_INITIALIZER; + + struct uuidset datapaths_to_sync = + UUIDSET_INITIALIZER(&datapaths_to_sync); + + const struct sbrec_igmp_group *sb_igmp; + SBREC_IGMP_GROUP_TABLE_FOR_EACH_TRACKED (sb_igmp, sbrec_igmp_group_table) { + struct ovn_datapath *od = + ovn_datapath_from_sbrec(&input_data->ls_datapaths->datapaths, + &input_data->lr_datapaths->datapaths, + sb_igmp->datapath); + /* Processing new IGMP group. */ + if (sbrec_igmp_group_is_new(sb_igmp)) { + + build_igmp_group_for_sb(sb_igmp, + od, + input_data->sbrec_mcast_group_by_name_dp, + input_data->ls_ports, &igmp_groups); + + + build_igmp_router_entries(od, + input_data->sbrec_mcast_group_by_name_dp, + &igmp_groups); + + /* Processing a deleted IGMP group. */ + } else if (sbrec_igmp_group_is_deleted(sb_igmp)) { + delete_igmp_group(sb_igmp, + od, + input_data->sbrec_mcast_group_by_name_dp, + lflows, + &datapaths_to_sync, + &actions, + &match); + /* Processing an updated IGMP group. */ + } else { + build_igmp_group_for_sb(sb_igmp, + od, + input_data->sbrec_mcast_group_by_name_dp, + input_data->ls_ports, &igmp_groups); + + /* Do not need to worry about building IGMP groups for multicast + * routers with relay enabled. Updating the IGMP group does not + * make any changes applicable to them. + */ + } + } + + + struct ovn_igmp_group *igmp_group; + HMAP_FOR_EACH_SAFE (igmp_group, hmap_node, &igmp_groups) { + if (igmp_group->datapath->nbr) { + /* For every router datapath each port of the peer switches + * need to be checked if they flood multicast. + */ + process_peer_switches_for_igmp(igmp_group, + &datapaths_to_sync, + &mcast_groups, + lflows, + &actions, + input_data->meter_groups); + } + build_mcast_group_from_igmp_group(igmp_group, + &mcast_groups, + &igmp_groups); + } + + /* After all the routers have been processed the flows for the + * the switches can be created */ + HMAP_FOR_EACH_SAFE (igmp_group, hmap_node, &igmp_groups) { + build_lswitch_ip_mcast_igmp_mld(igmp_group, lflows, &actions, &match); + + uuidset_insert(&datapaths_to_sync, &igmp_group->datapath->key); + } + + struct ovn_multicast *mc; + HMAP_FOR_EACH_SAFE (mc, hmap_node, &mcast_groups) { + const struct sbrec_multicast_group *sbmc = + mcast_group_lookup(input_data->sbrec_mcast_group_by_name_dp, + mc->group->name, mc->datapath->sb); + if (sbmc) { + for (size_t i = 0; i < mc->n_ports; i++) { + sbrec_multicast_group_update_ports_addvalue(sbmc, + mc->ports[i]->sb); + } + } else { + sbmc = create_sb_multicast_group(ovnsb_txn, mc->datapath->sb, + mc->group->name, mc->group->key); + ovn_multicast_update_sbrec(mc, sbmc); + } + if (mc->datapath->nbr) { + build_mcast_lookup_flows_for_lrouter(mc->datapath, + lflows, + &match, + &actions, + mc->datapath->igmp_lflow_ref); + + uuidset_insert(&datapaths_to_sync, &mc->datapath->key); + } + ovn_multicast_destroy(&mcast_groups, mc); + } + struct uuidset_node *uuidnode; + UUIDSET_FOR_EACH (uuidnode, &datapaths_to_sync) { + const struct ovn_datapath *dp = + ovn_datapath_find(&input_data->ls_datapaths->datapaths, + &uuidnode->uuid); + if (!dp) { + dp = ovn_datapath_find(&input_data->lr_datapaths->datapaths, + &uuidnode->uuid); + } + + lflow_ref_sync_lflows(dp->igmp_lflow_ref, + lflows, ovnsb_txn, + input_data->ls_datapaths, + input_data->lr_datapaths, + input_data->ovn_internal_version_changed, + input_data->sbrec_logical_flow_table, + input_data->sbrec_logical_dp_group_table); + } + + ds_destroy(&match); + ds_destroy(&actions); + + /* It is required to cleanup igmp_groups after multicast_groups */ + HMAP_FOR_EACH_SAFE (igmp_group, hmap_node, &igmp_groups) { + ovn_igmp_group_destroy(&igmp_groups, igmp_group); + } + hmap_destroy(&igmp_groups); + hmap_destroy(&mcast_groups); + uuidset_destroy(&datapaths_to_sync); + + return true; +} + +/* Process the deletion of an IGMP group. We only need to remove the + * multicast_groups associated with the datapath the IGMP group is on and the + */ +static void +delete_igmp_group(const struct sbrec_igmp_group *sb_igmp, + struct ovn_datapath *od, + struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp, + struct lflow_table *lflows, + struct uuidset *datapaths_to_sync, + struct ds *actions, + struct ds *match) +{ + /* The igmp group for multicast routers does not have a + * multicast group, skip the deletion + */ + if (!strcmp(sb_igmp->address, OVN_IGMP_GROUP_MROUTERS)) { + return; + } + + for (size_t i = 0; i < od->n_router_ports; i++) { + struct ovn_port *port = od->router_ports[i]; + lflow_ref_unlink_lflows(port->peer->od->igmp_lflow_ref); + build_mcast_lookup_flows_for_lrouter(od->router_ports[i]->peer->od, + lflows, + match, + actions, + port->peer->od->igmp_lflow_ref); + + uuidset_insert(datapaths_to_sync, &od->router_ports[i]->peer->od->key); + const struct sbrec_multicast_group *sbmc = + mcast_group_lookup(sbrec_mcast_group_by_name_dp, + sb_igmp->address, port->peer->od->sb); + if (sbmc) { + sbrec_multicast_group_delete(sbmc); + } + } + const struct sbrec_multicast_group *sbmc = + mcast_group_lookup(sbrec_mcast_group_by_name_dp, + sb_igmp->address, sb_igmp->datapath); + int removedPorts = 0; + for (size_t i = 0; i <sb_igmp->n_ports; i++) { + + if (smap_get_bool(&sb_igmp->ports[i]->options, + "mcast_flood", + false)) { + /* If the port has mcast_flood set it is not in + * the multicast_group for this igmp_group + */ + continue; + } + sbrec_multicast_group_update_ports_delvalue(sbmc, + sb_igmp->ports[i]); + removedPorts++; + } + if (sb_igmp->n_ports >= sbmc->n_ports) { + lflow_ref_unlink_lflows(od->igmp_lflow_ref); + uuidset_insert(datapaths_to_sync, &od->key); + } + /* if the deletion removed all the ports from the + * multicast_group delete it from the database + */ + if (removedPorts == sbmc->n_ports) { + sbrec_multicast_group_delete(sbmc); + } +} + static void build_igmp_group_for_sb(const struct sbrec_igmp_group *sb_igmp, struct ovn_datapath *od, @@ -19452,3 +19682,42 @@ build_mcast_group_from_igmp_group(struct ovn_igmp_group *igmp_group, */ ovn_igmp_group_aggregate_ports(igmp_group, mcast_groups); } + +/* For a given igmp_group check all the peer switches on the igmp_groups + * datapath for ports with mcast_info.flood set, If a port does + * mcast_info.flood set add it to the MC_STATIC group and prepare to update the + * datapaths logical flows. + */ +static void +process_peer_switches_for_igmp(struct ovn_igmp_group *igmp_group, + struct uuidset *datapaths_to_sync, + struct hmap *mcast_groups, + struct lflow_table *lflows, + struct ds *actions, + const struct shash *meter_groups) +{ + + for (size_t i = 0; i < igmp_group->datapath->n_ls_peers; i++) { + struct ovn_datapath *ls_peer = igmp_group->datapath->ls_peers[i]; + struct ovn_port *op; + + HMAP_FOR_EACH_SAFE (op, dp_node, &ls_peer->ports) { + /* If this port is configured to always flood multicast traffic + * add it to the MC_STATIC group. + */ + if (op->mcast_info.flood) { + ovn_multicast_add(mcast_groups, &mc_static, op); + op->od->mcast_info.sw.flood_static = true; + + lflow_ref_unlink_lflows(op->od->igmp_lflow_ref); + build_lswitch_destination_lookup_bmcast(op->od, + lflows, + actions, + meter_groups, + NULL); + + uuidset_insert(datapaths_to_sync, &op->od->key); + } + } + } +} diff --git a/northd/northd.h b/northd/northd.h index d60c944df..7987105ac 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -375,6 +375,7 @@ struct ovn_datapath { /* The logical router group to which this datapath belongs. * Valid only if it is logical router datapath. NULL otherwise. */ struct lrouter_group *lr_group; + struct lflow_ref *igmp_lflow_ref; /* Map of ovn_port objects belonging to this datapath. * This map doesn't include derived ports. */ @@ -862,4 +863,10 @@ is_vxlan_mode(const struct smap *nb_options, uint32_t get_ovn_max_dp_key_local(bool _vxlan_mode); +bool +handle_igmp_change(struct ovsdb_idl_txn *ovnsb_txn, + const struct sbrec_igmp_group_table *sbrec_igmp_group_table, + struct lflow_input *input_data, + struct lflow_table *lflows); + #endif /* NORTHD_H */ diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 4eae1c67c..06385f515 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -14,6 +14,7 @@ m4_define([_DUMP_DB_TABLES], [ ovn-sbctl list port_group >> $1 ovn-sbctl list bfd >> $1 ovn-sbctl dump-flows > lflows_$1 + ovn-sbctl list multicast_group > mgroup_$1 ]) # CHECK_NO_CHANGE_AFTER_RECOMPUTE @@ -10949,6 +10950,92 @@ OVN_CLEANUP([hv1]) AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([IGMP incremental processing]) + +check_recompute_counter() { + lflow_recomp=$(as northd ovn-appctl -t ovn-northd inc-engine/show-stats lflow recompute) + AT_CHECK([test x$lflow_recomp = x$1]) +} +ovn_start + +net_add n1 +sim_add hv1 +as hv1 + +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.11 + +sim_add hv2 +as hv2 + +check ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.2 + +check ovn-nbctl ls-add sw1 +check ovn-nbctl ls-add sw2 + +check ovn-nbctl lsp-add sw1 sw1-p11 +check ovn-nbctl lsp-add sw2 sw2-p21 +#check ovn-nbctl set Logical_Switch_Port sw2-p21 options:mcast_flood='true' + +check ovn-nbctl lr-add rtr +check ovn-nbctl lrp-add rtr rtr-sw1 00:00:00:00:01:00 10.0.0.254/24 +check ovn-nbctl lrp-add rtr rtr-sw2 00:00:00:00:02:00 10.0.0.254/24 + + +check ovn-nbctl lsp-add sw1 sw1-rtr \ + -- lsp-set-type sw1-rtr router \ + -- lsp-set-addresses sw1-rtr 00:00:00:00:01:00 \ + -- lsp-set-options sw1-rtr router-port=rtr-sw1 + +check ovn-nbctl lsp-add sw2 sw2-rtr \ + -- lsp-set-type sw2-rtr router \ + -- lsp-set-addresses sw1-rtr 00:00:00:00:02:00 \ + -- lsp-set-options sw2-rtr router-port=rtr-sw2 + +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +#create IGMP_GROUP 239.0.1.68 with port sw1-p11 +ovn-sbctl create IGMP_GROUP address=239.0.1.68 \ + datapath=$(fetch_column Datapath_Binding _uuid external_ids:name=sw1) \ + chassis=$(fetch_column Chassis _uuid name=hv1) \ + chassis_name=hv1 \ + ports=$(fetch_column Port_Binding _uuid logical_port=sw1-p11) + +check ovn-nbctl --wait=sb sync +wait_row_count IGMP_GROUP 1 address=239.0.1.68 +wait_row_count MULTICAST_GROUP 1 name="239.0.1.68" +wait_row_count MULTICAST_GROUP 1 name="239.0.1.68" ports='[['$(fetch_column Port_Binding _uuid logical_port=sw1-p11)']]' +check_recompute_counter 0 +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +check ovn-nbctl set logical_router rtr \ + options:mcast_relay="true" +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +#update IGMP_GROUP 239.0.1.68 to include sw2-p21 +ovn-sbctl add igmp_group $(fetch_column IGMP_GROUP _uuid address=239.0.1.68) ports $(fetch_column Port_Binding _uuid logical_port=sw2-p21) + +ovn-nbctl --wait=sb sync +wait_row_count IGMP_GROUP 1 address=239.0.1.68 +# a new multicast_group is created +wait_row_count MULTICAST_GROUP 2 name=239.0.1.68 +check_recompute_counter 0 +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +#delete IGMP_GROUP 239.0.1.68 +ovn-sbctl destroy igmp_group $(fetch_column IGMP_GROUP _uuid address=239.0.1.68) +check ovn-nbctl --wait=sb sync +check_recompute_counter 0 +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +wait_row_count IGMP_GROUP 0 address=239.0.1.68 +wait_row_count MULTICAST_GROUP 0 name=239.0.1.68 + +OVN_CLEANUP([hv1], [hv2]) +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD_NO_HV([ AT_SETUP([ACL/Meter incremental processing - no northd recompute]) ovn_start -- 2.47.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
