[ovs-dev,2/3] northd.c: Maintain links between ovn_port and lflow.

For incremental processing, it is important to maintain relationship
between the inputs and the logical flows generated. This patch creates
the links between ovn_port and logical flows. The same data structure
may be expanded to maintain links between logical flows and other types
of inputs.

This patch also refactors the temp_lflow_list operations to
collected_lflows with helper functions to start and end collecting. It
still uses global variables just to avoid updating all the lflow_add_...
related code all over the northd.c file.

Signed-off-by: Han Zhou <hzhou@ovn.org>
---
 northd/northd.c | 271 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 178 insertions(+), 93 deletions(-)

On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hzhou@ovn.org> wrote:
>
> For incremental processing, it is important to maintain relationship
> between the inputs and the logical flows generated. This patch creates
> the links between ovn_port and logical flows. The same data structure
> may be expanded to maintain links between logical flows and other types
> of inputs.
>
> This patch also refactors the temp_lflow_list operations to
> collected_lflows with helper functions to start and end collecting. It
> still uses global variables just to avoid updating all the lflow_add_...
> related code all over the northd.c file.
>
> Signed-off-by: Han Zhou <hzhou@ovn.org>

Hi Han,

Please see a few comments below.  I did review all the 3 patches in the series.
They LGTM overall.  I'd like to do some more testing before providing my Acks.


> ---
>  northd/northd.c | 271 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 178 insertions(+), 93 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 98f528f93cfc..aa0f853ce2db 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -1457,6 +1457,19 @@ struct ovn_port_routable_addresses {
>      size_t n_addrs;
>  };
>
> +/* A node that maintains link between an object (such as an ovn_port) and
> + * a lflow. */
> +struct lflow_ref_node {
> +    /* This list follows different lflows referenced by the same object. List
> +     * head is, for example, ovn_port->lflows.  */
> +    struct ovs_list lflow_list_node;
> +    /* This list follows different objects that reference the same lflow. List
> +     * head is ovn_lflow->referenced_by. */
> +    struct ovs_list ref_list_node;
> +    /* The lflow. */
> +    struct ovn_lflow *lflow;
> +};
> +
>  /* A logical switch port or logical router port.
>   *
>   * In steady state, an ovn_port points to a northbound Logical_Switch_Port
> @@ -1548,6 +1561,28 @@ struct ovn_port {
>
>      /* Temporarily used for traversing a list (or hmap) of ports. */
>      bool visited;
> +
> +    /* List of struct lflow_ref_node that points to the lflows generated by
> +     * this ovn_port.
> +     *
> +     * This data is initialized and destroyed by the en_northd node, but
> +     * populated and used only by the en_lflow node. Ideally this data should
> +     * be maintained as part of en_lflow's data (struct lflow_data): a hash
> +     * index from ovn_port key to lflows.  However, it would be less efficient
> +     * and more complex:
> +     *
> +     * 1. It would require an extra search (using the index) to find the
> +     * lflows.
> +     *
> +     * 2. Building the index needs to be thread-safe, using either a global
> +     * lock which is obviously less efficient, or hash-based lock array which
> +     * is more complex.
> +     *
> +     * Adding the list here is more straightforward. The drawback is that we
> +     * need to keep in mind that this data belongs to en_lflow node, so never
> +     * access it from any other nodes.
> +     */
> +    struct ovs_list lflows;
>  };


>
>  static bool lsp_can_be_inc_processed(const struct nbrec_logical_switch_port *);
> @@ -1635,6 +1670,8 @@ ovn_port_create(struct hmap *ports, const char *key,
>      ovn_port_set_nb(op, nbsp, nbrp);
>      op->l3dgw_port = op->cr_port = NULL;
>      hmap_insert(ports, &op->key_node, hash_string(op->key, 0));
> +
> +    ovs_list_init(&op->lflows);
>      return op;
>  }
>
> @@ -1665,6 +1702,13 @@ ovn_port_destroy_orphan(struct ovn_port *port)
>      destroy_lport_addresses(&port->proxy_arp_addrs);
>      free(port->json_key);
>      free(port->key);
> +
> +    struct lflow_ref_node *l;
> +    LIST_FOR_EACH_SAFE (l, lflow_list_node, &port->lflows) {
> +        ovs_list_remove(&l->lflow_list_node);
> +        ovs_list_remove(&l->ref_list_node);
> +        free(l);
> +    }
>      free(port);
>  }
>
> @@ -4893,6 +4937,7 @@ static struct ovn_port *
>  ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports,
>                 const char *key, const struct nbrec_logical_switch_port *nbsp,
>                 struct ovn_datapath *od, const struct sbrec_port_binding *sb,
> +               struct ovs_list *lflows,
>                 const struct sbrec_mirror_table *sbrec_mirror_table,
>                 const struct sbrec_chassis_table *sbrec_chassis_table,
>                 struct ovsdb_idl_index *sbrec_chassis_by_name,
> @@ -4903,6 +4948,9 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports,
>      parse_lsp_addrs(op);
>      op->od = od;
>      hmap_insert(&od->ports, &op->dp_node, hmap_node_hash(&op->key_node));
> +    if (lflows) {
> +        ovs_list_splice(&op->lflows, lflows->next, lflows);
> +    }
>
>      /* Assign explicitly requested tunnel ids first. */
>      if (!ovn_port_assign_requested_tnl_id(sbrec_chassis_table, op)) {
> @@ -5082,7 +5130,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
>                      goto fail;
>                  }
>                  op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> -                                    new_nbsp->name, new_nbsp, od, NULL,
> +                                    new_nbsp->name, new_nbsp, od, NULL, NULL,
>                                      ni->sbrec_mirror_table,
>                                      ni->sbrec_chassis_table,
>                                      ni->sbrec_chassis_by_name,
> @@ -5114,13 +5162,16 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
>                      op->visited = true;
>                      continue;
>                  }
> +                struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows);
> +                ovs_list_splice(&lflows, op->lflows.next, &op->lflows);
>                  ovn_port_destroy(&nd->ls_ports, op);
>                  op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> -                                    new_nbsp->name, new_nbsp, od, sb,
> +                                    new_nbsp->name, new_nbsp, od, sb, &lflows,
>                                      ni->sbrec_mirror_table,
>                                      ni->sbrec_chassis_table,
>                                      ni->sbrec_chassis_by_name,
>                                      ni->sbrec_chassis_by_hostname);
> +                ovs_assert(ovs_list_is_empty(&lflows));
>                  if (!op) {
>                      goto fail;
>                  }
> @@ -5577,7 +5628,8 @@ ovn_igmp_group_destroy(struct hmap *igmp_groups,
>
>  struct ovn_lflow {
>      struct hmap_node hmap_node;
> -    struct ovs_list list_node;
> +    struct ovs_list list_node;   /* For temporary list of lflows. Don't remove
> +                                    at destroy. */
>
>      struct ovn_datapath *od;     /* 'logical_datapath' in SB schema.  */
>      unsigned long *dpg_bitmap;   /* Bitmap of all datapaths by their 'index'.*/
> @@ -5591,6 +5643,8 @@ struct ovn_lflow {
>      size_t n_ods;                /* Number of datapaths referenced by 'od' and
>                                    * 'dpg_bitmap'. */
>      struct ovn_dp_group *dpg;    /* Link to unique Sb datapath group. */
> +
> +    struct ovs_list referenced_by;  /* List of struct lflow_ref_node. */
>      const char *where;
>
>      struct uuid sb_uuid;            /* SB DB row uuid, specified by northd. */
> @@ -5640,6 +5694,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,
>                 char *stage_hint, const char *where)
>  {
>      ovs_list_init(&lflow->list_node);
> +    ovs_list_init(&lflow->referenced_by);
>      lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len);
>      lflow->od = od;
>      lflow->stage = stage;
> @@ -5767,20 +5822,30 @@ ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref,
>      }
>  }
>
> +/* This global variable collects the lflows generated by do_ovn_lflow_add().
> + * start_collecting_lflows() will enable the lflow collection and the calls to
> + * do_ovn_lflow_add (or the macros ovn_lflow_add_...) will add generated lflows
> + * to the list. end_collecting_lflows() will disable it. */
> +static thread_local struct ovs_list collected_lflows;
> +static thread_local bool collecting_lflows = false;
> +
> +static void
> +start_collecting_lflows(void)
> +{
> +    ovs_assert(!collecting_lflows);
> +    ovs_list_init(&collected_lflows);
> +    collecting_lflows = true;
> +}
> +
> +static void
> +end_collecting_lflows(void)
> +{
> +    ovs_assert(collecting_lflows);
> +    collecting_lflows = false;
> +}
> +

I think we can avoid these functions and the thread local variable
"collected_lflows".

I'd suggest the below

----------------------------

static void
do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od,
                 const unsigned long *dp_bitmap, size_t dp_bitmap_len,
                 uint32_t hash, enum ovn_stage stage, uint16_t priority,
                 const char *match, const char *actions, const char *io_port,
                 struct ovs_list *lflow_ref_list,
                 const struct ovsdb_idl_row *stage_hint,
                 const char *where, const char *ctrl_meter)
    OVS_REQUIRES(fake_hash_mutex)
{
    ...
    ...
    /* At the end. */
    if (lflow_ref_list) {
        struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
        lfrn->lflow = lflow;
        ovs_list_insert(lflow_ref_list, &lfrn->lflow_list_node);
        ovs_list_insert(&lflow->referenced_by, &lfrn->ref_list_node);
    }
}


#define ovn_lflow_add_with_lport_and_hint(LFLOW_MAP, OD, STAGE, PRIORITY, \
                                          MATCH, ACTIONS, IN_OUT_PORT, \
                                          LFLOW_REF_LIST, STAGE_HINT) \
    ovn_lflow_add_at(LFLOW_MAP, OD, NULL, 0, STAGE, PRIORITY, MATCH, ACTIONS, \
                     IN_OUT_PORT, LFLOW_REF_LIST, NULL, STAGE_HINT, \
                     OVS_SOURCE_LOCATOR)

And pass &op->lflows in ovn_lflow_add_with_lport_and_hint()

-----------------------------

What do you think ?  Definitely this would result in a bit more work
as it would require a lot of (tedious) changes.
I think this is a better approach.

Also I'm planning to work on top of your patches to add I-P for load
balancers in lflow engine (or perhaps I-P for datapath changes)

My idea is to have a lflow ref list stored in "struct ovn_datapath"
similar to the way you have done in this patch in "struct ovn_port"

And while adding the flows using one of the macro variants
'ovn_lflow_add*' pass &od->lflows.

Please let me know your comments.

Only concern I have with this patch is the "op->lflows" modified by
the lflow engine node.
But I agree with your added comments and also thinking to use the same
approach for datapath I-P handling,
And I don't have a better approach at the moment. So I'm fine with it.

Thanks
Numan


>  /* Adds a row with the specified contents to the Logical_Flow table.
> - * Version to use when hash bucket locking is NOT required.
> - *
> - * Note: This function can add generated lflows to the global variable
> - * temp_lflow_list as its output, controlled by the global variable
> - * add_lflow_to_temp_list. The caller of the ovn_lflow_add_... marcros can get
> - * a list of lflows generated by setting add_lflow_to_temp_list to true. The
> - * caller is responsible for initializing the temp_lflow_list, and also
> - * reset the add_lflow_to_temp_list to false when it is no longer needed.
> - * XXX: this mechanism is temporary and will be replaced when we add hash index
> - * to lflow_data and refactor related functions.
> - */
> -static bool add_lflow_to_temp_list = false;
> -static struct ovs_list temp_lflow_list;
> + * Version to use when hash bucket locking is NOT required. */
>  static void
>  do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od,
>                   const unsigned long *dp_bitmap, size_t dp_bitmap_len,
> @@ -5797,7 +5862,7 @@ do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od,
>      size_t bitmap_len = od ? ods_size(od->datapaths) : dp_bitmap_len;
>      ovs_assert(bitmap_len);
>
> -    if (add_lflow_to_temp_list) {
> +    if (collecting_lflows) {
>          ovs_assert(od);
>          ovs_assert(!dp_bitmap);
>      } else {
> @@ -5829,8 +5894,8 @@ do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od,
>          thread_lflow_counter++;
>      }
>
> -    if (add_lflow_to_temp_list) {
> -        ovs_list_insert(&temp_lflow_list, &lflow->list_node);
> +    if (collecting_lflows) {
> +        ovs_list_insert(&collected_lflows, &lflow->list_node);
>      }
>  }
>
> @@ -5950,10 +6015,28 @@ ovn_lflow_destroy(struct hmap *lflows, struct ovn_lflow *lflow)
>          free(lflow->io_port);
>          free(lflow->stage_hint);
>          free(lflow->ctrl_meter);
> +        struct lflow_ref_node *l;
> +        LIST_FOR_EACH_SAFE (l, ref_list_node, &lflow->referenced_by) {
> +            ovs_list_remove(&l->lflow_list_node);
> +            ovs_list_remove(&l->ref_list_node);
> +            free(l);
> +        }
>          free(lflow);
>      }
>  }
>
> +static void
> +link_ovn_port_to_lflows(struct ovn_port *op, struct ovs_list *lflows)
> +{
> +    struct ovn_lflow *f;
> +    LIST_FOR_EACH (f, list_node, lflows) {
> +        struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
> +        lfrn->lflow = f;
> +        ovs_list_insert(&op->lflows, &lfrn->lflow_list_node);
> +        ovs_list_insert(&f->referenced_by, &lfrn->ref_list_node);
> +    }
> +}
> +
>  static bool
>  build_dhcpv4_action(struct ovn_port *op, ovs_be32 offer_ip,
>                      struct ds *options_action, struct ds *response_action,
> @@ -15483,6 +15566,7 @@ build_lswitch_and_lrouter_iterate_by_lsp(struct ovn_port *op,
>                                           struct hmap *lflows)
>  {
>      ovs_assert(op->nbsp);
> +    start_collecting_lflows();
>
>      /* Build Logical Switch Flows. */
>      build_lswitch_port_sec_op(op, lflows, actions, match);
> @@ -15497,6 +15581,9 @@ build_lswitch_and_lrouter_iterate_by_lsp(struct ovn_port *op,
>      /* Build Logical Router Flows. */
>      build_ip_routing_flows_for_router_type_lsp(op, lr_ports, lflows);
>      build_arp_resolve_flows_for_lsp(op, lflows, lr_ports, match, actions);
> +
> +    link_ovn_port_to_lflows(op, &collected_lflows);
> +    end_collecting_lflows();
>  }
>
>  /* Helper function to combine all lflow generation which is iterated by logical
> @@ -16223,14 +16310,10 @@ bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn,
>  {
>      struct ls_change *ls_change;
>      LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
> -        ovs_list_init(&temp_lflow_list);
> -        add_lflow_to_temp_list = true;
>          if (!ovs_list_is_empty(&ls_change->updated_ports) ||
>              !ovs_list_is_empty(&ls_change->deleted_ports)) {
>              /* XXX: implement lflow index so that we can handle updated and
>               * deleted LSPs incrementally. */
> -            ovs_list_init(&temp_lflow_list);
> -            add_lflow_to_temp_list = false;
>              return false;
>          }
>
> @@ -16277,83 +16360,85 @@ bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn,
>                  sbrec_multicast_group_update_ports_addvalue(sbmc_unknown,
>                                                              op->sb);
>              }
> -        }
> -        /* Sync the newly added flows to SB. */
> -        struct ovn_lflow *lflow;
> -        LIST_FOR_EACH (lflow, list_node, &temp_lflow_list) {
> -            size_t n_datapaths;
> -            struct ovn_datapath **datapaths_array;
> -            if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) {
> -                n_datapaths = ods_size(lflow_input->ls_datapaths);
> -                datapaths_array = lflow_input->ls_datapaths->array;
> -            } else {
> -                n_datapaths = ods_size(lflow_input->lr_datapaths);
> -                datapaths_array = lflow_input->lr_datapaths->array;
> -            }
> -            uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap, n_datapaths);
> -            ovs_assert(n_ods == 1);
> -            /* There is only one datapath, so it should be moved out of the
> -             * group to a single 'od'. */
> -            size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0,
> -                                       n_datapaths);
>
> -            bitmap_set0(lflow->dpg_bitmap, index);
> -            lflow->od = datapaths_array[index];
> -
> -            /* Logical flow should be re-hashed to allow lookups. */
> -            uint32_t hash = hmap_node_hash(&lflow->hmap_node);
> -            /* Remove from lflows. */
> -            hmap_remove(lflows, &lflow->hmap_node);
> -            hash = ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid,
> -                                                  hash);
> -            /* Add back. */
> -            hmap_insert(lflows, &lflow->hmap_node, hash);
> -
> -            /* Sync to SB. */
> -            const struct sbrec_logical_flow *sbflow;
> -            lflow->sb_uuid = uuid_random();
> -            sbflow = sbrec_logical_flow_insert_persist_uuid(ovnsb_txn,
> -                                                            &lflow->sb_uuid);
> -            const char *pipeline = ovn_stage_get_pipeline_name(lflow->stage);
> -            uint8_t table = ovn_stage_get_table(lflow->stage);
> -            sbrec_logical_flow_set_logical_datapath(sbflow, lflow->od->sb);
> -            sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
> -            sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> -            sbrec_logical_flow_set_table_id(sbflow, table);
> -            sbrec_logical_flow_set_priority(sbflow, lflow->priority);
> -            sbrec_logical_flow_set_match(sbflow, lflow->match);
> -            sbrec_logical_flow_set_actions(sbflow, lflow->actions);
> -            if (lflow->io_port) {
> -                struct smap tags = SMAP_INITIALIZER(&tags);
> -                smap_add(&tags, "in_out_port", lflow->io_port);
> -                sbrec_logical_flow_set_tags(sbflow, &tags);
> -                smap_destroy(&tags);
> -            }
> -            sbrec_logical_flow_set_controller_meter(sbflow, lflow->ctrl_meter);
> -            /* Trim the source locator lflow->where, which looks something like
> -             * "ovn/northd/northd.c:1234", down to just the part following the
> -             * last slash, e.g. "northd.c:1234". */
> -            const char *slash = strrchr(lflow->where, '/');
> +            /* Sync the newly added flows to SB. */
> +            struct lflow_ref_node *lfrn;
> +            LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
> +                struct ovn_lflow *lflow = lfrn->lflow;
> +                size_t n_datapaths;
> +                struct ovn_datapath **datapaths_array;
> +                if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) {
> +                    n_datapaths = ods_size(lflow_input->ls_datapaths);
> +                    datapaths_array = lflow_input->ls_datapaths->array;
> +                } else {
> +                    n_datapaths = ods_size(lflow_input->lr_datapaths);
> +                    datapaths_array = lflow_input->lr_datapaths->array;
> +                }
> +                uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap, n_datapaths);
> +                ovs_assert(n_ods == 1);
> +                /* There is only one datapath, so it should be moved out of the
> +                 * group to a single 'od'. */
> +                size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0,
> +                                           n_datapaths);
> +
> +                bitmap_set0(lflow->dpg_bitmap, index);
> +                lflow->od = datapaths_array[index];
> +
> +                /* Logical flow should be re-hashed to allow lookups. */
> +                uint32_t hash = hmap_node_hash(&lflow->hmap_node);
> +                /* Remove from lflows. */
> +                hmap_remove(lflows, &lflow->hmap_node);
> +                hash = ovn_logical_flow_hash_datapath(
> +                                          &lflow->od->sb->header_.uuid, hash);
> +                /* Add back. */
> +                hmap_insert(lflows, &lflow->hmap_node, hash);
> +
> +                /* Sync to SB. */
> +                const struct sbrec_logical_flow *sbflow;
> +                lflow->sb_uuid = uuid_random();
> +                sbflow = sbrec_logical_flow_insert_persist_uuid(
> +                                                ovnsb_txn, &lflow->sb_uuid);
> +                const char *pipeline = ovn_stage_get_pipeline_name(
> +                                                               lflow->stage);
> +                uint8_t table = ovn_stage_get_table(lflow->stage);
> +                sbrec_logical_flow_set_logical_datapath(sbflow, lflow->od->sb);
> +                sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
> +                sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> +                sbrec_logical_flow_set_table_id(sbflow, table);
> +                sbrec_logical_flow_set_priority(sbflow, lflow->priority);
> +                sbrec_logical_flow_set_match(sbflow, lflow->match);
> +                sbrec_logical_flow_set_actions(sbflow, lflow->actions);
> +                if (lflow->io_port) {
> +                    struct smap tags = SMAP_INITIALIZER(&tags);
> +                    smap_add(&tags, "in_out_port", lflow->io_port);
> +                    sbrec_logical_flow_set_tags(sbflow, &tags);
> +                    smap_destroy(&tags);
> +                }
> +                sbrec_logical_flow_set_controller_meter(sbflow,
> +                                                        lflow->ctrl_meter);
> +                /* Trim the source locator lflow->where, which looks something
> +                 * like "ovn/northd/northd.c:1234", down to just the part
> +                 * following the last slash, e.g. "northd.c:1234". */
> +                const char *slash = strrchr(lflow->where, '/');
>  #if _WIN32
> -            const char *backslash = strrchr(lflow->where, '\\');
> -            if (!slash || backslash > slash) {
> -                slash = backslash;
> -            }
> +                const char *backslash = strrchr(lflow->where, '\\');
> +                if (!slash || backslash > slash) {
> +                    slash = backslash;
> +                }
>  #endif
> -            const char *where = slash ? slash + 1 : lflow->where;
> +                const char *where = slash ? slash + 1 : lflow->where;
>
> -            struct smap ids = SMAP_INITIALIZER(&ids);
> -            smap_add(&ids, "stage-name", ovn_stage_to_str(lflow->stage));
> -            smap_add(&ids, "source", where);
> -            if (lflow->stage_hint) {
> -                smap_add(&ids, "stage-hint", lflow->stage_hint);
> +                struct smap ids = SMAP_INITIALIZER(&ids);
> +                smap_add(&ids, "stage-name", ovn_stage_to_str(lflow->stage));
> +                smap_add(&ids, "source", where);
> +                if (lflow->stage_hint) {
> +                    smap_add(&ids, "stage-hint", lflow->stage_hint);
> +                }
> +                sbrec_logical_flow_set_external_ids(sbflow, &ids);
> +                smap_destroy(&ids);
>              }
> -            sbrec_logical_flow_set_external_ids(sbflow, &ids);
> -            smap_destroy(&ids);
>          }
>      }
> -    ovs_list_init(&temp_lflow_list);
> -    add_lflow_to_temp_list = false;
>      return true;
>
>  }
> --
> 2.30.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Mon, Jun 26, 2023 at 7:25 AM Numan Siddique <numans@ovn.org> wrote:
>
> On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hzhou@ovn.org> wrote:
> >
> > For incremental processing, it is important to maintain relationship
> > between the inputs and the logical flows generated. This patch creates
> > the links between ovn_port and logical flows. The same data structure
> > may be expanded to maintain links between logical flows and other types
> > of inputs.
> >
> > This patch also refactors the temp_lflow_list operations to
> > collected_lflows with helper functions to start and end collecting. It
> > still uses global variables just to avoid updating all the lflow_add_...
> > related code all over the northd.c file.
> >
> > Signed-off-by: Han Zhou <hzhou@ovn.org>
>
> Hi Han,
>
> Please see a few comments below.  I did review all the 3 patches in the
series.
> They LGTM overall.  I'd like to do some more testing before providing my
Acks.
>

Thanks for your review!

>
> > ---
> >  northd/northd.c | 271 +++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 178 insertions(+), 93 deletions(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 98f528f93cfc..aa0f853ce2db 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -1457,6 +1457,19 @@ struct ovn_port_routable_addresses {
> >      size_t n_addrs;
> >  };
> >
> > +/* A node that maintains link between an object (such as an ovn_port)
and
> > + * a lflow. */
> > +struct lflow_ref_node {
> > +    /* This list follows different lflows referenced by the same
object. List
> > +     * head is, for example, ovn_port->lflows.  */
> > +    struct ovs_list lflow_list_node;
> > +    /* This list follows different objects that reference the same
lflow. List
> > +     * head is ovn_lflow->referenced_by. */
> > +    struct ovs_list ref_list_node;
> > +    /* The lflow. */
> > +    struct ovn_lflow *lflow;
> > +};
> > +
> >  /* A logical switch port or logical router port.
> >   *
> >   * In steady state, an ovn_port points to a northbound
Logical_Switch_Port
> > @@ -1548,6 +1561,28 @@ struct ovn_port {
> >
> >      /* Temporarily used for traversing a list (or hmap) of ports. */
> >      bool visited;
> > +
> > +    /* List of struct lflow_ref_node that points to the lflows
generated by
> > +     * this ovn_port.
> > +     *
> > +     * This data is initialized and destroyed by the en_northd node,
but
> > +     * populated and used only by the en_lflow node. Ideally this data
should
> > +     * be maintained as part of en_lflow's data (struct lflow_data): a
hash
> > +     * index from ovn_port key to lflows.  However, it would be less
efficient
> > +     * and more complex:
> > +     *
> > +     * 1. It would require an extra search (using the index) to find
the
> > +     * lflows.
> > +     *
> > +     * 2. Building the index needs to be thread-safe, using either a
global
> > +     * lock which is obviously less efficient, or hash-based lock
array which
> > +     * is more complex.
> > +     *
> > +     * Adding the list here is more straightforward. The drawback is
that we
> > +     * need to keep in mind that this data belongs to en_lflow node,
so never
> > +     * access it from any other nodes.
> > +     */
> > +    struct ovs_list lflows;
> >  };
>
>
> >
> >  static bool lsp_can_be_inc_processed(const struct
nbrec_logical_switch_port *);
> > @@ -1635,6 +1670,8 @@ ovn_port_create(struct hmap *ports, const char
*key,
> >      ovn_port_set_nb(op, nbsp, nbrp);
> >      op->l3dgw_port = op->cr_port = NULL;
> >      hmap_insert(ports, &op->key_node, hash_string(op->key, 0));
> > +
> > +    ovs_list_init(&op->lflows);
> >      return op;
> >  }
> >
> > @@ -1665,6 +1702,13 @@ ovn_port_destroy_orphan(struct ovn_port *port)
> >      destroy_lport_addresses(&port->proxy_arp_addrs);
> >      free(port->json_key);
> >      free(port->key);
> > +
> > +    struct lflow_ref_node *l;
> > +    LIST_FOR_EACH_SAFE (l, lflow_list_node, &port->lflows) {
> > +        ovs_list_remove(&l->lflow_list_node);
> > +        ovs_list_remove(&l->ref_list_node);
> > +        free(l);
> > +    }
> >      free(port);
> >  }
> >
> > @@ -4893,6 +4937,7 @@ static struct ovn_port *
> >  ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports,
> >                 const char *key, const struct nbrec_logical_switch_port
*nbsp,
> >                 struct ovn_datapath *od, const struct
sbrec_port_binding *sb,
> > +               struct ovs_list *lflows,
> >                 const struct sbrec_mirror_table *sbrec_mirror_table,
> >                 const struct sbrec_chassis_table *sbrec_chassis_table,
> >                 struct ovsdb_idl_index *sbrec_chassis_by_name,
> > @@ -4903,6 +4948,9 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn,
struct hmap *ls_ports,
> >      parse_lsp_addrs(op);
> >      op->od = od;
> >      hmap_insert(&od->ports, &op->dp_node,
hmap_node_hash(&op->key_node));
> > +    if (lflows) {
> > +        ovs_list_splice(&op->lflows, lflows->next, lflows);
> > +    }
> >
> >      /* Assign explicitly requested tunnel ids first. */
> >      if (!ovn_port_assign_requested_tnl_id(sbrec_chassis_table, op)) {
> > @@ -5082,7 +5130,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
> >                      goto fail;
> >                  }
> >                  op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> > -                                    new_nbsp->name, new_nbsp, od, NULL,
> > +                                    new_nbsp->name, new_nbsp, od,
NULL, NULL,
> >                                      ni->sbrec_mirror_table,
> >                                      ni->sbrec_chassis_table,
> >                                      ni->sbrec_chassis_by_name,
> > @@ -5114,13 +5162,16 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
> >                      op->visited = true;
> >                      continue;
> >                  }
> > +                struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows);
> > +                ovs_list_splice(&lflows, op->lflows.next, &op->lflows);
> >                  ovn_port_destroy(&nd->ls_ports, op);
> >                  op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> > -                                    new_nbsp->name, new_nbsp, od, sb,
> > +                                    new_nbsp->name, new_nbsp, od, sb,
&lflows,
> >                                      ni->sbrec_mirror_table,
> >                                      ni->sbrec_chassis_table,
> >                                      ni->sbrec_chassis_by_name,
> >                                      ni->sbrec_chassis_by_hostname);
> > +                ovs_assert(ovs_list_is_empty(&lflows));
> >                  if (!op) {
> >                      goto fail;
> >                  }
> > @@ -5577,7 +5628,8 @@ ovn_igmp_group_destroy(struct hmap *igmp_groups,
> >
> >  struct ovn_lflow {
> >      struct hmap_node hmap_node;
> > -    struct ovs_list list_node;
> > +    struct ovs_list list_node;   /* For temporary list of lflows.
Don't remove
> > +                                    at destroy. */
> >
> >      struct ovn_datapath *od;     /* 'logical_datapath' in SB schema.
 */
> >      unsigned long *dpg_bitmap;   /* Bitmap of all datapaths by their
'index'.*/
> > @@ -5591,6 +5643,8 @@ struct ovn_lflow {
> >      size_t n_ods;                /* Number of datapaths referenced by
'od' and
> >                                    * 'dpg_bitmap'. */
> >      struct ovn_dp_group *dpg;    /* Link to unique Sb datapath group.
*/
> > +
> > +    struct ovs_list referenced_by;  /* List of struct lflow_ref_node.
*/
> >      const char *where;
> >
> >      struct uuid sb_uuid;            /* SB DB row uuid, specified by
northd. */
> > @@ -5640,6 +5694,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct
ovn_datapath *od,
> >                 char *stage_hint, const char *where)
> >  {
> >      ovs_list_init(&lflow->list_node);
> > +    ovs_list_init(&lflow->referenced_by);
> >      lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len);
> >      lflow->od = od;
> >      lflow->stage = stage;
> > @@ -5767,20 +5822,30 @@ ovn_dp_group_add_with_reference(struct
ovn_lflow *lflow_ref,
> >      }
> >  }
> >
> > +/* This global variable collects the lflows generated by
do_ovn_lflow_add().
> > + * start_collecting_lflows() will enable the lflow collection and the
calls to
> > + * do_ovn_lflow_add (or the macros ovn_lflow_add_...) will add
generated lflows
> > + * to the list. end_collecting_lflows() will disable it. */
> > +static thread_local struct ovs_list collected_lflows;
> > +static thread_local bool collecting_lflows = false;
> > +
> > +static void
> > +start_collecting_lflows(void)
> > +{
> > +    ovs_assert(!collecting_lflows);
> > +    ovs_list_init(&collected_lflows);
> > +    collecting_lflows = true;
> > +}
> > +
> > +static void
> > +end_collecting_lflows(void)
> > +{
> > +    ovs_assert(collecting_lflows);
> > +    collecting_lflows = false;
> > +}
> > +
>
> I think we can avoid these functions and the thread local variable
> "collected_lflows".
>
> I'd suggest the below
>
> ----------------------------
>
> static void
> do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od,
>                  const unsigned long *dp_bitmap, size_t dp_bitmap_len,
>                  uint32_t hash, enum ovn_stage stage, uint16_t priority,
>                  const char *match, const char *actions, const char
*io_port,
>                  struct ovs_list *lflow_ref_list,
>                  const struct ovsdb_idl_row *stage_hint,
>                  const char *where, const char *ctrl_meter)
>     OVS_REQUIRES(fake_hash_mutex)
> {
>     ...
>     ...
>     /* At the end. */
>     if (lflow_ref_list) {
>         struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
>         lfrn->lflow = lflow;
>         ovs_list_insert(lflow_ref_list, &lfrn->lflow_list_node);
>         ovs_list_insert(&lflow->referenced_by, &lfrn->ref_list_node);
>     }
> }
>
>
> #define ovn_lflow_add_with_lport_and_hint(LFLOW_MAP, OD, STAGE, PRIORITY,
\
>                                           MATCH, ACTIONS, IN_OUT_PORT, \
>                                           LFLOW_REF_LIST, STAGE_HINT) \
>     ovn_lflow_add_at(LFLOW_MAP, OD, NULL, 0, STAGE, PRIORITY, MATCH,
ACTIONS, \
>                      IN_OUT_PORT, LFLOW_REF_LIST, NULL, STAGE_HINT, \
>                      OVS_SOURCE_LOCATOR)
>
> And pass &op->lflows in ovn_lflow_add_with_lport_and_hint()
>
> -----------------------------
>
> What do you think ?  Definitely this would result in a bit more work
> as it would require a lot of (tedious) changes.
> I think this is a better approach.
>
Firstly, I think it is not good to use "lflow_ref_list" directly in the
parameter, because there can be more than one object contributing to the
lflow generation. When we implement I-P for more inputs, a single lflow may
be referenced by multiple objects. We can't pass multiple lflow_ref_list to
the function either, because the number of such lists is unknown. For
example, a lflow may be generated as a result of a LSP, a DP and a LB
backend. If we want to implement I-P for LSP, DP and LB backend, we need to
track the reference for all of them. So the current idea is just to collect
a list of lflows generated by a higher level function, such as the
build_lswitch_and_lrouter_iterate_by_lsp. When implementing I-P for more
than one object of the same lflow, this needs to be more fine-grained.

Secondly, I agree with you adding a new parameter to the do_ovn_lflow_add
is cleaner. For the collected list I mentioned, it can be a parameter
instead of a thread local variable. However, as you mentioned, the change
is going to be all over the northd.c code, not only for the
ovn_lflow_add_xxx, but also the functions calling the ovn_lflow_add_xxx
macros, and upper layer functions calling those functions, and so on.
Unfortunately C doesn't support optional args. At this moment I am not sure
if the interface is stable enough for the incremental-processing, so I am
not sure if it is worth such a big change. If we need to modify them again
later, all such changes are going to be wasted. On the other hand, although
the thread local variable is not the best way, I think it is still clear
and manageable, if we call the start_collecting_lflows and
end_collecting_lflows in pairs. So, is it ok to leave it for this patch and
in the future when this mechanism proves to work well for more I-P, we can
have a separate patch to refactor (which will include all the tedious
function call changes). What do you think?

Thanks,
Han

> Also I'm planning to work on top of your patches to add I-P for load
> balancers in lflow engine (or perhaps I-P for datapath changes)
>
> My idea is to have a lflow ref list stored in "struct ovn_datapath"
> similar to the way you have done in this patch in "struct ovn_port"
>
> And while adding the flows using one of the macro variants
> 'ovn_lflow_add*' pass &od->lflows.
>
> Please let me know your comments.
>
> Only concern I have with this patch is the "op->lflows" modified by
> the lflow engine node.
> But I agree with your added comments and also thinking to use the same
> approach for datapath I-P handling,
> And I don't have a better approach at the moment. So I'm fine with it.
>
> Thanks
> Numan
>
>
> >  /* Adds a row with the specified contents to the Logical_Flow table.
> > - * Version to use when hash bucket locking is NOT required.
> > - *
> > - * Note: This function can add generated lflows to the global variable
> > - * temp_lflow_list as its output, controlled by the global variable
> > - * add_lflow_to_temp_list. The caller of the ovn_lflow_add_... marcros
can get
> > - * a list of lflows generated by setting add_lflow_to_temp_list to
true. The
> > - * caller is responsible for initializing the temp_lflow_list, and also
> > - * reset the add_lflow_to_temp_list to false when it is no longer
needed.
> > - * XXX: this mechanism is temporary and will be replaced when we add
hash index
> > - * to lflow_data and refactor related functions.
> > - */
> > -static bool add_lflow_to_temp_list = false;
> > -static struct ovs_list temp_lflow_list;
> > + * Version to use when hash bucket locking is NOT required. */
> >  static void
> >  do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od,
> >                   const unsigned long *dp_bitmap, size_t dp_bitmap_len,
> > @@ -5797,7 +5862,7 @@ do_ovn_lflow_add(struct hmap *lflow_map, const
struct ovn_datapath *od,
> >      size_t bitmap_len = od ? ods_size(od->datapaths) : dp_bitmap_len;
> >      ovs_assert(bitmap_len);
> >
> > -    if (add_lflow_to_temp_list) {
> > +    if (collecting_lflows) {
> >          ovs_assert(od);
> >          ovs_assert(!dp_bitmap);
> >      } else {
> > @@ -5829,8 +5894,8 @@ do_ovn_lflow_add(struct hmap *lflow_map, const
struct ovn_datapath *od,
> >          thread_lflow_counter++;
> >      }
> >
> > -    if (add_lflow_to_temp_list) {
> > -        ovs_list_insert(&temp_lflow_list, &lflow->list_node);
> > +    if (collecting_lflows) {
> > +        ovs_list_insert(&collected_lflows, &lflow->list_node);
> >      }
> >  }
> >
> > @@ -5950,10 +6015,28 @@ ovn_lflow_destroy(struct hmap *lflows, struct
ovn_lflow *lflow)
> >          free(lflow->io_port);
> >          free(lflow->stage_hint);
> >          free(lflow->ctrl_meter);
> > +        struct lflow_ref_node *l;
> > +        LIST_FOR_EACH_SAFE (l, ref_list_node, &lflow->referenced_by) {
> > +            ovs_list_remove(&l->lflow_list_node);
> > +            ovs_list_remove(&l->ref_list_node);
> > +            free(l);
> > +        }
> >          free(lflow);
> >      }
> >  }
> >
> > +static void
> > +link_ovn_port_to_lflows(struct ovn_port *op, struct ovs_list *lflows)
> > +{
> > +    struct ovn_lflow *f;
> > +    LIST_FOR_EACH (f, list_node, lflows) {
> > +        struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
> > +        lfrn->lflow = f;
> > +        ovs_list_insert(&op->lflows, &lfrn->lflow_list_node);
> > +        ovs_list_insert(&f->referenced_by, &lfrn->ref_list_node);
> > +    }
> > +}
> > +
> >  static bool
> >  build_dhcpv4_action(struct ovn_port *op, ovs_be32 offer_ip,
> >                      struct ds *options_action, struct ds
*response_action,
> > @@ -15483,6 +15566,7 @@ build_lswitch_and_lrouter_iterate_by_lsp(struct
ovn_port *op,
> >                                           struct hmap *lflows)
> >  {
> >      ovs_assert(op->nbsp);
> > +    start_collecting_lflows();
> >
> >      /* Build Logical Switch Flows. */
> >      build_lswitch_port_sec_op(op, lflows, actions, match);
> > @@ -15497,6 +15581,9 @@ build_lswitch_and_lrouter_iterate_by_lsp(struct
ovn_port *op,
> >      /* Build Logical Router Flows. */
> >      build_ip_routing_flows_for_router_type_lsp(op, lr_ports, lflows);
> >      build_arp_resolve_flows_for_lsp(op, lflows, lr_ports, match,
actions);
> > +
> > +    link_ovn_port_to_lflows(op, &collected_lflows);
> > +    end_collecting_lflows();
> >  }
> >
> >  /* Helper function to combine all lflow generation which is iterated
by logical
> > @@ -16223,14 +16310,10 @@ bool lflow_handle_northd_ls_changes(struct
ovsdb_idl_txn *ovnsb_txn,
> >  {
> >      struct ls_change *ls_change;
> >      LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
> > -        ovs_list_init(&temp_lflow_list);
> > -        add_lflow_to_temp_list = true;
> >          if (!ovs_list_is_empty(&ls_change->updated_ports) ||
> >              !ovs_list_is_empty(&ls_change->deleted_ports)) {
> >              /* XXX: implement lflow index so that we can handle
updated and
> >               * deleted LSPs incrementally. */
> > -            ovs_list_init(&temp_lflow_list);
> > -            add_lflow_to_temp_list = false;
> >              return false;
> >          }
> >
> > @@ -16277,83 +16360,85 @@ bool lflow_handle_northd_ls_changes(struct
ovsdb_idl_txn *ovnsb_txn,
> >
 sbrec_multicast_group_update_ports_addvalue(sbmc_unknown,
> >                                                              op->sb);
> >              }
> > -        }
> > -        /* Sync the newly added flows to SB. */
> > -        struct ovn_lflow *lflow;
> > -        LIST_FOR_EACH (lflow, list_node, &temp_lflow_list) {
> > -            size_t n_datapaths;
> > -            struct ovn_datapath **datapaths_array;
> > -            if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH)
{
> > -                n_datapaths = ods_size(lflow_input->ls_datapaths);
> > -                datapaths_array = lflow_input->ls_datapaths->array;
> > -            } else {
> > -                n_datapaths = ods_size(lflow_input->lr_datapaths);
> > -                datapaths_array = lflow_input->lr_datapaths->array;
> > -            }
> > -            uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap,
n_datapaths);
> > -            ovs_assert(n_ods == 1);
> > -            /* There is only one datapath, so it should be moved out
of the
> > -             * group to a single 'od'. */
> > -            size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0,
> > -                                       n_datapaths);
> >
> > -            bitmap_set0(lflow->dpg_bitmap, index);
> > -            lflow->od = datapaths_array[index];
> > -
> > -            /* Logical flow should be re-hashed to allow lookups. */
> > -            uint32_t hash = hmap_node_hash(&lflow->hmap_node);
> > -            /* Remove from lflows. */
> > -            hmap_remove(lflows, &lflow->hmap_node);
> > -            hash =
ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid,
> > -                                                  hash);
> > -            /* Add back. */
> > -            hmap_insert(lflows, &lflow->hmap_node, hash);
> > -
> > -            /* Sync to SB. */
> > -            const struct sbrec_logical_flow *sbflow;
> > -            lflow->sb_uuid = uuid_random();
> > -            sbflow = sbrec_logical_flow_insert_persist_uuid(ovnsb_txn,
> > -
 &lflow->sb_uuid);
> > -            const char *pipeline =
ovn_stage_get_pipeline_name(lflow->stage);
> > -            uint8_t table = ovn_stage_get_table(lflow->stage);
> > -            sbrec_logical_flow_set_logical_datapath(sbflow,
lflow->od->sb);
> > -            sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
> > -            sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> > -            sbrec_logical_flow_set_table_id(sbflow, table);
> > -            sbrec_logical_flow_set_priority(sbflow, lflow->priority);
> > -            sbrec_logical_flow_set_match(sbflow, lflow->match);
> > -            sbrec_logical_flow_set_actions(sbflow, lflow->actions);
> > -            if (lflow->io_port) {
> > -                struct smap tags = SMAP_INITIALIZER(&tags);
> > -                smap_add(&tags, "in_out_port", lflow->io_port);
> > -                sbrec_logical_flow_set_tags(sbflow, &tags);
> > -                smap_destroy(&tags);
> > -            }
> > -            sbrec_logical_flow_set_controller_meter(sbflow,
lflow->ctrl_meter);
> > -            /* Trim the source locator lflow->where, which looks
something like
> > -             * "ovn/northd/northd.c:1234", down to just the part
following the
> > -             * last slash, e.g. "northd.c:1234". */
> > -            const char *slash = strrchr(lflow->where, '/');
> > +            /* Sync the newly added flows to SB. */
> > +            struct lflow_ref_node *lfrn;
> > +            LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
> > +                struct ovn_lflow *lflow = lfrn->lflow;
> > +                size_t n_datapaths;
> > +                struct ovn_datapath **datapaths_array;
> > +                if (ovn_stage_to_datapath_type(lflow->stage) ==
DP_SWITCH) {
> > +                    n_datapaths = ods_size(lflow_input->ls_datapaths);
> > +                    datapaths_array = lflow_input->ls_datapaths->array;
> > +                } else {
> > +                    n_datapaths = ods_size(lflow_input->lr_datapaths);
> > +                    datapaths_array = lflow_input->lr_datapaths->array;
> > +                }
> > +                uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap,
n_datapaths);
> > +                ovs_assert(n_ods == 1);
> > +                /* There is only one datapath, so it should be moved
out of the
> > +                 * group to a single 'od'. */
> > +                size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0,
> > +                                           n_datapaths);
> > +
> > +                bitmap_set0(lflow->dpg_bitmap, index);
> > +                lflow->od = datapaths_array[index];
> > +
> > +                /* Logical flow should be re-hashed to allow lookups.
*/
> > +                uint32_t hash = hmap_node_hash(&lflow->hmap_node);
> > +                /* Remove from lflows. */
> > +                hmap_remove(lflows, &lflow->hmap_node);
> > +                hash = ovn_logical_flow_hash_datapath(
> > +
 &lflow->od->sb->header_.uuid, hash);
> > +                /* Add back. */
> > +                hmap_insert(lflows, &lflow->hmap_node, hash);
> > +
> > +                /* Sync to SB. */
> > +                const struct sbrec_logical_flow *sbflow;
> > +                lflow->sb_uuid = uuid_random();
> > +                sbflow = sbrec_logical_flow_insert_persist_uuid(
> > +                                                ovnsb_txn,
&lflow->sb_uuid);
> > +                const char *pipeline = ovn_stage_get_pipeline_name(
> > +
lflow->stage);
> > +                uint8_t table = ovn_stage_get_table(lflow->stage);
> > +                sbrec_logical_flow_set_logical_datapath(sbflow,
lflow->od->sb);
> > +                sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
> > +                sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> > +                sbrec_logical_flow_set_table_id(sbflow, table);
> > +                sbrec_logical_flow_set_priority(sbflow,
lflow->priority);
> > +                sbrec_logical_flow_set_match(sbflow, lflow->match);
> > +                sbrec_logical_flow_set_actions(sbflow, lflow->actions);
> > +                if (lflow->io_port) {
> > +                    struct smap tags = SMAP_INITIALIZER(&tags);
> > +                    smap_add(&tags, "in_out_port", lflow->io_port);
> > +                    sbrec_logical_flow_set_tags(sbflow, &tags);
> > +                    smap_destroy(&tags);
> > +                }
> > +                sbrec_logical_flow_set_controller_meter(sbflow,
> > +
 lflow->ctrl_meter);
> > +                /* Trim the source locator lflow->where, which looks
something
> > +                 * like "ovn/northd/northd.c:1234", down to just the
part
> > +                 * following the last slash, e.g. "northd.c:1234". */
> > +                const char *slash = strrchr(lflow->where, '/');
> >  #if _WIN32
> > -            const char *backslash = strrchr(lflow->where, '\\');
> > -            if (!slash || backslash > slash) {
> > -                slash = backslash;
> > -            }
> > +                const char *backslash = strrchr(lflow->where, '\\');
> > +                if (!slash || backslash > slash) {
> > +                    slash = backslash;
> > +                }
> >  #endif
> > -            const char *where = slash ? slash + 1 : lflow->where;
> > +                const char *where = slash ? slash + 1 : lflow->where;
> >
> > -            struct smap ids = SMAP_INITIALIZER(&ids);
> > -            smap_add(&ids, "stage-name",
ovn_stage_to_str(lflow->stage));
> > -            smap_add(&ids, "source", where);
> > -            if (lflow->stage_hint) {
> > -                smap_add(&ids, "stage-hint", lflow->stage_hint);
> > +                struct smap ids = SMAP_INITIALIZER(&ids);
> > +                smap_add(&ids, "stage-name",
ovn_stage_to_str(lflow->stage));
> > +                smap_add(&ids, "source", where);
> > +                if (lflow->stage_hint) {
> > +                    smap_add(&ids, "stage-hint", lflow->stage_hint);
> > +                }
> > +                sbrec_logical_flow_set_external_ids(sbflow, &ids);
> > +                smap_destroy(&ids);
> >              }
> > -            sbrec_logical_flow_set_external_ids(sbflow, &ids);
> > -            smap_destroy(&ids);
> >          }
> >      }
> > -    ovs_list_init(&temp_lflow_list);
> > -    add_lflow_to_temp_list = false;
> >      return true;
> >
> >  }
> > --
> > 2.30.2
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >

On Mon, Jun 26, 2023 at 10:34 PM Han Zhou <hzhou@ovn.org> wrote:
>
> On Mon, Jun 26, 2023 at 7:25 AM Numan Siddique <numans@ovn.org> wrote:
> >
> > On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hzhou@ovn.org> wrote:
> > >
> > > For incremental processing, it is important to maintain relationship
> > > between the inputs and the logical flows generated. This patch creates
> > > the links between ovn_port and logical flows. The same data structure
> > > may be expanded to maintain links between logical flows and other types
> > > of inputs.
> > >
> > > This patch also refactors the temp_lflow_list operations to
> > > collected_lflows with helper functions to start and end collecting. It
> > > still uses global variables just to avoid updating all the lflow_add_...
> > > related code all over the northd.c file.
> > >
> > > Signed-off-by: Han Zhou <hzhou@ovn.org>
> >
> > Hi Han,
> >
> > Please see a few comments below.  I did review all the 3 patches in the
> series.
> > They LGTM overall.  I'd like to do some more testing before providing my
> Acks.
> >
>
> Thanks for your review!
>
> >
> > > ---
> > >  northd/northd.c | 271 +++++++++++++++++++++++++++++++-----------------
> > >  1 file changed, 178 insertions(+), 93 deletions(-)
> > >
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index 98f528f93cfc..aa0f853ce2db 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -1457,6 +1457,19 @@ struct ovn_port_routable_addresses {
> > >      size_t n_addrs;
> > >  };
> > >
> > > +/* A node that maintains link between an object (such as an ovn_port)
> and
> > > + * a lflow. */
> > > +struct lflow_ref_node {
> > > +    /* This list follows different lflows referenced by the same
> object. List
> > > +     * head is, for example, ovn_port->lflows.  */
> > > +    struct ovs_list lflow_list_node;
> > > +    /* This list follows different objects that reference the same
> lflow. List
> > > +     * head is ovn_lflow->referenced_by. */
> > > +    struct ovs_list ref_list_node;
> > > +    /* The lflow. */
> > > +    struct ovn_lflow *lflow;
> > > +};
> > > +
> > >  /* A logical switch port or logical router port.
> > >   *
> > >   * In steady state, an ovn_port points to a northbound
> Logical_Switch_Port
> > > @@ -1548,6 +1561,28 @@ struct ovn_port {
> > >
> > >      /* Temporarily used for traversing a list (or hmap) of ports. */
> > >      bool visited;
> > > +
> > > +    /* List of struct lflow_ref_node that points to the lflows
> generated by
> > > +     * this ovn_port.
> > > +     *
> > > +     * This data is initialized and destroyed by the en_northd node,
> but
> > > +     * populated and used only by the en_lflow node. Ideally this data
> should
> > > +     * be maintained as part of en_lflow's data (struct lflow_data): a
> hash
> > > +     * index from ovn_port key to lflows.  However, it would be less
> efficient
> > > +     * and more complex:
> > > +     *
> > > +     * 1. It would require an extra search (using the index) to find
> the
> > > +     * lflows.
> > > +     *
> > > +     * 2. Building the index needs to be thread-safe, using either a
> global
> > > +     * lock which is obviously less efficient, or hash-based lock
> array which
> > > +     * is more complex.
> > > +     *
> > > +     * Adding the list here is more straightforward. The drawback is
> that we
> > > +     * need to keep in mind that this data belongs to en_lflow node,
> so never
> > > +     * access it from any other nodes.
> > > +     */
> > > +    struct ovs_list lflows;
> > >  };
> >
> >
> > >
> > >  static bool lsp_can_be_inc_processed(const struct
> nbrec_logical_switch_port *);
> > > @@ -1635,6 +1670,8 @@ ovn_port_create(struct hmap *ports, const char
> *key,
> > >      ovn_port_set_nb(op, nbsp, nbrp);
> > >      op->l3dgw_port = op->cr_port = NULL;
> > >      hmap_insert(ports, &op->key_node, hash_string(op->key, 0));
> > > +
> > > +    ovs_list_init(&op->lflows);
> > >      return op;
> > >  }
> > >
> > > @@ -1665,6 +1702,13 @@ ovn_port_destroy_orphan(struct ovn_port *port)
> > >      destroy_lport_addresses(&port->proxy_arp_addrs);
> > >      free(port->json_key);
> > >      free(port->key);
> > > +
> > > +    struct lflow_ref_node *l;
> > > +    LIST_FOR_EACH_SAFE (l, lflow_list_node, &port->lflows) {
> > > +        ovs_list_remove(&l->lflow_list_node);
> > > +        ovs_list_remove(&l->ref_list_node);
> > > +        free(l);
> > > +    }
> > >      free(port);
> > >  }
> > >
> > > @@ -4893,6 +4937,7 @@ static struct ovn_port *
> > >  ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports,
> > >                 const char *key, const struct nbrec_logical_switch_port
> *nbsp,
> > >                 struct ovn_datapath *od, const struct
> sbrec_port_binding *sb,
> > > +               struct ovs_list *lflows,
> > >                 const struct sbrec_mirror_table *sbrec_mirror_table,
> > >                 const struct sbrec_chassis_table *sbrec_chassis_table,
> > >                 struct ovsdb_idl_index *sbrec_chassis_by_name,
> > > @@ -4903,6 +4948,9 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn,
> struct hmap *ls_ports,
> > >      parse_lsp_addrs(op);
> > >      op->od = od;
> > >      hmap_insert(&od->ports, &op->dp_node,
> hmap_node_hash(&op->key_node));
> > > +    if (lflows) {
> > > +        ovs_list_splice(&op->lflows, lflows->next, lflows);
> > > +    }
> > >
> > >      /* Assign explicitly requested tunnel ids first. */
> > >      if (!ovn_port_assign_requested_tnl_id(sbrec_chassis_table, op)) {
> > > @@ -5082,7 +5130,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> > >                      goto fail;
> > >                  }
> > >                  op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> > > -                                    new_nbsp->name, new_nbsp, od, NULL,
> > > +                                    new_nbsp->name, new_nbsp, od,
> NULL, NULL,
> > >                                      ni->sbrec_mirror_table,
> > >                                      ni->sbrec_chassis_table,
> > >                                      ni->sbrec_chassis_by_name,
> > > @@ -5114,13 +5162,16 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> > >                      op->visited = true;
> > >                      continue;
> > >                  }
> > > +                struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows);
> > > +                ovs_list_splice(&lflows, op->lflows.next, &op->lflows);
> > >                  ovn_port_destroy(&nd->ls_ports, op);
> > >                  op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> > > -                                    new_nbsp->name, new_nbsp, od, sb,
> > > +                                    new_nbsp->name, new_nbsp, od, sb,
> &lflows,
> > >                                      ni->sbrec_mirror_table,
> > >                                      ni->sbrec_chassis_table,
> > >                                      ni->sbrec_chassis_by_name,
> > >                                      ni->sbrec_chassis_by_hostname);
> > > +                ovs_assert(ovs_list_is_empty(&lflows));
> > >                  if (!op) {
> > >                      goto fail;
> > >                  }
> > > @@ -5577,7 +5628,8 @@ ovn_igmp_group_destroy(struct hmap *igmp_groups,
> > >
> > >  struct ovn_lflow {
> > >      struct hmap_node hmap_node;
> > > -    struct ovs_list list_node;
> > > +    struct ovs_list list_node;   /* For temporary list of lflows.
> Don't remove
> > > +                                    at destroy. */
> > >
> > >      struct ovn_datapath *od;     /* 'logical_datapath' in SB schema.
>  */
> > >      unsigned long *dpg_bitmap;   /* Bitmap of all datapaths by their
> 'index'.*/
> > > @@ -5591,6 +5643,8 @@ struct ovn_lflow {
> > >      size_t n_ods;                /* Number of datapaths referenced by
> 'od' and
> > >                                    * 'dpg_bitmap'. */
> > >      struct ovn_dp_group *dpg;    /* Link to unique Sb datapath group.
> */
> > > +
> > > +    struct ovs_list referenced_by;  /* List of struct lflow_ref_node.
> */
> > >      const char *where;
> > >
> > >      struct uuid sb_uuid;            /* SB DB row uuid, specified by
> northd. */
> > > @@ -5640,6 +5694,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct
> ovn_datapath *od,
> > >                 char *stage_hint, const char *where)
> > >  {
> > >      ovs_list_init(&lflow->list_node);
> > > +    ovs_list_init(&lflow->referenced_by);
> > >      lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len);
> > >      lflow->od = od;
> > >      lflow->stage = stage;
> > > @@ -5767,20 +5822,30 @@ ovn_dp_group_add_with_reference(struct
> ovn_lflow *lflow_ref,
> > >      }
> > >  }
> > >
> > > +/* This global variable collects the lflows generated by
> do_ovn_lflow_add().
> > > + * start_collecting_lflows() will enable the lflow collection and the
> calls to
> > > + * do_ovn_lflow_add (or the macros ovn_lflow_add_...) will add
> generated lflows
> > > + * to the list. end_collecting_lflows() will disable it. */
> > > +static thread_local struct ovs_list collected_lflows;
> > > +static thread_local bool collecting_lflows = false;
> > > +
> > > +static void
> > > +start_collecting_lflows(void)
> > > +{
> > > +    ovs_assert(!collecting_lflows);
> > > +    ovs_list_init(&collected_lflows);
> > > +    collecting_lflows = true;
> > > +}
> > > +
> > > +static void
> > > +end_collecting_lflows(void)
> > > +{
> > > +    ovs_assert(collecting_lflows);
> > > +    collecting_lflows = false;
> > > +}
> > > +
> >
> > I think we can avoid these functions and the thread local variable
> > "collected_lflows".
> >
> > I'd suggest the below
> >
> > ----------------------------
> >
> > static void
> > do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od,
> >                  const unsigned long *dp_bitmap, size_t dp_bitmap_len,
> >                  uint32_t hash, enum ovn_stage stage, uint16_t priority,
> >                  const char *match, const char *actions, const char
> *io_port,
> >                  struct ovs_list *lflow_ref_list,
> >                  const struct ovsdb_idl_row *stage_hint,
> >                  const char *where, const char *ctrl_meter)
> >     OVS_REQUIRES(fake_hash_mutex)
> > {
> >     ...
> >     ...
> >     /* At the end. */
> >     if (lflow_ref_list) {
> >         struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
> >         lfrn->lflow = lflow;
> >         ovs_list_insert(lflow_ref_list, &lfrn->lflow_list_node);
> >         ovs_list_insert(&lflow->referenced_by, &lfrn->ref_list_node);
> >     }
> > }
> >
> >
> > #define ovn_lflow_add_with_lport_and_hint(LFLOW_MAP, OD, STAGE, PRIORITY,
> \
> >                                           MATCH, ACTIONS, IN_OUT_PORT, \
> >                                           LFLOW_REF_LIST, STAGE_HINT) \
> >     ovn_lflow_add_at(LFLOW_MAP, OD, NULL, 0, STAGE, PRIORITY, MATCH,
> ACTIONS, \
> >                      IN_OUT_PORT, LFLOW_REF_LIST, NULL, STAGE_HINT, \
> >                      OVS_SOURCE_LOCATOR)
> >
> > And pass &op->lflows in ovn_lflow_add_with_lport_and_hint()
> >
> > -----------------------------
> >
> > What do you think ?  Definitely this would result in a bit more work
> > as it would require a lot of (tedious) changes.
> > I think this is a better approach.
> >
> Firstly, I think it is not good to use "lflow_ref_list" directly in the
> parameter, because there can be more than one object contributing to the
> lflow generation. When we implement I-P for more inputs, a single lflow may
> be referenced by multiple objects. We can't pass multiple lflow_ref_list to
> the function either, because the number of such lists is unknown. For
> example, a lflow may be generated as a result of a LSP, a DP and a LB
> backend. If we want to implement I-P for LSP, DP and LB backend, we need to
> track the reference for all of them. So the current idea is just to collect
> a list of lflows generated by a higher level function, such as the
> build_lswitch_and_lrouter_iterate_by_lsp. When implementing I-P for more
> than one object of the same lflow, this needs to be more fine-grained.

Agree.  I'm working on the I-P for datapath changes in lflow engine and
as a flow can be referenced by multiple datapaths,  I agree this needs to
be tracked properly.  And definitely that is out of scope of this patch series.


>
> Secondly, I agree with you adding a new parameter to the do_ovn_lflow_add
> is cleaner. For the collected list I mentioned, it can be a parameter
> instead of a thread local variable. However, as you mentioned, the change
> is going to be all over the northd.c code, not only for the
> ovn_lflow_add_xxx, but also the functions calling the ovn_lflow_add_xxx
> macros, and upper layer functions calling those functions, and so on.
> Unfortunately C doesn't support optional args. At this moment I am not sure
> if the interface is stable enough for the incremental-processing, so I am
> not sure if it is worth such a big change. If we need to modify them again
> later, all such changes are going to be wasted. On the other hand, although
> the thread local variable is not the best way, I think it is still clear
> and manageable, if we call the start_collecting_lflows and
> end_collecting_lflows in pairs. So, is it ok to leave it for this patch and
> in the future when this mechanism proves to work well for more I-P, we can
> have a separate patch to refactor (which will include all the tedious
> function call changes). What do you think?

I agree.  It is definitely tedious to do all the changes.  I'm ok with
the approach taken
in this patch series.

Also request to please take a look at the load balancer I-P patch
series - http://patchwork.ozlabs.org/project/ovn/list/?series=361083&state=*

Thanks
Numan

>
> Thanks,
> Han
>
> > Also I'm planning to work on top of your patches to add I-P for load
> > balancers in lflow engine (or perhaps I-P for datapath changes)
> >
> > My idea is to have a lflow ref list stored in "struct ovn_datapath"
> > similar to the way you have done in this patch in "struct ovn_port"
> >
> > And while adding the flows using one of the macro variants
> > 'ovn_lflow_add*' pass &od->lflows.
> >
> > Please let me know your comments.
> >
> > Only concern I have with this patch is the "op->lflows" modified by
> > the lflow engine node.
> > But I agree with your added comments and also thinking to use the same
> > approach for datapath I-P handling,
> > And I don't have a better approach at the moment. So I'm fine with it.
> >
> > Thanks
> > Numan
> >
> >
> > >  /* Adds a row with the specified contents to the Logical_Flow table.
> > > - * Version to use when hash bucket locking is NOT required.
> > > - *
> > > - * Note: This function can add generated lflows to the global variable
> > > - * temp_lflow_list as its output, controlled by the global variable
> > > - * add_lflow_to_temp_list. The caller of the ovn_lflow_add_... marcros
> can get
> > > - * a list of lflows generated by setting add_lflow_to_temp_list to
> true. The
> > > - * caller is responsible for initializing the temp_lflow_list, and also
> > > - * reset the add_lflow_to_temp_list to false when it is no longer
> needed.
> > > - * XXX: this mechanism is temporary and will be replaced when we add
> hash index
> > > - * to lflow_data and refactor related functions.
> > > - */
> > > -static bool add_lflow_to_temp_list = false;
> > > -static struct ovs_list temp_lflow_list;
> > > + * Version to use when hash bucket locking is NOT required. */
> > >  static void
> > >  do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od,
> > >                   const unsigned long *dp_bitmap, size_t dp_bitmap_len,
> > > @@ -5797,7 +5862,7 @@ do_ovn_lflow_add(struct hmap *lflow_map, const
> struct ovn_datapath *od,
> > >      size_t bitmap_len = od ? ods_size(od->datapaths) : dp_bitmap_len;
> > >      ovs_assert(bitmap_len);
> > >
> > > -    if (add_lflow_to_temp_list) {
> > > +    if (collecting_lflows) {
> > >          ovs_assert(od);
> > >          ovs_assert(!dp_bitmap);
> > >      } else {
> > > @@ -5829,8 +5894,8 @@ do_ovn_lflow_add(struct hmap *lflow_map, const
> struct ovn_datapath *od,
> > >          thread_lflow_counter++;
> > >      }
> > >
> > > -    if (add_lflow_to_temp_list) {
> > > -        ovs_list_insert(&temp_lflow_list, &lflow->list_node);
> > > +    if (collecting_lflows) {
> > > +        ovs_list_insert(&collected_lflows, &lflow->list_node);
> > >      }
> > >  }
> > >
> > > @@ -5950,10 +6015,28 @@ ovn_lflow_destroy(struct hmap *lflows, struct
> ovn_lflow *lflow)
> > >          free(lflow->io_port);
> > >          free(lflow->stage_hint);
> > >          free(lflow->ctrl_meter);
> > > +        struct lflow_ref_node *l;
> > > +        LIST_FOR_EACH_SAFE (l, ref_list_node, &lflow->referenced_by) {
> > > +            ovs_list_remove(&l->lflow_list_node);
> > > +            ovs_list_remove(&l->ref_list_node);
> > > +            free(l);
> > > +        }
> > >          free(lflow);
> > >      }
> > >  }
> > >
> > > +static void
> > > +link_ovn_port_to_lflows(struct ovn_port *op, struct ovs_list *lflows)
> > > +{
> > > +    struct ovn_lflow *f;
> > > +    LIST_FOR_EACH (f, list_node, lflows) {
> > > +        struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
> > > +        lfrn->lflow = f;
> > > +        ovs_list_insert(&op->lflows, &lfrn->lflow_list_node);
> > > +        ovs_list_insert(&f->referenced_by, &lfrn->ref_list_node);
> > > +    }
> > > +}
> > > +
> > >  static bool
> > >  build_dhcpv4_action(struct ovn_port *op, ovs_be32 offer_ip,
> > >                      struct ds *options_action, struct ds
> *response_action,
> > > @@ -15483,6 +15566,7 @@ build_lswitch_and_lrouter_iterate_by_lsp(struct
> ovn_port *op,
> > >                                           struct hmap *lflows)
> > >  {
> > >      ovs_assert(op->nbsp);
> > > +    start_collecting_lflows();
> > >
> > >      /* Build Logical Switch Flows. */
> > >      build_lswitch_port_sec_op(op, lflows, actions, match);
> > > @@ -15497,6 +15581,9 @@ build_lswitch_and_lrouter_iterate_by_lsp(struct
> ovn_port *op,
> > >      /* Build Logical Router Flows. */
> > >      build_ip_routing_flows_for_router_type_lsp(op, lr_ports, lflows);
> > >      build_arp_resolve_flows_for_lsp(op, lflows, lr_ports, match,
> actions);
> > > +
> > > +    link_ovn_port_to_lflows(op, &collected_lflows);
> > > +    end_collecting_lflows();
> > >  }
> > >
> > >  /* Helper function to combine all lflow generation which is iterated
> by logical
> > > @@ -16223,14 +16310,10 @@ bool lflow_handle_northd_ls_changes(struct
> ovsdb_idl_txn *ovnsb_txn,
> > >  {
> > >      struct ls_change *ls_change;
> > >      LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
> > > -        ovs_list_init(&temp_lflow_list);
> > > -        add_lflow_to_temp_list = true;
> > >          if (!ovs_list_is_empty(&ls_change->updated_ports) ||
> > >              !ovs_list_is_empty(&ls_change->deleted_ports)) {
> > >              /* XXX: implement lflow index so that we can handle
> updated and
> > >               * deleted LSPs incrementally. */
> > > -            ovs_list_init(&temp_lflow_list);
> > > -            add_lflow_to_temp_list = false;
> > >              return false;
> > >          }
> > >
> > > @@ -16277,83 +16360,85 @@ bool lflow_handle_northd_ls_changes(struct
> ovsdb_idl_txn *ovnsb_txn,
> > >
>  sbrec_multicast_group_update_ports_addvalue(sbmc_unknown,
> > >                                                              op->sb);
> > >              }
> > > -        }
> > > -        /* Sync the newly added flows to SB. */
> > > -        struct ovn_lflow *lflow;
> > > -        LIST_FOR_EACH (lflow, list_node, &temp_lflow_list) {
> > > -            size_t n_datapaths;
> > > -            struct ovn_datapath **datapaths_array;
> > > -            if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH)
> {
> > > -                n_datapaths = ods_size(lflow_input->ls_datapaths);
> > > -                datapaths_array = lflow_input->ls_datapaths->array;
> > > -            } else {
> > > -                n_datapaths = ods_size(lflow_input->lr_datapaths);
> > > -                datapaths_array = lflow_input->lr_datapaths->array;
> > > -            }
> > > -            uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap,
> n_datapaths);
> > > -            ovs_assert(n_ods == 1);
> > > -            /* There is only one datapath, so it should be moved out
> of the
> > > -             * group to a single 'od'. */
> > > -            size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0,
> > > -                                       n_datapaths);
> > >
> > > -            bitmap_set0(lflow->dpg_bitmap, index);
> > > -            lflow->od = datapaths_array[index];
> > > -
> > > -            /* Logical flow should be re-hashed to allow lookups. */
> > > -            uint32_t hash = hmap_node_hash(&lflow->hmap_node);
> > > -            /* Remove from lflows. */
> > > -            hmap_remove(lflows, &lflow->hmap_node);
> > > -            hash =
> ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid,
> > > -                                                  hash);
> > > -            /* Add back. */
> > > -            hmap_insert(lflows, &lflow->hmap_node, hash);
> > > -
> > > -            /* Sync to SB. */
> > > -            const struct sbrec_logical_flow *sbflow;
> > > -            lflow->sb_uuid = uuid_random();
> > > -            sbflow = sbrec_logical_flow_insert_persist_uuid(ovnsb_txn,
> > > -
>  &lflow->sb_uuid);
> > > -            const char *pipeline =
> ovn_stage_get_pipeline_name(lflow->stage);
> > > -            uint8_t table = ovn_stage_get_table(lflow->stage);
> > > -            sbrec_logical_flow_set_logical_datapath(sbflow,
> lflow->od->sb);
> > > -            sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
> > > -            sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> > > -            sbrec_logical_flow_set_table_id(sbflow, table);
> > > -            sbrec_logical_flow_set_priority(sbflow, lflow->priority);
> > > -            sbrec_logical_flow_set_match(sbflow, lflow->match);
> > > -            sbrec_logical_flow_set_actions(sbflow, lflow->actions);
> > > -            if (lflow->io_port) {
> > > -                struct smap tags = SMAP_INITIALIZER(&tags);
> > > -                smap_add(&tags, "in_out_port", lflow->io_port);
> > > -                sbrec_logical_flow_set_tags(sbflow, &tags);
> > > -                smap_destroy(&tags);
> > > -            }
> > > -            sbrec_logical_flow_set_controller_meter(sbflow,
> lflow->ctrl_meter);
> > > -            /* Trim the source locator lflow->where, which looks
> something like
> > > -             * "ovn/northd/northd.c:1234", down to just the part
> following the
> > > -             * last slash, e.g. "northd.c:1234". */
> > > -            const char *slash = strrchr(lflow->where, '/');
> > > +            /* Sync the newly added flows to SB. */
> > > +            struct lflow_ref_node *lfrn;
> > > +            LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
> > > +                struct ovn_lflow *lflow = lfrn->lflow;
> > > +                size_t n_datapaths;
> > > +                struct ovn_datapath **datapaths_array;
> > > +                if (ovn_stage_to_datapath_type(lflow->stage) ==
> DP_SWITCH) {
> > > +                    n_datapaths = ods_size(lflow_input->ls_datapaths);
> > > +                    datapaths_array = lflow_input->ls_datapaths->array;
> > > +                } else {
> > > +                    n_datapaths = ods_size(lflow_input->lr_datapaths);
> > > +                    datapaths_array = lflow_input->lr_datapaths->array;
> > > +                }
> > > +                uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap,
> n_datapaths);
> > > +                ovs_assert(n_ods == 1);
> > > +                /* There is only one datapath, so it should be moved
> out of the
> > > +                 * group to a single 'od'. */
> > > +                size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0,
> > > +                                           n_datapaths);
> > > +
> > > +                bitmap_set0(lflow->dpg_bitmap, index);
> > > +                lflow->od = datapaths_array[index];
> > > +
> > > +                /* Logical flow should be re-hashed to allow lookups.
> */
> > > +                uint32_t hash = hmap_node_hash(&lflow->hmap_node);
> > > +                /* Remove from lflows. */
> > > +                hmap_remove(lflows, &lflow->hmap_node);
> > > +                hash = ovn_logical_flow_hash_datapath(
> > > +
>  &lflow->od->sb->header_.uuid, hash);
> > > +                /* Add back. */
> > > +                hmap_insert(lflows, &lflow->hmap_node, hash);
> > > +
> > > +                /* Sync to SB. */
> > > +                const struct sbrec_logical_flow *sbflow;
> > > +                lflow->sb_uuid = uuid_random();
> > > +                sbflow = sbrec_logical_flow_insert_persist_uuid(
> > > +                                                ovnsb_txn,
> &lflow->sb_uuid);
> > > +                const char *pipeline = ovn_stage_get_pipeline_name(
> > > +
> lflow->stage);
> > > +                uint8_t table = ovn_stage_get_table(lflow->stage);
> > > +                sbrec_logical_flow_set_logical_datapath(sbflow,
> lflow->od->sb);
> > > +                sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
> > > +                sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> > > +                sbrec_logical_flow_set_table_id(sbflow, table);
> > > +                sbrec_logical_flow_set_priority(sbflow,
> lflow->priority);
> > > +                sbrec_logical_flow_set_match(sbflow, lflow->match);
> > > +                sbrec_logical_flow_set_actions(sbflow, lflow->actions);
> > > +                if (lflow->io_port) {
> > > +                    struct smap tags = SMAP_INITIALIZER(&tags);
> > > +                    smap_add(&tags, "in_out_port", lflow->io_port);
> > > +                    sbrec_logical_flow_set_tags(sbflow, &tags);
> > > +                    smap_destroy(&tags);
> > > +                }
> > > +                sbrec_logical_flow_set_controller_meter(sbflow,
> > > +
>  lflow->ctrl_meter);
> > > +                /* Trim the source locator lflow->where, which looks
> something
> > > +                 * like "ovn/northd/northd.c:1234", down to just the
> part
> > > +                 * following the last slash, e.g. "northd.c:1234". */
> > > +                const char *slash = strrchr(lflow->where, '/');
> > >  #if _WIN32
> > > -            const char *backslash = strrchr(lflow->where, '\\');
> > > -            if (!slash || backslash > slash) {
> > > -                slash = backslash;
> > > -            }
> > > +                const char *backslash = strrchr(lflow->where, '\\');
> > > +                if (!slash || backslash > slash) {
> > > +                    slash = backslash;
> > > +                }
> > >  #endif
> > > -            const char *where = slash ? slash + 1 : lflow->where;
> > > +                const char *where = slash ? slash + 1 : lflow->where;
> > >
> > > -            struct smap ids = SMAP_INITIALIZER(&ids);
> > > -            smap_add(&ids, "stage-name",
> ovn_stage_to_str(lflow->stage));
> > > -            smap_add(&ids, "source", where);
> > > -            if (lflow->stage_hint) {
> > > -                smap_add(&ids, "stage-hint", lflow->stage_hint);
> > > +                struct smap ids = SMAP_INITIALIZER(&ids);
> > > +                smap_add(&ids, "stage-name",
> ovn_stage_to_str(lflow->stage));
> > > +                smap_add(&ids, "source", where);
> > > +                if (lflow->stage_hint) {
> > > +                    smap_add(&ids, "stage-hint", lflow->stage_hint);
> > > +                }
> > > +                sbrec_logical_flow_set_external_ids(sbflow, &ids);
> > > +                smap_destroy(&ids);
> > >              }
> > > -            sbrec_logical_flow_set_external_ids(sbflow, &ids);
> > > -            smap_destroy(&ids);
> > >          }
> > >      }
> > > -    ovs_list_init(&temp_lflow_list);
> > > -    add_lflow_to_temp_list = false;
> > >      return true;
> > >
> > >  }
> > > --
> > > 2.30.2
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 6/27/23 10:23, Numan Siddique wrote:
> On Mon, Jun 26, 2023 at 10:34 PM Han Zhou <hzhou@ovn.org> wrote:
>>
>> On Mon, Jun 26, 2023 at 7:25 AM Numan Siddique <numans@ovn.org> wrote:
>>>
>>> On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hzhou@ovn.org> wrote:
>>>>
>>>> For incremental processing, it is important to maintain relationship
>>>> between the inputs and the logical flows generated. This patch creates
>>>> the links between ovn_port and logical flows. The same data structure
>>>> may be expanded to maintain links between logical flows and other types
>>>> of inputs.
>>>>
>>>> This patch also refactors the temp_lflow_list operations to
>>>> collected_lflows with helper functions to start and end collecting. It
>>>> still uses global variables just to avoid updating all the lflow_add_...
>>>> related code all over the northd.c file.
>>>>
>>>> Signed-off-by: Han Zhou <hzhou@ovn.org>
>>>
>>> Hi Han,
>>>
>>> Please see a few comments below.  I did review all the 3 patches in the
>> series.
>>> They LGTM overall.  I'd like to do some more testing before providing my
>> Acks.
>>>
>>
>> Thanks for your review!
>>
>>>
>>>> ---
>>>>  northd/northd.c | 271 +++++++++++++++++++++++++++++++-----------------
>>>>  1 file changed, 178 insertions(+), 93 deletions(-)
>>>>
>>>> diff --git a/northd/northd.c b/northd/northd.c
>>>> index 98f528f93cfc..aa0f853ce2db 100644
>>>> --- a/northd/northd.c
>>>> +++ b/northd/northd.c
>>>> @@ -1457,6 +1457,19 @@ struct ovn_port_routable_addresses {
>>>>      size_t n_addrs;
>>>>  };
>>>>
>>>> +/* A node that maintains link between an object (such as an ovn_port)
>> and
>>>> + * a lflow. */
>>>> +struct lflow_ref_node {
>>>> +    /* This list follows different lflows referenced by the same
>> object. List
>>>> +     * head is, for example, ovn_port->lflows.  */
>>>> +    struct ovs_list lflow_list_node;
>>>> +    /* This list follows different objects that reference the same
>> lflow. List
>>>> +     * head is ovn_lflow->referenced_by. */
>>>> +    struct ovs_list ref_list_node;
>>>> +    /* The lflow. */
>>>> +    struct ovn_lflow *lflow;
>>>> +};
>>>> +
>>>>  /* A logical switch port or logical router port.
>>>>   *
>>>>   * In steady state, an ovn_port points to a northbound
>> Logical_Switch_Port
>>>> @@ -1548,6 +1561,28 @@ struct ovn_port {
>>>>
>>>>      /* Temporarily used for traversing a list (or hmap) of ports. */
>>>>      bool visited;
>>>> +
>>>> +    /* List of struct lflow_ref_node that points to the lflows
>> generated by
>>>> +     * this ovn_port.
>>>> +     *
>>>> +     * This data is initialized and destroyed by the en_northd node,
>> but
>>>> +     * populated and used only by the en_lflow node. Ideally this data
>> should
>>>> +     * be maintained as part of en_lflow's data (struct lflow_data): a
>> hash
>>>> +     * index from ovn_port key to lflows.  However, it would be less
>> efficient
>>>> +     * and more complex:
>>>> +     *
>>>> +     * 1. It would require an extra search (using the index) to find
>> the
>>>> +     * lflows.
>>>> +     *
>>>> +     * 2. Building the index needs to be thread-safe, using either a
>> global
>>>> +     * lock which is obviously less efficient, or hash-based lock
>> array which
>>>> +     * is more complex.
>>>> +     *
>>>> +     * Adding the list here is more straightforward. The drawback is
>> that we
>>>> +     * need to keep in mind that this data belongs to en_lflow node,
>> so never
>>>> +     * access it from any other nodes.
>>>> +     */
>>>> +    struct ovs_list lflows;
>>>>  };
>>>
>>>
>>>>
>>>>  static bool lsp_can_be_inc_processed(const struct
>> nbrec_logical_switch_port *);
>>>> @@ -1635,6 +1670,8 @@ ovn_port_create(struct hmap *ports, const char
>> *key,
>>>>      ovn_port_set_nb(op, nbsp, nbrp);
>>>>      op->l3dgw_port = op->cr_port = NULL;
>>>>      hmap_insert(ports, &op->key_node, hash_string(op->key, 0));
>>>> +
>>>> +    ovs_list_init(&op->lflows);
>>>>      return op;
>>>>  }
>>>>
>>>> @@ -1665,6 +1702,13 @@ ovn_port_destroy_orphan(struct ovn_port *port)
>>>>      destroy_lport_addresses(&port->proxy_arp_addrs);
>>>>      free(port->json_key);
>>>>      free(port->key);
>>>> +
>>>> +    struct lflow_ref_node *l;
>>>> +    LIST_FOR_EACH_SAFE (l, lflow_list_node, &port->lflows) {
>>>> +        ovs_list_remove(&l->lflow_list_node);
>>>> +        ovs_list_remove(&l->ref_list_node);
>>>> +        free(l);
>>>> +    }
>>>>      free(port);
>>>>  }
>>>>
>>>> @@ -4893,6 +4937,7 @@ static struct ovn_port *
>>>>  ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports,
>>>>                 const char *key, const struct nbrec_logical_switch_port
>> *nbsp,
>>>>                 struct ovn_datapath *od, const struct
>> sbrec_port_binding *sb,
>>>> +               struct ovs_list *lflows,
>>>>                 const struct sbrec_mirror_table *sbrec_mirror_table,
>>>>                 const struct sbrec_chassis_table *sbrec_chassis_table,
>>>>                 struct ovsdb_idl_index *sbrec_chassis_by_name,
>>>> @@ -4903,6 +4948,9 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn,
>> struct hmap *ls_ports,
>>>>      parse_lsp_addrs(op);
>>>>      op->od = od;
>>>>      hmap_insert(&od->ports, &op->dp_node,
>> hmap_node_hash(&op->key_node));
>>>> +    if (lflows) {
>>>> +        ovs_list_splice(&op->lflows, lflows->next, lflows);
>>>> +    }
>>>>
>>>>      /* Assign explicitly requested tunnel ids first. */
>>>>      if (!ovn_port_assign_requested_tnl_id(sbrec_chassis_table, op)) {
>>>> @@ -5082,7 +5130,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>>>>                      goto fail;
>>>>                  }
>>>>                  op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
>>>> -                                    new_nbsp->name, new_nbsp, od, NULL,
>>>> +                                    new_nbsp->name, new_nbsp, od,
>> NULL, NULL,
>>>>                                      ni->sbrec_mirror_table,
>>>>                                      ni->sbrec_chassis_table,
>>>>                                      ni->sbrec_chassis_by_name,
>>>> @@ -5114,13 +5162,16 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>>>>                      op->visited = true;
>>>>                      continue;
>>>>                  }
>>>> +                struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows);
>>>> +                ovs_list_splice(&lflows, op->lflows.next, &op->lflows);
>>>>                  ovn_port_destroy(&nd->ls_ports, op);
>>>>                  op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
>>>> -                                    new_nbsp->name, new_nbsp, od, sb,
>>>> +                                    new_nbsp->name, new_nbsp, od, sb,
>> &lflows,
>>>>                                      ni->sbrec_mirror_table,
>>>>                                      ni->sbrec_chassis_table,
>>>>                                      ni->sbrec_chassis_by_name,
>>>>                                      ni->sbrec_chassis_by_hostname);
>>>> +                ovs_assert(ovs_list_is_empty(&lflows));
>>>>                  if (!op) {
>>>>                      goto fail;
>>>>                  }
>>>> @@ -5577,7 +5628,8 @@ ovn_igmp_group_destroy(struct hmap *igmp_groups,
>>>>
>>>>  struct ovn_lflow {
>>>>      struct hmap_node hmap_node;
>>>> -    struct ovs_list list_node;
>>>> +    struct ovs_list list_node;   /* For temporary list of lflows.
>> Don't remove
>>>> +                                    at destroy. */
>>>>
>>>>      struct ovn_datapath *od;     /* 'logical_datapath' in SB schema.
>>  */
>>>>      unsigned long *dpg_bitmap;   /* Bitmap of all datapaths by their
>> 'index'.*/
>>>> @@ -5591,6 +5643,8 @@ struct ovn_lflow {
>>>>      size_t n_ods;                /* Number of datapaths referenced by
>> 'od' and
>>>>                                    * 'dpg_bitmap'. */
>>>>      struct ovn_dp_group *dpg;    /* Link to unique Sb datapath group.
>> */
>>>> +
>>>> +    struct ovs_list referenced_by;  /* List of struct lflow_ref_node.
>> */
>>>>      const char *where;
>>>>
>>>>      struct uuid sb_uuid;            /* SB DB row uuid, specified by
>> northd. */
>>>> @@ -5640,6 +5694,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct
>> ovn_datapath *od,
>>>>                 char *stage_hint, const char *where)
>>>>  {
>>>>      ovs_list_init(&lflow->list_node);
>>>> +    ovs_list_init(&lflow->referenced_by);
>>>>      lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len);
>>>>      lflow->od = od;
>>>>      lflow->stage = stage;
>>>> @@ -5767,20 +5822,30 @@ ovn_dp_group_add_with_reference(struct
>> ovn_lflow *lflow_ref,
>>>>      }
>>>>  }
>>>>
>>>> +/* This global variable collects the lflows generated by
>> do_ovn_lflow_add().
>>>> + * start_collecting_lflows() will enable the lflow collection and the
>> calls to
>>>> + * do_ovn_lflow_add (or the macros ovn_lflow_add_...) will add
>> generated lflows
>>>> + * to the list. end_collecting_lflows() will disable it. */
>>>> +static thread_local struct ovs_list collected_lflows;
>>>> +static thread_local bool collecting_lflows = false;
>>>> +
>>>> +static void
>>>> +start_collecting_lflows(void)
>>>> +{
>>>> +    ovs_assert(!collecting_lflows);
>>>> +    ovs_list_init(&collected_lflows);
>>>> +    collecting_lflows = true;
>>>> +}
>>>> +
>>>> +static void
>>>> +end_collecting_lflows(void)
>>>> +{
>>>> +    ovs_assert(collecting_lflows);
>>>> +    collecting_lflows = false;
>>>> +}
>>>> +
>>>
>>> I think we can avoid these functions and the thread local variable
>>> "collected_lflows".
>>>
>>> I'd suggest the below
>>>
>>> ----------------------------
>>>
>>> static void
>>> do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od,
>>>                  const unsigned long *dp_bitmap, size_t dp_bitmap_len,
>>>                  uint32_t hash, enum ovn_stage stage, uint16_t priority,
>>>                  const char *match, const char *actions, const char
>> *io_port,
>>>                  struct ovs_list *lflow_ref_list,
>>>                  const struct ovsdb_idl_row *stage_hint,
>>>                  const char *where, const char *ctrl_meter)
>>>     OVS_REQUIRES(fake_hash_mutex)
>>> {
>>>     ...
>>>     ...
>>>     /* At the end. */
>>>     if (lflow_ref_list) {
>>>         struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
>>>         lfrn->lflow = lflow;
>>>         ovs_list_insert(lflow_ref_list, &lfrn->lflow_list_node);
>>>         ovs_list_insert(&lflow->referenced_by, &lfrn->ref_list_node);
>>>     }
>>> }
>>>
>>>
>>> #define ovn_lflow_add_with_lport_and_hint(LFLOW_MAP, OD, STAGE, PRIORITY,
>> \
>>>                                           MATCH, ACTIONS, IN_OUT_PORT, \
>>>                                           LFLOW_REF_LIST, STAGE_HINT) \
>>>     ovn_lflow_add_at(LFLOW_MAP, OD, NULL, 0, STAGE, PRIORITY, MATCH,
>> ACTIONS, \
>>>                      IN_OUT_PORT, LFLOW_REF_LIST, NULL, STAGE_HINT, \
>>>                      OVS_SOURCE_LOCATOR)
>>>
>>> And pass &op->lflows in ovn_lflow_add_with_lport_and_hint()
>>>
>>> -----------------------------
>>>
>>> What do you think ?  Definitely this would result in a bit more work
>>> as it would require a lot of (tedious) changes.
>>> I think this is a better approach.
>>>
>> Firstly, I think it is not good to use "lflow_ref_list" directly in the
>> parameter, because there can be more than one object contributing to the
>> lflow generation. When we implement I-P for more inputs, a single lflow may
>> be referenced by multiple objects. We can't pass multiple lflow_ref_list to
>> the function either, because the number of such lists is unknown. For
>> example, a lflow may be generated as a result of a LSP, a DP and a LB
>> backend. If we want to implement I-P for LSP, DP and LB backend, we need to
>> track the reference for all of them. So the current idea is just to collect
>> a list of lflows generated by a higher level function, such as the
>> build_lswitch_and_lrouter_iterate_by_lsp. When implementing I-P for more
>> than one object of the same lflow, this needs to be more fine-grained.

I'm still reviewing this patch but my first impression was that we can
probably try to use 'struct objdep_mgr' (defined in lib/objdep.h) to
model all these (potentially many-to-many) relationships.  We do similar
things in ovn-controller.

> 
> Agree.  I'm working on the I-P for datapath changes in lflow engine and
> as a flow can be referenced by multiple datapaths,  I agree this needs to
> be tracked properly.  And definitely that is out of scope of this patch series.
> 
> 
>>
>> Secondly, I agree with you adding a new parameter to the do_ovn_lflow_add
>> is cleaner. For the collected list I mentioned, it can be a parameter
>> instead of a thread local variable. However, as you mentioned, the change
>> is going to be all over the northd.c code, not only for the
>> ovn_lflow_add_xxx, but also the functions calling the ovn_lflow_add_xxx
>> macros, and upper layer functions calling those functions, and so on.
>> Unfortunately C doesn't support optional args. At this moment I am not sure
>> if the interface is stable enough for the incremental-processing, so I am
>> not sure if it is worth such a big change. If we need to modify them again
>> later, all such changes are going to be wasted. On the other hand, although
>> the thread local variable is not the best way, I think it is still clear
>> and manageable, if we call the start_collecting_lflows and
>> end_collecting_lflows in pairs. So, is it ok to leave it for this patch and
>> in the future when this mechanism proves to work well for more I-P, we can
>> have a separate patch to refactor (which will include all the tedious
>> function call changes). What do you think?
> 
> I agree.  It is definitely tedious to do all the changes.  I'm ok with
> the approach taken
> in this patch series.
> 
> Also request to please take a look at the load balancer I-P patch
> series - http://patchwork.ozlabs.org/project/ovn/list/?series=361083&state=*
> 
> Thanks
> Numan
> 
>>
>> Thanks,
>> Han
>>
>>> Also I'm planning to work on top of your patches to add I-P for load
>>> balancers in lflow engine (or perhaps I-P for datapath changes)
>>>
>>> My idea is to have a lflow ref list stored in "struct ovn_datapath"
>>> similar to the way you have done in this patch in "struct ovn_port"
>>>
>>> And while adding the flows using one of the macro variants
>>> 'ovn_lflow_add*' pass &od->lflows.
>>>
>>> Please let me know your comments.
>>>
>>> Only concern I have with this patch is the "op->lflows" modified by
>>> the lflow engine node.
>>> But I agree with your added comments and also thinking to use the same
>>> approach for datapath I-P handling,
>>> And I don't have a better approach at the moment. So I'm fine with it.
>>>
>>> Thanks
>>> Numan
>>>
>>>
>>>>  /* Adds a row with the specified contents to the Logical_Flow table.
>>>> - * Version to use when hash bucket locking is NOT required.
>>>> - *
>>>> - * Note: This function can add generated lflows to the global variable
>>>> - * temp_lflow_list as its output, controlled by the global variable
>>>> - * add_lflow_to_temp_list. The caller of the ovn_lflow_add_... marcros
>> can get
>>>> - * a list of lflows generated by setting add_lflow_to_temp_list to
>> true. The
>>>> - * caller is responsible for initializing the temp_lflow_list, and also
>>>> - * reset the add_lflow_to_temp_list to false when it is no longer
>> needed.
>>>> - * XXX: this mechanism is temporary and will be replaced when we add
>> hash index
>>>> - * to lflow_data and refactor related functions.
>>>> - */
>>>> -static bool add_lflow_to_temp_list = false;
>>>> -static struct ovs_list temp_lflow_list;
>>>> + * Version to use when hash bucket locking is NOT required. */
>>>>  static void
>>>>  do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od,
>>>>                   const unsigned long *dp_bitmap, size_t dp_bitmap_len,
>>>> @@ -5797,7 +5862,7 @@ do_ovn_lflow_add(struct hmap *lflow_map, const
>> struct ovn_datapath *od,
>>>>      size_t bitmap_len = od ? ods_size(od->datapaths) : dp_bitmap_len;
>>>>      ovs_assert(bitmap_len);
>>>>
>>>> -    if (add_lflow_to_temp_list) {
>>>> +    if (collecting_lflows) {
>>>>          ovs_assert(od);
>>>>          ovs_assert(!dp_bitmap);
>>>>      } else {
>>>> @@ -5829,8 +5894,8 @@ do_ovn_lflow_add(struct hmap *lflow_map, const
>> struct ovn_datapath *od,
>>>>          thread_lflow_counter++;
>>>>      }
>>>>
>>>> -    if (add_lflow_to_temp_list) {
>>>> -        ovs_list_insert(&temp_lflow_list, &lflow->list_node);
>>>> +    if (collecting_lflows) {
>>>> +        ovs_list_insert(&collected_lflows, &lflow->list_node);
>>>>      }
>>>>  }
>>>>
>>>> @@ -5950,10 +6015,28 @@ ovn_lflow_destroy(struct hmap *lflows, struct
>> ovn_lflow *lflow)
>>>>          free(lflow->io_port);
>>>>          free(lflow->stage_hint);
>>>>          free(lflow->ctrl_meter);
>>>> +        struct lflow_ref_node *l;
>>>> +        LIST_FOR_EACH_SAFE (l, ref_list_node, &lflow->referenced_by) {
>>>> +            ovs_list_remove(&l->lflow_list_node);
>>>> +            ovs_list_remove(&l->ref_list_node);
>>>> +            free(l);
>>>> +        }
>>>>          free(lflow);
>>>>      }
>>>>  }
>>>>
>>>> +static void
>>>> +link_ovn_port_to_lflows(struct ovn_port *op, struct ovs_list *lflows)
>>>> +{
>>>> +    struct ovn_lflow *f;
>>>> +    LIST_FOR_EACH (f, list_node, lflows) {
>>>> +        struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
>>>> +        lfrn->lflow = f;
>>>> +        ovs_list_insert(&op->lflows, &lfrn->lflow_list_node);
>>>> +        ovs_list_insert(&f->referenced_by, &lfrn->ref_list_node);
>>>> +    }
>>>> +}
>>>> +
>>>>  static bool
>>>>  build_dhcpv4_action(struct ovn_port *op, ovs_be32 offer_ip,
>>>>                      struct ds *options_action, struct ds
>> *response_action,
>>>> @@ -15483,6 +15566,7 @@ build_lswitch_and_lrouter_iterate_by_lsp(struct
>> ovn_port *op,
>>>>                                           struct hmap *lflows)
>>>>  {
>>>>      ovs_assert(op->nbsp);
>>>> +    start_collecting_lflows();
>>>>
>>>>      /* Build Logical Switch Flows. */
>>>>      build_lswitch_port_sec_op(op, lflows, actions, match);
>>>> @@ -15497,6 +15581,9 @@ build_lswitch_and_lrouter_iterate_by_lsp(struct
>> ovn_port *op,
>>>>      /* Build Logical Router Flows. */
>>>>      build_ip_routing_flows_for_router_type_lsp(op, lr_ports, lflows);
>>>>      build_arp_resolve_flows_for_lsp(op, lflows, lr_ports, match,
>> actions);
>>>> +
>>>> +    link_ovn_port_to_lflows(op, &collected_lflows);
>>>> +    end_collecting_lflows();
>>>>  }
>>>>
>>>>  /* Helper function to combine all lflow generation which is iterated
>> by logical
>>>> @@ -16223,14 +16310,10 @@ bool lflow_handle_northd_ls_changes(struct
>> ovsdb_idl_txn *ovnsb_txn,
>>>>  {
>>>>      struct ls_change *ls_change;
>>>>      LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
>>>> -        ovs_list_init(&temp_lflow_list);
>>>> -        add_lflow_to_temp_list = true;
>>>>          if (!ovs_list_is_empty(&ls_change->updated_ports) ||
>>>>              !ovs_list_is_empty(&ls_change->deleted_ports)) {
>>>>              /* XXX: implement lflow index so that we can handle
>> updated and
>>>>               * deleted LSPs incrementally. */
>>>> -            ovs_list_init(&temp_lflow_list);
>>>> -            add_lflow_to_temp_list = false;
>>>>              return false;
>>>>          }
>>>>
>>>> @@ -16277,83 +16360,85 @@ bool lflow_handle_northd_ls_changes(struct
>> ovsdb_idl_txn *ovnsb_txn,
>>>>
>>  sbrec_multicast_group_update_ports_addvalue(sbmc_unknown,
>>>>                                                              op->sb);
>>>>              }
>>>> -        }
>>>> -        /* Sync the newly added flows to SB. */
>>>> -        struct ovn_lflow *lflow;
>>>> -        LIST_FOR_EACH (lflow, list_node, &temp_lflow_list) {
>>>> -            size_t n_datapaths;
>>>> -            struct ovn_datapath **datapaths_array;
>>>> -            if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH)
>> {
>>>> -                n_datapaths = ods_size(lflow_input->ls_datapaths);
>>>> -                datapaths_array = lflow_input->ls_datapaths->array;
>>>> -            } else {
>>>> -                n_datapaths = ods_size(lflow_input->lr_datapaths);
>>>> -                datapaths_array = lflow_input->lr_datapaths->array;
>>>> -            }
>>>> -            uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap,
>> n_datapaths);
>>>> -            ovs_assert(n_ods == 1);
>>>> -            /* There is only one datapath, so it should be moved out
>> of the
>>>> -             * group to a single 'od'. */
>>>> -            size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0,
>>>> -                                       n_datapaths);
>>>>
>>>> -            bitmap_set0(lflow->dpg_bitmap, index);
>>>> -            lflow->od = datapaths_array[index];
>>>> -
>>>> -            /* Logical flow should be re-hashed to allow lookups. */
>>>> -            uint32_t hash = hmap_node_hash(&lflow->hmap_node);
>>>> -            /* Remove from lflows. */
>>>> -            hmap_remove(lflows, &lflow->hmap_node);
>>>> -            hash =
>> ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid,
>>>> -                                                  hash);
>>>> -            /* Add back. */
>>>> -            hmap_insert(lflows, &lflow->hmap_node, hash);
>>>> -
>>>> -            /* Sync to SB. */
>>>> -            const struct sbrec_logical_flow *sbflow;
>>>> -            lflow->sb_uuid = uuid_random();
>>>> -            sbflow = sbrec_logical_flow_insert_persist_uuid(ovnsb_txn,
>>>> -
>>  &lflow->sb_uuid);
>>>> -            const char *pipeline =
>> ovn_stage_get_pipeline_name(lflow->stage);
>>>> -            uint8_t table = ovn_stage_get_table(lflow->stage);
>>>> -            sbrec_logical_flow_set_logical_datapath(sbflow,
>> lflow->od->sb);
>>>> -            sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
>>>> -            sbrec_logical_flow_set_pipeline(sbflow, pipeline);
>>>> -            sbrec_logical_flow_set_table_id(sbflow, table);
>>>> -            sbrec_logical_flow_set_priority(sbflow, lflow->priority);
>>>> -            sbrec_logical_flow_set_match(sbflow, lflow->match);
>>>> -            sbrec_logical_flow_set_actions(sbflow, lflow->actions);
>>>> -            if (lflow->io_port) {
>>>> -                struct smap tags = SMAP_INITIALIZER(&tags);
>>>> -                smap_add(&tags, "in_out_port", lflow->io_port);
>>>> -                sbrec_logical_flow_set_tags(sbflow, &tags);
>>>> -                smap_destroy(&tags);
>>>> -            }
>>>> -            sbrec_logical_flow_set_controller_meter(sbflow,
>> lflow->ctrl_meter);
>>>> -            /* Trim the source locator lflow->where, which looks
>> something like
>>>> -             * "ovn/northd/northd.c:1234", down to just the part
>> following the
>>>> -             * last slash, e.g. "northd.c:1234". */
>>>> -            const char *slash = strrchr(lflow->where, '/');
>>>> +            /* Sync the newly added flows to SB. */
>>>> +            struct lflow_ref_node *lfrn;
>>>> +            LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
>>>> +                struct ovn_lflow *lflow = lfrn->lflow;
>>>> +                size_t n_datapaths;
>>>> +                struct ovn_datapath **datapaths_array;
>>>> +                if (ovn_stage_to_datapath_type(lflow->stage) ==
>> DP_SWITCH) {
>>>> +                    n_datapaths = ods_size(lflow_input->ls_datapaths);
>>>> +                    datapaths_array = lflow_input->ls_datapaths->array;
>>>> +                } else {
>>>> +                    n_datapaths = ods_size(lflow_input->lr_datapaths);
>>>> +                    datapaths_array = lflow_input->lr_datapaths->array;
>>>> +                }
>>>> +                uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap,
>> n_datapaths);
>>>> +                ovs_assert(n_ods == 1);
>>>> +                /* There is only one datapath, so it should be moved
>> out of the
>>>> +                 * group to a single 'od'. */
>>>> +                size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0,
>>>> +                                           n_datapaths);
>>>> +
>>>> +                bitmap_set0(lflow->dpg_bitmap, index);
>>>> +                lflow->od = datapaths_array[index];
>>>> +
>>>> +                /* Logical flow should be re-hashed to allow lookups.
>> */
>>>> +                uint32_t hash = hmap_node_hash(&lflow->hmap_node);
>>>> +                /* Remove from lflows. */
>>>> +                hmap_remove(lflows, &lflow->hmap_node);
>>>> +                hash = ovn_logical_flow_hash_datapath(
>>>> +
>>  &lflow->od->sb->header_.uuid, hash);
>>>> +                /* Add back. */
>>>> +                hmap_insert(lflows, &lflow->hmap_node, hash);
>>>> +
>>>> +                /* Sync to SB. */
>>>> +                const struct sbrec_logical_flow *sbflow;
>>>> +                lflow->sb_uuid = uuid_random();
>>>> +                sbflow = sbrec_logical_flow_insert_persist_uuid(
>>>> +                                                ovnsb_txn,
>> &lflow->sb_uuid);
>>>> +                const char *pipeline = ovn_stage_get_pipeline_name(
>>>> +
>> lflow->stage);
>>>> +                uint8_t table = ovn_stage_get_table(lflow->stage);
>>>> +                sbrec_logical_flow_set_logical_datapath(sbflow,
>> lflow->od->sb);
>>>> +                sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
>>>> +                sbrec_logical_flow_set_pipeline(sbflow, pipeline);
>>>> +                sbrec_logical_flow_set_table_id(sbflow, table);
>>>> +                sbrec_logical_flow_set_priority(sbflow,
>> lflow->priority);
>>>> +                sbrec_logical_flow_set_match(sbflow, lflow->match);
>>>> +                sbrec_logical_flow_set_actions(sbflow, lflow->actions);
>>>> +                if (lflow->io_port) {
>>>> +                    struct smap tags = SMAP_INITIALIZER(&tags);
>>>> +                    smap_add(&tags, "in_out_port", lflow->io_port);
>>>> +                    sbrec_logical_flow_set_tags(sbflow, &tags);
>>>> +                    smap_destroy(&tags);
>>>> +                }
>>>> +                sbrec_logical_flow_set_controller_meter(sbflow,
>>>> +
>>  lflow->ctrl_meter);
>>>> +                /* Trim the source locator lflow->where, which looks
>> something
>>>> +                 * like "ovn/northd/northd.c:1234", down to just the
>> part
>>>> +                 * following the last slash, e.g. "northd.c:1234". */
>>>> +                const char *slash = strrchr(lflow->where, '/');
>>>>  #if _WIN32
>>>> -            const char *backslash = strrchr(lflow->where, '\\');
>>>> -            if (!slash || backslash > slash) {
>>>> -                slash = backslash;
>>>> -            }
>>>> +                const char *backslash = strrchr(lflow->where, '\\');
>>>> +                if (!slash || backslash > slash) {
>>>> +                    slash = backslash;
>>>> +                }
>>>>  #endif
>>>> -            const char *where = slash ? slash + 1 : lflow->where;
>>>> +                const char *where = slash ? slash + 1 : lflow->where;
>>>>
>>>> -            struct smap ids = SMAP_INITIALIZER(&ids);
>>>> -            smap_add(&ids, "stage-name",
>> ovn_stage_to_str(lflow->stage));
>>>> -            smap_add(&ids, "source", where);
>>>> -            if (lflow->stage_hint) {
>>>> -                smap_add(&ids, "stage-hint", lflow->stage_hint);
>>>> +                struct smap ids = SMAP_INITIALIZER(&ids);
>>>> +                smap_add(&ids, "stage-name",
>> ovn_stage_to_str(lflow->stage));
>>>> +                smap_add(&ids, "source", where);
>>>> +                if (lflow->stage_hint) {
>>>> +                    smap_add(&ids, "stage-hint", lflow->stage_hint);
>>>> +                }
>>>> +                sbrec_logical_flow_set_external_ids(sbflow, &ids);
>>>> +                smap_destroy(&ids);
>>>>              }
>>>> -            sbrec_logical_flow_set_external_ids(sbflow, &ids);
>>>> -            smap_destroy(&ids);
>>>>          }
>>>>      }
>>>> -    ovs_list_init(&temp_lflow_list);
>>>> -    add_lflow_to_temp_list = false;
>>>>      return true;
>>>>
>>>>  }
>>>> --
>>>> 2.30.2
>>>>
>>>> _______________________________________________
>>>> dev mailing list
>>>> dev@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Thu, Jun 29, 2023 at 9:19 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 6/27/23 10:23, Numan Siddique wrote:
> > On Mon, Jun 26, 2023 at 10:34 PM Han Zhou <hzhou@ovn.org> wrote:
> >>
> >> On Mon, Jun 26, 2023 at 7:25 AM Numan Siddique <numans@ovn.org> wrote:
> >>>
> >>> On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hzhou@ovn.org> wrote:
> >>>>
> >>>> For incremental processing, it is important to maintain relationship
> >>>> between the inputs and the logical flows generated. This patch
creates
> >>>> the links between ovn_port and logical flows. The same data structure
> >>>> may be expanded to maintain links between logical flows and other
types
> >>>> of inputs.
> >>>>
> >>>> This patch also refactors the temp_lflow_list operations to
> >>>> collected_lflows with helper functions to start and end collecting.
It
> >>>> still uses global variables just to avoid updating all the
lflow_add_...
> >>>> related code all over the northd.c file.
> >>>>
> >>>> Signed-off-by: Han Zhou <hzhou@ovn.org>
> >>>
> >>> Hi Han,
> >>>
> >>> Please see a few comments below.  I did review all the 3 patches in
the
> >> series.
> >>> They LGTM overall.  I'd like to do some more testing before providing
my
> >> Acks.
> >>>
> >>
> >> Thanks for your review!
> >>
> >>>
> >>>> ---
> >>>>  northd/northd.c | 271
+++++++++++++++++++++++++++++++-----------------
> >>>>  1 file changed, 178 insertions(+), 93 deletions(-)
> >>>>
> >>>> diff --git a/northd/northd.c b/northd/northd.c
> >>>> index 98f528f93cfc..aa0f853ce2db 100644
> >>>> --- a/northd/northd.c
> >>>> +++ b/northd/northd.c
> >>>> @@ -1457,6 +1457,19 @@ struct ovn_port_routable_addresses {
> >>>>      size_t n_addrs;
> >>>>  };
> >>>>
> >>>> +/* A node that maintains link between an object (such as an
ovn_port)
> >> and
> >>>> + * a lflow. */
> >>>> +struct lflow_ref_node {
> >>>> +    /* This list follows different lflows referenced by the same
> >> object. List
> >>>> +     * head is, for example, ovn_port->lflows.  */
> >>>> +    struct ovs_list lflow_list_node;
> >>>> +    /* This list follows different objects that reference the same
> >> lflow. List
> >>>> +     * head is ovn_lflow->referenced_by. */
> >>>> +    struct ovs_list ref_list_node;
> >>>> +    /* The lflow. */
> >>>> +    struct ovn_lflow *lflow;
> >>>> +};
> >>>> +
> >>>>  /* A logical switch port or logical router port.
> >>>>   *
> >>>>   * In steady state, an ovn_port points to a northbound
> >> Logical_Switch_Port
> >>>> @@ -1548,6 +1561,28 @@ struct ovn_port {
> >>>>
> >>>>      /* Temporarily used for traversing a list (or hmap) of ports. */
> >>>>      bool visited;
> >>>> +
> >>>> +    /* List of struct lflow_ref_node that points to the lflows
> >> generated by
> >>>> +     * this ovn_port.
> >>>> +     *
> >>>> +     * This data is initialized and destroyed by the en_northd node,
> >> but
> >>>> +     * populated and used only by the en_lflow node. Ideally this
data
> >> should
> >>>> +     * be maintained as part of en_lflow's data (struct
lflow_data): a
> >> hash
> >>>> +     * index from ovn_port key to lflows.  However, it would be less
> >> efficient
> >>>> +     * and more complex:
> >>>> +     *
> >>>> +     * 1. It would require an extra search (using the index) to find
> >> the
> >>>> +     * lflows.
> >>>> +     *
> >>>> +     * 2. Building the index needs to be thread-safe, using either a
> >> global
> >>>> +     * lock which is obviously less efficient, or hash-based lock
> >> array which
> >>>> +     * is more complex.
> >>>> +     *
> >>>> +     * Adding the list here is more straightforward. The drawback is
> >> that we
> >>>> +     * need to keep in mind that this data belongs to en_lflow node,
> >> so never
> >>>> +     * access it from any other nodes.
> >>>> +     */
> >>>> +    struct ovs_list lflows;
> >>>>  };
> >>>
> >>>
> >>>>
> >>>>  static bool lsp_can_be_inc_processed(const struct
> >> nbrec_logical_switch_port *);
> >>>> @@ -1635,6 +1670,8 @@ ovn_port_create(struct hmap *ports, const char
> >> *key,
> >>>>      ovn_port_set_nb(op, nbsp, nbrp);
> >>>>      op->l3dgw_port = op->cr_port = NULL;
> >>>>      hmap_insert(ports, &op->key_node, hash_string(op->key, 0));
> >>>> +
> >>>> +    ovs_list_init(&op->lflows);
> >>>>      return op;
> >>>>  }
> >>>>
> >>>> @@ -1665,6 +1702,13 @@ ovn_port_destroy_orphan(struct ovn_port *port)
> >>>>      destroy_lport_addresses(&port->proxy_arp_addrs);
> >>>>      free(port->json_key);
> >>>>      free(port->key);
> >>>> +
> >>>> +    struct lflow_ref_node *l;
> >>>> +    LIST_FOR_EACH_SAFE (l, lflow_list_node, &port->lflows) {
> >>>> +        ovs_list_remove(&l->lflow_list_node);
> >>>> +        ovs_list_remove(&l->ref_list_node);
> >>>> +        free(l);
> >>>> +    }
> >>>>      free(port);
> >>>>  }
> >>>>
> >>>> @@ -4893,6 +4937,7 @@ static struct ovn_port *
> >>>>  ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap
*ls_ports,
> >>>>                 const char *key, const struct
nbrec_logical_switch_port
> >> *nbsp,
> >>>>                 struct ovn_datapath *od, const struct
> >> sbrec_port_binding *sb,
> >>>> +               struct ovs_list *lflows,
> >>>>                 const struct sbrec_mirror_table *sbrec_mirror_table,
> >>>>                 const struct sbrec_chassis_table
*sbrec_chassis_table,
> >>>>                 struct ovsdb_idl_index *sbrec_chassis_by_name,
> >>>> @@ -4903,6 +4948,9 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn,
> >> struct hmap *ls_ports,
> >>>>      parse_lsp_addrs(op);
> >>>>      op->od = od;
> >>>>      hmap_insert(&od->ports, &op->dp_node,
> >> hmap_node_hash(&op->key_node));
> >>>> +    if (lflows) {
> >>>> +        ovs_list_splice(&op->lflows, lflows->next, lflows);
> >>>> +    }
> >>>>
> >>>>      /* Assign explicitly requested tunnel ids first. */
> >>>>      if (!ovn_port_assign_requested_tnl_id(sbrec_chassis_table, op))
{
> >>>> @@ -5082,7 +5130,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
> >> *ovnsb_idl_txn,
> >>>>                      goto fail;
> >>>>                  }
> >>>>                  op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> >>>> -                                    new_nbsp->name, new_nbsp, od,
NULL,
> >>>> +                                    new_nbsp->name, new_nbsp, od,
> >> NULL, NULL,
> >>>>                                      ni->sbrec_mirror_table,
> >>>>                                      ni->sbrec_chassis_table,
> >>>>                                      ni->sbrec_chassis_by_name,
> >>>> @@ -5114,13 +5162,16 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
> >> *ovnsb_idl_txn,
> >>>>                      op->visited = true;
> >>>>                      continue;
> >>>>                  }
> >>>> +                struct ovs_list lflows =
OVS_LIST_INITIALIZER(&lflows);
> >>>> +                ovs_list_splice(&lflows, op->lflows.next,
&op->lflows);
> >>>>                  ovn_port_destroy(&nd->ls_ports, op);
> >>>>                  op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> >>>> -                                    new_nbsp->name, new_nbsp, od,
sb,
> >>>> +                                    new_nbsp->name, new_nbsp, od,
sb,
> >> &lflows,
> >>>>                                      ni->sbrec_mirror_table,
> >>>>                                      ni->sbrec_chassis_table,
> >>>>                                      ni->sbrec_chassis_by_name,
> >>>>                                      ni->sbrec_chassis_by_hostname);
> >>>> +                ovs_assert(ovs_list_is_empty(&lflows));
> >>>>                  if (!op) {
> >>>>                      goto fail;
> >>>>                  }
> >>>> @@ -5577,7 +5628,8 @@ ovn_igmp_group_destroy(struct hmap
*igmp_groups,
> >>>>
> >>>>  struct ovn_lflow {
> >>>>      struct hmap_node hmap_node;
> >>>> -    struct ovs_list list_node;
> >>>> +    struct ovs_list list_node;   /* For temporary list of lflows.
> >> Don't remove
> >>>> +                                    at destroy. */
> >>>>
> >>>>      struct ovn_datapath *od;     /* 'logical_datapath' in SB schema.
> >>  */
> >>>>      unsigned long *dpg_bitmap;   /* Bitmap of all datapaths by their
> >> 'index'.*/
> >>>> @@ -5591,6 +5643,8 @@ struct ovn_lflow {
> >>>>      size_t n_ods;                /* Number of datapaths referenced
by
> >> 'od' and
> >>>>                                    * 'dpg_bitmap'. */
> >>>>      struct ovn_dp_group *dpg;    /* Link to unique Sb datapath
group.
> >> */
> >>>> +
> >>>> +    struct ovs_list referenced_by;  /* List of struct
lflow_ref_node.
> >> */
> >>>>      const char *where;
> >>>>
> >>>>      struct uuid sb_uuid;            /* SB DB row uuid, specified by
> >> northd. */
> >>>> @@ -5640,6 +5694,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct
> >> ovn_datapath *od,
> >>>>                 char *stage_hint, const char *where)
> >>>>  {
> >>>>      ovs_list_init(&lflow->list_node);
> >>>> +    ovs_list_init(&lflow->referenced_by);
> >>>>      lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len);
> >>>>      lflow->od = od;
> >>>>      lflow->stage = stage;
> >>>> @@ -5767,20 +5822,30 @@ ovn_dp_group_add_with_reference(struct
> >> ovn_lflow *lflow_ref,
> >>>>      }
> >>>>  }
> >>>>
> >>>> +/* This global variable collects the lflows generated by
> >> do_ovn_lflow_add().
> >>>> + * start_collecting_lflows() will enable the lflow collection and
the
> >> calls to
> >>>> + * do_ovn_lflow_add (or the macros ovn_lflow_add_...) will add
> >> generated lflows
> >>>> + * to the list. end_collecting_lflows() will disable it. */
> >>>> +static thread_local struct ovs_list collected_lflows;
> >>>> +static thread_local bool collecting_lflows = false;
> >>>> +
> >>>> +static void
> >>>> +start_collecting_lflows(void)
> >>>> +{
> >>>> +    ovs_assert(!collecting_lflows);
> >>>> +    ovs_list_init(&collected_lflows);
> >>>> +    collecting_lflows = true;
> >>>> +}
> >>>> +
> >>>> +static void
> >>>> +end_collecting_lflows(void)
> >>>> +{
> >>>> +    ovs_assert(collecting_lflows);
> >>>> +    collecting_lflows = false;
> >>>> +}
> >>>> +
> >>>
> >>> I think we can avoid these functions and the thread local variable
> >>> "collected_lflows".
> >>>
> >>> I'd suggest the below
> >>>
> >>> ----------------------------
> >>>
> >>> static void
> >>> do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath
*od,
> >>>                  const unsigned long *dp_bitmap, size_t dp_bitmap_len,
> >>>                  uint32_t hash, enum ovn_stage stage, uint16_t
priority,
> >>>                  const char *match, const char *actions, const char
> >> *io_port,
> >>>                  struct ovs_list *lflow_ref_list,
> >>>                  const struct ovsdb_idl_row *stage_hint,
> >>>                  const char *where, const char *ctrl_meter)
> >>>     OVS_REQUIRES(fake_hash_mutex)
> >>> {
> >>>     ...
> >>>     ...
> >>>     /* At the end. */
> >>>     if (lflow_ref_list) {
> >>>         struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
> >>>         lfrn->lflow = lflow;
> >>>         ovs_list_insert(lflow_ref_list, &lfrn->lflow_list_node);
> >>>         ovs_list_insert(&lflow->referenced_by, &lfrn->ref_list_node);
> >>>     }
> >>> }
> >>>
> >>>
> >>> #define ovn_lflow_add_with_lport_and_hint(LFLOW_MAP, OD, STAGE,
PRIORITY,
> >> \
> >>>                                           MATCH, ACTIONS,
IN_OUT_PORT, \
> >>>                                           LFLOW_REF_LIST, STAGE_HINT)
\
> >>>     ovn_lflow_add_at(LFLOW_MAP, OD, NULL, 0, STAGE, PRIORITY, MATCH,
> >> ACTIONS, \
> >>>                      IN_OUT_PORT, LFLOW_REF_LIST, NULL, STAGE_HINT, \
> >>>                      OVS_SOURCE_LOCATOR)
> >>>
> >>> And pass &op->lflows in ovn_lflow_add_with_lport_and_hint()
> >>>
> >>> -----------------------------
> >>>
> >>> What do you think ?  Definitely this would result in a bit more work
> >>> as it would require a lot of (tedious) changes.
> >>> I think this is a better approach.
> >>>
> >> Firstly, I think it is not good to use "lflow_ref_list" directly in the
> >> parameter, because there can be more than one object contributing to
the
> >> lflow generation. When we implement I-P for more inputs, a single
lflow may
> >> be referenced by multiple objects. We can't pass multiple
lflow_ref_list to
> >> the function either, because the number of such lists is unknown. For
> >> example, a lflow may be generated as a result of a LSP, a DP and a LB
> >> backend. If we want to implement I-P for LSP, DP and LB backend, we
need to
> >> track the reference for all of them. So the current idea is just to
collect
> >> a list of lflows generated by a higher level function, such as the
> >> build_lswitch_and_lrouter_iterate_by_lsp. When implementing I-P for
more
> >> than one object of the same lflow, this needs to be more fine-grained.
>
> I'm still reviewing this patch but my first impression was that we can
> probably try to use 'struct objdep_mgr' (defined in lib/objdep.h) to
> model all these (potentially many-to-many) relationships.  We do similar
> things in ovn-controller.
>
My first impression was similar, but later I figured out it is different
(and more complex than ovn-controller in general) and we will need to make
changes to objdep_mgr. However, until more input I-P is implemented I am
not sure if I will be able to get a general enough abstraction in
objdep_mgr that can handle all the use cases. On the other hand, the
references between LSP and related lflows are quite simple, so I went ahead
and implemented the lists (embeded into ovn_port and lflow without extra
hash tables), with minimum effort, to make it work at least for this
scenario and should also work for similar scenarios. I think we can
refactor the reference implementation any time but it may be better to get
more I-P implemented, during the process we can always revisit and see if
objdep_mgr can be extended or another common dependency/reference library
for ovn-northd can be created, etc. What do you think?

Regards,
Han

> >
> > Agree.  I'm working on the I-P for datapath changes in lflow engine and
> > as a flow can be referenced by multiple datapaths,  I agree this needs
to
> > be tracked properly.  And definitely that is out of scope of this patch
series.
> >
> >
> >>
> >> Secondly, I agree with you adding a new parameter to the
do_ovn_lflow_add
> >> is cleaner. For the collected list I mentioned, it can be a parameter
> >> instead of a thread local variable. However, as you mentioned, the
change
> >> is going to be all over the northd.c code, not only for the
> >> ovn_lflow_add_xxx, but also the functions calling the ovn_lflow_add_xxx
> >> macros, and upper layer functions calling those functions, and so on.
> >> Unfortunately C doesn't support optional args. At this moment I am not
sure
> >> if the interface is stable enough for the incremental-processing, so I
am
> >> not sure if it is worth such a big change. If we need to modify them
again
> >> later, all such changes are going to be wasted. On the other hand,
although
> >> the thread local variable is not the best way, I think it is still
clear
> >> and manageable, if we call the start_collecting_lflows and
> >> end_collecting_lflows in pairs. So, is it ok to leave it for this
patch and
> >> in the future when this mechanism proves to work well for more I-P, we
can
> >> have a separate patch to refactor (which will include all the tedious
> >> function call changes). What do you think?
> >
> > I agree.  It is definitely tedious to do all the changes.  I'm ok with
> > the approach taken
> > in this patch series.
> >
> > Also request to please take a look at the load balancer I-P patch
> > series -
http://patchwork.ozlabs.org/project/ovn/list/?series=361083&state=*
> >
> > Thanks
> > Numan
> >
> >>
> >> Thanks,
> >> Han
> >>
> >>> Also I'm planning to work on top of your patches to add I-P for load
> >>> balancers in lflow engine (or perhaps I-P for datapath changes)
> >>>
> >>> My idea is to have a lflow ref list stored in "struct ovn_datapath"
> >>> similar to the way you have done in this patch in "struct ovn_port"
> >>>
> >>> And while adding the flows using one of the macro variants
> >>> 'ovn_lflow_add*' pass &od->lflows.
> >>>
> >>> Please let me know your comments.
> >>>
> >>> Only concern I have with this patch is the "op->lflows" modified by
> >>> the lflow engine node.
> >>> But I agree with your added comments and also thinking to use the same
> >>> approach for datapath I-P handling,
> >>> And I don't have a better approach at the moment. So I'm fine with it.
> >>>
> >>> Thanks
> >>> Numan
> >>>
> >>>
> >>>>  /* Adds a row with the specified contents to the Logical_Flow table.
> >>>> - * Version to use when hash bucket locking is NOT required.
> >>>> - *
> >>>> - * Note: This function can add generated lflows to the global
variable
> >>>> - * temp_lflow_list as its output, controlled by the global variable
> >>>> - * add_lflow_to_temp_list. The caller of the ovn_lflow_add_...
marcros
> >> can get
> >>>> - * a list of lflows generated by setting add_lflow_to_temp_list to
> >> true. The
> >>>> - * caller is responsible for initializing the temp_lflow_list, and
also
> >>>> - * reset the add_lflow_to_temp_list to false when it is no longer
> >> needed.
> >>>> - * XXX: this mechanism is temporary and will be replaced when we add
> >> hash index
> >>>> - * to lflow_data and refactor related functions.
> >>>> - */
> >>>> -static bool add_lflow_to_temp_list = false;
> >>>> -static struct ovs_list temp_lflow_list;
> >>>> + * Version to use when hash bucket locking is NOT required. */
> >>>>  static void
> >>>>  do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath
*od,
> >>>>                   const unsigned long *dp_bitmap, size_t
dp_bitmap_len,
> >>>> @@ -5797,7 +5862,7 @@ do_ovn_lflow_add(struct hmap *lflow_map, const
> >> struct ovn_datapath *od,
> >>>>      size_t bitmap_len = od ? ods_size(od->datapaths) :
dp_bitmap_len;
> >>>>      ovs_assert(bitmap_len);
> >>>>
> >>>> -    if (add_lflow_to_temp_list) {
> >>>> +    if (collecting_lflows) {
> >>>>          ovs_assert(od);
> >>>>          ovs_assert(!dp_bitmap);
> >>>>      } else {
> >>>> @@ -5829,8 +5894,8 @@ do_ovn_lflow_add(struct hmap *lflow_map, const
> >> struct ovn_datapath *od,
> >>>>          thread_lflow_counter++;
> >>>>      }
> >>>>
> >>>> -    if (add_lflow_to_temp_list) {
> >>>> -        ovs_list_insert(&temp_lflow_list, &lflow->list_node);
> >>>> +    if (collecting_lflows) {
> >>>> +        ovs_list_insert(&collected_lflows, &lflow->list_node);
> >>>>      }
> >>>>  }
> >>>>
> >>>> @@ -5950,10 +6015,28 @@ ovn_lflow_destroy(struct hmap *lflows, struct
> >> ovn_lflow *lflow)
> >>>>          free(lflow->io_port);
> >>>>          free(lflow->stage_hint);
> >>>>          free(lflow->ctrl_meter);
> >>>> +        struct lflow_ref_node *l;
> >>>> +        LIST_FOR_EACH_SAFE (l, ref_list_node,
&lflow->referenced_by) {
> >>>> +            ovs_list_remove(&l->lflow_list_node);
> >>>> +            ovs_list_remove(&l->ref_list_node);
> >>>> +            free(l);
> >>>> +        }
> >>>>          free(lflow);
> >>>>      }
> >>>>  }
> >>>>
> >>>> +static void
> >>>> +link_ovn_port_to_lflows(struct ovn_port *op, struct ovs_list
*lflows)
> >>>> +{
> >>>> +    struct ovn_lflow *f;
> >>>> +    LIST_FOR_EACH (f, list_node, lflows) {
> >>>> +        struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
> >>>> +        lfrn->lflow = f;
> >>>> +        ovs_list_insert(&op->lflows, &lfrn->lflow_list_node);
> >>>> +        ovs_list_insert(&f->referenced_by, &lfrn->ref_list_node);
> >>>> +    }
> >>>> +}
> >>>> +
> >>>>  static bool
> >>>>  build_dhcpv4_action(struct ovn_port *op, ovs_be32 offer_ip,
> >>>>                      struct ds *options_action, struct ds
> >> *response_action,
> >>>> @@ -15483,6 +15566,7 @@
build_lswitch_and_lrouter_iterate_by_lsp(struct
> >> ovn_port *op,
> >>>>                                           struct hmap *lflows)
> >>>>  {
> >>>>      ovs_assert(op->nbsp);
> >>>> +    start_collecting_lflows();
> >>>>
> >>>>      /* Build Logical Switch Flows. */
> >>>>      build_lswitch_port_sec_op(op, lflows, actions, match);
> >>>> @@ -15497,6 +15581,9 @@
build_lswitch_and_lrouter_iterate_by_lsp(struct
> >> ovn_port *op,
> >>>>      /* Build Logical Router Flows. */
> >>>>      build_ip_routing_flows_for_router_type_lsp(op, lr_ports,
lflows);
> >>>>      build_arp_resolve_flows_for_lsp(op, lflows, lr_ports, match,
> >> actions);
> >>>> +
> >>>> +    link_ovn_port_to_lflows(op, &collected_lflows);
> >>>> +    end_collecting_lflows();
> >>>>  }
> >>>>
> >>>>  /* Helper function to combine all lflow generation which is iterated
> >> by logical
> >>>> @@ -16223,14 +16310,10 @@ bool lflow_handle_northd_ls_changes(struct
> >> ovsdb_idl_txn *ovnsb_txn,
> >>>>  {
> >>>>      struct ls_change *ls_change;
> >>>>      LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
> >>>> -        ovs_list_init(&temp_lflow_list);
> >>>> -        add_lflow_to_temp_list = true;
> >>>>          if (!ovs_list_is_empty(&ls_change->updated_ports) ||
> >>>>              !ovs_list_is_empty(&ls_change->deleted_ports)) {
> >>>>              /* XXX: implement lflow index so that we can handle
> >> updated and
> >>>>               * deleted LSPs incrementally. */
> >>>> -            ovs_list_init(&temp_lflow_list);
> >>>> -            add_lflow_to_temp_list = false;
> >>>>              return false;
> >>>>          }
> >>>>
> >>>> @@ -16277,83 +16360,85 @@ bool lflow_handle_northd_ls_changes(struct
> >> ovsdb_idl_txn *ovnsb_txn,
> >>>>
> >>  sbrec_multicast_group_update_ports_addvalue(sbmc_unknown,
> >>>>                                                              op->sb);
> >>>>              }
> >>>> -        }
> >>>> -        /* Sync the newly added flows to SB. */
> >>>> -        struct ovn_lflow *lflow;
> >>>> -        LIST_FOR_EACH (lflow, list_node, &temp_lflow_list) {
> >>>> -            size_t n_datapaths;
> >>>> -            struct ovn_datapath **datapaths_array;
> >>>> -            if (ovn_stage_to_datapath_type(lflow->stage) ==
DP_SWITCH)
> >> {
> >>>> -                n_datapaths = ods_size(lflow_input->ls_datapaths);
> >>>> -                datapaths_array = lflow_input->ls_datapaths->array;
> >>>> -            } else {
> >>>> -                n_datapaths = ods_size(lflow_input->lr_datapaths);
> >>>> -                datapaths_array = lflow_input->lr_datapaths->array;
> >>>> -            }
> >>>> -            uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap,
> >> n_datapaths);
> >>>> -            ovs_assert(n_ods == 1);
> >>>> -            /* There is only one datapath, so it should be moved out
> >> of the
> >>>> -             * group to a single 'od'. */
> >>>> -            size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0,
> >>>> -                                       n_datapaths);
> >>>>
> >>>> -            bitmap_set0(lflow->dpg_bitmap, index);
> >>>> -            lflow->od = datapaths_array[index];
> >>>> -
> >>>> -            /* Logical flow should be re-hashed to allow lookups. */
> >>>> -            uint32_t hash = hmap_node_hash(&lflow->hmap_node);
> >>>> -            /* Remove from lflows. */
> >>>> -            hmap_remove(lflows, &lflow->hmap_node);
> >>>> -            hash =
> >> ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid,
> >>>> -                                                  hash);
> >>>> -            /* Add back. */
> >>>> -            hmap_insert(lflows, &lflow->hmap_node, hash);
> >>>> -
> >>>> -            /* Sync to SB. */
> >>>> -            const struct sbrec_logical_flow *sbflow;
> >>>> -            lflow->sb_uuid = uuid_random();
> >>>> -            sbflow =
sbrec_logical_flow_insert_persist_uuid(ovnsb_txn,
> >>>> -
> >>  &lflow->sb_uuid);
> >>>> -            const char *pipeline =
> >> ovn_stage_get_pipeline_name(lflow->stage);
> >>>> -            uint8_t table = ovn_stage_get_table(lflow->stage);
> >>>> -            sbrec_logical_flow_set_logical_datapath(sbflow,
> >> lflow->od->sb);
> >>>> -            sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
> >>>> -            sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> >>>> -            sbrec_logical_flow_set_table_id(sbflow, table);
> >>>> -            sbrec_logical_flow_set_priority(sbflow,
lflow->priority);
> >>>> -            sbrec_logical_flow_set_match(sbflow, lflow->match);
> >>>> -            sbrec_logical_flow_set_actions(sbflow, lflow->actions);
> >>>> -            if (lflow->io_port) {
> >>>> -                struct smap tags = SMAP_INITIALIZER(&tags);
> >>>> -                smap_add(&tags, "in_out_port", lflow->io_port);
> >>>> -                sbrec_logical_flow_set_tags(sbflow, &tags);
> >>>> -                smap_destroy(&tags);
> >>>> -            }
> >>>> -            sbrec_logical_flow_set_controller_meter(sbflow,
> >> lflow->ctrl_meter);
> >>>> -            /* Trim the source locator lflow->where, which looks
> >> something like
> >>>> -             * "ovn/northd/northd.c:1234", down to just the part
> >> following the
> >>>> -             * last slash, e.g. "northd.c:1234". */
> >>>> -            const char *slash = strrchr(lflow->where, '/');
> >>>> +            /* Sync the newly added flows to SB. */
> >>>> +            struct lflow_ref_node *lfrn;
> >>>> +            LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
> >>>> +                struct ovn_lflow *lflow = lfrn->lflow;
> >>>> +                size_t n_datapaths;
> >>>> +                struct ovn_datapath **datapaths_array;
> >>>> +                if (ovn_stage_to_datapath_type(lflow->stage) ==
> >> DP_SWITCH) {
> >>>> +                    n_datapaths =
ods_size(lflow_input->ls_datapaths);
> >>>> +                    datapaths_array =
lflow_input->ls_datapaths->array;
> >>>> +                } else {
> >>>> +                    n_datapaths =
ods_size(lflow_input->lr_datapaths);
> >>>> +                    datapaths_array =
lflow_input->lr_datapaths->array;
> >>>> +                }
> >>>> +                uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap,
> >> n_datapaths);
> >>>> +                ovs_assert(n_ods == 1);
> >>>> +                /* There is only one datapath, so it should be moved
> >> out of the
> >>>> +                 * group to a single 'od'. */
> >>>> +                size_t index = bitmap_scan(lflow->dpg_bitmap, true,
0,
> >>>> +                                           n_datapaths);
> >>>> +
> >>>> +                bitmap_set0(lflow->dpg_bitmap, index);
> >>>> +                lflow->od = datapaths_array[index];
> >>>> +
> >>>> +                /* Logical flow should be re-hashed to allow
lookups.
> >> */
> >>>> +                uint32_t hash = hmap_node_hash(&lflow->hmap_node);
> >>>> +                /* Remove from lflows. */
> >>>> +                hmap_remove(lflows, &lflow->hmap_node);
> >>>> +                hash = ovn_logical_flow_hash_datapath(
> >>>> +
> >>  &lflow->od->sb->header_.uuid, hash);
> >>>> +                /* Add back. */
> >>>> +                hmap_insert(lflows, &lflow->hmap_node, hash);
> >>>> +
> >>>> +                /* Sync to SB. */
> >>>> +                const struct sbrec_logical_flow *sbflow;
> >>>> +                lflow->sb_uuid = uuid_random();
> >>>> +                sbflow = sbrec_logical_flow_insert_persist_uuid(
> >>>> +                                                ovnsb_txn,
> >> &lflow->sb_uuid);
> >>>> +                const char *pipeline = ovn_stage_get_pipeline_name(
> >>>> +
> >> lflow->stage);
> >>>> +                uint8_t table = ovn_stage_get_table(lflow->stage);
> >>>> +                sbrec_logical_flow_set_logical_datapath(sbflow,
> >> lflow->od->sb);
> >>>> +                sbrec_logical_flow_set_logical_dp_group(sbflow,
NULL);
> >>>> +                sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> >>>> +                sbrec_logical_flow_set_table_id(sbflow, table);
> >>>> +                sbrec_logical_flow_set_priority(sbflow,
> >> lflow->priority);
> >>>> +                sbrec_logical_flow_set_match(sbflow, lflow->match);
> >>>> +                sbrec_logical_flow_set_actions(sbflow,
lflow->actions);
> >>>> +                if (lflow->io_port) {
> >>>> +                    struct smap tags = SMAP_INITIALIZER(&tags);
> >>>> +                    smap_add(&tags, "in_out_port", lflow->io_port);
> >>>> +                    sbrec_logical_flow_set_tags(sbflow, &tags);
> >>>> +                    smap_destroy(&tags);
> >>>> +                }
> >>>> +                sbrec_logical_flow_set_controller_meter(sbflow,
> >>>> +
> >>  lflow->ctrl_meter);
> >>>> +                /* Trim the source locator lflow->where, which looks
> >> something
> >>>> +                 * like "ovn/northd/northd.c:1234", down to just the
> >> part
> >>>> +                 * following the last slash, e.g. "northd.c:1234".
*/
> >>>> +                const char *slash = strrchr(lflow->where, '/');
> >>>>  #if _WIN32
> >>>> -            const char *backslash = strrchr(lflow->where, '\\');
> >>>> -            if (!slash || backslash > slash) {
> >>>> -                slash = backslash;
> >>>> -            }
> >>>> +                const char *backslash = strrchr(lflow->where, '\\');
> >>>> +                if (!slash || backslash > slash) {
> >>>> +                    slash = backslash;
> >>>> +                }
> >>>>  #endif
> >>>> -            const char *where = slash ? slash + 1 : lflow->where;
> >>>> +                const char *where = slash ? slash + 1 :
lflow->where;
> >>>>
> >>>> -            struct smap ids = SMAP_INITIALIZER(&ids);
> >>>> -            smap_add(&ids, "stage-name",
> >> ovn_stage_to_str(lflow->stage));
> >>>> -            smap_add(&ids, "source", where);
> >>>> -            if (lflow->stage_hint) {
> >>>> -                smap_add(&ids, "stage-hint", lflow->stage_hint);
> >>>> +                struct smap ids = SMAP_INITIALIZER(&ids);
> >>>> +                smap_add(&ids, "stage-name",
> >> ovn_stage_to_str(lflow->stage));
> >>>> +                smap_add(&ids, "source", where);
> >>>> +                if (lflow->stage_hint) {
> >>>> +                    smap_add(&ids, "stage-hint", lflow->stage_hint);
> >>>> +                }
> >>>> +                sbrec_logical_flow_set_external_ids(sbflow, &ids);
> >>>> +                smap_destroy(&ids);
> >>>>              }
> >>>> -            sbrec_logical_flow_set_external_ids(sbflow, &ids);
> >>>> -            smap_destroy(&ids);
> >>>>          }
> >>>>      }
> >>>> -    ovs_list_init(&temp_lflow_list);
> >>>> -    add_lflow_to_temp_list = false;
> >>>>      return true;
> >>>>
> >>>>  }
> >>>> --
> >>>> 2.30.2
> >>>>
> >>>> _______________________________________________
> >>>> dev mailing list
> >>>> dev@openvswitch.org
> >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>>>
> >> _______________________________________________
> >> dev mailing list
> >> dev@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Fri, Jun 30, 2023 at 7:00 AM Han Zhou <hzhou@ovn.org> wrote:
>
> On Thu, Jun 29, 2023 at 9:19 AM Dumitru Ceara <dceara@redhat.com> wrote:
> >
> > On 6/27/23 10:23, Numan Siddique wrote:
> > > On Mon, Jun 26, 2023 at 10:34 PM Han Zhou <hzhou@ovn.org> wrote:
> > >>
> > >> On Mon, Jun 26, 2023 at 7:25 AM Numan Siddique <numans@ovn.org> wrote:
> > >>>
> > >>> On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hzhou@ovn.org> wrote:
> > >>>>
> > >>>> For incremental processing, it is important to maintain relationship
> > >>>> between the inputs and the logical flows generated. This patch
> creates
> > >>>> the links between ovn_port and logical flows. The same data structure
> > >>>> may be expanded to maintain links between logical flows and other
> types
> > >>>> of inputs.
> > >>>>
> > >>>> This patch also refactors the temp_lflow_list operations to
> > >>>> collected_lflows with helper functions to start and end collecting.
> It
> > >>>> still uses global variables just to avoid updating all the
> lflow_add_...
> > >>>> related code all over the northd.c file.
> > >>>>
> > >>>> Signed-off-by: Han Zhou <hzhou@ovn.org>
> > >>>
> > >>> Hi Han,
> > >>>
> > >>> Please see a few comments below.  I did review all the 3 patches in
> the
> > >> series.
> > >>> They LGTM overall.  I'd like to do some more testing before providing
> my
> > >> Acks.
> > >>>
> > >>
> > >> Thanks for your review!
> > >>
> > >>>
> > >>>> ---
> > >>>>  northd/northd.c | 271
> +++++++++++++++++++++++++++++++-----------------
> > >>>>  1 file changed, 178 insertions(+), 93 deletions(-)
> > >>>>
> > >>>> diff --git a/northd/northd.c b/northd/northd.c
> > >>>> index 98f528f93cfc..aa0f853ce2db 100644
> > >>>> --- a/northd/northd.c
> > >>>> +++ b/northd/northd.c
> > >>>> @@ -1457,6 +1457,19 @@ struct ovn_port_routable_addresses {
> > >>>>      size_t n_addrs;
> > >>>>  };
> > >>>>
> > >>>> +/* A node that maintains link between an object (such as an
> ovn_port)
> > >> and
> > >>>> + * a lflow. */
> > >>>> +struct lflow_ref_node {
> > >>>> +    /* This list follows different lflows referenced by the same
> > >> object. List
> > >>>> +     * head is, for example, ovn_port->lflows.  */
> > >>>> +    struct ovs_list lflow_list_node;
> > >>>> +    /* This list follows different objects that reference the same
> > >> lflow. List
> > >>>> +     * head is ovn_lflow->referenced_by. */
> > >>>> +    struct ovs_list ref_list_node;
> > >>>> +    /* The lflow. */
> > >>>> +    struct ovn_lflow *lflow;
> > >>>> +};
> > >>>> +
> > >>>>  /* A logical switch port or logical router port.
> > >>>>   *
> > >>>>   * In steady state, an ovn_port points to a northbound
> > >> Logical_Switch_Port
> > >>>> @@ -1548,6 +1561,28 @@ struct ovn_port {
> > >>>>
> > >>>>      /* Temporarily used for traversing a list (or hmap) of ports. */
> > >>>>      bool visited;
> > >>>> +
> > >>>> +    /* List of struct lflow_ref_node that points to the lflows
> > >> generated by
> > >>>> +     * this ovn_port.
> > >>>> +     *
> > >>>> +     * This data is initialized and destroyed by the en_northd node,
> > >> but
> > >>>> +     * populated and used only by the en_lflow node. Ideally this
> data
> > >> should
> > >>>> +     * be maintained as part of en_lflow's data (struct
> lflow_data): a
> > >> hash
> > >>>> +     * index from ovn_port key to lflows.  However, it would be less
> > >> efficient
> > >>>> +     * and more complex:
> > >>>> +     *
> > >>>> +     * 1. It would require an extra search (using the index) to find
> > >> the
> > >>>> +     * lflows.
> > >>>> +     *
> > >>>> +     * 2. Building the index needs to be thread-safe, using either a
> > >> global
> > >>>> +     * lock which is obviously less efficient, or hash-based lock
> > >> array which
> > >>>> +     * is more complex.
> > >>>> +     *
> > >>>> +     * Adding the list here is more straightforward. The drawback is
> > >> that we
> > >>>> +     * need to keep in mind that this data belongs to en_lflow node,
> > >> so never
> > >>>> +     * access it from any other nodes.
> > >>>> +     */
> > >>>> +    struct ovs_list lflows;
> > >>>>  };
> > >>>
> > >>>
> > >>>>
> > >>>>  static bool lsp_can_be_inc_processed(const struct
> > >> nbrec_logical_switch_port *);
> > >>>> @@ -1635,6 +1670,8 @@ ovn_port_create(struct hmap *ports, const char
> > >> *key,
> > >>>>      ovn_port_set_nb(op, nbsp, nbrp);
> > >>>>      op->l3dgw_port = op->cr_port = NULL;
> > >>>>      hmap_insert(ports, &op->key_node, hash_string(op->key, 0));
> > >>>> +
> > >>>> +    ovs_list_init(&op->lflows);
> > >>>>      return op;
> > >>>>  }
> > >>>>
> > >>>> @@ -1665,6 +1702,13 @@ ovn_port_destroy_orphan(struct ovn_port *port)
> > >>>>      destroy_lport_addresses(&port->proxy_arp_addrs);
> > >>>>      free(port->json_key);
> > >>>>      free(port->key);
> > >>>> +
> > >>>> +    struct lflow_ref_node *l;
> > >>>> +    LIST_FOR_EACH_SAFE (l, lflow_list_node, &port->lflows) {
> > >>>> +        ovs_list_remove(&l->lflow_list_node);
> > >>>> +        ovs_list_remove(&l->ref_list_node);
> > >>>> +        free(l);
> > >>>> +    }
> > >>>>      free(port);
> > >>>>  }
> > >>>>
> > >>>> @@ -4893,6 +4937,7 @@ static struct ovn_port *
> > >>>>  ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap
> *ls_ports,
> > >>>>                 const char *key, const struct
> nbrec_logical_switch_port
> > >> *nbsp,
> > >>>>                 struct ovn_datapath *od, const struct
> > >> sbrec_port_binding *sb,
> > >>>> +               struct ovs_list *lflows,
> > >>>>                 const struct sbrec_mirror_table *sbrec_mirror_table,
> > >>>>                 const struct sbrec_chassis_table
> *sbrec_chassis_table,
> > >>>>                 struct ovsdb_idl_index *sbrec_chassis_by_name,
> > >>>> @@ -4903,6 +4948,9 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn,
> > >> struct hmap *ls_ports,
> > >>>>      parse_lsp_addrs(op);
> > >>>>      op->od = od;
> > >>>>      hmap_insert(&od->ports, &op->dp_node,
> > >> hmap_node_hash(&op->key_node));
> > >>>> +    if (lflows) {
> > >>>> +        ovs_list_splice(&op->lflows, lflows->next, lflows);
> > >>>> +    }
> > >>>>
> > >>>>      /* Assign explicitly requested tunnel ids first. */
> > >>>>      if (!ovn_port_assign_requested_tnl_id(sbrec_chassis_table, op))
> {
> > >>>> @@ -5082,7 +5130,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
> > >> *ovnsb_idl_txn,
> > >>>>                      goto fail;
> > >>>>                  }
> > >>>>                  op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> > >>>> -                                    new_nbsp->name, new_nbsp, od,
> NULL,
> > >>>> +                                    new_nbsp->name, new_nbsp, od,
> > >> NULL, NULL,
> > >>>>                                      ni->sbrec_mirror_table,
> > >>>>                                      ni->sbrec_chassis_table,
> > >>>>                                      ni->sbrec_chassis_by_name,
> > >>>> @@ -5114,13 +5162,16 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
> > >> *ovnsb_idl_txn,
> > >>>>                      op->visited = true;
> > >>>>                      continue;
> > >>>>                  }
> > >>>> +                struct ovs_list lflows =
> OVS_LIST_INITIALIZER(&lflows);
> > >>>> +                ovs_list_splice(&lflows, op->lflows.next,
> &op->lflows);
> > >>>>                  ovn_port_destroy(&nd->ls_ports, op);
> > >>>>                  op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
> > >>>> -                                    new_nbsp->name, new_nbsp, od,
> sb,
> > >>>> +                                    new_nbsp->name, new_nbsp, od,
> sb,
> > >> &lflows,
> > >>>>                                      ni->sbrec_mirror_table,
> > >>>>                                      ni->sbrec_chassis_table,
> > >>>>                                      ni->sbrec_chassis_by_name,
> > >>>>                                      ni->sbrec_chassis_by_hostname);
> > >>>> +                ovs_assert(ovs_list_is_empty(&lflows));
> > >>>>                  if (!op) {
> > >>>>                      goto fail;
> > >>>>                  }
> > >>>> @@ -5577,7 +5628,8 @@ ovn_igmp_group_destroy(struct hmap
> *igmp_groups,
> > >>>>
> > >>>>  struct ovn_lflow {
> > >>>>      struct hmap_node hmap_node;
> > >>>> -    struct ovs_list list_node;
> > >>>> +    struct ovs_list list_node;   /* For temporary list of lflows.
> > >> Don't remove
> > >>>> +                                    at destroy. */
> > >>>>
> > >>>>      struct ovn_datapath *od;     /* 'logical_datapath' in SB schema.
> > >>  */
> > >>>>      unsigned long *dpg_bitmap;   /* Bitmap of all datapaths by their
> > >> 'index'.*/
> > >>>> @@ -5591,6 +5643,8 @@ struct ovn_lflow {
> > >>>>      size_t n_ods;                /* Number of datapaths referenced
> by
> > >> 'od' and
> > >>>>                                    * 'dpg_bitmap'. */
> > >>>>      struct ovn_dp_group *dpg;    /* Link to unique Sb datapath
> group.
> > >> */
> > >>>> +
> > >>>> +    struct ovs_list referenced_by;  /* List of struct
> lflow_ref_node.
> > >> */
> > >>>>      const char *where;
> > >>>>
> > >>>>      struct uuid sb_uuid;            /* SB DB row uuid, specified by
> > >> northd. */
> > >>>> @@ -5640,6 +5694,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct
> > >> ovn_datapath *od,
> > >>>>                 char *stage_hint, const char *where)
> > >>>>  {
> > >>>>      ovs_list_init(&lflow->list_node);
> > >>>> +    ovs_list_init(&lflow->referenced_by);
> > >>>>      lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len);
> > >>>>      lflow->od = od;
> > >>>>      lflow->stage = stage;
> > >>>> @@ -5767,20 +5822,30 @@ ovn_dp_group_add_with_reference(struct
> > >> ovn_lflow *lflow_ref,
> > >>>>      }
> > >>>>  }
> > >>>>
> > >>>> +/* This global variable collects the lflows generated by
> > >> do_ovn_lflow_add().
> > >>>> + * start_collecting_lflows() will enable the lflow collection and
> the
> > >> calls to
> > >>>> + * do_ovn_lflow_add (or the macros ovn_lflow_add_...) will add
> > >> generated lflows
> > >>>> + * to the list. end_collecting_lflows() will disable it. */
> > >>>> +static thread_local struct ovs_list collected_lflows;
> > >>>> +static thread_local bool collecting_lflows = false;
> > >>>> +
> > >>>> +static void
> > >>>> +start_collecting_lflows(void)
> > >>>> +{
> > >>>> +    ovs_assert(!collecting_lflows);
> > >>>> +    ovs_list_init(&collected_lflows);
> > >>>> +    collecting_lflows = true;
> > >>>> +}
> > >>>> +
> > >>>> +static void
> > >>>> +end_collecting_lflows(void)
> > >>>> +{
> > >>>> +    ovs_assert(collecting_lflows);
> > >>>> +    collecting_lflows = false;
> > >>>> +}
> > >>>> +
> > >>>
> > >>> I think we can avoid these functions and the thread local variable
> > >>> "collected_lflows".
> > >>>
> > >>> I'd suggest the below
> > >>>
> > >>> ----------------------------
> > >>>
> > >>> static void
> > >>> do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath
> *od,
> > >>>                  const unsigned long *dp_bitmap, size_t dp_bitmap_len,
> > >>>                  uint32_t hash, enum ovn_stage stage, uint16_t
> priority,
> > >>>                  const char *match, const char *actions, const char
> > >> *io_port,
> > >>>                  struct ovs_list *lflow_ref_list,
> > >>>                  const struct ovsdb_idl_row *stage_hint,
> > >>>                  const char *where, const char *ctrl_meter)
> > >>>     OVS_REQUIRES(fake_hash_mutex)
> > >>> {
> > >>>     ...
> > >>>     ...
> > >>>     /* At the end. */
> > >>>     if (lflow_ref_list) {
> > >>>         struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
> > >>>         lfrn->lflow = lflow;
> > >>>         ovs_list_insert(lflow_ref_list, &lfrn->lflow_list_node);
> > >>>         ovs_list_insert(&lflow->referenced_by, &lfrn->ref_list_node);
> > >>>     }
> > >>> }
> > >>>
> > >>>
> > >>> #define ovn_lflow_add_with_lport_and_hint(LFLOW_MAP, OD, STAGE,
> PRIORITY,
> > >> \
> > >>>                                           MATCH, ACTIONS,
> IN_OUT_PORT, \
> > >>>                                           LFLOW_REF_LIST, STAGE_HINT)
> \
> > >>>     ovn_lflow_add_at(LFLOW_MAP, OD, NULL, 0, STAGE, PRIORITY, MATCH,
> > >> ACTIONS, \
> > >>>                      IN_OUT_PORT, LFLOW_REF_LIST, NULL, STAGE_HINT, \
> > >>>                      OVS_SOURCE_LOCATOR)
> > >>>
> > >>> And pass &op->lflows in ovn_lflow_add_with_lport_and_hint()
> > >>>
> > >>> -----------------------------
> > >>>
> > >>> What do you think ?  Definitely this would result in a bit more work
> > >>> as it would require a lot of (tedious) changes.
> > >>> I think this is a better approach.
> > >>>
> > >> Firstly, I think it is not good to use "lflow_ref_list" directly in the
> > >> parameter, because there can be more than one object contributing to
> the
> > >> lflow generation. When we implement I-P for more inputs, a single
> lflow may
> > >> be referenced by multiple objects. We can't pass multiple
> lflow_ref_list to
> > >> the function either, because the number of such lists is unknown. For
> > >> example, a lflow may be generated as a result of a LSP, a DP and a LB
> > >> backend. If we want to implement I-P for LSP, DP and LB backend, we
> need to
> > >> track the reference for all of them. So the current idea is just to
> collect
> > >> a list of lflows generated by a higher level function, such as the
> > >> build_lswitch_and_lrouter_iterate_by_lsp. When implementing I-P for
> more
> > >> than one object of the same lflow, this needs to be more fine-grained.
> >
> > I'm still reviewing this patch but my first impression was that we can
> > probably try to use 'struct objdep_mgr' (defined in lib/objdep.h) to
> > model all these (potentially many-to-many) relationships.  We do similar
> > things in ovn-controller.
> >
> My first impression was similar, but later I figured out it is different
> (and more complex than ovn-controller in general) and we will need to make
> changes to objdep_mgr. However, until more input I-P is implemented I am
> not sure if I will be able to get a general enough abstraction in
> objdep_mgr that can handle all the use cases. On the other hand, the
> references between LSP and related lflows are quite simple, so I went ahead
> and implemented the lists (embeded into ovn_port and lflow without extra
> hash tables), with minimum effort, to make it work at least for this
> scenario and should also work for similar scenarios. I think we can
> refactor the reference implementation any time but it may be better to get
> more I-P implemented, during the process we can always revisit and see if
> objdep_mgr can be extended or another common dependency/reference library
> for ovn-northd can be created, etc. What do you think?

As part of my refactor and northd I-P for load balancers and data
paths,  I'll try to see
if objdep_mgr can be used.  For now with just lport I-P,  I suppose it's fine.

Acked-by: Numan Siddique <numans@ovn.org>

Numan

>
> Regards,
> Han
>
> > >
> > > Agree.  I'm working on the I-P for datapath changes in lflow engine and
> > > as a flow can be referenced by multiple datapaths,  I agree this needs
> to
> > > be tracked properly.  And definitely that is out of scope of this patch
> series.
> > >
> > >
> > >>
> > >> Secondly, I agree with you adding a new parameter to the
> do_ovn_lflow_add
> > >> is cleaner. For the collected list I mentioned, it can be a parameter
> > >> instead of a thread local variable. However, as you mentioned, the
> change
> > >> is going to be all over the northd.c code, not only for the
> > >> ovn_lflow_add_xxx, but also the functions calling the ovn_lflow_add_xxx
> > >> macros, and upper layer functions calling those functions, and so on.
> > >> Unfortunately C doesn't support optional args. At this moment I am not
> sure
> > >> if the interface is stable enough for the incremental-processing, so I
> am
> > >> not sure if it is worth such a big change. If we need to modify them
> again
> > >> later, all such changes are going to be wasted. On the other hand,
> although
> > >> the thread local variable is not the best way, I think it is still
> clear
> > >> and manageable, if we call the start_collecting_lflows and
> > >> end_collecting_lflows in pairs. So, is it ok to leave it for this
> patch and
> > >> in the future when this mechanism proves to work well for more I-P, we
> can
> > >> have a separate patch to refactor (which will include all the tedious
> > >> function call changes). What do you think?
> > >
> > > I agree.  It is definitely tedious to do all the changes.  I'm ok with
> > > the approach taken
> > > in this patch series.
> > >
> > > Also request to please take a look at the load balancer I-P patch
> > > series -
> http://patchwork.ozlabs.org/project/ovn/list/?series=361083&state=*
> > >
> > > Thanks
> > > Numan
> > >
> > >>
> > >> Thanks,
> > >> Han
> > >>
> > >>> Also I'm planning to work on top of your patches to add I-P for load
> > >>> balancers in lflow engine (or perhaps I-P for datapath changes)
> > >>>
> > >>> My idea is to have a lflow ref list stored in "struct ovn_datapath"
> > >>> similar to the way you have done in this patch in "struct ovn_port"
> > >>>
> > >>> And while adding the flows using one of the macro variants
> > >>> 'ovn_lflow_add*' pass &od->lflows.
> > >>>
> > >>> Please let me know your comments.
> > >>>
> > >>> Only concern I have with this patch is the "op->lflows" modified by
> > >>> the lflow engine node.
> > >>> But I agree with your added comments and also thinking to use the same
> > >>> approach for datapath I-P handling,
> > >>> And I don't have a better approach at the moment. So I'm fine with it.
> > >>>
> > >>> Thanks
> > >>> Numan
> > >>>
> > >>>
> > >>>>  /* Adds a row with the specified contents to the Logical_Flow table.
> > >>>> - * Version to use when hash bucket locking is NOT required.
> > >>>> - *
> > >>>> - * Note: This function can add generated lflows to the global
> variable
> > >>>> - * temp_lflow_list as its output, controlled by the global variable
> > >>>> - * add_lflow_to_temp_list. The caller of the ovn_lflow_add_...
> marcros
> > >> can get
> > >>>> - * a list of lflows generated by setting add_lflow_to_temp_list to
> > >> true. The
> > >>>> - * caller is responsible for initializing the temp_lflow_list, and
> also
> > >>>> - * reset the add_lflow_to_temp_list to false when it is no longer
> > >> needed.
> > >>>> - * XXX: this mechanism is temporary and will be replaced when we add
> > >> hash index
> > >>>> - * to lflow_data and refactor related functions.
> > >>>> - */
> > >>>> -static bool add_lflow_to_temp_list = false;
> > >>>> -static struct ovs_list temp_lflow_list;
> > >>>> + * Version to use when hash bucket locking is NOT required. */
> > >>>>  static void
> > >>>>  do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath
> *od,
> > >>>>                   const unsigned long *dp_bitmap, size_t
> dp_bitmap_len,
> > >>>> @@ -5797,7 +5862,7 @@ do_ovn_lflow_add(struct hmap *lflow_map, const
> > >> struct ovn_datapath *od,
> > >>>>      size_t bitmap_len = od ? ods_size(od->datapaths) :
> dp_bitmap_len;
> > >>>>      ovs_assert(bitmap_len);
> > >>>>
> > >>>> -    if (add_lflow_to_temp_list) {
> > >>>> +    if (collecting_lflows) {
> > >>>>          ovs_assert(od);
> > >>>>          ovs_assert(!dp_bitmap);
> > >>>>      } else {
> > >>>> @@ -5829,8 +5894,8 @@ do_ovn_lflow_add(struct hmap *lflow_map, const
> > >> struct ovn_datapath *od,
> > >>>>          thread_lflow_counter++;
> > >>>>      }
> > >>>>
> > >>>> -    if (add_lflow_to_temp_list) {
> > >>>> -        ovs_list_insert(&temp_lflow_list, &lflow->list_node);
> > >>>> +    if (collecting_lflows) {
> > >>>> +        ovs_list_insert(&collected_lflows, &lflow->list_node);
> > >>>>      }
> > >>>>  }
> > >>>>
> > >>>> @@ -5950,10 +6015,28 @@ ovn_lflow_destroy(struct hmap *lflows, struct
> > >> ovn_lflow *lflow)
> > >>>>          free(lflow->io_port);
> > >>>>          free(lflow->stage_hint);
> > >>>>          free(lflow->ctrl_meter);
> > >>>> +        struct lflow_ref_node *l;
> > >>>> +        LIST_FOR_EACH_SAFE (l, ref_list_node,
> &lflow->referenced_by) {
> > >>>> +            ovs_list_remove(&l->lflow_list_node);
> > >>>> +            ovs_list_remove(&l->ref_list_node);
> > >>>> +            free(l);
> > >>>> +        }
> > >>>>          free(lflow);
> > >>>>      }
> > >>>>  }
> > >>>>
> > >>>> +static void
> > >>>> +link_ovn_port_to_lflows(struct ovn_port *op, struct ovs_list
> *lflows)
> > >>>> +{
> > >>>> +    struct ovn_lflow *f;
> > >>>> +    LIST_FOR_EACH (f, list_node, lflows) {
> > >>>> +        struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
> > >>>> +        lfrn->lflow = f;
> > >>>> +        ovs_list_insert(&op->lflows, &lfrn->lflow_list_node);
> > >>>> +        ovs_list_insert(&f->referenced_by, &lfrn->ref_list_node);
> > >>>> +    }
> > >>>> +}
> > >>>> +
> > >>>>  static bool
> > >>>>  build_dhcpv4_action(struct ovn_port *op, ovs_be32 offer_ip,
> > >>>>                      struct ds *options_action, struct ds
> > >> *response_action,
> > >>>> @@ -15483,6 +15566,7 @@
> build_lswitch_and_lrouter_iterate_by_lsp(struct
> > >> ovn_port *op,
> > >>>>                                           struct hmap *lflows)
> > >>>>  {
> > >>>>      ovs_assert(op->nbsp);
> > >>>> +    start_collecting_lflows();
> > >>>>
> > >>>>      /* Build Logical Switch Flows. */
> > >>>>      build_lswitch_port_sec_op(op, lflows, actions, match);
> > >>>> @@ -15497,6 +15581,9 @@
> build_lswitch_and_lrouter_iterate_by_lsp(struct
> > >> ovn_port *op,
> > >>>>      /* Build Logical Router Flows. */
> > >>>>      build_ip_routing_flows_for_router_type_lsp(op, lr_ports,
> lflows);
> > >>>>      build_arp_resolve_flows_for_lsp(op, lflows, lr_ports, match,
> > >> actions);
> > >>>> +
> > >>>> +    link_ovn_port_to_lflows(op, &collected_lflows);
> > >>>> +    end_collecting_lflows();
> > >>>>  }
> > >>>>
> > >>>>  /* Helper function to combine all lflow generation which is iterated
> > >> by logical
> > >>>> @@ -16223,14 +16310,10 @@ bool lflow_handle_northd_ls_changes(struct
> > >> ovsdb_idl_txn *ovnsb_txn,
> > >>>>  {
> > >>>>      struct ls_change *ls_change;
> > >>>>      LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
> > >>>> -        ovs_list_init(&temp_lflow_list);
> > >>>> -        add_lflow_to_temp_list = true;
> > >>>>          if (!ovs_list_is_empty(&ls_change->updated_ports) ||
> > >>>>              !ovs_list_is_empty(&ls_change->deleted_ports)) {
> > >>>>              /* XXX: implement lflow index so that we can handle
> > >> updated and
> > >>>>               * deleted LSPs incrementally. */
> > >>>> -            ovs_list_init(&temp_lflow_list);
> > >>>> -            add_lflow_to_temp_list = false;
> > >>>>              return false;
> > >>>>          }
> > >>>>
> > >>>> @@ -16277,83 +16360,85 @@ bool lflow_handle_northd_ls_changes(struct
> > >> ovsdb_idl_txn *ovnsb_txn,
> > >>>>
> > >>  sbrec_multicast_group_update_ports_addvalue(sbmc_unknown,
> > >>>>                                                              op->sb);
> > >>>>              }
> > >>>> -        }
> > >>>> -        /* Sync the newly added flows to SB. */
> > >>>> -        struct ovn_lflow *lflow;
> > >>>> -        LIST_FOR_EACH (lflow, list_node, &temp_lflow_list) {
> > >>>> -            size_t n_datapaths;
> > >>>> -            struct ovn_datapath **datapaths_array;
> > >>>> -            if (ovn_stage_to_datapath_type(lflow->stage) ==
> DP_SWITCH)
> > >> {
> > >>>> -                n_datapaths = ods_size(lflow_input->ls_datapaths);
> > >>>> -                datapaths_array = lflow_input->ls_datapaths->array;
> > >>>> -            } else {
> > >>>> -                n_datapaths = ods_size(lflow_input->lr_datapaths);
> > >>>> -                datapaths_array = lflow_input->lr_datapaths->array;
> > >>>> -            }
> > >>>> -            uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap,
> > >> n_datapaths);
> > >>>> -            ovs_assert(n_ods == 1);
> > >>>> -            /* There is only one datapath, so it should be moved out
> > >> of the
> > >>>> -             * group to a single 'od'. */
> > >>>> -            size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0,
> > >>>> -                                       n_datapaths);
> > >>>>
> > >>>> -            bitmap_set0(lflow->dpg_bitmap, index);
> > >>>> -            lflow->od = datapaths_array[index];
> > >>>> -
> > >>>> -            /* Logical flow should be re-hashed to allow lookups. */
> > >>>> -            uint32_t hash = hmap_node_hash(&lflow->hmap_node);
> > >>>> -            /* Remove from lflows. */
> > >>>> -            hmap_remove(lflows, &lflow->hmap_node);
> > >>>> -            hash =
> > >> ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid,
> > >>>> -                                                  hash);
> > >>>> -            /* Add back. */
> > >>>> -            hmap_insert(lflows, &lflow->hmap_node, hash);
> > >>>> -
> > >>>> -            /* Sync to SB. */
> > >>>> -            const struct sbrec_logical_flow *sbflow;
> > >>>> -            lflow->sb_uuid = uuid_random();
> > >>>> -            sbflow =
> sbrec_logical_flow_insert_persist_uuid(ovnsb_txn,
> > >>>> -
> > >>  &lflow->sb_uuid);
> > >>>> -            const char *pipeline =
> > >> ovn_stage_get_pipeline_name(lflow->stage);
> > >>>> -            uint8_t table = ovn_stage_get_table(lflow->stage);
> > >>>> -            sbrec_logical_flow_set_logical_datapath(sbflow,
> > >> lflow->od->sb);
> > >>>> -            sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
> > >>>> -            sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> > >>>> -            sbrec_logical_flow_set_table_id(sbflow, table);
> > >>>> -            sbrec_logical_flow_set_priority(sbflow,
> lflow->priority);
> > >>>> -            sbrec_logical_flow_set_match(sbflow, lflow->match);
> > >>>> -            sbrec_logical_flow_set_actions(sbflow, lflow->actions);
> > >>>> -            if (lflow->io_port) {
> > >>>> -                struct smap tags = SMAP_INITIALIZER(&tags);
> > >>>> -                smap_add(&tags, "in_out_port", lflow->io_port);
> > >>>> -                sbrec_logical_flow_set_tags(sbflow, &tags);
> > >>>> -                smap_destroy(&tags);
> > >>>> -            }
> > >>>> -            sbrec_logical_flow_set_controller_meter(sbflow,
> > >> lflow->ctrl_meter);
> > >>>> -            /* Trim the source locator lflow->where, which looks
> > >> something like
> > >>>> -             * "ovn/northd/northd.c:1234", down to just the part
> > >> following the
> > >>>> -             * last slash, e.g. "northd.c:1234". */
> > >>>> -            const char *slash = strrchr(lflow->where, '/');
> > >>>> +            /* Sync the newly added flows to SB. */
> > >>>> +            struct lflow_ref_node *lfrn;
> > >>>> +            LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
> > >>>> +                struct ovn_lflow *lflow = lfrn->lflow;
> > >>>> +                size_t n_datapaths;
> > >>>> +                struct ovn_datapath **datapaths_array;
> > >>>> +                if (ovn_stage_to_datapath_type(lflow->stage) ==
> > >> DP_SWITCH) {
> > >>>> +                    n_datapaths =
> ods_size(lflow_input->ls_datapaths);
> > >>>> +                    datapaths_array =
> lflow_input->ls_datapaths->array;
> > >>>> +                } else {
> > >>>> +                    n_datapaths =
> ods_size(lflow_input->lr_datapaths);
> > >>>> +                    datapaths_array =
> lflow_input->lr_datapaths->array;
> > >>>> +                }
> > >>>> +                uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap,
> > >> n_datapaths);
> > >>>> +                ovs_assert(n_ods == 1);
> > >>>> +                /* There is only one datapath, so it should be moved
> > >> out of the
> > >>>> +                 * group to a single 'od'. */
> > >>>> +                size_t index = bitmap_scan(lflow->dpg_bitmap, true,
> 0,
> > >>>> +                                           n_datapaths);
> > >>>> +
> > >>>> +                bitmap_set0(lflow->dpg_bitmap, index);
> > >>>> +                lflow->od = datapaths_array[index];
> > >>>> +
> > >>>> +                /* Logical flow should be re-hashed to allow
> lookups.
> > >> */
> > >>>> +                uint32_t hash = hmap_node_hash(&lflow->hmap_node);
> > >>>> +                /* Remove from lflows. */
> > >>>> +                hmap_remove(lflows, &lflow->hmap_node);
> > >>>> +                hash = ovn_logical_flow_hash_datapath(
> > >>>> +
> > >>  &lflow->od->sb->header_.uuid, hash);
> > >>>> +                /* Add back. */
> > >>>> +                hmap_insert(lflows, &lflow->hmap_node, hash);
> > >>>> +
> > >>>> +                /* Sync to SB. */
> > >>>> +                const struct sbrec_logical_flow *sbflow;
> > >>>> +                lflow->sb_uuid = uuid_random();
> > >>>> +                sbflow = sbrec_logical_flow_insert_persist_uuid(
> > >>>> +                                                ovnsb_txn,
> > >> &lflow->sb_uuid);
> > >>>> +                const char *pipeline = ovn_stage_get_pipeline_name(
> > >>>> +
> > >> lflow->stage);
> > >>>> +                uint8_t table = ovn_stage_get_table(lflow->stage);
> > >>>> +                sbrec_logical_flow_set_logical_datapath(sbflow,
> > >> lflow->od->sb);
> > >>>> +                sbrec_logical_flow_set_logical_dp_group(sbflow,
> NULL);
> > >>>> +                sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> > >>>> +                sbrec_logical_flow_set_table_id(sbflow, table);
> > >>>> +                sbrec_logical_flow_set_priority(sbflow,
> > >> lflow->priority);
> > >>>> +                sbrec_logical_flow_set_match(sbflow, lflow->match);
> > >>>> +                sbrec_logical_flow_set_actions(sbflow,
> lflow->actions);
> > >>>> +                if (lflow->io_port) {
> > >>>> +                    struct smap tags = SMAP_INITIALIZER(&tags);
> > >>>> +                    smap_add(&tags, "in_out_port", lflow->io_port);
> > >>>> +                    sbrec_logical_flow_set_tags(sbflow, &tags);
> > >>>> +                    smap_destroy(&tags);
> > >>>> +                }
> > >>>> +                sbrec_logical_flow_set_controller_meter(sbflow,
> > >>>> +
> > >>  lflow->ctrl_meter);
> > >>>> +                /* Trim the source locator lflow->where, which looks
> > >> something
> > >>>> +                 * like "ovn/northd/northd.c:1234", down to just the
> > >> part
> > >>>> +                 * following the last slash, e.g. "northd.c:1234".
> */
> > >>>> +                const char *slash = strrchr(lflow->where, '/');
> > >>>>  #if _WIN32
> > >>>> -            const char *backslash = strrchr(lflow->where, '\\');
> > >>>> -            if (!slash || backslash > slash) {
> > >>>> -                slash = backslash;
> > >>>> -            }
> > >>>> +                const char *backslash = strrchr(lflow->where, '\\');
> > >>>> +                if (!slash || backslash > slash) {
> > >>>> +                    slash = backslash;
> > >>>> +                }
> > >>>>  #endif
> > >>>> -            const char *where = slash ? slash + 1 : lflow->where;
> > >>>> +                const char *where = slash ? slash + 1 :
> lflow->where;
> > >>>>
> > >>>> -            struct smap ids = SMAP_INITIALIZER(&ids);
> > >>>> -            smap_add(&ids, "stage-name",
> > >> ovn_stage_to_str(lflow->stage));
> > >>>> -            smap_add(&ids, "source", where);
> > >>>> -            if (lflow->stage_hint) {
> > >>>> -                smap_add(&ids, "stage-hint", lflow->stage_hint);
> > >>>> +                struct smap ids = SMAP_INITIALIZER(&ids);
> > >>>> +                smap_add(&ids, "stage-name",
> > >> ovn_stage_to_str(lflow->stage));
> > >>>> +                smap_add(&ids, "source", where);
> > >>>> +                if (lflow->stage_hint) {
> > >>>> +                    smap_add(&ids, "stage-hint", lflow->stage_hint);
> > >>>> +                }
> > >>>> +                sbrec_logical_flow_set_external_ids(sbflow, &ids);
> > >>>> +                smap_destroy(&ids);
> > >>>>              }
> > >>>> -            sbrec_logical_flow_set_external_ids(sbflow, &ids);
> > >>>> -            smap_destroy(&ids);
> > >>>>          }
> > >>>>      }
> > >>>> -    ovs_list_init(&temp_lflow_list);
> > >>>> -    add_lflow_to_temp_list = false;
> > >>>>      return true;
> > >>>>
> > >>>>  }
> > >>>> --
> > >>>> 2.30.2
> > >>>>
> > >>>> _______________________________________________
> > >>>> dev mailing list
> > >>>> dev@openvswitch.org
> > >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >>>>
> > >> _______________________________________________
> > >> dev mailing list
> > >> dev@openvswitch.org
> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Thu, Jun 29, 2023 at 10:10 PM Numan Siddique <numans@ovn.org> wrote:
>
> On Fri, Jun 30, 2023 at 7:00 AM Han Zhou <hzhou@ovn.org> wrote:
> >
> > On Thu, Jun 29, 2023 at 9:19 AM Dumitru Ceara <dceara@redhat.com> wrote:
> > >
> > > On 6/27/23 10:23, Numan Siddique wrote:
> > > > On Mon, Jun 26, 2023 at 10:34 PM Han Zhou <hzhou@ovn.org> wrote:
> > > >>
> > > >> On Mon, Jun 26, 2023 at 7:25 AM Numan Siddique <numans@ovn.org>
wrote:
> > > >>>
> > > >>> On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hzhou@ovn.org> wrote:
> > > >>>>
> > > >>>> For incremental processing, it is important to maintain
relationship
> > > >>>> between the inputs and the logical flows generated. This patch
> > creates
> > > >>>> the links between ovn_port and logical flows. The same data
structure
> > > >>>> may be expanded to maintain links between logical flows and other
> > types
> > > >>>> of inputs.
> > > >>>>
> > > >>>> This patch also refactors the temp_lflow_list operations to
> > > >>>> collected_lflows with helper functions to start and end
collecting.
> > It
> > > >>>> still uses global variables just to avoid updating all the
> > lflow_add_...
> > > >>>> related code all over the northd.c file.
> > > >>>>
> > > >>>> Signed-off-by: Han Zhou <hzhou@ovn.org>
> > > >>>
> > > >>> Hi Han,
> > > >>>
> > > >>> Please see a few comments below.  I did review all the 3 patches
in
> > the
> > > >> series.
> > > >>> They LGTM overall.  I'd like to do some more testing before
providing
> > my
> > > >> Acks.
> > > >>>
> > > >>
> > > >> Thanks for your review!
> > > >>
> > > >>>
> > > >>>> ---
> > > >>>>  northd/northd.c | 271
> > +++++++++++++++++++++++++++++++-----------------
> > > >>>>  1 file changed, 178 insertions(+), 93 deletions(-)
> > > >>>>
> > > >>>> diff --git a/northd/northd.c b/northd/northd.c
> > > >>>> index 98f528f93cfc..aa0f853ce2db 100644
> > > >>>> --- a/northd/northd.c
> > > >>>> +++ b/northd/northd.c
> > > >>>> @@ -1457,6 +1457,19 @@ struct ovn_port_routable_addresses {
> > > >>>>      size_t n_addrs;
> > > >>>>  };
> > > >>>>
> > > >>>> +/* A node that maintains link between an object (such as an
> > ovn_port)
> > > >> and
> > > >>>> + * a lflow. */
> > > >>>> +struct lflow_ref_node {
> > > >>>> +    /* This list follows different lflows referenced by the same
> > > >> object. List
> > > >>>> +     * head is, for example, ovn_port->lflows.  */
> > > >>>> +    struct ovs_list lflow_list_node;
> > > >>>> +    /* This list follows different objects that reference the
same
> > > >> lflow. List
> > > >>>> +     * head is ovn_lflow->referenced_by. */
> > > >>>> +    struct ovs_list ref_list_node;
> > > >>>> +    /* The lflow. */
> > > >>>> +    struct ovn_lflow *lflow;
> > > >>>> +};
> > > >>>> +
> > > >>>>  /* A logical switch port or logical router port.
> > > >>>>   *
> > > >>>>   * In steady state, an ovn_port points to a northbound
> > > >> Logical_Switch_Port
> > > >>>> @@ -1548,6 +1561,28 @@ struct ovn_port {
> > > >>>>
> > > >>>>      /* Temporarily used for traversing a list (or hmap) of
ports. */
> > > >>>>      bool visited;
> > > >>>> +
> > > >>>> +    /* List of struct lflow_ref_node that points to the lflows
> > > >> generated by
> > > >>>> +     * this ovn_port.
> > > >>>> +     *
> > > >>>> +     * This data is initialized and destroyed by the en_northd
node,
> > > >> but
> > > >>>> +     * populated and used only by the en_lflow node. Ideally
this
> > data
> > > >> should
> > > >>>> +     * be maintained as part of en_lflow's data (struct
> > lflow_data): a
> > > >> hash
> > > >>>> +     * index from ovn_port key to lflows.  However, it would be
less
> > > >> efficient
> > > >>>> +     * and more complex:
> > > >>>> +     *
> > > >>>> +     * 1. It would require an extra search (using the index) to
find
> > > >> the
> > > >>>> +     * lflows.
> > > >>>> +     *
> > > >>>> +     * 2. Building the index needs to be thread-safe, using
either a
> > > >> global
> > > >>>> +     * lock which is obviously less efficient, or hash-based
lock
> > > >> array which
> > > >>>> +     * is more complex.
> > > >>>> +     *
> > > >>>> +     * Adding the list here is more straightforward. The
drawback is
> > > >> that we
> > > >>>> +     * need to keep in mind that this data belongs to en_lflow
node,
> > > >> so never
> > > >>>> +     * access it from any other nodes.
> > > >>>> +     */
> > > >>>> +    struct ovs_list lflows;
> > > >>>>  };
> > > >>>
> > > >>>
> > > >>>>
> > > >>>>  static bool lsp_can_be_inc_processed(const struct
> > > >> nbrec_logical_switch_port *);
> > > >>>> @@ -1635,6 +1670,8 @@ ovn_port_create(struct hmap *ports, const
char
> > > >> *key,
> > > >>>>      ovn_port_set_nb(op, nbsp, nbrp);
> > > >>>>      op->l3dgw_port = op->cr_port = NULL;
> > > >>>>      hmap_insert(ports, &op->key_node, hash_string(op->key, 0));
> > > >>>> +
> > > >>>> +    ovs_list_init(&op->lflows);
> > > >>>>      return op;
> > > >>>>  }
> > > >>>>
> > > >>>> @@ -1665,6 +1702,13 @@ ovn_port_destroy_orphan(struct ovn_port
*port)
> > > >>>>      destroy_lport_addresses(&port->proxy_arp_addrs);
> > > >>>>      free(port->json_key);
> > > >>>>      free(port->key);
> > > >>>> +
> > > >>>> +    struct lflow_ref_node *l;
> > > >>>> +    LIST_FOR_EACH_SAFE (l, lflow_list_node, &port->lflows) {
> > > >>>> +        ovs_list_remove(&l->lflow_list_node);
> > > >>>> +        ovs_list_remove(&l->ref_list_node);
> > > >>>> +        free(l);
> > > >>>> +    }
> > > >>>>      free(port);
> > > >>>>  }
> > > >>>>
> > > >>>> @@ -4893,6 +4937,7 @@ static struct ovn_port *
> > > >>>>  ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap
> > *ls_ports,
> > > >>>>                 const char *key, const struct
> > nbrec_logical_switch_port
> > > >> *nbsp,
> > > >>>>                 struct ovn_datapath *od, const struct
> > > >> sbrec_port_binding *sb,
> > > >>>> +               struct ovs_list *lflows,
> > > >>>>                 const struct sbrec_mirror_table
*sbrec_mirror_table,
> > > >>>>                 const struct sbrec_chassis_table
> > *sbrec_chassis_table,
> > > >>>>                 struct ovsdb_idl_index *sbrec_chassis_by_name,
> > > >>>> @@ -4903,6 +4948,9 @@ ls_port_create(struct ovsdb_idl_txn
*ovnsb_txn,
> > > >> struct hmap *ls_ports,
> > > >>>>      parse_lsp_addrs(op);
> > > >>>>      op->od = od;
> > > >>>>      hmap_insert(&od->ports, &op->dp_node,
> > > >> hmap_node_hash(&op->key_node));
> > > >>>> +    if (lflows) {
> > > >>>> +        ovs_list_splice(&op->lflows, lflows->next, lflows);
> > > >>>> +    }
> > > >>>>
> > > >>>>      /* Assign explicitly requested tunnel ids first. */
> > > >>>>      if (!ovn_port_assign_requested_tnl_id(sbrec_chassis_table,
op))
> > {
> > > >>>> @@ -5082,7 +5130,7 @@ northd_handle_ls_changes(struct
ovsdb_idl_txn
> > > >> *ovnsb_idl_txn,
> > > >>>>                      goto fail;
> > > >>>>                  }
> > > >>>>                  op = ls_port_create(ovnsb_idl_txn,
&nd->ls_ports,
> > > >>>> -                                    new_nbsp->name, new_nbsp,
od,
> > NULL,
> > > >>>> +                                    new_nbsp->name, new_nbsp,
od,
> > > >> NULL, NULL,
> > > >>>>                                      ni->sbrec_mirror_table,
> > > >>>>                                      ni->sbrec_chassis_table,
> > > >>>>                                      ni->sbrec_chassis_by_name,
> > > >>>> @@ -5114,13 +5162,16 @@ northd_handle_ls_changes(struct
ovsdb_idl_txn
> > > >> *ovnsb_idl_txn,
> > > >>>>                      op->visited = true;
> > > >>>>                      continue;
> > > >>>>                  }
> > > >>>> +                struct ovs_list lflows =
> > OVS_LIST_INITIALIZER(&lflows);
> > > >>>> +                ovs_list_splice(&lflows, op->lflows.next,
> > &op->lflows);
> > > >>>>                  ovn_port_destroy(&nd->ls_ports, op);
> > > >>>>                  op = ls_port_create(ovnsb_idl_txn,
&nd->ls_ports,
> > > >>>> -                                    new_nbsp->name, new_nbsp,
od,
> > sb,
> > > >>>> +                                    new_nbsp->name, new_nbsp,
od,
> > sb,
> > > >> &lflows,
> > > >>>>                                      ni->sbrec_mirror_table,
> > > >>>>                                      ni->sbrec_chassis_table,
> > > >>>>                                      ni->sbrec_chassis_by_name,
> > > >>>>
 ni->sbrec_chassis_by_hostname);
> > > >>>> +                ovs_assert(ovs_list_is_empty(&lflows));
> > > >>>>                  if (!op) {
> > > >>>>                      goto fail;
> > > >>>>                  }
> > > >>>> @@ -5577,7 +5628,8 @@ ovn_igmp_group_destroy(struct hmap
> > *igmp_groups,
> > > >>>>
> > > >>>>  struct ovn_lflow {
> > > >>>>      struct hmap_node hmap_node;
> > > >>>> -    struct ovs_list list_node;
> > > >>>> +    struct ovs_list list_node;   /* For temporary list of
lflows.
> > > >> Don't remove
> > > >>>> +                                    at destroy. */
> > > >>>>
> > > >>>>      struct ovn_datapath *od;     /* 'logical_datapath' in SB
schema.
> > > >>  */
> > > >>>>      unsigned long *dpg_bitmap;   /* Bitmap of all datapaths by
their
> > > >> 'index'.*/
> > > >>>> @@ -5591,6 +5643,8 @@ struct ovn_lflow {
> > > >>>>      size_t n_ods;                /* Number of datapaths
referenced
> > by
> > > >> 'od' and
> > > >>>>                                    * 'dpg_bitmap'. */
> > > >>>>      struct ovn_dp_group *dpg;    /* Link to unique Sb datapath
> > group.
> > > >> */
> > > >>>> +
> > > >>>> +    struct ovs_list referenced_by;  /* List of struct
> > lflow_ref_node.
> > > >> */
> > > >>>>      const char *where;
> > > >>>>
> > > >>>>      struct uuid sb_uuid;            /* SB DB row uuid,
specified by
> > > >> northd. */
> > > >>>> @@ -5640,6 +5694,7 @@ ovn_lflow_init(struct ovn_lflow *lflow,
struct
> > > >> ovn_datapath *od,
> > > >>>>                 char *stage_hint, const char *where)
> > > >>>>  {
> > > >>>>      ovs_list_init(&lflow->list_node);
> > > >>>> +    ovs_list_init(&lflow->referenced_by);
> > > >>>>      lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len);
> > > >>>>      lflow->od = od;
> > > >>>>      lflow->stage = stage;
> > > >>>> @@ -5767,20 +5822,30 @@ ovn_dp_group_add_with_reference(struct
> > > >> ovn_lflow *lflow_ref,
> > > >>>>      }
> > > >>>>  }
> > > >>>>
> > > >>>> +/* This global variable collects the lflows generated by
> > > >> do_ovn_lflow_add().
> > > >>>> + * start_collecting_lflows() will enable the lflow collection
and
> > the
> > > >> calls to
> > > >>>> + * do_ovn_lflow_add (or the macros ovn_lflow_add_...) will add
> > > >> generated lflows
> > > >>>> + * to the list. end_collecting_lflows() will disable it. */
> > > >>>> +static thread_local struct ovs_list collected_lflows;
> > > >>>> +static thread_local bool collecting_lflows = false;
> > > >>>> +
> > > >>>> +static void
> > > >>>> +start_collecting_lflows(void)
> > > >>>> +{
> > > >>>> +    ovs_assert(!collecting_lflows);
> > > >>>> +    ovs_list_init(&collected_lflows);
> > > >>>> +    collecting_lflows = true;
> > > >>>> +}
> > > >>>> +
> > > >>>> +static void
> > > >>>> +end_collecting_lflows(void)
> > > >>>> +{
> > > >>>> +    ovs_assert(collecting_lflows);
> > > >>>> +    collecting_lflows = false;
> > > >>>> +}
> > > >>>> +
> > > >>>
> > > >>> I think we can avoid these functions and the thread local variable
> > > >>> "collected_lflows".
> > > >>>
> > > >>> I'd suggest the below
> > > >>>
> > > >>> ----------------------------
> > > >>>
> > > >>> static void
> > > >>> do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath
> > *od,
> > > >>>                  const unsigned long *dp_bitmap, size_t
dp_bitmap_len,
> > > >>>                  uint32_t hash, enum ovn_stage stage, uint16_t
> > priority,
> > > >>>                  const char *match, const char *actions, const
char
> > > >> *io_port,
> > > >>>                  struct ovs_list *lflow_ref_list,
> > > >>>                  const struct ovsdb_idl_row *stage_hint,
> > > >>>                  const char *where, const char *ctrl_meter)
> > > >>>     OVS_REQUIRES(fake_hash_mutex)
> > > >>> {
> > > >>>     ...
> > > >>>     ...
> > > >>>     /* At the end. */
> > > >>>     if (lflow_ref_list) {
> > > >>>         struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
> > > >>>         lfrn->lflow = lflow;
> > > >>>         ovs_list_insert(lflow_ref_list, &lfrn->lflow_list_node);
> > > >>>         ovs_list_insert(&lflow->referenced_by,
&lfrn->ref_list_node);
> > > >>>     }
> > > >>> }
> > > >>>
> > > >>>
> > > >>> #define ovn_lflow_add_with_lport_and_hint(LFLOW_MAP, OD, STAGE,
> > PRIORITY,
> > > >> \
> > > >>>                                           MATCH, ACTIONS,
> > IN_OUT_PORT, \
> > > >>>                                           LFLOW_REF_LIST,
STAGE_HINT)
> > \
> > > >>>     ovn_lflow_add_at(LFLOW_MAP, OD, NULL, 0, STAGE, PRIORITY,
MATCH,
> > > >> ACTIONS, \
> > > >>>                      IN_OUT_PORT, LFLOW_REF_LIST, NULL,
STAGE_HINT, \
> > > >>>                      OVS_SOURCE_LOCATOR)
> > > >>>
> > > >>> And pass &op->lflows in ovn_lflow_add_with_lport_and_hint()
> > > >>>
> > > >>> -----------------------------
> > > >>>
> > > >>> What do you think ?  Definitely this would result in a bit more
work
> > > >>> as it would require a lot of (tedious) changes.
> > > >>> I think this is a better approach.
> > > >>>
> > > >> Firstly, I think it is not good to use "lflow_ref_list" directly
in the
> > > >> parameter, because there can be more than one object contributing
to
> > the
> > > >> lflow generation. When we implement I-P for more inputs, a single
> > lflow may
> > > >> be referenced by multiple objects. We can't pass multiple
> > lflow_ref_list to
> > > >> the function either, because the number of such lists is unknown.
For
> > > >> example, a lflow may be generated as a result of a LSP, a DP and a
LB
> > > >> backend. If we want to implement I-P for LSP, DP and LB backend, we
> > need to
> > > >> track the reference for all of them. So the current idea is just to
> > collect
> > > >> a list of lflows generated by a higher level function, such as the
> > > >> build_lswitch_and_lrouter_iterate_by_lsp. When implementing I-P for
> > more
> > > >> than one object of the same lflow, this needs to be more
fine-grained.
> > >
> > > I'm still reviewing this patch but my first impression was that we can
> > > probably try to use 'struct objdep_mgr' (defined in lib/objdep.h) to
> > > model all these (potentially many-to-many) relationships.  We do
similar
> > > things in ovn-controller.
> > >
> > My first impression was similar, but later I figured out it is different
> > (and more complex than ovn-controller in general) and we will need to
make
> > changes to objdep_mgr. However, until more input I-P is implemented I am
> > not sure if I will be able to get a general enough abstraction in
> > objdep_mgr that can handle all the use cases. On the other hand, the
> > references between LSP and related lflows are quite simple, so I went
ahead
> > and implemented the lists (embeded into ovn_port and lflow without extra
> > hash tables), with minimum effort, to make it work at least for this
> > scenario and should also work for similar scenarios. I think we can
> > refactor the reference implementation any time but it may be better to
get
> > more I-P implemented, during the process we can always revisit and see
if
> > objdep_mgr can be extended or another common dependency/reference
library
> > for ovn-northd can be created, etc. What do you think?
>
> As part of my refactor and northd I-P for load balancers and data
> paths,  I'll try to see
> if objdep_mgr can be used.  For now with just lport I-P,  I suppose it's
fine.
>
> Acked-by: Numan Siddique <numans@ovn.org>
>
Haven't heard back from Dumitru for a while, so I assume you are fine with
this (but please share any time if you have more comments).
Thank you both, Numan and Dumitru. I applied this to main.

Han

> Numan
>
> >
> > Regards,
> > Han
> >
> > > >
> > > > Agree.  I'm working on the I-P for datapath changes in lflow engine
and
> > > > as a flow can be referenced by multiple datapaths,  I agree this
needs
> > to
> > > > be tracked properly.  And definitely that is out of scope of this
patch
> > series.
> > > >
> > > >
> > > >>
> > > >> Secondly, I agree with you adding a new parameter to the
> > do_ovn_lflow_add
> > > >> is cleaner. For the collected list I mentioned, it can be a
parameter
> > > >> instead of a thread local variable. However, as you mentioned, the
> > change
> > > >> is going to be all over the northd.c code, not only for the
> > > >> ovn_lflow_add_xxx, but also the functions calling the
ovn_lflow_add_xxx
> > > >> macros, and upper layer functions calling those functions, and so
on.
> > > >> Unfortunately C doesn't support optional args. At this moment I am
not
> > sure
> > > >> if the interface is stable enough for the incremental-processing,
so I
> > am
> > > >> not sure if it is worth such a big change. If we need to modify
them
> > again
> > > >> later, all such changes are going to be wasted. On the other hand,
> > although
> > > >> the thread local variable is not the best way, I think it is still
> > clear
> > > >> and manageable, if we call the start_collecting_lflows and
> > > >> end_collecting_lflows in pairs. So, is it ok to leave it for this
> > patch and
> > > >> in the future when this mechanism proves to work well for more
I-P, we
> > can
> > > >> have a separate patch to refactor (which will include all the
tedious
> > > >> function call changes). What do you think?
> > > >
> > > > I agree.  It is definitely tedious to do all the changes.  I'm ok
with
> > > > the approach taken
> > > > in this patch series.
> > > >
> > > > Also request to please take a look at the load balancer I-P patch
> > > > series -
> > http://patchwork.ozlabs.org/project/ovn/list/?series=361083&state=*
> > > >
> > > > Thanks
> > > > Numan
> > > >
> > > >>
> > > >> Thanks,
> > > >> Han
> > > >>
> > > >>> Also I'm planning to work on top of your patches to add I-P for
load
> > > >>> balancers in lflow engine (or perhaps I-P for datapath changes)
> > > >>>
> > > >>> My idea is to have a lflow ref list stored in "struct
ovn_datapath"
> > > >>> similar to the way you have done in this patch in "struct
ovn_port"
> > > >>>
> > > >>> And while adding the flows using one of the macro variants
> > > >>> 'ovn_lflow_add*' pass &od->lflows.
> > > >>>
> > > >>> Please let me know your comments.
> > > >>>
> > > >>> Only concern I have with this patch is the "op->lflows" modified
by
> > > >>> the lflow engine node.
> > > >>> But I agree with your added comments and also thinking to use the
same
> > > >>> approach for datapath I-P handling,
> > > >>> And I don't have a better approach at the moment. So I'm fine
with it.
> > > >>>
> > > >>> Thanks
> > > >>> Numan
> > > >>>
> > > >>>
> > > >>>>  /* Adds a row with the specified contents to the Logical_Flow
table.
> > > >>>> - * Version to use when hash bucket locking is NOT required.
> > > >>>> - *
> > > >>>> - * Note: This function can add generated lflows to the global
> > variable
> > > >>>> - * temp_lflow_list as its output, controlled by the global
variable
> > > >>>> - * add_lflow_to_temp_list. The caller of the ovn_lflow_add_...
> > marcros
> > > >> can get
> > > >>>> - * a list of lflows generated by setting add_lflow_to_temp_list
to
> > > >> true. The
> > > >>>> - * caller is responsible for initializing the temp_lflow_list,
and
> > also
> > > >>>> - * reset the add_lflow_to_temp_list to false when it is no
longer
> > > >> needed.
> > > >>>> - * XXX: this mechanism is temporary and will be replaced when
we add
> > > >> hash index
> > > >>>> - * to lflow_data and refactor related functions.
> > > >>>> - */
> > > >>>> -static bool add_lflow_to_temp_list = false;
> > > >>>> -static struct ovs_list temp_lflow_list;
> > > >>>> + * Version to use when hash bucket locking is NOT required. */
> > > >>>>  static void
> > > >>>>  do_ovn_lflow_add(struct hmap *lflow_map, const struct
ovn_datapath
> > *od,
> > > >>>>                   const unsigned long *dp_bitmap, size_t
> > dp_bitmap_len,
> > > >>>> @@ -5797,7 +5862,7 @@ do_ovn_lflow_add(struct hmap *lflow_map,
const
> > > >> struct ovn_datapath *od,
> > > >>>>      size_t bitmap_len = od ? ods_size(od->datapaths) :
> > dp_bitmap_len;
> > > >>>>      ovs_assert(bitmap_len);
> > > >>>>
> > > >>>> -    if (add_lflow_to_temp_list) {
> > > >>>> +    if (collecting_lflows) {
> > > >>>>          ovs_assert(od);
> > > >>>>          ovs_assert(!dp_bitmap);
> > > >>>>      } else {
> > > >>>> @@ -5829,8 +5894,8 @@ do_ovn_lflow_add(struct hmap *lflow_map,
const
> > > >> struct ovn_datapath *od,
> > > >>>>          thread_lflow_counter++;
> > > >>>>      }
> > > >>>>
> > > >>>> -    if (add_lflow_to_temp_list) {
> > > >>>> -        ovs_list_insert(&temp_lflow_list, &lflow->list_node);
> > > >>>> +    if (collecting_lflows) {
> > > >>>> +        ovs_list_insert(&collected_lflows, &lflow->list_node);
> > > >>>>      }
> > > >>>>  }
> > > >>>>
> > > >>>> @@ -5950,10 +6015,28 @@ ovn_lflow_destroy(struct hmap *lflows,
struct
> > > >> ovn_lflow *lflow)
> > > >>>>          free(lflow->io_port);
> > > >>>>          free(lflow->stage_hint);
> > > >>>>          free(lflow->ctrl_meter);
> > > >>>> +        struct lflow_ref_node *l;
> > > >>>> +        LIST_FOR_EACH_SAFE (l, ref_list_node,
> > &lflow->referenced_by) {
> > > >>>> +            ovs_list_remove(&l->lflow_list_node);
> > > >>>> +            ovs_list_remove(&l->ref_list_node);
> > > >>>> +            free(l);
> > > >>>> +        }
> > > >>>>          free(lflow);
> > > >>>>      }
> > > >>>>  }
> > > >>>>
> > > >>>> +static void
> > > >>>> +link_ovn_port_to_lflows(struct ovn_port *op, struct ovs_list
> > *lflows)
> > > >>>> +{
> > > >>>> +    struct ovn_lflow *f;
> > > >>>> +    LIST_FOR_EACH (f, list_node, lflows) {
> > > >>>> +        struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
> > > >>>> +        lfrn->lflow = f;
> > > >>>> +        ovs_list_insert(&op->lflows, &lfrn->lflow_list_node);
> > > >>>> +        ovs_list_insert(&f->referenced_by,
&lfrn->ref_list_node);
> > > >>>> +    }
> > > >>>> +}
> > > >>>> +
> > > >>>>  static bool
> > > >>>>  build_dhcpv4_action(struct ovn_port *op, ovs_be32 offer_ip,
> > > >>>>                      struct ds *options_action, struct ds
> > > >> *response_action,
> > > >>>> @@ -15483,6 +15566,7 @@
> > build_lswitch_and_lrouter_iterate_by_lsp(struct
> > > >> ovn_port *op,
> > > >>>>                                           struct hmap *lflows)
> > > >>>>  {
> > > >>>>      ovs_assert(op->nbsp);
> > > >>>> +    start_collecting_lflows();
> > > >>>>
> > > >>>>      /* Build Logical Switch Flows. */
> > > >>>>      build_lswitch_port_sec_op(op, lflows, actions, match);
> > > >>>> @@ -15497,6 +15581,9 @@
> > build_lswitch_and_lrouter_iterate_by_lsp(struct
> > > >> ovn_port *op,
> > > >>>>      /* Build Logical Router Flows. */
> > > >>>>      build_ip_routing_flows_for_router_type_lsp(op, lr_ports,
> > lflows);
> > > >>>>      build_arp_resolve_flows_for_lsp(op, lflows, lr_ports, match,
> > > >> actions);
> > > >>>> +
> > > >>>> +    link_ovn_port_to_lflows(op, &collected_lflows);
> > > >>>> +    end_collecting_lflows();
> > > >>>>  }
> > > >>>>
> > > >>>>  /* Helper function to combine all lflow generation which is
iterated
> > > >> by logical
> > > >>>> @@ -16223,14 +16310,10 @@ bool
lflow_handle_northd_ls_changes(struct
> > > >> ovsdb_idl_txn *ovnsb_txn,
> > > >>>>  {
> > > >>>>      struct ls_change *ls_change;
> > > >>>>      LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
> > > >>>> -        ovs_list_init(&temp_lflow_list);
> > > >>>> -        add_lflow_to_temp_list = true;
> > > >>>>          if (!ovs_list_is_empty(&ls_change->updated_ports) ||
> > > >>>>              !ovs_list_is_empty(&ls_change->deleted_ports)) {
> > > >>>>              /* XXX: implement lflow index so that we can handle
> > > >> updated and
> > > >>>>               * deleted LSPs incrementally. */
> > > >>>> -            ovs_list_init(&temp_lflow_list);
> > > >>>> -            add_lflow_to_temp_list = false;
> > > >>>>              return false;
> > > >>>>          }
> > > >>>>
> > > >>>> @@ -16277,83 +16360,85 @@ bool
lflow_handle_northd_ls_changes(struct
> > > >> ovsdb_idl_txn *ovnsb_txn,
> > > >>>>
> > > >>  sbrec_multicast_group_update_ports_addvalue(sbmc_unknown,
> > > >>>>
 op->sb);
> > > >>>>              }
> > > >>>> -        }
> > > >>>> -        /* Sync the newly added flows to SB. */
> > > >>>> -        struct ovn_lflow *lflow;
> > > >>>> -        LIST_FOR_EACH (lflow, list_node, &temp_lflow_list) {
> > > >>>> -            size_t n_datapaths;
> > > >>>> -            struct ovn_datapath **datapaths_array;
> > > >>>> -            if (ovn_stage_to_datapath_type(lflow->stage) ==
> > DP_SWITCH)
> > > >> {
> > > >>>> -                n_datapaths =
ods_size(lflow_input->ls_datapaths);
> > > >>>> -                datapaths_array =
lflow_input->ls_datapaths->array;
> > > >>>> -            } else {
> > > >>>> -                n_datapaths =
ods_size(lflow_input->lr_datapaths);
> > > >>>> -                datapaths_array =
lflow_input->lr_datapaths->array;
> > > >>>> -            }
> > > >>>> -            uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap,
> > > >> n_datapaths);
> > > >>>> -            ovs_assert(n_ods == 1);
> > > >>>> -            /* There is only one datapath, so it should be
moved out
> > > >> of the
> > > >>>> -             * group to a single 'od'. */
> > > >>>> -            size_t index = bitmap_scan(lflow->dpg_bitmap, true,
0,
> > > >>>> -                                       n_datapaths);
> > > >>>>
> > > >>>> -            bitmap_set0(lflow->dpg_bitmap, index);
> > > >>>> -            lflow->od = datapaths_array[index];
> > > >>>> -
> > > >>>> -            /* Logical flow should be re-hashed to allow
lookups. */
> > > >>>> -            uint32_t hash = hmap_node_hash(&lflow->hmap_node);
> > > >>>> -            /* Remove from lflows. */
> > > >>>> -            hmap_remove(lflows, &lflow->hmap_node);
> > > >>>> -            hash =
> > > >> ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid,
> > > >>>> -                                                  hash);
> > > >>>> -            /* Add back. */
> > > >>>> -            hmap_insert(lflows, &lflow->hmap_node, hash);
> > > >>>> -
> > > >>>> -            /* Sync to SB. */
> > > >>>> -            const struct sbrec_logical_flow *sbflow;
> > > >>>> -            lflow->sb_uuid = uuid_random();
> > > >>>> -            sbflow =
> > sbrec_logical_flow_insert_persist_uuid(ovnsb_txn,
> > > >>>> -
> > > >>  &lflow->sb_uuid);
> > > >>>> -            const char *pipeline =
> > > >> ovn_stage_get_pipeline_name(lflow->stage);
> > > >>>> -            uint8_t table = ovn_stage_get_table(lflow->stage);
> > > >>>> -            sbrec_logical_flow_set_logical_datapath(sbflow,
> > > >> lflow->od->sb);
> > > >>>> -            sbrec_logical_flow_set_logical_dp_group(sbflow,
NULL);
> > > >>>> -            sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> > > >>>> -            sbrec_logical_flow_set_table_id(sbflow, table);
> > > >>>> -            sbrec_logical_flow_set_priority(sbflow,
> > lflow->priority);
> > > >>>> -            sbrec_logical_flow_set_match(sbflow, lflow->match);
> > > >>>> -            sbrec_logical_flow_set_actions(sbflow,
lflow->actions);
> > > >>>> -            if (lflow->io_port) {
> > > >>>> -                struct smap tags = SMAP_INITIALIZER(&tags);
> > > >>>> -                smap_add(&tags, "in_out_port", lflow->io_port);
> > > >>>> -                sbrec_logical_flow_set_tags(sbflow, &tags);
> > > >>>> -                smap_destroy(&tags);
> > > >>>> -            }
> > > >>>> -            sbrec_logical_flow_set_controller_meter(sbflow,
> > > >> lflow->ctrl_meter);
> > > >>>> -            /* Trim the source locator lflow->where, which looks
> > > >> something like
> > > >>>> -             * "ovn/northd/northd.c:1234", down to just the part
> > > >> following the
> > > >>>> -             * last slash, e.g. "northd.c:1234". */
> > > >>>> -            const char *slash = strrchr(lflow->where, '/');
> > > >>>> +            /* Sync the newly added flows to SB. */
> > > >>>> +            struct lflow_ref_node *lfrn;
> > > >>>> +            LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
> > > >>>> +                struct ovn_lflow *lflow = lfrn->lflow;
> > > >>>> +                size_t n_datapaths;
> > > >>>> +                struct ovn_datapath **datapaths_array;
> > > >>>> +                if (ovn_stage_to_datapath_type(lflow->stage) ==
> > > >> DP_SWITCH) {
> > > >>>> +                    n_datapaths =
> > ods_size(lflow_input->ls_datapaths);
> > > >>>> +                    datapaths_array =
> > lflow_input->ls_datapaths->array;
> > > >>>> +                } else {
> > > >>>> +                    n_datapaths =
> > ods_size(lflow_input->lr_datapaths);
> > > >>>> +                    datapaths_array =
> > lflow_input->lr_datapaths->array;
> > > >>>> +                }
> > > >>>> +                uint32_t n_ods =
bitmap_count1(lflow->dpg_bitmap,
> > > >> n_datapaths);
> > > >>>> +                ovs_assert(n_ods == 1);
> > > >>>> +                /* There is only one datapath, so it should be
moved
> > > >> out of the
> > > >>>> +                 * group to a single 'od'. */
> > > >>>> +                size_t index = bitmap_scan(lflow->dpg_bitmap,
true,
> > 0,
> > > >>>> +                                           n_datapaths);
> > > >>>> +
> > > >>>> +                bitmap_set0(lflow->dpg_bitmap, index);
> > > >>>> +                lflow->od = datapaths_array[index];
> > > >>>> +
> > > >>>> +                /* Logical flow should be re-hashed to allow
> > lookups.
> > > >> */
> > > >>>> +                uint32_t hash =
hmap_node_hash(&lflow->hmap_node);
> > > >>>> +                /* Remove from lflows. */
> > > >>>> +                hmap_remove(lflows, &lflow->hmap_node);
> > > >>>> +                hash = ovn_logical_flow_hash_datapath(
> > > >>>> +
> > > >>  &lflow->od->sb->header_.uuid, hash);
> > > >>>> +                /* Add back. */
> > > >>>> +                hmap_insert(lflows, &lflow->hmap_node, hash);
> > > >>>> +
> > > >>>> +                /* Sync to SB. */
> > > >>>> +                const struct sbrec_logical_flow *sbflow;
> > > >>>> +                lflow->sb_uuid = uuid_random();
> > > >>>> +                sbflow = sbrec_logical_flow_insert_persist_uuid(
> > > >>>> +                                                ovnsb_txn,
> > > >> &lflow->sb_uuid);
> > > >>>> +                const char *pipeline =
ovn_stage_get_pipeline_name(
> > > >>>> +
> > > >> lflow->stage);
> > > >>>> +                uint8_t table =
ovn_stage_get_table(lflow->stage);
> > > >>>> +                sbrec_logical_flow_set_logical_datapath(sbflow,
> > > >> lflow->od->sb);
> > > >>>> +                sbrec_logical_flow_set_logical_dp_group(sbflow,
> > NULL);
> > > >>>> +                sbrec_logical_flow_set_pipeline(sbflow,
pipeline);
> > > >>>> +                sbrec_logical_flow_set_table_id(sbflow, table);
> > > >>>> +                sbrec_logical_flow_set_priority(sbflow,
> > > >> lflow->priority);
> > > >>>> +                sbrec_logical_flow_set_match(sbflow,
lflow->match);
> > > >>>> +                sbrec_logical_flow_set_actions(sbflow,
> > lflow->actions);
> > > >>>> +                if (lflow->io_port) {
> > > >>>> +                    struct smap tags = SMAP_INITIALIZER(&tags);
> > > >>>> +                    smap_add(&tags, "in_out_port",
lflow->io_port);
> > > >>>> +                    sbrec_logical_flow_set_tags(sbflow, &tags);
> > > >>>> +                    smap_destroy(&tags);
> > > >>>> +                }
> > > >>>> +                sbrec_logical_flow_set_controller_meter(sbflow,
> > > >>>> +
> > > >>  lflow->ctrl_meter);
> > > >>>> +                /* Trim the source locator lflow->where, which
looks
> > > >> something
> > > >>>> +                 * like "ovn/northd/northd.c:1234", down to
just the
> > > >> part
> > > >>>> +                 * following the last slash, e.g.
"northd.c:1234".
> > */
> > > >>>> +                const char *slash = strrchr(lflow->where, '/');
> > > >>>>  #if _WIN32
> > > >>>> -            const char *backslash = strrchr(lflow->where, '\\');
> > > >>>> -            if (!slash || backslash > slash) {
> > > >>>> -                slash = backslash;
> > > >>>> -            }
> > > >>>> +                const char *backslash = strrchr(lflow->where,
'\\');
> > > >>>> +                if (!slash || backslash > slash) {
> > > >>>> +                    slash = backslash;
> > > >>>> +                }
> > > >>>>  #endif
> > > >>>> -            const char *where = slash ? slash + 1 :
lflow->where;
> > > >>>> +                const char *where = slash ? slash + 1 :
> > lflow->where;
> > > >>>>
> > > >>>> -            struct smap ids = SMAP_INITIALIZER(&ids);
> > > >>>> -            smap_add(&ids, "stage-name",
> > > >> ovn_stage_to_str(lflow->stage));
> > > >>>> -            smap_add(&ids, "source", where);
> > > >>>> -            if (lflow->stage_hint) {
> > > >>>> -                smap_add(&ids, "stage-hint", lflow->stage_hint);
> > > >>>> +                struct smap ids = SMAP_INITIALIZER(&ids);
> > > >>>> +                smap_add(&ids, "stage-name",
> > > >> ovn_stage_to_str(lflow->stage));
> > > >>>> +                smap_add(&ids, "source", where);
> > > >>>> +                if (lflow->stage_hint) {
> > > >>>> +                    smap_add(&ids, "stage-hint",
lflow->stage_hint);
> > > >>>> +                }
> > > >>>> +                sbrec_logical_flow_set_external_ids(sbflow,
&ids);
> > > >>>> +                smap_destroy(&ids);
> > > >>>>              }
> > > >>>> -            sbrec_logical_flow_set_external_ids(sbflow, &ids);
> > > >>>> -            smap_destroy(&ids);
> > > >>>>          }
> > > >>>>      }
> > > >>>> -    ovs_list_init(&temp_lflow_list);
> > > >>>> -    add_lflow_to_temp_list = false;
> > > >>>>      return true;
> > > >>>>
> > > >>>>  }
> > > >>>> --
> > > >>>> 2.30.2
> > > >>>>
> > > >>>> _______________________________________________
> > > >>>> dev mailing list
> > > >>>> dev@openvswitch.org
> > > >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >>>>
> > > >> _______________________________________________
> > > >> dev mailing list
> > > >> dev@openvswitch.org
> > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 7/6/23 12:13, Han Zhou wrote:
> On Thu, Jun 29, 2023 at 10:10 PM Numan Siddique <numans@ovn.org> wrote:
>>
>> On Fri, Jun 30, 2023 at 7:00 AM Han Zhou <hzhou@ovn.org> wrote:
>>>
>>> On Thu, Jun 29, 2023 at 9:19 AM Dumitru Ceara <dceara@redhat.com> wrote:
>>>>
>>>> On 6/27/23 10:23, Numan Siddique wrote:
>>>>> On Mon, Jun 26, 2023 at 10:34 PM Han Zhou <hzhou@ovn.org> wrote:
>>>>>>
>>>>>> On Mon, Jun 26, 2023 at 7:25 AM Numan Siddique <numans@ovn.org>
> wrote:
>>>>>>>
>>>>>>> On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hzhou@ovn.org> wrote:
>>>>>>>>
>>>>>>>> For incremental processing, it is important to maintain
> relationship
>>>>>>>> between the inputs and the logical flows generated. This patch
>>> creates
>>>>>>>> the links between ovn_port and logical flows. The same data
> structure
>>>>>>>> may be expanded to maintain links between logical flows and other
>>> types
>>>>>>>> of inputs.
>>>>>>>>
>>>>>>>> This patch also refactors the temp_lflow_list operations to
>>>>>>>> collected_lflows with helper functions to start and end
> collecting.
>>> It
>>>>>>>> still uses global variables just to avoid updating all the
>>> lflow_add_...
>>>>>>>> related code all over the northd.c file.
>>>>>>>>
>>>>>>>> Signed-off-by: Han Zhou <hzhou@ovn.org>
>>>>>>>
>>>>>>> Hi Han,
>>>>>>>
>>>>>>> Please see a few comments below.  I did review all the 3 patches
> in
>>> the
>>>>>> series.
>>>>>>> They LGTM overall.  I'd like to do some more testing before
> providing
>>> my
>>>>>> Acks.
>>>>>>>
>>>>>>
>>>>>> Thanks for your review!
>>>>>>
>>>>>>>
>>>>>>>> ---
>>>>>>>>  northd/northd.c | 271
>>> +++++++++++++++++++++++++++++++-----------------
>>>>>>>>  1 file changed, 178 insertions(+), 93 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/northd/northd.c b/northd/northd.c
>>>>>>>> index 98f528f93cfc..aa0f853ce2db 100644
>>>>>>>> --- a/northd/northd.c
>>>>>>>> +++ b/northd/northd.c
>>>>>>>> @@ -1457,6 +1457,19 @@ struct ovn_port_routable_addresses {
>>>>>>>>      size_t n_addrs;
>>>>>>>>  };
>>>>>>>>
>>>>>>>> +/* A node that maintains link between an object (such as an
>>> ovn_port)
>>>>>> and
>>>>>>>> + * a lflow. */
>>>>>>>> +struct lflow_ref_node {
>>>>>>>> +    /* This list follows different lflows referenced by the same
>>>>>> object. List
>>>>>>>> +     * head is, for example, ovn_port->lflows.  */
>>>>>>>> +    struct ovs_list lflow_list_node;
>>>>>>>> +    /* This list follows different objects that reference the
> same
>>>>>> lflow. List
>>>>>>>> +     * head is ovn_lflow->referenced_by. */
>>>>>>>> +    struct ovs_list ref_list_node;
>>>>>>>> +    /* The lflow. */
>>>>>>>> +    struct ovn_lflow *lflow;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>  /* A logical switch port or logical router port.
>>>>>>>>   *
>>>>>>>>   * In steady state, an ovn_port points to a northbound
>>>>>> Logical_Switch_Port
>>>>>>>> @@ -1548,6 +1561,28 @@ struct ovn_port {
>>>>>>>>
>>>>>>>>      /* Temporarily used for traversing a list (or hmap) of
> ports. */
>>>>>>>>      bool visited;
>>>>>>>> +
>>>>>>>> +    /* List of struct lflow_ref_node that points to the lflows
>>>>>> generated by
>>>>>>>> +     * this ovn_port.
>>>>>>>> +     *
>>>>>>>> +     * This data is initialized and destroyed by the en_northd
> node,
>>>>>> but
>>>>>>>> +     * populated and used only by the en_lflow node. Ideally
> this
>>> data
>>>>>> should
>>>>>>>> +     * be maintained as part of en_lflow's data (struct
>>> lflow_data): a
>>>>>> hash
>>>>>>>> +     * index from ovn_port key to lflows.  However, it would be
> less
>>>>>> efficient
>>>>>>>> +     * and more complex:
>>>>>>>> +     *
>>>>>>>> +     * 1. It would require an extra search (using the index) to
> find
>>>>>> the
>>>>>>>> +     * lflows.
>>>>>>>> +     *
>>>>>>>> +     * 2. Building the index needs to be thread-safe, using
> either a
>>>>>> global
>>>>>>>> +     * lock which is obviously less efficient, or hash-based
> lock
>>>>>> array which
>>>>>>>> +     * is more complex.
>>>>>>>> +     *
>>>>>>>> +     * Adding the list here is more straightforward. The
> drawback is
>>>>>> that we
>>>>>>>> +     * need to keep in mind that this data belongs to en_lflow
> node,
>>>>>> so never
>>>>>>>> +     * access it from any other nodes.
>>>>>>>> +     */
>>>>>>>> +    struct ovs_list lflows;
>>>>>>>>  };
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>  static bool lsp_can_be_inc_processed(const struct
>>>>>> nbrec_logical_switch_port *);
>>>>>>>> @@ -1635,6 +1670,8 @@ ovn_port_create(struct hmap *ports, const
> char
>>>>>> *key,
>>>>>>>>      ovn_port_set_nb(op, nbsp, nbrp);
>>>>>>>>      op->l3dgw_port = op->cr_port = NULL;
>>>>>>>>      hmap_insert(ports, &op->key_node, hash_string(op->key, 0));
>>>>>>>> +
>>>>>>>> +    ovs_list_init(&op->lflows);
>>>>>>>>      return op;
>>>>>>>>  }
>>>>>>>>
>>>>>>>> @@ -1665,6 +1702,13 @@ ovn_port_destroy_orphan(struct ovn_port
> *port)
>>>>>>>>      destroy_lport_addresses(&port->proxy_arp_addrs);
>>>>>>>>      free(port->json_key);
>>>>>>>>      free(port->key);
>>>>>>>> +
>>>>>>>> +    struct lflow_ref_node *l;
>>>>>>>> +    LIST_FOR_EACH_SAFE (l, lflow_list_node, &port->lflows) {
>>>>>>>> +        ovs_list_remove(&l->lflow_list_node);
>>>>>>>> +        ovs_list_remove(&l->ref_list_node);
>>>>>>>> +        free(l);
>>>>>>>> +    }
>>>>>>>>      free(port);
>>>>>>>>  }
>>>>>>>>
>>>>>>>> @@ -4893,6 +4937,7 @@ static struct ovn_port *
>>>>>>>>  ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap
>>> *ls_ports,
>>>>>>>>                 const char *key, const struct
>>> nbrec_logical_switch_port
>>>>>> *nbsp,
>>>>>>>>                 struct ovn_datapath *od, const struct
>>>>>> sbrec_port_binding *sb,
>>>>>>>> +               struct ovs_list *lflows,
>>>>>>>>                 const struct sbrec_mirror_table
> *sbrec_mirror_table,
>>>>>>>>                 const struct sbrec_chassis_table
>>> *sbrec_chassis_table,
>>>>>>>>                 struct ovsdb_idl_index *sbrec_chassis_by_name,
>>>>>>>> @@ -4903,6 +4948,9 @@ ls_port_create(struct ovsdb_idl_txn
> *ovnsb_txn,
>>>>>> struct hmap *ls_ports,
>>>>>>>>      parse_lsp_addrs(op);
>>>>>>>>      op->od = od;
>>>>>>>>      hmap_insert(&od->ports, &op->dp_node,
>>>>>> hmap_node_hash(&op->key_node));
>>>>>>>> +    if (lflows) {
>>>>>>>> +        ovs_list_splice(&op->lflows, lflows->next, lflows);
>>>>>>>> +    }
>>>>>>>>
>>>>>>>>      /* Assign explicitly requested tunnel ids first. */
>>>>>>>>      if (!ovn_port_assign_requested_tnl_id(sbrec_chassis_table,
> op))
>>> {
>>>>>>>> @@ -5082,7 +5130,7 @@ northd_handle_ls_changes(struct
> ovsdb_idl_txn
>>>>>> *ovnsb_idl_txn,
>>>>>>>>                      goto fail;
>>>>>>>>                  }
>>>>>>>>                  op = ls_port_create(ovnsb_idl_txn,
> &nd->ls_ports,
>>>>>>>> -                                    new_nbsp->name, new_nbsp,
> od,
>>> NULL,
>>>>>>>> +                                    new_nbsp->name, new_nbsp,
> od,
>>>>>> NULL, NULL,
>>>>>>>>                                      ni->sbrec_mirror_table,
>>>>>>>>                                      ni->sbrec_chassis_table,
>>>>>>>>                                      ni->sbrec_chassis_by_name,
>>>>>>>> @@ -5114,13 +5162,16 @@ northd_handle_ls_changes(struct
> ovsdb_idl_txn
>>>>>> *ovnsb_idl_txn,
>>>>>>>>                      op->visited = true;
>>>>>>>>                      continue;
>>>>>>>>                  }
>>>>>>>> +                struct ovs_list lflows =
>>> OVS_LIST_INITIALIZER(&lflows);
>>>>>>>> +                ovs_list_splice(&lflows, op->lflows.next,
>>> &op->lflows);
>>>>>>>>                  ovn_port_destroy(&nd->ls_ports, op);
>>>>>>>>                  op = ls_port_create(ovnsb_idl_txn,
> &nd->ls_ports,
>>>>>>>> -                                    new_nbsp->name, new_nbsp,
> od,
>>> sb,
>>>>>>>> +                                    new_nbsp->name, new_nbsp,
> od,
>>> sb,
>>>>>> &lflows,
>>>>>>>>                                      ni->sbrec_mirror_table,
>>>>>>>>                                      ni->sbrec_chassis_table,
>>>>>>>>                                      ni->sbrec_chassis_by_name,
>>>>>>>>
>  ni->sbrec_chassis_by_hostname);
>>>>>>>> +                ovs_assert(ovs_list_is_empty(&lflows));
>>>>>>>>                  if (!op) {
>>>>>>>>                      goto fail;
>>>>>>>>                  }
>>>>>>>> @@ -5577,7 +5628,8 @@ ovn_igmp_group_destroy(struct hmap
>>> *igmp_groups,
>>>>>>>>
>>>>>>>>  struct ovn_lflow {
>>>>>>>>      struct hmap_node hmap_node;
>>>>>>>> -    struct ovs_list list_node;
>>>>>>>> +    struct ovs_list list_node;   /* For temporary list of
> lflows.
>>>>>> Don't remove
>>>>>>>> +                                    at destroy. */
>>>>>>>>
>>>>>>>>      struct ovn_datapath *od;     /* 'logical_datapath' in SB
> schema.
>>>>>>  */
>>>>>>>>      unsigned long *dpg_bitmap;   /* Bitmap of all datapaths by
> their
>>>>>> 'index'.*/
>>>>>>>> @@ -5591,6 +5643,8 @@ struct ovn_lflow {
>>>>>>>>      size_t n_ods;                /* Number of datapaths
> referenced
>>> by
>>>>>> 'od' and
>>>>>>>>                                    * 'dpg_bitmap'. */
>>>>>>>>      struct ovn_dp_group *dpg;    /* Link to unique Sb datapath
>>> group.
>>>>>> */
>>>>>>>> +
>>>>>>>> +    struct ovs_list referenced_by;  /* List of struct
>>> lflow_ref_node.
>>>>>> */
>>>>>>>>      const char *where;
>>>>>>>>
>>>>>>>>      struct uuid sb_uuid;            /* SB DB row uuid,
> specified by
>>>>>> northd. */
>>>>>>>> @@ -5640,6 +5694,7 @@ ovn_lflow_init(struct ovn_lflow *lflow,
> struct
>>>>>> ovn_datapath *od,
>>>>>>>>                 char *stage_hint, const char *where)
>>>>>>>>  {
>>>>>>>>      ovs_list_init(&lflow->list_node);
>>>>>>>> +    ovs_list_init(&lflow->referenced_by);
>>>>>>>>      lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len);
>>>>>>>>      lflow->od = od;
>>>>>>>>      lflow->stage = stage;
>>>>>>>> @@ -5767,20 +5822,30 @@ ovn_dp_group_add_with_reference(struct
>>>>>> ovn_lflow *lflow_ref,
>>>>>>>>      }
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +/* This global variable collects the lflows generated by
>>>>>> do_ovn_lflow_add().
>>>>>>>> + * start_collecting_lflows() will enable the lflow collection
> and
>>> the
>>>>>> calls to
>>>>>>>> + * do_ovn_lflow_add (or the macros ovn_lflow_add_...) will add
>>>>>> generated lflows
>>>>>>>> + * to the list. end_collecting_lflows() will disable it. */
>>>>>>>> +static thread_local struct ovs_list collected_lflows;
>>>>>>>> +static thread_local bool collecting_lflows = false;
>>>>>>>> +
>>>>>>>> +static void
>>>>>>>> +start_collecting_lflows(void)
>>>>>>>> +{
>>>>>>>> +    ovs_assert(!collecting_lflows);
>>>>>>>> +    ovs_list_init(&collected_lflows);
>>>>>>>> +    collecting_lflows = true;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void
>>>>>>>> +end_collecting_lflows(void)
>>>>>>>> +{
>>>>>>>> +    ovs_assert(collecting_lflows);
>>>>>>>> +    collecting_lflows = false;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>
>>>>>>> I think we can avoid these functions and the thread local variable
>>>>>>> "collected_lflows".
>>>>>>>
>>>>>>> I'd suggest the below
>>>>>>>
>>>>>>> ----------------------------
>>>>>>>
>>>>>>> static void
>>>>>>> do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath
>>> *od,
>>>>>>>                  const unsigned long *dp_bitmap, size_t
> dp_bitmap_len,
>>>>>>>                  uint32_t hash, enum ovn_stage stage, uint16_t
>>> priority,
>>>>>>>                  const char *match, const char *actions, const
> char
>>>>>> *io_port,
>>>>>>>                  struct ovs_list *lflow_ref_list,
>>>>>>>                  const struct ovsdb_idl_row *stage_hint,
>>>>>>>                  const char *where, const char *ctrl_meter)
>>>>>>>     OVS_REQUIRES(fake_hash_mutex)
>>>>>>> {
>>>>>>>     ...
>>>>>>>     ...
>>>>>>>     /* At the end. */
>>>>>>>     if (lflow_ref_list) {
>>>>>>>         struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn);
>>>>>>>         lfrn->lflow = lflow;
>>>>>>>         ovs_list_insert(lflow_ref_list, &lfrn->lflow_list_node);
>>>>>>>         ovs_list_insert(&lflow->referenced_by,
> &lfrn->ref_list_node);
>>>>>>>     }
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> #define ovn_lflow_add_with_lport_and_hint(LFLOW_MAP, OD, STAGE,
>>> PRIORITY,
>>>>>> \
>>>>>>>                                           MATCH, ACTIONS,
>>> IN_OUT_PORT, \
>>>>>>>                                           LFLOW_REF_LIST,
> STAGE_HINT)
>>> \
>>>>>>>     ovn_lflow_add_at(LFLOW_MAP, OD, NULL, 0, STAGE, PRIORITY,
> MATCH,
>>>>>> ACTIONS, \
>>>>>>>                      IN_OUT_PORT, LFLOW_REF_LIST, NULL,
> STAGE_HINT, \
>>>>>>>                      OVS_SOURCE_LOCATOR)
>>>>>>>
>>>>>>> And pass &op->lflows in ovn_lflow_add_with_lport_and_hint()
>>>>>>>
>>>>>>> -----------------------------
>>>>>>>
>>>>>>> What do you think ?  Definitely this would result in a bit more
> work
>>>>>>> as it would require a lot of (tedious) changes.
>>>>>>> I think this is a better approach.
>>>>>>>
>>>>>> Firstly, I think it is not good to use "lflow_ref_list" directly
> in the
>>>>>> parameter, because there can be more than one object contributing
> to
>>> the
>>>>>> lflow generation. When we implement I-P for more inputs, a single
>>> lflow may
>>>>>> be referenced by multiple objects. We can't pass multiple
>>> lflow_ref_list to
>>>>>> the function either, because the number of such lists is unknown.
> For
>>>>>> example, a lflow may be generated as a result of a LSP, a DP and a
> LB
>>>>>> backend. If we want to implement I-P for LSP, DP and LB backend, we
>>> need to
>>>>>> track the reference for all of them. So the current idea is just to
>>> collect
>>>>>> a list of lflows generated by a higher level function, such as the
>>>>>> build_lswitch_and_lrouter_iterate_by_lsp. When implementing I-P for
>>> more
>>>>>> than one object of the same lflow, this needs to be more
> fine-grained.
>>>>
>>>> I'm still reviewing this patch but my first impression was that we can
>>>> probably try to use 'struct objdep_mgr' (defined in lib/objdep.h) to
>>>> model all these (potentially many-to-many) relationships.  We do
> similar
>>>> things in ovn-controller.
>>>>
>>> My first impression was similar, but later I figured out it is different
>>> (and more complex than ovn-controller in general) and we will need to
> make
>>> changes to objdep_mgr. However, until more input I-P is implemented I am
>>> not sure if I will be able to get a general enough abstraction in
>>> objdep_mgr that can handle all the use cases. On the other hand, the
>>> references between LSP and related lflows are quite simple, so I went
> ahead
>>> and implemented the lists (embeded into ovn_port and lflow without extra
>>> hash tables), with minimum effort, to make it work at least for this
>>> scenario and should also work for similar scenarios. I think we can
>>> refactor the reference implementation any time but it may be better to
> get
>>> more I-P implemented, during the process we can always revisit and see
> if
>>> objdep_mgr can be extended or another common dependency/reference
> library
>>> for ovn-northd can be created, etc. What do you think?
>>
>> As part of my refactor and northd I-P for load balancers and data
>> paths,  I'll try to see
>> if objdep_mgr can be used.  For now with just lport I-P,  I suppose it's
> fine.
>>
>> Acked-by: Numan Siddique <numans@ovn.org>
>>
> Haven't heard back from Dumitru for a while, so I assume you are fine with
> this (but please share any time if you have more comments).
> Thank you both, Numan and Dumitru. I applied this to main.
> 

Sorry, Han, I'm quite slow in reviewing.  I'm OK with you applying the
patches, I wouldn't want to block development.  If I find anything later
on I'll post follow up fixes or reports.

I still think it's best to reuse objdep_mgr and I think Numan is working
on changing to that in his series so we're fine.

Regards,
Dumitru