On 11/28/23 03:37, num...@ovn.org wrote:
> From: Numan Siddique <num...@ovn.org>
>
> Since northd tracked data has the changed lb data, northd
> engine handler for lflow engine now handles the lb changes
> incrementally. All the lflows generated for each lb is
> stored in the ovn_lb_datapaths->lflow_ref and this is used
> similar to how we handle ovn_port changes.
>
> Signed-off-by: Numan Siddique <num...@ovn.org>
> ---
> northd/en-lflow.c | 11 +++---
> northd/lflow-mgr.c | 41 ++++++++++++++++++-
> northd/lflow-mgr.h | 3 ++
> northd/northd.c | 95 +++++++++++++++++++++++++++++++++++++++++----
> northd/northd.h | 28 +++++++++++++
> tests/ovn-northd.at | 30 ++++++++++----
> 6 files changed, 186 insertions(+), 22 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index 65d2f45ebc..13284b5556 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -125,11 +125,6 @@ lflow_northd_handler(struct engine_node *node,
> return false;
> }
>
> - /* Fall back to recompute if load balancers have changed. */
> - if (northd_has_lbs_in_tracked_data(&northd_data->trk_data)) {
> - return false;
> - }
> -
> const struct engine_context *eng_ctx = engine_get_context();
> struct lflow_data *lflow_data = data;
>
> @@ -142,6 +137,12 @@ lflow_northd_handler(struct engine_node *node,
> return false;
> }
>
> + if (!lflow_handle_northd_lb_changes(eng_ctx->ovnsb_idl_txn,
> + &northd_data->trk_data.trk_lbs,
> + &lflow_input, lflow_data->lflow_table)) {
> + return false;
> + }
> +
> engine_set_node_state(node, EN_UPDATED);
> return true;
> }
> diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
> index 08962e9172..d779e7e087 100644
> --- a/northd/lflow-mgr.c
> +++ b/northd/lflow-mgr.c
> @@ -105,6 +105,10 @@ static void ovn_dp_group_add_with_reference(struct
> ovn_lflow *,
> size_t bitmap_len);
>
> static void unlink_lflows_from_datapath(struct lflow_ref *);
> +static void unlink_lflows_from_all_datapaths(struct lflow_ref *,
> + size_t n_ls_datapaths,
> + size_t n_lr_datapaths);
> +
lflow_ref_clear_and_sync_lflows() takes 'const struct ovn_datapaths
*ls_datapaths' and 'const struct ovn_datapaths *lr_datapaths'. Let's do
that here too to be consistent.
> static void lflow_ref_sync_lflows_to_sb__(struct lflow_ref *,
> struct lflow_table *,
> struct ovsdb_idl_txn *ovnsb_txn,
> @@ -394,6 +398,15 @@ lflow_ref_clear_lflows(struct lflow_ref *lflow_ref)
> unlink_lflows_from_datapath(lflow_ref);
> }
>
> +void
> +lflow_ref_clear_lflows_for_all_dps(struct lflow_ref *lflow_ref,
> + size_t n_ls_datapaths,
> + size_t n_lr_datapaths)
Same comment about consistency here too.
While we're at it, all we care is the max bitmap index for a 'struct
ovn_lflow'->dpg_bitmap. We don't really need to pass all these
ls_datapaths/lr_datapaths everywhere, I think. Can't we just use
'struct ovn_lflow'->n_ods?
Alternatively, can 'struct lflow_ref' store a pointer to switch/router
'struct ovn_datapaths'?
> +{
> + unlink_lflows_from_all_datapaths(lflow_ref, n_ls_datapaths,
> + n_lr_datapaths);
> +}
> +
> void
> lflow_ref_clear_and_sync_lflows(struct lflow_ref *lflow_ref,
> struct lflow_table *lflow_table,
> @@ -462,7 +475,9 @@ lflow_table_add_lflow(struct lflow_table *lflow_table,
> /* lflow_ref_node for this lflow doesn't exist yet. Add it. */
> struct lflow_ref_node *ref_node = xzalloc(sizeof *ref_node);
> ref_node->lflow = lflow;
> - ref_node->dp_index = od->index;
> + if (od) {
> + ref_node->dp_index = od->index;
> + }
> ovs_list_insert(&lflow_ref->lflows_ref_list,
> &ref_node->ref_list_node);
>
> @@ -1047,6 +1062,30 @@ unlink_lflows_from_datapath(struct lflow_ref
> *lflow_ref)
> }
> }
>
> +static void
> +unlink_lflows_from_all_datapaths(struct lflow_ref *lflow_ref,
> + size_t n_ls_datapaths,
> + size_t n_lr_datapaths)
> +{
> + struct lflow_ref_node *ref_node;
> + struct ovn_lflow *lflow;
> + LIST_FOR_EACH (ref_node, ref_list_node, &lflow_ref->lflows_ref_list) {
> + size_t n_datapaths;
> + size_t index;
> +
> + lflow = ref_node->lflow;
> + if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) {
> + n_datapaths = n_ls_datapaths;
> + } else {
> + n_datapaths = n_lr_datapaths;
> + }
> +
> + BITMAP_FOR_EACH_1 (index, n_datapaths, lflow->dpg_bitmap) {
> + bitmap_set0(lflow->dpg_bitmap, index);
> + }
> + }
> +}
> +
> static void
> lflow_ref_sync_lflows_to_sb__(struct lflow_ref *lflow_ref,
> struct lflow_table *lflow_table,
> diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h
> index 02b74aa131..c65cd70e71 100644
> --- a/northd/lflow-mgr.h
> +++ b/northd/lflow-mgr.h
> @@ -54,6 +54,9 @@ struct lflow_ref *lflow_ref_alloc(const char *res_name);
> void lflow_ref_destroy(struct lflow_ref *);
> void lflow_ref_reset(struct lflow_ref *lflow_ref);
> void lflow_ref_clear_lflows(struct lflow_ref *);
> +void lflow_ref_clear_lflows_for_all_dps(struct lflow_ref *,
> + size_t n_ls_datapaths,
> + size_t n_lr_datapaths);
> void lflow_ref_clear_and_sync_lflows(struct lflow_ref *,
> struct lflow_table *lflow_table,
> struct ovsdb_idl_txn *ovnsb_txn,
> diff --git a/northd/northd.c b/northd/northd.c
> index 104068e293..f56916b3da 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3523,6 +3523,7 @@ ovn_lb_datapaths_create(const struct ovn_northd_lb *lb,
> size_t n_ls_datapaths,
> lb_dps->lb = lb;
> lb_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths);
> lb_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths);
> + lb_dps->lflow_ref = lflow_ref_alloc(lb->nlb->name);
>
> return lb_dps;
> }
> @@ -3532,6 +3533,7 @@ ovn_lb_datapaths_destroy(struct ovn_lb_datapaths
> *lb_dps)
> {
> bitmap_free(lb_dps->nb_lr_map);
> bitmap_free(lb_dps->nb_ls_map);
> + lflow_ref_destroy(lb_dps->lflow_ref);
> free(lb_dps);
> }
>
> @@ -16063,11 +16065,11 @@ build_lflows_thread(void *arg)
> lsi->lflows,
> &lsi->match,
> &lsi->actions,
> - NULL);
> + lb_dps->lflow_ref);
> build_lrouter_defrag_flows_for_lb(lb_dps, lsi->lflows,
> lsi->lr_datapaths,
> &lsi->match,
> - NULL);
> + lb_dps->lflow_ref);
> build_lrouter_flows_for_lb(lb_dps, lsi->lflows,
> lsi->meter_groups,
> lsi->lr_datapaths,
> @@ -16075,14 +16077,14 @@ build_lflows_thread(void *arg)
> lsi->features,
> lsi->svc_monitor_map,
> &lsi->match, &lsi->actions,
> - NULL);
> + lb_dps->lflow_ref);
> build_lswitch_flows_for_lb(lb_dps, lsi->lflows,
> lsi->meter_groups,
> lsi->ls_datapaths,
> lsi->features,
> lsi->svc_monitor_map,
> &lsi->match, &lsi->actions,
> - NULL);
> + lb_dps->lflow_ref);
> }
> }
> for (bnum = control->id;
> @@ -16298,20 +16300,20 @@ build_lswitch_and_lrouter_flows(const struct
> ovn_datapaths *ls_datapaths,
> build_lswitch_arp_nd_service_monitor(lb_dps->lb, lsi.ls_ports,
> lsi.lflows, &lsi.actions,
> &lsi.match,
> - NULL);
> + lb_dps->lflow_ref);
> build_lrouter_defrag_flows_for_lb(lb_dps, lsi.lflows,
> lsi.lr_datapaths, &lsi.match,
> - NULL);
> + lb_dps->lflow_ref);
> build_lrouter_flows_for_lb(lb_dps, lsi.lflows, lsi.meter_groups,
> lsi.lr_datapaths, lsi.lr_sful_table,
> lsi.features, lsi.svc_monitor_map,
> &lsi.match, &lsi.actions,
> - NULL);
> + lb_dps->lflow_ref);
> build_lswitch_flows_for_lb(lb_dps, lsi.lflows, lsi.meter_groups,
> lsi.ls_datapaths, lsi.features,
> lsi.svc_monitor_map,
> &lsi.match, &lsi.actions,
> - NULL);
> + lb_dps->lflow_ref);
> }
> stopwatch_stop(LFLOWS_LBS_STOPWATCH_NAME, time_msec());
>
> @@ -16483,11 +16485,17 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn,
> void
> reset_lflow_refs_for_northd_resources(struct lflow_input *lflow_input)
> {
> + struct ovn_lb_datapaths *lb_dps;
> struct ovn_port *op;
> +
Nit: this whitespace change should've went in patch 08/16 (northd:
Refactor lflow management into a separate module.)
> HMAP_FOR_EACH (op, key_node, lflow_input->ls_ports) {
> lflow_ref_reset(op->lflow_ref);
> lflow_ref_reset(op->stateful_lflow_ref);
> }
> +
> + HMAP_FOR_EACH (lb_dps, hmap_node, lflow_input->lb_datapaths_map) {
> + lflow_ref_reset(lb_dps->lflow_ref);
> + }
> }
>
[...]
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
