[ovs-dev] [PATCH ovn v5 3/6] northd: Introduce struct northd_data

Han Zhou hzhou at ovn.org
Wed Nov 17 06:41:59 UTC 2021


On Tue, Nov 9, 2021 at 11:36 AM Mark Gray <mrmarkgray at gmail.com> wrote:
>
> From: Mark Gray <mark.d.gray at redhat.com>
>
> 'struct northd_data' is used to hold the global state data for the
> incremental processing node 'en_northd'. This structure will also hold
> 'struct northd_input' which will hold references to output data from
> its input nodes. In particular, this will hold references to database
tables
> and indexes. 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.
> * 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 as it is read from DB as
>   suggested by Han.
> * Refactor northd.c to use 'struct northd_data' and 'struct northd_input'
>   where applicable.
>
> Signed-off-by: Mark Gray <mark.d.gray at redhat.com>
> ---
>  lib/inc-proc-eng.h       |   3 +
>  northd/en-northd.c       | 107 ++++++-
>  northd/inc-proc-northd.c |  42 ++-
>  northd/inc-proc-northd.h |   6 +-
>  northd/northd.c          | 632 ++++++++++++++++++++++-----------------
>  northd/northd.h          |  73 ++++-
>  northd/ovn-northd.c      |  87 ++----
>  7 files changed, 596 insertions(+), 354 deletions(-)
>
> diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
> index f89a40bd54ca..1823750c814c 100644
> --- a/lib/inc-proc-eng.h
> +++ b/lib/inc-proc-eng.h
> @@ -72,6 +72,9 @@ struct engine_context {
>      struct ovsdb_idl_txn *ovs_idl_txn;
>      struct ovsdb_idl_txn *ovnsb_idl_txn;
>      struct ovsdb_idl_txn *ovnnb_idl_txn;
> +
> +    struct ovsdb_idl_loop *ovnsb_idl_loop;
> +
>      void *client_ctx;
>  };
>
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index d310fa4dd31f..f39dbd9da3e0 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -20,26 +20,123 @@
>
>  #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_context *ctx = eng_ctx->client_ctx;
> -    ovn_db_run(ctx);
>
> +    struct northd_input input_data;
> +
> +    northd_destroy(data);
> +    northd_init(data);
> +
> +    input_data.sbrec_chassis_by_name =
> +        engine_ovsdb_node_get_index(
> +            engine_get_input("SB_chassis", node),
> +            "sbrec_chassis_by_name");
> +    input_data.sbrec_chassis_by_hostname =
> +        engine_ovsdb_node_get_index(
> +            engine_get_input("SB_chassis", node),
> +            "sbrec_chassis_by_hostname");
> +    input_data.sbrec_ha_chassis_grp_by_name =
> +        engine_ovsdb_node_get_index(
> +            engine_get_input("SB_ha_chassis_group", node),
> +            "sbrec_ha_chassis_grp_by_name");
> +    input_data.sbrec_mcast_group_by_name_dp =
> +        engine_ovsdb_node_get_index(
> +            engine_get_input("SB_multicast_group", node),
> +            "sbrec_mcast_group_by_name");
> +    input_data.sbrec_ip_mcast_by_dp =
> +        engine_ovsdb_node_get_index(
> +            engine_get_input("SB_ip_multicast", node),
> +            "sbrec_ip_mcast_by_dp");
> +
> +    input_data.nbrec_nb_global_table =
> +        EN_OVSDB_GET(engine_get_input("NB_nb_global", node));
> +    input_data.nbrec_logical_switch =
> +        EN_OVSDB_GET(engine_get_input("NB_logical_switch", node));
> +    input_data.nbrec_logical_router =
> +        EN_OVSDB_GET(engine_get_input("NB_logical_router", node));
> +    input_data.nbrec_load_balancer_table =
> +        EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
> +    input_data.nbrec_port_group_table =
> +        EN_OVSDB_GET(engine_get_input("NB_port_group", node));
> +    input_data.nbrec_bfd_table =
> +        EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> +    input_data.nbrec_address_set_table =
> +        EN_OVSDB_GET(engine_get_input("NB_address_set", node));
> +    input_data.nbrec_meter_table =
> +        EN_OVSDB_GET(engine_get_input("NB_meter", node));
> +    input_data.nbrec_acl_table =
> +        EN_OVSDB_GET(engine_get_input("NB_acl", node));
> +
> +    input_data.sbrec_sb_global_table =
> +        EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
> +    input_data.sbrec_datapath_binding_table =
> +        EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node));
> +    input_data.sbrec_port_binding_table =
> +        EN_OVSDB_GET(engine_get_input("SB_port_binding", node));
> +    input_data.sbrec_mac_binding_table =
> +        EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
> +    input_data.sbrec_ha_chassis_group_table =
> +        EN_OVSDB_GET(engine_get_input("SB_ha_chassis_group", node));
> +    input_data.sbrec_chassis =
> +        EN_OVSDB_GET(engine_get_input("SB_chassis", node));
> +    input_data.sbrec_fdb_table =
> +        EN_OVSDB_GET(engine_get_input("SB_fdb", node));
> +    input_data.sbrec_load_balancer_table =
> +        EN_OVSDB_GET(engine_get_input("SB_load_balancer", node));
> +    input_data.sbrec_service_monitor_table =
> +        EN_OVSDB_GET(engine_get_input("SB_service_monitor", node));
> +    input_data.sbrec_bfd_table =
> +        EN_OVSDB_GET(engine_get_input("SB_bfd", node));
> +    input_data.sbrec_logical_flow_table =
> +        EN_OVSDB_GET(engine_get_input("SB_logical_flow", node));
> +    input_data.sbrec_multicast_group_table =
> +        EN_OVSDB_GET(engine_get_input("SB_multicast_group", node));
> +    input_data.sbrec_address_set_table =
> +        EN_OVSDB_GET(engine_get_input("SB_address_set", node));
> +    input_data.sbrec_port_group_table =
> +        EN_OVSDB_GET(engine_get_input("SB_port_group", node));
> +    input_data.sbrec_meter_table =
> +        EN_OVSDB_GET(engine_get_input("SB_meter", node));
> +    input_data.sbrec_dns_table =
> +        EN_OVSDB_GET(engine_get_input("SB_dns", node));
> +    input_data.sbrec_ip_multicast_table =
> +        EN_OVSDB_GET(engine_get_input("SB_ip_multicast", node));
> +    input_data.sbrec_igmp_group_table =
> +        EN_OVSDB_GET(engine_get_input("SB_igmp_group", node));
> +    input_data.sbrec_chassis_private_table =
> +        EN_OVSDB_GET(engine_get_input("SB_chassis_private", node));
> +
> +    northd_run(&input_data, data,
> +               eng_ctx->ovnnb_idl_txn,
> +               eng_ctx->ovnsb_idl_txn,
> +               eng_ctx->ovnsb_idl_loop);
>      engine_set_node_state(node, EN_UPDATED);
>
>  }
>  void *en_northd_init(struct engine_node *node OVS_UNUSED,
>                       struct engine_arg *arg OVS_UNUSED)
>  {
> -    return NULL;
> +    struct northd_data *data = xmalloc(sizeof *data);
> +
> +    northd_init(data);
> +
> +    return data;
>  }
>
> -void en_northd_cleanup(void *data OVS_UNUSED)
> +void en_northd_cleanup(void *data)
>  {
> +    northd_destroy(data);
> +    free(data);

This will cause double free. I saw that you removed it in the next patch,
but it shouldn't be here in the first place. I don't think it needs another
revision, and I can just remove this line before merging.

...

> +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();

Yes, it does look weird here since macam is a static member of the ipam
module but it is updated in the I-P engine. We should either avoid updating
it in I-P engine, or move the data as part of the I-P engine. But I think
it is ok to leave it as a follow-up patch whichever method is appropriate.

Thanks,
Han


More information about the dev mailing list