On 11/15/23 19:22, Numan Siddique wrote:
> On Wed, Nov 15, 2023 at 12:12 AM Han Zhou <[email protected]> wrote:
>>
>> On Thu, Oct 26, 2023 at 11:14 AM <[email protected]> wrote:
>>>
>>> From: Numan Siddique <[email protected]>
>>>
>>> northd engine tracking data now has the following tracking data
>>> - changed ovn_ports (right now only changed logical switch ports are
>>> tracked.)
>>> - changed load balancers.
>>>
>>> This separation becomes easier to add lflow handling for these
>>> changes in lflow northd engine handler. This patch doesn't
>>> handle the load balancer changes in lflow handler. It will
>>> be handled in upcoming commits.
>>>
>>> Signed-off-by: Numan Siddique <[email protected]>
>>> ---
Hi Numan,
I have a couple of comments too below, nothing fundamental but
hopefully something that would simplify the code a bit.
Regards,
Dumitru
>>> northd/en-lflow.c | 11 +-
>>> northd/en-northd.c | 13 +-
>>> northd/en-sync-sb.c | 10 +-
>>> northd/northd.c | 446 ++++++++++++++++++++++++--------------------
>>> northd/northd.h | 63 ++++---
>>> tests/ovn-northd.at | 10 +-
>>> 6 files changed, 313 insertions(+), 240 deletions(-)
>>>
>>> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
>>> index 2b84fef0ef..96d03b7ada 100644
>>> --- a/northd/en-lflow.c
>>> +++ b/northd/en-lflow.c
>>> @@ -108,8 +108,8 @@ lflow_northd_handler(struct engine_node *node,
>>> return false;
>>> }
>>>
>>> - /* Fall back to recompute if lb related data has changed. */
>>> - if (northd_data->lb_changed) {
>>> + /* Fall back to recompute if load balancers have changed. */
>>> + if
>> (northd_has_lbs_in_tracked_data(&northd_data->trk_northd_changes)) {
>>> return false;
>>> }
>>>
>>> @@ -119,13 +119,14 @@ lflow_northd_handler(struct engine_node *node,
>>> struct lflow_input lflow_input;
>>> lflow_get_input_data(node, &lflow_input);
>>>
>>> - if (!lflow_handle_northd_ls_changes(eng_ctx->ovnsb_idl_txn,
>>> - &northd_data->tracked_ls_changes,
>>> - &lflow_input,
>> &lflow_data->lflows)) {
>>> + if (!lflow_handle_northd_port_changes(eng_ctx->ovnsb_idl_txn,
>>> +
>> &northd_data->trk_northd_changes.trk_ovn_ports,
>>> + &lflow_input, &lflow_data->lflows)) {
>>> return false;
>>> }
>>>
>>> engine_set_node_state(node, EN_UPDATED);
>>> +
Nit: unrelated?
>>> return true;
>>> }
>>>
>>> diff --git a/northd/en-northd.c b/northd/en-northd.c
>>> index aa0f20f0c2..96c2ce9f69 100644
>>> --- a/northd/en-northd.c
>>> +++ b/northd/en-northd.c
>>> @@ -230,15 +230,16 @@ northd_lb_data_handler(struct engine_node *node,
>> void *data)
>>> &nd->ls_datapaths,
>>> &nd->lr_datapaths,
>>> &nd->lb_datapaths_map,
>>> - &nd->lb_group_datapaths_map)) {
>>> + &nd->lb_group_datapaths_map,
>>> + &nd->trk_northd_changes)) {
>>> return false;
>>> }
>>>
>>> - /* Indicate the depedendant engine nodes that load balancer/group
>>> - * related data has changed (including association to logical
>>> - * switch/router). */
>>> - nd->lb_changed = true;
>>> - engine_set_node_state(node, EN_UPDATED);
>>> + if (northd_has_lbs_in_tracked_data(&nd->trk_northd_changes)) {
>>> + nd->change_tracked = true;
>>> + engine_set_node_state(node, EN_UPDATED);
>>> + }
>>> +
>>> return true;
>>> }
>>>
>>> diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
>>> index 2ec3bf54f8..2540fcfb97 100644
>>> --- a/northd/en-sync-sb.c
>>> +++ b/northd/en-sync-sb.c
>>> @@ -236,7 +236,8 @@ sync_to_sb_lb_northd_handler(struct engine_node
>> *node, void *data OVS_UNUSED)
>>> {
>>> struct northd_data *nd = engine_get_input_data("northd", node);
>>>
>>> - if (!nd->change_tracked || nd->lb_changed) {
>>> + if (!nd->change_tracked ||
>>> + northd_has_lbs_in_tracked_data(&nd->trk_northd_changes)) {
>>> /* Return false if no tracking data or if lbs changed. */
>>> return false;
>>> }
>>> @@ -306,11 +307,14 @@ sync_to_sb_pb_northd_handler(struct engine_node
>> *node, void *data OVS_UNUSED)
>>> }
>>>
>>> struct northd_data *nd = engine_get_input_data("northd", node);
>>> - if (!nd->change_tracked) {
>>> + if (!nd->change_tracked ||
>>> + northd_has_lbs_in_tracked_data(&nd->trk_northd_changes)) {
>>> + /* Return false if no tracking data or if lbs changed. */
>>> return false;
>>> }
>>>
>>> - if (!sync_pbs_for_northd_ls_changes(&nd->tracked_ls_changes)) {
>>> + if (!sync_pbs_for_northd_changed_ovn_ports(
>>> + &nd->trk_northd_changes.trk_ovn_ports)) {
>>> return false;
>>> }
>>>
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index f8b046d83e..df22a9c658 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -4894,21 +4894,20 @@ sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn,
>> struct hmap *ls_ports)
>>> }
>>>
>>> /* Sync the SB Port bindings for the added and updated logical switch
>> ports
>>> - * of the tracked logical switches (from the northd engine node). */
>>> + * of the tracked northd engine data. */
>>> bool
>>> -sync_pbs_for_northd_ls_changes(struct tracked_ls_changes *ls_changes)
>>> +sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports
>> *trk_ovn_ports)
>>> {
>>> - struct ls_change *ls_change;
>>> - LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
>>> - struct ovn_port *op;
>>> -
>>> - LIST_FOR_EACH (op, list, &ls_change->added_ports) {
>>> - sync_pb_for_op(op);
>>> - }
>>> + struct hmapx_node *hmapx_node;
>>> + struct ovn_port *op;
>>> + HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) {
>>> + op = hmapx_node->data;
>>> + sync_pb_for_op(op);
>>> + }
>>>
>>> - LIST_FOR_EACH (op, list, &ls_change->updated_ports) {
>>> - sync_pb_for_op(op);
>>> - }
>>> + HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) {
>>> + op = hmapx_node->data;
>>> + sync_pb_for_op(op);
>>> }
>>>
>>> return true;
>>> @@ -5110,34 +5109,95 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
>>> }
>>>
>>> static void
>>> -destroy_tracked_ls_change(struct ls_change *ls_change)
>>> +destroy_tracked_ovn_ports(struct tracked_ovn_ports *trk_ovn_ports)
>>> {
>>> - struct ovn_port *op;
>>> - LIST_FOR_EACH (op, list, &ls_change->added_ports) {
>>> - ovs_list_remove(&op->list);
>>> - }
>>> - LIST_FOR_EACH (op, list, &ls_change->updated_ports) {
>>> - ovs_list_remove(&op->list);
>>> + struct hmapx_node *hmapx_node;
>>> + HMAPX_FOR_EACH_SAFE (hmapx_node, &trk_ovn_ports->deleted) {
>>> + ovn_port_destroy_orphan(hmapx_node->data);
>>> + hmapx_delete(&trk_ovn_ports->deleted, hmapx_node);
>>> }
>>> - LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) {
>>> - ovs_list_remove(&op->list);
>>> - ovn_port_destroy_orphan(op);
>>> +
>>> + hmapx_clear(&trk_ovn_ports->created);
>>> + hmapx_clear(&trk_ovn_ports->updated);
>>> +}
>>> +
>>> +static void
>>> +destroy_tracked_lbs(struct tracked_lbs *trk_lbs)
>>> +{
>>> + struct hmapx_node *hmapx_node;
>>> + HMAPX_FOR_EACH_SAFE (hmapx_node, &trk_lbs->deleted) {
>>> + ovn_lb_datapaths_destroy(hmapx_node->data);
>>> + hmapx_delete(&trk_lbs->deleted, hmapx_node);
>>> }
>>> +
>>> + hmapx_clear(&trk_lbs->crupdated);
>>> +}
>>> +
>>> +static void
>>> +add_op_to_northd_tracked_ports(struct hmapx *tracked_ovn_ports,
>>> + struct ovn_port *op)
>>> +{
>>> + hmapx_add(tracked_ovn_ports, op);
>>> }
>>>
>>> void
>>> destroy_northd_data_tracked_changes(struct northd_data *nd)
>>> {
>>> - struct ls_change *ls_change;
>>> - LIST_FOR_EACH_SAFE (ls_change, list_node,
>>> - &nd->tracked_ls_changes.updated) {
>>> - destroy_tracked_ls_change(ls_change);
>>> - ovs_list_remove(&ls_change->list_node);
>>> - free(ls_change);
>>> - }
>>> -
>>> + struct northd_tracked_data *trk_changes = &nd->trk_northd_changes;
>>> + destroy_tracked_ovn_ports(&trk_changes->trk_ovn_ports);
>>> + destroy_tracked_lbs(&trk_changes->trk_lbs);
>>> nd->change_tracked = false;
>>> - nd->lb_changed = false;
>>> +}
>>> +
>>> +static void
>>> +init_northd_tracked_data(struct northd_data *nd)
>>> +{
>>> + struct northd_tracked_data *trk_changes = &nd->trk_northd_changes;
>>> + hmapx_init(&trk_changes->trk_ovn_ports.created);
>>> + hmapx_init(&trk_changes->trk_ovn_ports.updated);
>>> + hmapx_init(&trk_changes->trk_ovn_ports.deleted);
>>> + hmapx_init(&trk_changes->trk_lbs.crupdated);
>>> + hmapx_init(&trk_changes->trk_lbs.deleted);
>>> +}
>>> +
>>> +static void
>>> +destroy_northd_tracked_data(struct northd_data *nd)
>>> +{
>>> + struct northd_tracked_data *trk_changes = &nd->trk_northd_changes;
>>> + hmapx_destroy(&trk_changes->trk_ovn_ports.created);
>>> + hmapx_destroy(&trk_changes->trk_ovn_ports.updated);
>>> + hmapx_destroy(&trk_changes->trk_ovn_ports.deleted);
>>> + hmapx_destroy(&trk_changes->trk_lbs.crupdated);
>>> + hmapx_destroy(&trk_changes->trk_lbs.deleted);
>>> +}
>>> +
>>> +
>>> +bool
>>> +northd_has_tracked_data(struct northd_tracked_data *trk_nd_changes)
>>> +{
>>> + return (!hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.created)
>>> + || !hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.updated)
>>> + || !hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.deleted)
>>> + || !hmapx_is_empty(&trk_nd_changes->trk_lbs.crupdated)
>>> + || !hmapx_is_empty(&trk_nd_changes->trk_lbs.deleted));
>>> +}
>>> +
>>> +bool
>>> +northd_has_only_ports_in_tracked_data(
>>> + struct northd_tracked_data *trk_nd_changes)
>>> +{
>>> + return (hmapx_is_empty(&trk_nd_changes->trk_lbs.crupdated)
>>> + && hmapx_is_empty(&trk_nd_changes->trk_lbs.deleted)
>>> + && (!hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.created)
>>> + || !hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.updated)
>>> + || !hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.deleted)));
>>> +}
>>> +
As far as I can tell the northd_has_only_ports_in_tracked_data() and
northd_has_tracked_data() are never used in this whole series.
What if we have a single function that returns a strong typed value
(enum) that identifies the change type instead? Something like (not
tested, just compiled):
enum northd_tracked_data_type {
NORTHD_TRACKED_NONE,
NORTHD_TRACKED_PORTS = (1 << 0),
NORTHD_TRACKED_LBS = (1 << 1),
};
enum northd_tracked_data_type
northd_tracked_data_classify(const struct northd_tracked_data *trk_nd_changes)
{
enum northd_tracked_data_type td_type = NORTHD_TRACKED_NONE;
if (hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.created)
&& hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.updated)
&& hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.deleted)
&& hmapx_is_empty(&trk_nd_changes->trk_lbs.crupdated)
&& hmapx_is_empty(&trk_nd_changes->trk_lbs.deleted)) {
goto done;
}
if (!hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.created)
|| !hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.updated)
|| !hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.deleted)) {
td_type |= NORTHD_TRACKED_PORTS;
}
if (!hmapx_is_empty(&trk_nd_changes->trk_lbs.crupdated)
|| !hmapx_is_empty(&trk_nd_changes->trk_lbs.deleted)) {
td_type |= NORTHD_TRACKED_LBS;
}
done:
return td_type;
}
We can also get rid of northd_data->change_tracked, it's included
the northd_tracked_data structure implicitly.
>>> +bool
>>> +northd_has_lbs_in_tracked_data(struct northd_tracked_data
>> *trk_nd_changes)
>>> +{
>>> + return (!hmapx_is_empty(&trk_nd_changes->trk_lbs.crupdated)
>>> + || !hmapx_is_empty(&trk_nd_changes->trk_lbs.deleted));
>>> }
>>>
>>> /* Check if a changed LSP can be handled incrementally within the I-P
>> engine
>>> @@ -5344,12 +5404,12 @@ check_lsp_changes_other_than_up(const struct
>> nbrec_logical_switch_port *nbsp)
>>> */
>>> static bool
>>> ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
>>> - const struct nbrec_logical_switch
>> *changed_ls,
>>> - const struct northd_input *ni,
>>> - struct northd_data *nd,
>>> - struct ovn_datapath *od,
>>> - struct ls_change *ls_change,
>>> - bool *updated)
>>> + const struct nbrec_logical_switch *changed_ls,
>>> + const struct northd_input *ni,
>>> + struct northd_data *nd,
>>> + struct ovn_datapath *od,
>>> + struct tracked_ovn_ports *trk_ports,
>>> + bool *updated)
>>> {
>>> bool ls_ports_changed = false;
>>> if (!nbrec_logical_switch_is_updated(changed_ls,
>>> @@ -5370,7 +5430,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>>> return true;
>>> }
>>>
>>> - ls_change->had_only_router_ports = (od->n_router_ports
>>> + bool ls_had_only_router_ports = (od->n_router_ports
>>> && (od->n_router_ports == hmap_count(&od->ports)));
>>>
>>> struct ovn_port *op;
>>> @@ -5396,8 +5456,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>>> if (!op) {
>>> goto fail;
>>> }
>>> - ovs_list_push_back(&ls_change->added_ports,
>>> - &op->list);
>>> + add_op_to_northd_tracked_ports(&trk_ports->created, op);
>>> } else if (ls_port_has_changed(op->nbsp, new_nbsp)) {
>>> /* Existing port updated */
>>> bool temp = false;
>>> @@ -5433,7 +5492,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>>> if (!op) {
>>> goto fail;
>>> }
>>> - ovs_list_push_back(&ls_change->updated_ports, &op->list);
>>> + add_op_to_northd_tracked_ports(&trk_ports->updated, op);
>>> }
>>> op->visited = true;
>>> }
>>> @@ -5442,15 +5501,14 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>>> HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) {
>>> if (!op->visited) {
>>> if (!op->lsp_can_be_inc_processed) {
>>> - goto fail_clean_deleted;
>>> + goto fail;
>>> }
>>> if (sset_contains(&nd->svc_monitor_lsps, op->key)) {
>>> /* This port was used for svc monitor, which may be
>>> * impacted by this deletion. Fallback to recompute. */
>>> - goto fail_clean_deleted;
>>> + goto fail;
>>> }
>>> - ovs_list_push_back(&ls_change->deleted_ports,
>>> - &op->list);
>>> + add_op_to_northd_tracked_ports(&trk_ports->deleted, op);
>>> hmap_remove(&nd->ls_ports, &op->key_node);
>>> hmap_remove(&od->ports, &op->dp_node);
>>> sbrec_port_binding_delete(op->sb);
>>> @@ -5459,20 +5517,29 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>>> }
>>> }
>>>
>>> - if (!ovs_list_is_empty(&ls_change->added_ports) ||
>>> - !ovs_list_is_empty(&ls_change->updated_ports) ||
>>> - !ovs_list_is_empty(&ls_change->deleted_ports)) {
>>> + bool ls_has_only_router_ports = (od->n_router_ports
>>> + && (od->n_router_ports == hmap_count(&od->ports)));
>>> +
>>> + /* If there were only router ports before the change or if there are
>> only
>>> + * router ports after the change, then add the router ports of the
>>> + * logical switch to the tracked 'updated' ovn ports. */
>>
>> This comment doesn't explain why we are doing this, so it may still be hard
>> to understand. The real reason as what I understand, is what explained in
>> the comment deleted by this patch:
>> /* There are lflows related to router ports that depends on whether
>> * there are switch ports on the logical switch (see
>> * build_lswitch_rport_arp_req_flow() for more details). Since this
>> * dependency changed, we need to regenerate lflows for each router
>> * port on this logical switch. */
>> It may be better to incorporate that here.
>
> Ack.
>
>
>>
>>> + if (ls_had_only_router_ports != ls_has_only_router_ports) {
>>> + for (size_t i = 0; i < od->n_router_ports; i++) {
>>> + op = od->router_ports[i];
>>> + add_op_to_northd_tracked_ports(&trk_ports->updated, op);
>>> + }
>>> + }
>>> +
>>> + if (!hmapx_is_empty(&trk_ports->created) ||
>>> + !hmapx_is_empty(&trk_ports->updated) ||
>>> + !hmapx_is_empty(&trk_ports->deleted)) {
>>> *updated = true;
>>> }
>>>
>>> return true;
>>>
>>> -fail_clean_deleted:
>>> - LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) {
>>> - ovn_port_destroy_orphan(op);
>>> - }
>>> -
>>> fail:
>>> + destroy_tracked_ovn_ports(trk_ports);
>>> return false;
>>> }
>>>
>>> @@ -5490,7 +5557,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>>> struct northd_data *nd)
>>> {
>>> const struct nbrec_logical_switch *changed_ls;
>>> - struct ls_change *ls_change = NULL;
>>> + struct northd_tracked_data *trk_nd_changes = &nd->trk_northd_changes;
>>>
>>> NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (changed_ls,
>>>
>> ni->nbrec_logical_switch_table) {
>>> @@ -5514,33 +5581,18 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>>> goto fail;
>>> }
>>>
>>> - ls_change = xzalloc(sizeof *ls_change);
>>> - ls_change->od = od;
>>> - ovs_list_init(&ls_change->added_ports);
>>> - ovs_list_init(&ls_change->deleted_ports);
>>> - ovs_list_init(&ls_change->updated_ports);
>>> -
>>> bool updated = false;
>>> if (!ls_handle_lsp_changes(ovnsb_idl_txn, changed_ls,
>>> - ni, nd, od, ls_change,
>>> + ni, nd, od,
>> &trk_nd_changes->trk_ovn_ports,
>>> &updated)) {
>>> - destroy_tracked_ls_change(ls_change);
>>> - free(ls_change);
>>> goto fail;
>>> }
>>>
>>> if (updated) {
>>> - ovs_list_push_back(&nd->tracked_ls_changes.updated,
>>> - &ls_change->list_node);
>>> - } else {
>>> - free(ls_change);
>>> + nd->change_tracked = true;
>>> }
>>> }
>>>
>>> - if (!ovs_list_is_empty(&nd->tracked_ls_changes.updated)) {
>>> - nd->change_tracked = true;
>>> - }
>>> -
>>> return true;
>>>
>>> fail:
>>> @@ -5715,7 +5767,8 @@ northd_handle_lb_data_changes(struct
>> tracked_lb_data *trk_lb_data,
>>> struct ovn_datapaths *ls_datapaths,
>>> struct ovn_datapaths *lr_datapaths,
>>> struct hmap *lb_datapaths_map,
>>> - struct hmap *lbgrp_datapaths_map)
>>> + struct hmap *lbgrp_datapaths_map,
>>> + struct northd_tracked_data *nd_changes)
>>> {
>>> if (trk_lb_data->has_health_checks) {
>>> /* Fall back to recompute since a tracked load balancer
>>> @@ -5778,7 +5831,9 @@ northd_handle_lb_data_changes(struct
>> tracked_lb_data *trk_lb_data,
>>> }
>>>
>>> hmap_remove(lb_datapaths_map, &lb_dps->hmap_node);
>>> - ovn_lb_datapaths_destroy(lb_dps);
>>> +
>>> + /* Add the deleted lb to the northd tracked data. */
>>> + hmapx_add(&nd_changes->trk_lbs.deleted, lb_dps);
>>> }
>>>
>>> /* Create the 'lb_dps' if not already created for each
>>> @@ -5795,6 +5850,9 @@ northd_handle_lb_data_changes(struct
>> tracked_lb_data *trk_lb_data,
>>> hmap_insert(lb_datapaths_map, &lb_dps->hmap_node,
>>> uuid_hash(lb_uuid));
>>> }
>>> +
>>> + /* Add the updated lb to the northd tracked data. */
>>> + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
>>> }
>>>
>>> struct ovn_lb_group_datapaths *lbgrp_dps;
>>> @@ -5825,6 +5883,9 @@ northd_handle_lb_data_changes(struct
>> tracked_lb_data *trk_lb_data,
>>> lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
>> &uuidnode->uuid);
>>> ovs_assert(lb_dps);
>>> ovn_lb_datapaths_add_ls(lb_dps, 1, &od);
>>> +
>>> + /* Add the lb to the northd tracked data. */
>>> + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
>>> }
>>>
>>> UUIDSET_FOR_EACH (uuidnode, &codlb->assoc_lbgrps) {
>>> @@ -5840,6 +5901,9 @@ northd_handle_lb_data_changes(struct
>> tracked_lb_data *trk_lb_data,
>>> lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
>> lb_uuid);
>>> ovs_assert(lb_dps);
>>> ovn_lb_datapaths_add_ls(lb_dps, 1, &od);
>>> +
>>> + /* Add the lb to the northd tracked data. */
>>> + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
>>> }
>>> }
>>>
>>> @@ -5860,6 +5924,9 @@ northd_handle_lb_data_changes(struct
>> tracked_lb_data *trk_lb_data,
>>> /* Add the lb_ips of lb_dps to the od. */
>>> build_lrouter_lb_ips(od->lb_ips, lb_dps->lb);
>>> build_lrouter_lb_reachable_ips(od, lb_dps->lb);
>>> +
>>> + /* Add the lb to the northd tracked data. */
>>> + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
>>> }
>>>
>>> UUIDSET_FOR_EACH (uuidnode, &codlb->assoc_lbgrps) {
>>> @@ -5879,6 +5946,9 @@ northd_handle_lb_data_changes(struct
>> tracked_lb_data *trk_lb_data,
>>> /* Add the lb_ips of lb_dps to the od. */
>>> build_lrouter_lb_ips(od->lb_ips, lb_dps->lb);
>>> build_lrouter_lb_reachable_ips(od, lb_dps->lb);
>>> +
>>> + /* Add the lb to the northd tracked data. */
>>> + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
>>> }
>>> }
>>>
>>> @@ -5957,6 +6027,9 @@ northd_handle_lb_data_changes(struct
>> tracked_lb_data *trk_lb_data,
>>> /* Re-evaluate 'od->has_lb_vip' */
>>> init_lb_for_datapath(od);
>>> }
>>> +
>>> + /* Add the lb to the northd tracked data. */
>>> + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
>>> }
>>> }
>>>
>>> @@ -16996,149 +17069,125 @@ delete_lflow_for_lsp(struct ovn_port *op,
>> bool is_update,
>>> return true;
>>> }
>>>
>>> -bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn,
>>> - struct tracked_ls_changes
>> *ls_changes,
>>> - struct lflow_input *lflow_input,
>>> - struct hmap *lflows)
>>> +bool
>>> +lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn,
>>> + struct tracked_ovn_ports *trk_ovn_ports,
>>> + struct lflow_input *lflow_input,
>>> + struct hmap *lflows)
>>> {
>>> - struct ls_change *ls_change;
>>> + struct hmapx_node *hmapx_node;
>>> + struct ovn_port *op;
>>>
>>> - LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
>>> - const struct sbrec_multicast_group *sbmc_flood =
>>> - mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
>>> - MC_FLOOD, ls_change->od->sb);
>>> - const struct sbrec_multicast_group *sbmc_flood_l2 =
>>> - mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
>>> - MC_FLOOD_L2, ls_change->od->sb);
>>> - const struct sbrec_multicast_group *sbmc_unknown =
>>> - mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
>>> - MC_UNKNOWN, ls_change->od->sb);
>>> + HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->deleted) {
>>> + op = hmapx_node->data;
>>> + /* We don't support lflow handling for deleted logical router
>>> + * ports yet. */
>>> + ovs_assert(op->nbsp);
>>>
>>> - struct ovn_port *op;
>>> - LIST_FOR_EACH (op, list, &ls_change->deleted_ports) {
>>> - if (!delete_lflow_for_lsp(op, false,
>>> -
>> lflow_input->sbrec_logical_flow_table,
>>> - lflows)) {
>>> + if (!delete_lflow_for_lsp(op, false,
>>> + lflow_input->sbrec_logical_flow_table,
>>> + lflows)) {
>>> return false;
>>> }
>>>
>>> - /* No need to update SB multicast groups, thanks to weak
>>> - * references. */
>>> - }
>>> + /* No need to update SB multicast groups, thanks to weak
>>> + * references. */
>>> + }
>>>
>>> - LIST_FOR_EACH (op, list, &ls_change->updated_ports) {
>>> - /* Delete old lflows. */
>>> - if (!delete_lflow_for_lsp(op, true,
>>> -
>> lflow_input->sbrec_logical_flow_table,
>>> - lflows)) {
>>> - return false;
>>> - }
>>> + HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) {
>>> + op = hmapx_node->data;
>>> + /* We don't support lflow handling for updated logical router
>>> + * ports yet. */
>>> + ovs_assert(op->nbsp);
>>>
>>> - /* Generate new lflows. */
>>> - struct ds match = DS_EMPTY_INITIALIZER;
>>> - struct ds actions = DS_EMPTY_INITIALIZER;
>>> - build_lswitch_and_lrouter_iterate_by_lsp(op,
>> lflow_input->ls_ports,
>>> -
>> lflow_input->lr_ports,
>>> -
>> lflow_input->meter_groups,
>>> - &match, &actions,
>>> - lflows);
>>> - ds_destroy(&match);
>>> - ds_destroy(&actions);
>>> -
>>> - /* SB port_binding is not deleted, so don't update SB
>> multicast
>>> - * groups. */
>>> -
>>> - /* Sync the new flows to SB. */
>>> - struct lflow_ref_node *lfrn;
>>> - LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
>>> - sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows,
>>> - lfrn->lflow);
>>> - }
>>> + /* Delete old lflows. */
>>> + if (!delete_lflow_for_lsp(op, true,
>>> + lflow_input->sbrec_logical_flow_table,
>>> + lflows)) {
>>> + return false;
>>> }
>>>
>>> - LIST_FOR_EACH (op, list, &ls_change->added_ports) {
>>> - struct ds match = DS_EMPTY_INITIALIZER;
>>> - struct ds actions = DS_EMPTY_INITIALIZER;
>>> - build_lswitch_and_lrouter_iterate_by_lsp(op,
>> lflow_input->ls_ports,
>>> -
>> lflow_input->lr_ports,
>>> -
>> lflow_input->meter_groups,
>>> - &match, &actions,
>>> - lflows);
>>> - ds_destroy(&match);
>>> - ds_destroy(&actions);
>>> -
>>> - /* Update SB multicast groups for the new port. */
>>> - if (!sbmc_flood) {
>>> - sbmc_flood = create_sb_multicast_group(ovnsb_txn,
>>> - ls_change->od->sb, MC_FLOOD,
>> OVN_MCAST_FLOOD_TUNNEL_KEY);
>>> - }
>>> - sbrec_multicast_group_update_ports_addvalue(sbmc_flood,
>> op->sb);
>>> + /* Generate new lflows. */
>>> + struct ds match = DS_EMPTY_INITIALIZER;
>>> + struct ds actions = DS_EMPTY_INITIALIZER;
>>> + build_lswitch_and_lrouter_iterate_by_lsp(op,
>> lflow_input->ls_ports,
>>> + lflow_input->lr_ports,
>>> +
>> lflow_input->meter_groups,
>>> + &match, &actions,
>>> + lflows);
>>> + ds_destroy(&match);
>>> + ds_destroy(&actions);
>>>
>>> - if (!sbmc_flood_l2) {
>>> - sbmc_flood_l2 = create_sb_multicast_group(ovnsb_txn,
>>> - ls_change->od->sb, MC_FLOOD_L2,
>>> - OVN_MCAST_FLOOD_L2_TUNNEL_KEY);
>>> - }
>>> - sbrec_multicast_group_update_ports_addvalue(sbmc_flood_l2,
>> op->sb);
>>> -
>>> - if (op->has_unknown) {
>>> - if (!sbmc_unknown) {
>>> - sbmc_unknown = create_sb_multicast_group(ovnsb_txn,
>>> - ls_change->od->sb, MC_UNKNOWN,
>>> - OVN_MCAST_UNKNOWN_TUNNEL_KEY);
>>> - }
>>> - sbrec_multicast_group_update_ports_addvalue(sbmc_unknown,
>>> - op->sb);
>>> - }
>>> + /* SB port_binding is not deleted, so don't update SB multicast
>>> + * groups. */
>>>
>>> - /* Sync the newly added flows to SB. */
>>> - struct lflow_ref_node *lfrn;
>>> - LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
>>> - sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows,
>>> - lfrn->lflow);
>>> - }
>>> + /* Sync the new flows to SB. */
>>> + struct lflow_ref_node *lfrn;
>>> + LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
>>> + sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows,
>>> + lfrn->lflow);
>>> }
>>> + }
>>>
>>> - bool ls_has_only_router_ports = (ls_change->od->n_router_ports &&
>>> - (ls_change->od->n_router_ports
>> ==
>>> -
>> hmap_count(&ls_change->od->ports)));
>>> -
>>> - if (ls_change->had_only_router_ports !=
>> ls_has_only_router_ports) {
>>> - /* There are lflows related to router ports that depends on
>> whether
>>> - * there are switch ports on the logical switch (see
>>> - * build_lswitch_rport_arp_req_flow() for more details).
>> Since this
>>> - * dependency changed, we need to regenerate lflows for each
>> router
>>> - * port on this logical switch. */
>>> - for (size_t i = 0; i < ls_change->od->n_router_ports; i++) {
>>> - op = ls_change->od->router_ports[i];
>>> -
>>> - /* Delete old lflows. */
>>> - if (!delete_lflow_for_lsp(op, "affected router",
>>> -
>> lflow_input->sbrec_logical_flow_table,
>>> - lflows)) {
>>> - return false;
>>> - }
>>> + HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) {
>>> + op = hmapx_node->data;
>>> + /* We don't support lflow handling for created logical router
>>> + * ports yet. */
>>> + ovs_assert(op->nbsp);
>>>
>>> - /* Generate new lflows. */
>>> - struct ds match = DS_EMPTY_INITIALIZER;
>>> - struct ds actions = DS_EMPTY_INITIALIZER;
>>> - build_lswitch_and_lrouter_iterate_by_lsp(op,
>>> - lflow_input->ls_ports, lflow_input->lr_ports,
>>> - lflow_input->meter_groups, &match, &actions, lflows);
>>> - ds_destroy(&match);
>>> - ds_destroy(&actions);
>>> -
>>> - /* Sync the new flows to SB. */
>>> - struct lflow_ref_node *lfrn;
>>> - LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
>>> - sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows,
>>> - lfrn->lflow);
>>> - }
>>> + const struct sbrec_multicast_group *sbmc_flood =
>>> + mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
>>> + MC_FLOOD, op->od->sb);
>>> + const struct sbrec_multicast_group *sbmc_flood_l2 =
>>> + mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
>>> + MC_FLOOD_L2, op->od->sb);
>>> + const struct sbrec_multicast_group *sbmc_unknown =
>>> + mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
>>> + MC_UNKNOWN, op->od->sb);
>>> +
>>> + struct ds match = DS_EMPTY_INITIALIZER;
>>> + struct ds actions = DS_EMPTY_INITIALIZER;
>>> + build_lswitch_and_lrouter_iterate_by_lsp(op,
>> lflow_input->ls_ports,
>>> +
>> lflow_input->lr_ports,
>>> +
>> lflow_input->meter_groups,
>>> + &match, &actions,
>>> + lflows);
>>> + ds_destroy(&match);
>>> + ds_destroy(&actions);
>>> +
>>> + /* Update SB multicast groups for the new port. */
>>> + if (!sbmc_flood) {
>>> + sbmc_flood = create_sb_multicast_group(ovnsb_txn,
>>> + op->od->sb, MC_FLOOD, OVN_MCAST_FLOOD_TUNNEL_KEY);
>>> + }
>>> + sbrec_multicast_group_update_ports_addvalue(sbmc_flood, op->sb);
>>> +
>>> + if (!sbmc_flood_l2) {
>>> + sbmc_flood_l2 = create_sb_multicast_group(ovnsb_txn,
>>> + op->od->sb, MC_FLOOD_L2,
>>> + OVN_MCAST_FLOOD_L2_TUNNEL_KEY);
>>> + }
>>> + sbrec_multicast_group_update_ports_addvalue(sbmc_flood_l2,
>> op->sb);
>>> +
>>> + if (op->has_unknown) {
>>> + if (!sbmc_unknown) {
>>> + sbmc_unknown = create_sb_multicast_group(ovnsb_txn,
>>> + op->od->sb, MC_UNKNOWN,
>>> + OVN_MCAST_UNKNOWN_TUNNEL_KEY);
>>> }
>>> + sbrec_multicast_group_update_ports_addvalue(sbmc_unknown,
>>> + op->sb);
>>> + }
>>> +
>>> + /* Sync the newly added flows to SB. */
>>> + struct lflow_ref_node *lfrn;
>>> + LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) {
>>> + sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows,
>>> + lfrn->lflow);
>>> }
>>> }
>>> - return true;
>>>
>>> + return true;
>>> }
>>>
>>> static bool
>>> @@ -17771,7 +17820,7 @@ northd_init(struct northd_data *data)
>>> sset_init(&data->svc_monitor_lsps);
>>> hmap_init(&data->svc_monitor_map);
>>> data->change_tracked = false;
>>> - ovs_list_init(&data->tracked_ls_changes.updated);
>>> + init_northd_tracked_data(data);
>>> }
>>>
>>> void
>>> @@ -17811,6 +17860,7 @@ northd_destroy(struct northd_data *data)
>>> destroy_debug_config();
>>>
>>> sset_destroy(&data->svc_monitor_lsps);
>>> + destroy_northd_tracked_data(data);
>>> }
>>>
>>> void
>>> diff --git a/northd/northd.h b/northd/northd.h
>>> index 5be7b5384d..2e4f125902 100644
>>> --- a/northd/northd.h
>>> +++ b/northd/northd.h
>>> @@ -83,22 +83,36 @@ struct ovn_datapaths {
>>> struct ovn_datapath **array;
>>> };
>>>
>>> -/* Track what's changed for a single LS.
>>> - * Now only track port changes. */
>>> -struct ls_change {
>>> - struct ovs_list list_node;
>>> - struct ovn_datapath *od;
>>> - struct ovs_list added_ports;
>>> - struct ovs_list deleted_ports;
>>> - struct ovs_list updated_ports;
>>> - bool had_only_router_ports;
>>> +struct tracked_ovn_ports {
>>> + /* tracked created ports.
>>> + * hmapx node data is 'struct ovn_port *' */
>>> + struct hmapx created;
>>> +
>>> + /* tracked updated ports.
>>> + * hmapx node data is 'struct ovn_port *' */
>>> + struct hmapx updated;
>>> +
>>> + /* tracked deleted ports.
>>> + * hmapx node data is 'struct ovn_port *' */
>>> + struct hmapx deleted;
>>> };
>>
>> Is there a reason why changing the structures from list to hmapx? It seems
>> to me the tracked ports will be traversed one by one and doesn't require
>> searching.
>
> Yes. There is a reason. The lflow engine node which uses this
> tracked data will
> not search for it. It just needs to iterate the changed ovn_ports. So
> it doesn't matter
> if it is a list or hmapx.
>
> But it becomes easier to add changed ovn_ports to hmapx and there is no need
> to
> have a list node in the 'struct ovn_port'.
>
>>
>>>
>>> -/* Track what's changed for logical switches.
>>> - * Now only track updated ones (added or deleted may be supported in the
>>> - * future). */
>>> -struct tracked_ls_changes {
>>> - struct ovs_list updated; /* Contains struct ls_change */
>>> +struct tracked_lbs {
>>> + /* Tracked created or updated load balancers.
>>> + * hmapx node data is 'struct ovn_lb_datapaths' */
>>> + struct hmapx crupdated;
>>> +
>>> + /* Tracked deleted lbs.
>>> + * hmapx node data is 'struct ovn_lb_datapaths' */
>>> + struct hmapx deleted;
>>> +};
>>> +
>>> +/* Track what's changed in the northd engine node.
>>> + * Now only tracks ovn_ports (of vif type) - created, updated
>>> + * and deleted. */
>>> +struct northd_tracked_data {
>>> + struct tracked_ovn_ports trk_ovn_ports;
>>
>> It may be better to name it trk_lsps or something similar, so that it is
>> clear that the field is tracking LSPs, not LRPs.
>
> Ack.
>
>
>>
>>> + struct tracked_lbs trk_lbs;
>>> };
>>>
>>> struct northd_data {
>>> @@ -114,10 +128,9 @@ struct northd_data {
>>> struct chassis_features features;
>>> struct sset svc_monitor_lsps;
>>> struct hmap svc_monitor_map;
>>> + /* Indicates if northd engine node has changes tracked or not. */
>>> bool change_tracked;
>>> - struct tracked_ls_changes tracked_ls_changes;
>>> - bool lb_changed; /* Indicates if load balancers changed or
>> association of
>>> - * load balancer to logical switch/router changed.
>> */
>>> + struct northd_tracked_data trk_northd_changes;
>>> };
>>>
>>> struct lflow_data {
>>> @@ -338,9 +351,10 @@ void northd_indices_create(struct northd_data *data,
>>> void build_lflows(struct ovsdb_idl_txn *ovnsb_txn,
>>> struct lflow_input *input_data,
>>> struct hmap *lflows);
>>> -bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn,
>>> - struct tracked_ls_changes *,
>>> - struct lflow_input *, struct hmap
>> *lflows);
>>> +bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn,
>>> + struct tracked_ovn_ports *,
>>> + struct lflow_input *,
>>> + struct hmap *lflows);
>>> bool northd_handle_sb_port_binding_changes(
>>> const struct sbrec_port_binding_table *, struct hmap *ls_ports);
>>>
>>> @@ -349,7 +363,8 @@ bool northd_handle_lb_data_changes(struct
>> tracked_lb_data *,
>>> struct ovn_datapaths *ls_datapaths,
>>> struct ovn_datapaths *lr_datapaths,
>>> struct hmap *lb_datapaths_map,
>>> - struct hmap *lbgrp_datapaths_map);
>>> + struct hmap *lbgrp_datapaths_map,
>>> + struct northd_tracked_data *);
>>>
>>> void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>>> const struct nbrec_bfd_table *,
>>> @@ -372,6 +387,10 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct
>> sbrec_load_balancer_table *,
>>> bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *);
>>>
>>> void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports);
>>> -bool sync_pbs_for_northd_ls_changes(struct tracked_ls_changes *);
>>> +bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *);
>>> +
>>> +bool northd_has_tracked_data(struct northd_tracked_data *);
>>> +bool northd_has_only_ports_in_tracked_data(struct northd_tracked_data *);
>>> +bool northd_has_lbs_in_tracked_data(struct northd_tracked_data *);
>>>
>>> #endif /* NORTHD_H */
>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>>> index 196fe01fbd..28c293473c 100644
>>> --- a/tests/ovn-northd.at
>>> +++ b/tests/ovn-northd.at
>>> @@ -10620,10 +10620,9 @@ check as northd ovn-appctl -t NORTHD_TYPE
>> inc-engine/clear-stats
>>> lbg1_uuid=$(ovn-nbctl --wait=sb create load_balancer_group name=lbg1)
>>> check_engine_stats lb_data norecompute compute
>>> check_engine_stats northd norecompute compute
>>> -check_engine_stats lflow recompute nocompute
>>> -check_engine_stats sync_to_sb_lb nocompute nocompute
>>
>> Why remove this check for sync_to_sb_lb?
>>
>
> I think I removed it by mistake.
>
> With this patch 'sync_to_sb_lb' will not result in recompute when you
> create an empty load balancer group (i.e lb group not having any load
> balancer in it).
> The same is true for 'lflow' engine node too.
>
> The reason is - northd_lb_data_handler() before this patch was setting
> northd_data->lb_changed = true for any lb_data change.
> northd handler for lflow engine and sync_to_sb_lb fall back to
> recompute if northd_data->lb_changed = true.
>
> But with this patch, northd_data->lb_changed is removed and we use
> the tracking data of the northd.
>
> ------
> /* Fall back to recompute if load balancers have changed. */
> if (northd_has_lbs_in_tracked_data(&northd_data->trk_northd_changes)) {
> return false;
> }
> ----
>
>
>
>>> -
>>> +check_engine_stats lflow norecompute nocompute
>>
>> Could you explain what change in this patch avoided the recompute of lflow?
>> It is not obvious to me and it is not mentioned in the commit message.
>
> Please see above. I'll update the commit message.
>
> Thanks for the reviews.
>
> Numan
>
>
>>
>>> CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>> +
>>> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
>>>
>>> lb1_uuid=$(fetch_column nb:Load_Balancer _uuid)
>>> @@ -10650,7 +10649,6 @@ check_engine_stats lb_data norecompute compute
>>> check_engine_stats northd norecompute compute
>>> check_engine_stats lflow recompute nocompute
>>> check_engine_stats sync_to_sb_lb recompute nocompute
>>> -
>>> CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
>>> @@ -10798,8 +10796,8 @@ check as northd ovn-appctl -t NORTHD_TYPE
>> inc-engine/clear-stats
>>> lbg1_uuid=$(ovn-nbctl --wait=sb create load_balancer_group name=lbg1)
>>> check_engine_stats lb_data norecompute compute
>>> check_engine_stats northd norecompute compute
>>> -check_engine_stats lflow recompute nocompute
>>> -check_engine_stats sync_to_sb_lb recompute nocompute
>>> +check_engine_stats lflow norecompute nocompute
>>> +check_engine_stats sync_to_sb_lb norecompute nocompute
>>>
>>> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
>>> check ovn-nbctl --wait=sb set load_balancer_group .
>> load_balancer="$lb2_uuid,$lb3_uuid,$lb4_uuid"
>>> --
>>> 2.41.0
>>>
>>> _______________________________________________
>>> dev mailing list
>>> [email protected]
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
