On 25/10/2021 04:21, Han Zhou wrote:
> On Mon, Oct 18, 2021 at 5:14 AM Mark Gray <mark.d.g...@redhat.com> wrote:
>>
>> 'struct northd_data' is used to hold the output data from the
>> incremental processing node 'en_northd'. This will be used, in
>> a later commit, as the input data to 'en_lflow'. In order to achieve
>> this, we refactor in the following way:
>>
>> * Introduce northd_init() which initializes this data.
>> * Introduce northd_destroy() which clears this data for a new iteration.
>> * Introduce northd_indices_create() which does a one-time creation of
>> indices for number of tables needed by northd.
>> * Remove 'ovn_internal_version' from the context passed to northd
>> as it can be determined within northd using ovn_get_internal_version.
>> * Remove 'use_parallel_build' from the context passed to northd
>> as it can be determined within northd using can_parallelize_hashes().
>> * Refactor northd.c to use 'struct northd_data' where applicable.
>>
>> Signed-off-by: Mark Gray <mark.d.g...@redhat.com>
>> ---
>> northd/en-northd.c | 28 ++++-
>> northd/en-northd.h | 4 +
>> northd/northd.c | 278 ++++++++++++++++++++++++--------------------
>> northd/northd.h | 28 ++++-
>> northd/ovn-northd.c | 28 +----
>> 5 files changed, 204 insertions(+), 162 deletions(-)
>>
>> diff --git a/northd/en-northd.c b/northd/en-northd.c
>> index 8dec51535af0..1b206521e152 100644
>> --- a/northd/en-northd.c
>> +++ b/northd/en-northd.c
>> @@ -20,26 +20,46 @@
>>
>> #include "en-northd.h"
>> #include "lib/inc-proc-eng.h"
>> +#include "openvswitch/list.h" /* TODO This is needed for
> ovn-parallel-hmap.h.
>> + * lib/ovn-parallel-hmap.h should be
> updated
>> + * to include this dependency itself */
>> +#include "lib/ovn-parallel-hmap.h"
>> #include "northd.h"
>> +#include "lib/util.h"
>> #include "openvswitch/vlog.h"
>>
>> VLOG_DEFINE_THIS_MODULE(en_northd);
>>
>> -void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
>> +void en_northd_run(struct engine_node *node, void *data)
>> {
>> const struct engine_context *eng_ctx = engine_get_context();
>> struct northd_idl_context *ctx = eng_ctx->client_ctx;
>> - ovn_db_run(ctx);
>> + struct northd_data *northd_data = ((struct ed_type_northd
> *)data)->data;
>>
>> + northd_destroy(northd_data);
>> + northd_init(northd_data);
>> +
>> + northd_run(ctx, northd_data);
>> engine_set_node_state(node, EN_UPDATED);
>>
>> }
>> void *en_northd_init(struct engine_node *node OVS_UNUSED,
>> - struct engine_arg *arg OVS_UNUSED)
>> + struct engine_arg *arg)
>> {
>> - return NULL;
>> + struct ed_type_northd *data = xmalloc(sizeof *data);
>> + data->data = xmalloc(sizeof *data->data);
>> +
>> + data->data->use_parallel_build = can_parallelize_hashes(false);
>> + northd_indices_create(data->data, arg->sb_idl);
>
> As mentioned in my comment in patch 2, the OVSDB indices should belong to
> the data of the corresponding OVSDB table, because 1) it is more natural,
> 2) different engine nodes can share the same input and so same OVSDB index.
> engine_ovsdb_node_add_index() can be used to add an index for an OVSDB
> table.
Yeah, I just discovered that the I-P framework allows for this model.
>
>> + northd_init(data->data);
>> +
>> + return data;
>> }
>>
>> void en_northd_cleanup(void *data OVS_UNUSED)
>> {
>> + struct northd_data *northd_data = ((struct ed_type_northd
> *)data)->data;
>> +
>> + northd_destroy(northd_data);
>> + free(northd_data);
>> }
>> diff --git a/northd/en-northd.h b/northd/en-northd.h
>> index 0e7f76245e69..da386298d821 100644
>> --- a/northd/en-northd.h
>> +++ b/northd/en-northd.h
>> @@ -9,6 +9,10 @@
>>
>> #include "lib/inc-proc-eng.h"
>>
>> +struct ed_type_northd {
>> + struct northd_data *data;
>> +};
>
> northd_data is for the engine node en-northd only, so why not define it
> directly in the struct ed_type_northd? It seems unnecessary to have another
> struct that just adds one more layer when accessing the fields.
> (besides, with the current nested structure, the outer one is not freed in
> the en_northd_cleanup())
Makes sense.
>
>> +
>> void en_northd_run(struct engine_node *node OVS_UNUSED, void *data
> OVS_UNUSED);
>> void *en_northd_init(struct engine_node *node OVS_UNUSED,
>> struct engine_arg *arg);
>> diff --git a/northd/northd.c b/northd/northd.c
>> index d61368c1e406..6426fcbe1215 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -2950,6 +2950,7 @@ chassis_group_list_changed(
>>
>> static void
>> sync_ha_chassis_group_for_sbpb(struct northd_idl_context *ctx,
>> + struct northd_data *data,
>> const struct nbrec_ha_chassis_group
> *nb_ha_grp,
>> struct ovsdb_idl_index
> *sbrec_chassis_by_name,
>> const struct sbrec_port_binding *pb)
>> @@ -2957,7 +2958,7 @@ sync_ha_chassis_group_for_sbpb(struct
> northd_idl_context *ctx,
>> bool new_sb_chassis_group = false;
>> const struct sbrec_ha_chassis_group *sb_ha_grp =
>> ha_chassis_group_lookup_by_name(
>> - ctx->sbrec_ha_chassis_grp_by_name, nb_ha_grp->name);
>> + data->sbrec_ha_chassis_grp_by_name, nb_ha_grp->name);
>>
>> if (!sb_ha_grp) {
>> sb_ha_grp = sbrec_ha_chassis_group_insert(ctx->ovnsb_txn);
>> @@ -3003,6 +3004,7 @@ sync_ha_chassis_group_for_sbpb(struct
> northd_idl_context *ctx,
>> static void
>> copy_gw_chassis_from_nbrp_to_sbpb(
>> struct northd_idl_context *ctx,
>> + struct northd_data *data,
>> struct ovsdb_idl_index *sbrec_chassis_by_name,
>> const struct nbrec_logical_router_port *lrp,
>> const struct sbrec_port_binding *port_binding)
>> @@ -3012,7 +3014,7 @@ copy_gw_chassis_from_nbrp_to_sbpb(
>> * for the distributed gateway router port. */
>> const struct sbrec_ha_chassis_group *sb_ha_chassis_group =
>> ha_chassis_group_lookup_by_name(
>> - ctx->sbrec_ha_chassis_grp_by_name, lrp->name);
>> + data->sbrec_ha_chassis_grp_by_name, lrp->name);
>> if (!sb_ha_chassis_group) {
>> sb_ha_chassis_group =
> sbrec_ha_chassis_group_insert(ctx->ovnsb_txn);
>> sbrec_ha_chassis_group_set_name(sb_ha_chassis_group, lrp->name);
>> @@ -3081,6 +3083,7 @@ ovn_update_ipv6_prefix(struct hmap *ports)
>>
>> static void
>> ovn_port_update_sbrec(struct northd_idl_context *ctx,
>> + struct northd_data *data,
>> struct ovsdb_idl_index *sbrec_chassis_by_name,
>> const struct ovn_port *op,
>> struct hmap *chassis_qdisc_queues,
>> @@ -3115,7 +3118,8 @@ ovn_port_update_sbrec(struct northd_idl_context
> *ctx,
>> }
>>
>> /* HA Chassis group is set. Ignore 'gateway_chassis'. */
>> - sync_ha_chassis_group_for_sbpb(ctx,
> op->nbrp->ha_chassis_group,
>> + sync_ha_chassis_group_for_sbpb(ctx, data,
>> +
> op->nbrp->ha_chassis_group,
>> sbrec_chassis_by_name,
> op->sb);
>> sset_add(active_ha_chassis_grps,
>> op->nbrp->ha_chassis_group->name);
>> @@ -3125,7 +3129,7 @@ ovn_port_update_sbrec(struct northd_idl_context
> *ctx,
>> * associated with the lrp. */
>> if (sbpb_gw_chassis_needs_update(op->sb, op->nbrp,
>> sbrec_chassis_by_name))
> {
>> - copy_gw_chassis_from_nbrp_to_sbpb(ctx,
>> + copy_gw_chassis_from_nbrp_to_sbpb(ctx, data,
>>
> sbrec_chassis_by_name,
>> op->nbrp, op->sb);
>> }
>> @@ -3249,7 +3253,7 @@ ovn_port_update_sbrec(struct northd_idl_context
> *ctx,
>> if (!strcmp(op->nbsp->type, "external")) {
>> if (op->nbsp->ha_chassis_group) {
>> sync_ha_chassis_group_for_sbpb(
>> - ctx, op->nbsp->ha_chassis_group,
>> + ctx, data, op->nbsp->ha_chassis_group,
>> sbrec_chassis_by_name, op->sb);
>> sset_add(active_ha_chassis_grps,
>> op->nbsp->ha_chassis_group->name);
>> @@ -3943,6 +3947,7 @@ ovn_port_allocate_key(struct northd_idl_context
> *ctx, struct hmap *ports,
>> * datapaths. */
>> static void
>> build_ports(struct northd_idl_context *ctx,
>> + struct northd_data *data,
>> struct ovsdb_idl_index *sbrec_chassis_by_name,
>> struct hmap *datapaths, struct hmap *ports)
>> {
>> @@ -3998,7 +4003,7 @@ build_ports(struct northd_idl_context *ctx,
>> if (op->nbsp) {
>> tag_alloc_create_new_tag(&tag_alloc_table, op->nbsp);
>> }
>> - ovn_port_update_sbrec(ctx, sbrec_chassis_by_name,
>> + ovn_port_update_sbrec(ctx, data, sbrec_chassis_by_name,
>> op, &chassis_qdisc_queues,
>> &active_ha_chassis_grps);
>> }
>> @@ -4006,7 +4011,7 @@ build_ports(struct northd_idl_context *ctx,
>> /* Add southbound record for each unmatched northbound record. */
>> LIST_FOR_EACH_SAFE (op, next, list, &nb_only) {
>> op->sb = sbrec_port_binding_insert(ctx->ovnsb_txn);
>> - ovn_port_update_sbrec(ctx, sbrec_chassis_by_name, op,
>> + ovn_port_update_sbrec(ctx, data, sbrec_chassis_by_name, op,
>> &chassis_qdisc_queues,
>> &active_ha_chassis_grps);
>> sbrec_port_binding_set_logical_port(op->sb, op->key);
>> @@ -4209,7 +4214,8 @@ ovn_igmp_group_find(struct hmap *igmp_groups,
>> }
>>
>> static struct ovn_igmp_group *
>> -ovn_igmp_group_add(struct northd_idl_context *ctx, struct hmap
> *igmp_groups,
>> +ovn_igmp_group_add(struct northd_data *data,
>> + struct hmap *igmp_groups,
>> struct ovn_datapath *datapath,
>> const struct in6_addr *address,
>> const char *address_s)
>> @@ -4221,7 +4227,7 @@ ovn_igmp_group_add(struct northd_idl_context *ctx,
> struct hmap *igmp_groups,
>> igmp_group = xmalloc(sizeof *igmp_group);
>>
>> const struct sbrec_multicast_group *mcgroup =
>> - mcast_group_lookup(ctx->sbrec_mcast_group_by_name_dp,
> address_s,
>> + mcast_group_lookup(data->sbrec_mcast_group_by_name_dp,
> address_s,
>> datapath->sb);
>>
>> igmp_group->datapath = datapath;
>> @@ -13323,12 +13329,7 @@ static bool reset_parallel = false;
>> /* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB
> database,
>> * constructing their contents based on the OVN_NB database. */
>> static void
>> -build_lflows(struct northd_idl_context *ctx, struct hmap *datapaths,
>> - struct hmap *ports, struct hmap *port_groups,
>> - struct hmap *mcgroups, struct hmap *igmp_groups,
>> - struct shash *meter_groups,
>> - struct hmap *lbs, struct hmap *bfd_connections,
>> - bool ovn_internal_version_changed)
>> +build_lflows(struct northd_idl_context *ctx, struct northd_data *data)
>> {
>> struct hmap lflows;
>>
>> @@ -13350,10 +13351,11 @@ build_lflows(struct northd_idl_context *ctx,
> struct hmap *datapaths,
>> use_parallel_build = false;
>> reset_parallel = true;
>> }
>> - build_lswitch_and_lrouter_flows(datapaths, ports,
>> - port_groups, &lflows, mcgroups,
>> - igmp_groups, meter_groups, lbs,
>> - bfd_connections);
>> + build_lswitch_and_lrouter_flows(&data->datapaths, &data->ports,
>> + &data->port_groups, &lflows,
>> + &data->mcast_groups,
> &data->igmp_groups,
>> + &data->meter_groups, &data->lbs,
>> + &data->bfd_connections);
>>
>> /* Parallel build may result in a suboptimal hash. Resize the
>> * hash to a correct size before doing lookups */
>> @@ -13422,7 +13424,8 @@ build_lflows(struct northd_idl_context *ctx,
> struct hmap *datapaths,
>> /* Find one valid datapath to get the datapath type. */
>> struct sbrec_datapath_binding *dp = sbflow->logical_datapath;
>> if (dp) {
>> - logical_datapath_od = ovn_datapath_from_sbrec(datapaths, dp);
>> + logical_datapath_od = ovn_datapath_from_sbrec(
>> + &data->datapaths, dp);
>> if (logical_datapath_od
>> && ovn_datapath_is_stale(logical_datapath_od)) {
>> logical_datapath_od = NULL;
>> @@ -13430,7 +13433,7 @@ build_lflows(struct northd_idl_context *ctx,
> struct hmap *datapaths,
>> }
>> for (i = 0; dp_group && i < dp_group->n_datapaths; i++) {
>> logical_datapath_od = ovn_datapath_from_sbrec(
>> - datapaths, dp_group->datapaths[i]);
>> + &data->datapaths,
> dp_group->datapaths[i]);
>> if (logical_datapath_od
>> && !ovn_datapath_is_stale(logical_datapath_od)) {
>> break;
>> @@ -13454,7 +13457,7 @@ build_lflows(struct northd_idl_context *ctx,
> struct hmap *datapaths,
>> sbflow->priority, sbflow->match, sbflow->actions,
>> sbflow->controller_meter, sbflow->hash);
>> if (lflow) {
>> - if (ovn_internal_version_changed) {
>> + if (data->ovn_internal_version_changed) {
>> const char *stage_name =
> smap_get_def(&sbflow->external_ids,
>> "stage-name", "");
>> const char *stage_hint =
> smap_get_def(&sbflow->external_ids,
>> @@ -13506,7 +13509,7 @@ build_lflows(struct northd_idl_context *ctx,
> struct hmap *datapaths,
>> /* Check all logical datapaths from the group. */
>> for (i = 0; i < dp_group->n_datapaths; i++) {
>> od[n_datapaths] = ovn_datapath_from_sbrec(
>> - datapaths,
> dp_group->datapaths[i]);
>> + &data->datapaths,
> dp_group->datapaths[i]);
>> if (!od[n_datapaths]
>> || ovn_datapath_is_stale(od[n_datapaths])) {
>> continue;
>> @@ -13602,7 +13605,7 @@ build_lflows(struct northd_idl_context *ctx,
> struct hmap *datapaths,
>> /* Push changes to the Multicast_Group table to database. */
>> const struct sbrec_multicast_group *sbmc, *next_sbmc;
>> SBREC_MULTICAST_GROUP_FOR_EACH_SAFE (sbmc, next_sbmc,
> ctx->ovnsb_idl) {
>
> We shouldn't use ovnsb_idl directly to get table access, because it is not
> from input dependencies. In the engine functions we should only rely on
> data retrieved from input nodes to ensure I-P correctness.
> The macro SBREC_MULTICAST_GROUP_TABLE_FOR_EACH is for this purpose.
> The variable of struct sbrec_multicast_group_table can be retrieved from
> the engine node's input using EN_OVSDB_GET(<input OVSDB node>).
> This is just an example, and all the NB/SB table access should be in this
> way for I-P.
Yes
>
>> - struct ovn_datapath *od = ovn_datapath_from_sbrec(datapaths,
>> + struct ovn_datapath *od =
> ovn_datapath_from_sbrec(&data->datapaths,
>>
> sbmc->datapath);
>>
>> if (!od || ovn_datapath_is_stale(od)) {
>> @@ -13612,18 +13615,19 @@ build_lflows(struct northd_idl_context *ctx,
> struct hmap *datapaths,
>>
>> struct multicast_group group = { .name = sbmc->name,
>> .key = sbmc->tunnel_key };
>> - struct ovn_multicast *mc = ovn_multicast_find(mcgroups, od,
> &group);
>> + struct ovn_multicast *mc =
> ovn_multicast_find(&data->mcast_groups,
>> + od, &group);
>> if (mc) {
>> ovn_multicast_update_sbrec(mc, sbmc);
>> - ovn_multicast_destroy(mcgroups, mc);
>> + ovn_multicast_destroy(&data->mcast_groups, mc);
>> } else {
>> sbrec_multicast_group_delete(sbmc);
>> }
>> }
>> struct ovn_multicast *mc, *next_mc;
>> - HMAP_FOR_EACH_SAFE (mc, next_mc, hmap_node, mcgroups) {
>> + HMAP_FOR_EACH_SAFE (mc, next_mc, hmap_node, &data->mcast_groups) {
>> if (!mc->datapath) {
>> - ovn_multicast_destroy(mcgroups, mc);
>> + ovn_multicast_destroy(&data->mcast_groups, mc);
>> continue;
>> }
>> sbmc = sbrec_multicast_group_insert(ctx->ovnsb_txn);
>> @@ -13631,7 +13635,7 @@ build_lflows(struct northd_idl_context *ctx,
> struct hmap *datapaths,
>> sbrec_multicast_group_set_name(sbmc, mc->group->name);
>> sbrec_multicast_group_set_tunnel_key(sbmc, mc->group->key);
>> ovn_multicast_update_sbrec(mc, sbmc);
>> - ovn_multicast_destroy(mcgroups, mc);
>> + ovn_multicast_destroy(&data->mcast_groups, mc);
>> }
>> }
>>
>> @@ -14141,7 +14145,8 @@ destroy_datapaths_and_ports(struct hmap
> *datapaths, struct hmap *ports,
>> }
>>
>> static void
>> -build_ip_mcast(struct northd_idl_context *ctx, struct hmap *datapaths)
>> +build_ip_mcast(struct northd_idl_context *ctx, struct northd_data *data,
>> + struct hmap *datapaths)
>> {
>> struct ovn_datapath *od;
>>
>> @@ -14151,7 +14156,7 @@ build_ip_mcast(struct northd_idl_context *ctx,
> struct hmap *datapaths)
>> }
>>
>> const struct sbrec_ip_multicast *ip_mcast =
>> - ip_mcast_lookup(ctx->sbrec_ip_mcast_by_dp, od->sb);
>> + ip_mcast_lookup(data->sbrec_ip_mcast_by_dp, od->sb);
>>
>> if (!ip_mcast) {
>> ip_mcast = sbrec_ip_multicast_insert(ctx->ovnsb_txn);
>> @@ -14172,6 +14177,7 @@ build_ip_mcast(struct northd_idl_context *ctx,
> struct hmap *datapaths)
>>
>> static void
>> build_mcast_groups(struct northd_idl_context *ctx,
>> + struct northd_data *data,
>> struct hmap *datapaths, struct hmap *ports,
>> struct hmap *mcast_groups,
>> struct hmap *igmp_groups)
>> @@ -14267,7 +14273,7 @@ build_mcast_groups(struct northd_idl_context *ctx,
>> * if the multicast group already exists.
>> */
>> struct ovn_igmp_group *igmp_group =
>> - ovn_igmp_group_add(ctx, igmp_groups, od, &group_address,
>> + ovn_igmp_group_add(data, igmp_groups, od, &group_address,
>> sb_igmp->address);
>>
>> /* Add the extracted ports to the IGMP group. */
>> @@ -14311,7 +14317,7 @@ build_mcast_groups(struct northd_idl_context *ctx,
>> }
>>
>> struct ovn_igmp_group *igmp_group_rtr =
>> - ovn_igmp_group_add(ctx, igmp_groups, router_port->od,
>> + ovn_igmp_group_add(data, igmp_groups,
> router_port->od,
>> address, igmp_group->mcgroup.name
> );
>> struct ovn_port **router_igmp_ports =
>> xmalloc(sizeof *router_igmp_ports);
>> @@ -14355,26 +14361,89 @@ build_meter_groups(struct northd_idl_context
> *ctx,
>> }
>> }
>>
>> +void
>> +northd_indices_create(struct northd_data *data,
>> + struct ovsdb_idl *ovnsb_idl)
>> +{
>> + data->sbrec_chassis_by_name = chassis_index_create(ovnsb_idl);
>> + data->sbrec_ha_chassis_grp_by_name
>> + = ha_chassis_group_index_create(ovnsb_idl);
>> + data->sbrec_mcast_group_by_name_dp
>> + = mcast_group_index_create(ovnsb_idl);
>> + data->sbrec_ip_mcast_by_dp = ip_mcast_index_create(ovnsb_idl);
>> +}
>> +
>> +void
>> +northd_init(struct northd_data *data)
>> +{
>> + hmap_init(&data->datapaths);
>> + hmap_init(&data->ports);
>> + hmap_init(&data->port_groups);
>> + hmap_init(&data->mcast_groups);
>> + hmap_init(&data->igmp_groups);
>> + shash_init(&data->meter_groups);
>> + hmap_init(&data->lbs);
>> + hmap_init(&data->bfd_connections);
>> + ovs_list_init(&data->lr_list);
>> + data->ovn_internal_version_changed = false;
>> +}
>> +
>> +void
>> +northd_destroy(struct northd_data *data)
>> +{
>> + struct ovn_northd_lb *lb;
>> + HMAP_FOR_EACH_POP (lb, hmap_node, &data->lbs) {
>> + ovn_northd_lb_destroy(lb);
>> + }
>> + hmap_destroy(&data->lbs);
>> +
>> + struct ovn_igmp_group *igmp_group, *next_igmp_group;
>> +
>> + HMAP_FOR_EACH_SAFE (igmp_group, next_igmp_group, hmap_node,
>> + &data->igmp_groups) {
>> + ovn_igmp_group_destroy(&data->igmp_groups, igmp_group);
>> + }
>> +
>> + struct ovn_port_group *pg, *next_pg;
>> + HMAP_FOR_EACH_SAFE (pg, next_pg, key_node, &data->port_groups) {
>> + ovn_port_group_destroy(&data->port_groups, pg);
>> + }
>> +
>> + hmap_destroy(&data->igmp_groups);
>> + hmap_destroy(&data->mcast_groups);
>> + hmap_destroy(&data->port_groups);
>> + hmap_destroy(&data->bfd_connections);
>> +
>> + struct shash_node *node, *next;
>> + SHASH_FOR_EACH_SAFE (node, next, &data->meter_groups) {
>> + shash_delete(&data->meter_groups, node);
>> + }
>> + shash_destroy(&data->meter_groups);
>> +
>> + /* XXX Having to explicitly clean up macam here
>> + * is a bit strange. We don't explicitly initialize
>> + * macam in this module, but this is the logical place
>> + * to clean it up. Ideally, more IPAM logic can be factored
>> + * out of ovn-northd and this can be taken care of there
>> + * as well.
>> + */
>> + cleanup_macam();
>> +
>> + destroy_datapaths_and_ports(&data->datapaths, &data->ports,
>> + &data->lr_list);
>> +}
>> +
>> static void
>> -ovnnb_db_run(struct northd_idl_context *ctx,
>> +ovnnb_db_run(struct northd_data *data,
>> + struct northd_idl_context *ctx,
>> struct ovsdb_idl_index *sbrec_chassis_by_name,
>> struct ovsdb_idl_loop *sb_loop,
>> - struct hmap *datapaths, struct hmap *ports,
>> - struct ovs_list *lr_list,
>> - int64_t loop_start_time,
>> - const char *ovn_internal_version)
>> + int64_t loop_start_time)
>> {
>> if (!ctx->ovnsb_txn || !ctx->ovnnb_txn) {
>> return;
>> }
>> stopwatch_start(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
>> - struct hmap port_groups;
>> - struct hmap mcast_groups;
>> - struct hmap igmp_groups;
>> - struct shash meter_groups = SHASH_INITIALIZER(&meter_groups);
>> - struct hmap lbs;
>> - struct hmap bfd_connections = HMAP_INITIALIZER(&bfd_connections);
>> - bool ovn_internal_version_changed = true;
>>
>> /* Sync ipsec configuration.
>> * Copy nb_cfg from northbound to southbound database.
>> @@ -14426,13 +14495,15 @@ ovnnb_db_run(struct northd_idl_context *ctx,
>> smap_replace(&options, "max_tunid", max_tunid);
>> free(max_tunid);
>>
>> + char *ovn_internal_version = ovn_get_internal_version();
>> if (!strcmp(ovn_internal_version,
>> smap_get_def(&options, "northd_internal_version", ""))) {
>> - ovn_internal_version_changed = false;
>> + data->ovn_internal_version_changed = false;
>> } else {
>> smap_replace(&options, "northd_internal_version",
>> ovn_internal_version);
>> }
>> + free(ovn_internal_version);
>>
>> if (!smap_equal(&nb->options, &options)) {
>> nbrec_nb_global_verify_options(nb);
>> @@ -14456,72 +14527,34 @@ ovnnb_db_run(struct northd_idl_context *ctx,
>> check_lsp_is_up = !smap_get_bool(&nb->options,
>> "ignore_lsp_down", true);
>>
>> - build_datapaths(ctx, datapaths, lr_list);
>> - build_ovn_lbs(ctx, datapaths, &lbs);
>> - build_lrouter_lbs(datapaths, &lbs);
>> - build_ports(ctx, sbrec_chassis_by_name, datapaths, ports);
>> - build_ovn_lr_lbs(datapaths, &lbs);
>> - build_ovn_lb_svcs(ctx, ports, &lbs);
>> - build_ipam(datapaths, ports);
>> - build_port_group_lswitches(ctx, &port_groups, ports);
>> - build_lrouter_groups(ports, lr_list);
>> - build_ip_mcast(ctx, datapaths);
>> - build_mcast_groups(ctx, datapaths, ports, &mcast_groups,
> &igmp_groups);
>> - build_meter_groups(ctx, &meter_groups);
>> - build_bfd_table(ctx, &bfd_connections, ports);
>> + build_datapaths(ctx, &data->datapaths, &data->lr_list);
>> + build_ovn_lbs(ctx, &data->datapaths, &data->lbs);
>> + build_lrouter_lbs(&data->datapaths, &data->lbs);
>> + build_ports(ctx, data, sbrec_chassis_by_name,
>> + &data->datapaths, &data->ports);
>> + build_ovn_lr_lbs(&data->datapaths, &data->lbs);
>> + build_ovn_lb_svcs(ctx, &data->ports, &data->lbs);
>> + build_ipam(&data->datapaths, &data->ports);
>> + build_port_group_lswitches(ctx, &data->port_groups, &data->ports);
>> + build_lrouter_groups(&data->ports, &data->lr_list);
>> + build_ip_mcast(ctx, data, &data->datapaths);
>> + build_mcast_groups(ctx, data, &data->datapaths, &data->ports,
>> + &data->mcast_groups, &data->igmp_groups);
>> + build_meter_groups(ctx, &data->meter_groups);
>> + build_bfd_table(ctx, &data->bfd_connections, &data->ports);
>> stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
>> stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
>> - build_lflows(ctx, datapaths, ports, &port_groups, &mcast_groups,
>> - &igmp_groups, &meter_groups, &lbs, &bfd_connections,
>> - ovn_internal_version_changed);
>> + build_lflows(ctx, data);
>> stopwatch_stop(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
>> stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
>> - ovn_update_ipv6_prefix(ports);
>> -
>> - sync_address_sets(ctx, datapaths);
>> - sync_port_groups(ctx, &port_groups);
>> - sync_meters(ctx, &meter_groups);
>> - sync_dns_entries(ctx, datapaths);
>> - cleanup_stale_fdp_entries(ctx, datapaths);
>> -
>> - struct ovn_northd_lb *lb;
>> - HMAP_FOR_EACH_POP (lb, hmap_node, &lbs) {
>> - ovn_northd_lb_destroy(lb);
>> - }
>> - hmap_destroy(&lbs);
>> -
>> - struct ovn_igmp_group *igmp_group, *next_igmp_group;
>> -
>> - HMAP_FOR_EACH_SAFE (igmp_group, next_igmp_group, hmap_node,
> &igmp_groups) {
>> - ovn_igmp_group_destroy(&igmp_groups, igmp_group);
>> - }
>> -
>> - struct ovn_port_group *pg, *next_pg;
>> - HMAP_FOR_EACH_SAFE (pg, next_pg, key_node, &port_groups) {
>> - ovn_port_group_destroy(&port_groups, pg);
>> - }
>> -
>> - bfd_cleanup_connections(ctx, &bfd_connections);
>> -
>> - hmap_destroy(&igmp_groups);
>> - hmap_destroy(&mcast_groups);
>> - hmap_destroy(&port_groups);
>> - hmap_destroy(&bfd_connections);
>> -
>> - struct shash_node *node, *next;
>> - SHASH_FOR_EACH_SAFE (node, next, &meter_groups) {
>> - shash_delete(&meter_groups, node);
>> - }
>> - shash_destroy(&meter_groups);
>> -
>> - /* XXX Having to explicitly clean up macam here
>> - * is a bit strange. We don't explicitly initialize
>> - * macam in this module, but this is the logical place
>> - * to clean it up. Ideally, more IPAM logic can be factored
>> - * out of ovn-northd and this can be taken care of there
>> - * as well.
>> - */
>> - cleanup_macam();
>> + ovn_update_ipv6_prefix(&data->ports);
>> +
>> + sync_address_sets(ctx, &data->datapaths);
>> + sync_port_groups(ctx, &data->port_groups);
>> + sync_meters(ctx, &data->meter_groups);
>> + sync_dns_entries(ctx, &data->datapaths);
>> + cleanup_stale_fdp_entries(ctx, &data->datapaths);
>> + bfd_cleanup_connections(ctx, &data->bfd_connections);
>> stopwatch_stop(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
>>
>> }
>> @@ -14638,7 +14671,7 @@ update_sb_ha_group_ref_chassis(struct
> northd_idl_context *ctx,
>> * - 'ref_chassis' of hagrp1.
>> */
>> static void
>> -build_ha_chassis_group_ref_chassis(struct northd_idl_context *ctx,
>> +build_ha_chassis_group_ref_chassis(struct northd_data *data,
>> const struct sbrec_port_binding *sb,
>> struct ovn_port *op,
>> struct shash *ha_ref_chassis_map)
>> @@ -14664,7 +14697,7 @@ build_ha_chassis_group_ref_chassis(struct
> northd_idl_context *ctx,
>> SSET_FOR_EACH (ha_group_name, &lr_group->ha_chassis_groups) {
>> const struct sbrec_ha_chassis_group *sb_ha_chassis_grp;
>> sb_ha_chassis_grp = ha_chassis_group_lookup_by_name(
>> - ctx->sbrec_ha_chassis_grp_by_name, ha_group_name);
>> + data->sbrec_ha_chassis_grp_by_name, ha_group_name);
>>
>> if (sb_ha_chassis_grp) {
>> struct ha_ref_chassis_info *ref_ch_info =
>> @@ -14679,7 +14712,8 @@ build_ha_chassis_group_ref_chassis(struct
> northd_idl_context *ctx,
>> * this column is not empty, it means we need to set the corresponding
> logical
>> * port as 'up' in the northbound DB. */
>> static void
>> -handle_port_binding_changes(struct northd_idl_context *ctx, struct hmap
> *ports,
>> +handle_port_binding_changes(struct northd_idl_context *ctx,
>> + struct northd_data *data, struct hmap *ports,
>> struct shash *ha_ref_chassis_map)
>> {
>> const struct sbrec_port_binding *sb;
>> @@ -14725,7 +14759,7 @@ handle_port_binding_changes(struct
> northd_idl_context *ctx, struct hmap *ports,
>> if (build_ha_chassis_ref && ctx->ovnsb_txn && sb->chassis) {
>> /* Check and add the chassis which has claimed this 'sb'
>> * to the ha chassis group's ref_chassis if required. */
>> - build_ha_chassis_group_ref_chassis(ctx, sb, op,
>> + build_ha_chassis_group_ref_chassis(data, sb, op,
>> ha_ref_chassis_map);
>> }
>> }
>> @@ -14786,6 +14820,7 @@ update_northbound_cfg(struct northd_idl_context
> *ctx,
>> /* Handle a fairly small set of changes in the southbound database. */
>> static void
>> ovnsb_db_run(struct northd_idl_context *ctx,
>> + struct northd_data *data,
>> struct ovsdb_idl_loop *sb_loop,
>> struct hmap *ports,
>> int64_t loop_start_time)
>> @@ -14795,7 +14830,7 @@ ovnsb_db_run(struct northd_idl_context *ctx,
>> }
>>
>> struct shash ha_ref_chassis_map =
> SHASH_INITIALIZER(&ha_ref_chassis_map);
>> - handle_port_binding_changes(ctx, ports, &ha_ref_chassis_map);
>> + handle_port_binding_changes(ctx, data, ports, &ha_ref_chassis_map);
>> update_northbound_cfg(ctx, sb_loop, loop_start_time);
>> if (ctx->ovnsb_txn) {
>> update_sb_ha_group_ref_chassis(ctx, &ha_ref_chassis_map);
>> @@ -14804,25 +14839,18 @@ ovnsb_db_run(struct northd_idl_context *ctx,
>> }
>>
>> void
>> -ovn_db_run(struct northd_idl_context *ctx)
>> +northd_run(struct northd_idl_context *ctx, struct northd_data *data)
>> {
>> - struct hmap datapaths, ports;
>> - struct ovs_list lr_list;
>> - ovs_list_init(&lr_list);
>> - hmap_init(&datapaths);
>> - hmap_init(&ports);
>> - use_parallel_build = ctx->use_parallel_build;
>> + use_parallel_build = data->use_parallel_build;
>
> This doesn't seem to really matter, because in ovn_nb_run() it is
> overridden by the NB option use_parallel_build. So it should be removed
> from the data. It is an input of this engine node.
> Meanwhile, there is a pending patch that moves the input from NB options to
> unix commands. I am not sure about the motivation:
> https://patchwork.ozlabs.org/project/ovn/patch/20210921154827.25940-2-anton.iva...@cambridgegreys.com/
>
Ack, I will remove it.
> Thanks,
> Han
>
>>
>> int64_t start_time = time_wall_msec();
>>
>> stopwatch_start(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
>> - ovnnb_db_run(ctx, ctx->sbrec_chassis_by_name, ctx->ovnsb_idl_loop,
>> - &datapaths, &ports, &lr_list, start_time,
>> - ctx->ovn_internal_version);
>> + ovnnb_db_run(data, ctx, data->sbrec_chassis_by_name,
> ctx->ovnsb_idl_loop,
>> + start_time);
>> stopwatch_stop(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
>> stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>> - ovnsb_db_run(ctx, ctx->ovnsb_idl_loop, &ports, start_time);
>> + ovnsb_db_run(ctx, data, ctx->ovnsb_idl_loop, &data->ports,
> start_time);
>> stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>> - destroy_datapaths_and_ports(&datapaths, &ports, &lr_list);
>> }
>>
>> diff --git a/northd/northd.h b/northd/northd.h
>> index ea7f841c7a49..d4bc5cf16541 100644
>> --- a/northd/northd.h
>> +++ b/northd/northd.h
>> @@ -16,24 +16,40 @@
>>
>> #include "ovsdb-idl.h"
>>
>> +#include "openvswitch/hmap.h"
>> +
>> struct northd_idl_context {
>> - const char *ovnnb_db;
>> - const char *ovnsb_db;
>> struct ovsdb_idl *ovnnb_idl;
>> struct ovsdb_idl *ovnsb_idl;
>> struct ovsdb_idl_loop *ovnnb_idl_loop;
>> struct ovsdb_idl_loop *ovnsb_idl_loop;
>> struct ovsdb_idl_txn *ovnnb_txn;
>> struct ovsdb_idl_txn *ovnsb_txn;
>> +};
>> +
>> +struct northd_data {
>> + struct hmap datapaths;
>> + struct hmap ports;
>> + struct hmap port_groups;
>> + struct hmap mcast_groups;
>> + struct hmap igmp_groups;
>> + struct shash meter_groups;
>> + struct hmap lbs;
>> + struct hmap bfd_connections;
>> + struct ovs_list lr_list;
>> + bool ovn_internal_version_changed;
>> + bool use_parallel_build;
>> +
>> struct ovsdb_idl_index *sbrec_chassis_by_name;
>> struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name;
>> struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp;
>> struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
>> -
>> - const char *ovn_internal_version;
>> - bool use_parallel_build;
>> };
>>
>> -void ovn_db_run(struct northd_idl_context *ctx);
>> +void northd_run(struct northd_idl_context *ctx, struct northd_data
> *data);
>> +void northd_destroy(struct northd_data *data);
>> +void northd_init(struct northd_data *data);
>> +void northd_indices_create(struct northd_data *data,
>> + struct ovsdb_idl *ovnsb_idl);
>>
>> #endif /* NORTHD_H */
>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>> index 3a780cef6606..c7192d2ba065 100644
>> --- a/northd/ovn-northd.c
>> +++ b/northd/ovn-northd.c
>> @@ -31,7 +31,6 @@
>> #include "ovsdb-idl.h"
>> #include "lib/ovn-l7.h"
>> #include "lib/ovn-nb-idl.h"
>> -#include "lib/ovn-parallel-hmap.h"
>> #include "lib/ovn-sb-idl.h"
>> #include "openvswitch/poll-loop.h"
>> #include "simap.h"
>> @@ -70,7 +69,6 @@ static const char *ssl_ca_cert_file;
>> #define DEFAULT_PROBE_INTERVAL_MSEC 5000
>> static int northd_probe_interval_nb = 0;
>> static int northd_probe_interval_sb = 0;
>> -static bool use_parallel_build = true;
>>
>> static const char *rbac_chassis_auth[] =
>> {"name"};
>> @@ -641,8 +639,6 @@ main(int argc, char *argv[])
>>
>> daemonize_complete();
>>
>> - use_parallel_build = can_parallelize_hashes(false);
>> -
>> /* We want to detect (almost) all changes to the ovn-nb db. */
>> struct ovsdb_idl_loop ovnnb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
>> ovsdb_idl_create(ovnnb_db, &nbrec_idl_class, true, true));
>> @@ -905,23 +901,10 @@ main(int argc, char *argv[])
>> ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_fdb_col_dp_key);
>> ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_fdb_col_port_key);
>>
>> - struct ovsdb_idl_index *sbrec_chassis_by_name
>> - = chassis_index_create(ovnsb_idl_loop.idl);
>> -
>> - struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name
>> - = ha_chassis_group_index_create(ovnsb_idl_loop.idl);
>> -
>> - struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp
>> - = mcast_group_index_create(ovnsb_idl_loop.idl);
>> -
>> - struct ovsdb_idl_index *sbrec_ip_mcast_by_dp
>> - = ip_mcast_index_create(ovnsb_idl_loop.idl);
>> -
>> unixctl_command_register("sb-connection-status", "", 0, 0,
>> ovn_conn_show, ovnsb_idl_loop.idl);
>>
>> - char *ovn_internal_version = ovn_get_internal_version();
>> - VLOG_INFO("OVN internal version is : [%s]", ovn_internal_version);
>> + VLOG_INFO("OVN internal version is : [%s]",
> ovn_get_internal_version());
>>
>> stopwatch_create(NORTHD_LOOP_STOPWATCH_NAME, SW_MS);
>> stopwatch_create(OVNNB_DB_RUN_STOPWATCH_NAME, SW_MS);
>> @@ -994,20 +977,12 @@ main(int argc, char *argv[])
>> }
>>
>> struct northd_idl_context ctx = {
>> - .ovnnb_db = ovnnb_db,
>> - .ovnsb_db = ovnsb_db,
>> .ovnnb_idl = ovnnb_idl_loop.idl,
>> .ovnnb_idl_loop = &ovnnb_idl_loop,
>> .ovnnb_txn = ovnnb_txn,
>> .ovnsb_idl = ovnsb_idl_loop.idl,
>> .ovnsb_idl_loop = &ovnsb_idl_loop,
>> .ovnsb_txn = ovnsb_txn,
>> - .sbrec_chassis_by_name = sbrec_chassis_by_name,
>> - .sbrec_ha_chassis_grp_by_name =
> sbrec_ha_chassis_grp_by_name,
>> - .sbrec_mcast_group_by_name_dp =
> sbrec_mcast_group_by_name_dp,
>> - .sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp,
>> - .use_parallel_build = use_parallel_build,
>> - .ovn_internal_version = ovn_internal_version,
>> };
>>
>> if (!state.had_lock &&
> ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
>> @@ -1107,7 +1082,6 @@ main(int argc, char *argv[])
>> }
>> inc_proc_northd_cleanup();
>>
>> - free(ovn_internal_version);
>> unixctl_server_destroy(unixctl);
>> ovsdb_idl_loop_destroy(&ovnnb_idl_loop);
>> ovsdb_idl_loop_destroy(&ovnsb_idl_loop);
>> --
>> 2.27.0
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
