On 6/27/23 10:23, Numan Siddique wrote:
> On Mon, Jun 26, 2023 at 10:34 PM Han Zhou <hz...@ovn.org> wrote:
>>
>> On Mon, Jun 26, 2023 at 7:25 AM Numan Siddique <num...@ovn.org> wrote:
>>>
>>> On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hz...@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 <hz...@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
>>>> d...@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
