On Fri, Jul 7, 2023 at 1:55 PM <num...@ovn.org> wrote:
>
> From: Numan Siddique <num...@ovn.org>
>
> For every a given load balancer group 'A', northd engine data maintains
> a bitmap of datapaths associated to this lb group. So when lb group 'A'
> gets associated to a logical switch 's1', the bitmap index of 's1' is set
> in its bitmap.
>
> In order to handle the load balancer group changes incrementally for a
> logical switch, we need to set and clear the bitmap bits accordingly.
> And this patch does it.
>
> Signed-off-by: Numan Siddique <num...@ovn.org>
> ---
> lib/lb.c | 45 +++++++++++++++---
> lib/lb.h | 33 ++++++--------
> northd/northd.c | 109 ++++++++++++++++++++++++++++++++++++++------
> tests/ovn-northd.at | 6 +--
> 4 files changed, 150 insertions(+), 43 deletions(-)
>
> diff --git a/lib/lb.c b/lib/lb.c
> index b2b6473c1d..a0426cc42e 100644
> --- a/lib/lb.c
> +++ b/lib/lb.c
> @@ -1100,7 +1100,7 @@ ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths
*lb_dps, size_t n,
>
> void
> ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *lb_dps, size_t n,
> - struct ovn_datapath **ods)
> + struct ovn_datapath **ods)
> {
> for (size_t i = 0; i < n; i++) {
> if (bitmap_is_set(lb_dps->nb_ls_map, ods[i]->index)) {
> @@ -1112,14 +1112,14 @@ ovn_lb_datapaths_remove_ls(struct
ovn_lb_datapaths *lb_dps, size_t n,
>
> struct ovn_lb_group_datapaths *
> ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group,
> - size_t max_ls_datapaths,
> - size_t max_lr_datapaths)
> + size_t n_ls_datapaths,
> + size_t n_lr_datapaths)
> {
> struct ovn_lb_group_datapaths *lb_group_dps =
> xzalloc(sizeof *lb_group_dps);
> lb_group_dps->lb_group = lb_group;
> - lb_group_dps->ls = xmalloc(max_ls_datapaths * sizeof
*lb_group_dps->ls);
> - lb_group_dps->lr = xmalloc(max_lr_datapaths * sizeof
*lb_group_dps->lr);
> + lb_group_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths);
> + lb_group_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths);
>
> return lb_group_dps;
> }
> @@ -1127,8 +1127,8 @@ ovn_lb_group_datapaths_create(const struct
ovn_lb_group *lb_group,
> void
> ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths
*lb_group_dps)
> {
> - free(lb_group_dps->ls);
> - free(lb_group_dps->lr);
> + bitmap_free(lb_group_dps->nb_lr_map);
> + bitmap_free(lb_group_dps->nb_ls_map);
> free(lb_group_dps);
> }
>
> @@ -1146,3 +1146,34 @@ ovn_lb_group_datapaths_find(const struct hmap
*lb_group_dps_map,
> }
> return NULL;
> }
> +
> +void
> +ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps,
size_t n,
> + struct ovn_datapath **ods)
> +{
> + for (size_t i = 0; i < n; i++) {
> + if (!bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) {
> + bitmap_set1(lbg_dps->nb_ls_map, ods[i]->index);
> + lbg_dps->n_nb_ls++;
> + }
> + }
> +}
> +
> +void
> +ovn_lb_group_datapaths_remove_ls(struct ovn_lb_group_datapaths *lbg_dps,
> + size_t n, struct ovn_datapath **ods)
> +{
> + for (size_t i = 0; i < n; i++) {
> + if (bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) {
> + bitmap_set0(lbg_dps->nb_ls_map, ods[i]->index);
> + lbg_dps->n_nb_ls--;
> + }
> + }
> +}
> +
> +void
> +ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps,
> + struct ovn_datapath *lr)
> +{
> + bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> +}
> diff --git a/lib/lb.h b/lib/lb.h
> index ac33333a7f..08723e8ef3 100644
> --- a/lib/lb.h
> +++ b/lib/lb.h
> @@ -19,6 +19,7 @@
>
> #include <sys/types.h>
> #include <netinet/in.h>
> +#include "lib/bitmap.h"
> #include "lib/smap.h"
> #include "openvswitch/hmap.h"
> #include "ovn-util.h"
> @@ -179,6 +180,9 @@ void ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths
*, size_t n,
> struct ovn_datapath **);
> void ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *, size_t n,
> struct ovn_datapath **);
> +void ovn_lb_datapaths_set_ls(struct ovn_lb_datapaths *, size_t n,
> + struct ovn_datapath **);
> +
> void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *, size_t n,
> struct ovn_datapath **);
>
> @@ -188,10 +192,11 @@ struct ovn_lb_group_datapaths {
> const struct ovn_lb_group *lb_group;
>
> /* Datapaths to which 'lb_group' is applied. */
> - size_t n_ls;
> - struct ovn_datapath **ls;
> - size_t n_lr;
> - struct ovn_datapath **lr;
> + size_t n_nb_ls;
> + unsigned long *nb_ls_map;
> +
> + size_t n_nb_lr;
> + unsigned long *nb_lr_map;
> };
>
> struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_create(
> @@ -202,21 +207,13 @@ void ovn_lb_group_datapaths_destroy(struct
ovn_lb_group_datapaths *);
> struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_find(
> const struct hmap *lb_group_dps, const struct uuid *);
>
> -static inline void
> -ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps,
size_t n,
> - struct ovn_datapath **ods)
> -{
> - memcpy(&lbg_dps->ls[lbg_dps->n_ls], ods, n * sizeof *ods);
> - lbg_dps->n_ls += n;
> -}
> -
This was O(1), but this patch changed it to O(n). Maybe we need some scale
test data to see the impact of recompute performance until we are confident
that recompute won't be triggered in most cases. I remember this "memcpy"
approach was for a significant optimization from Ilya. cc @Ilya Maximets
<i.maxim...@ovn.org> in case he has more comments.
Thanks,
Han
> -static inline void
> -ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps,
> - struct ovn_datapath *lr)
> -{
> - lbg_dps->lr[lbg_dps->n_lr++] = lr;
> -}
> +void ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *,
size_t n,
> + struct ovn_datapath **);
> +void ovn_lb_group_datapaths_remove_ls(struct ovn_lb_group_datapaths *,
> + size_t n, struct ovn_datapath **);
>
> +void ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *,
> + struct ovn_datapath *lr);
> struct ovn_controller_lb {
> struct hmap_node hmap_node;
>
> diff --git a/northd/northd.c b/northd/northd.c
> index bf02190f7c..20a58afa6b 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -4102,7 +4102,8 @@ associate_ls_lbs(struct ovn_datapath *ls_od, struct
hmap *lb_datapaths_map)
>
> static void
> associate_ls_lb_groups(struct ovn_datapath *ls_od,
> - struct hmap *lb_group_datapaths_map)
> + struct hmap *lb_group_datapaths_map,
> + struct hmap *lb_datapaths_map)
> {
> const struct nbrec_load_balancer_group *nbrec_lb_group;
> struct ovn_lb_group_datapaths *lb_group_dps;
> @@ -4120,6 +4121,15 @@ associate_ls_lb_groups(struct ovn_datapath *ls_od,
> ovs_assert(lb_group_dps);
> ovn_lb_group_datapaths_add_ls(lb_group_dps, 1, &ls_od);
> ls_od->lb_group_uuids[i] = *lb_group_uuid;
> +
> + struct ovn_lb_datapaths *lb_dps;
> + for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) {
> + const struct uuid *lb_uuid =
> + &lb_group_dps->lb_group->lbs[j]->nlb->header_.uuid;
> + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> + ovs_assert(lb_dps);
> + ovn_lb_datapaths_add_ls(lb_dps, 1, &ls_od);
> + }
> }
> }
>
> @@ -4159,7 +4169,7 @@ build_lb_datapaths(const struct hmap *lbs, const
struct hmap *lb_groups,
> continue;
> }
> associate_ls_lbs(od, lb_datapaths_map);
> - associate_ls_lb_groups(od, lb_group_datapaths_map);
> + associate_ls_lb_groups(od, lb_group_datapaths_map,
lb_datapaths_map);
> }
>
> HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
> @@ -4219,10 +4229,12 @@ build_lb_datapaths(const struct hmap *lbs, const
struct hmap *lb_groups,
> &lb_group_dps->lb_group->lbs[j]->nlb->header_.uuid;
> lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> ovs_assert(lb_dps);
> - ovn_lb_datapaths_add_ls(lb_dps, lb_group_dps->n_ls,
> - lb_group_dps->ls);
> - ovn_lb_datapaths_add_lr(lb_dps, lb_group_dps->n_lr,
> - lb_group_dps->lr);
> + size_t index;
> + BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths),
> + lb_group_dps->nb_lr_map) {
> + od = lr_datapaths->array[index];
> + ovn_lb_datapaths_add_lr(lb_dps, 1, &od);
> + }
> }
> }
> }
> @@ -4401,8 +4413,9 @@ build_lswitch_lbs_from_lrouter(struct ovn_datapaths
*lr_datapaths,
>
> struct ovn_lb_group_datapaths *lb_group_dps;
> HMAP_FOR_EACH (lb_group_dps, hmap_node, lb_group_dps_map) {
> - for (size_t i = 0; i < lb_group_dps->n_lr; i++) {
> - struct ovn_datapath *od = lb_group_dps->lr[i];
> + BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths),
> + lb_group_dps->nb_lr_map) {
> + struct ovn_datapath *od = lr_datapaths->array[index];
> ovn_lb_group_datapaths_add_ls(lb_group_dps, od->n_ls_peers,
> od->ls_peers);
> for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) {
> @@ -5072,7 +5085,8 @@ check_unsupported_inc_proc_for_ls_changes(
> for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) {
> if (nbrec_logical_switch_is_updated(ls, col)) {
> if (col == NBREC_LOGICAL_SWITCH_COL_PORTS ||
> - col == NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER) {
> + col == NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER ||
> + col == NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER_GROUP) {
> continue;
> }
> return true;
> @@ -5103,12 +5117,6 @@ check_unsupported_inc_proc_for_ls_changes(
> return true;
> }
> }
> - for (size_t i = 0; i < ls->n_load_balancer_group; i++) {
> - if
(nbrec_load_balancer_group_row_get_seqno(ls->load_balancer_group[i],
> - OVSDB_IDL_CHANGE_MODIFY) > 0) {
> - return true;
> - }
> - }
> for (size_t i = 0; i < ls->n_qos_rules; i++) {
> if (nbrec_qos_row_get_seqno(ls->qos_rules[i],
> OVSDB_IDL_CHANGE_MODIFY) > 0) {
> @@ -5338,6 +5346,68 @@ ls_check_and_handle_lb_changes(const struct
nbrec_logical_switch *changed_ls,
> return true;
> }
>
> +static bool
> +ls_check_and_handle_lb_group_changes(
> + const struct nbrec_logical_switch *changed_ls,
> + struct hmap *lb_group_datapaths_map,
> + struct hmap *lb_datapaths_map,
> + struct ovn_datapath *od,
> + struct ls_change *ls_change,
> + bool *updated)
> +{
> + bool lbg_modified = false;
> + /* Check if lb_groups changed or not */
> + if (!nbrec_logical_switch_is_updated(changed_ls,
> + NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER_GROUP)) {
> + /* load_balancer_group column of logical switch hasn't changed,
but
> + * its possible that a load balancer group may have changed (like
> + * a load balancer added or removed from the group). */
> + for (size_t i = 0; i < changed_ls->n_load_balancer_group; i++) {
> + if (nbrec_load_balancer_group_row_get_seqno(
> + changed_ls->load_balancer_group[i],
> + OVSDB_IDL_CHANGE_MODIFY) > 0) {
> + lbg_modified = true;
> + break;
> + }
> + }
> + } else {
> + /* load_balancer_group column of logical switch has changed. */
> + lbg_modified = true;
> + }
> +
> + if (!lbg_modified) {
> + /* Nothing changed */
> + return true;
> + }
> +
> + /* Disassociate load balancer group (and its load balancers) from
> + * the datapath and rebuild the association again. */
> + struct ovn_lb_group_datapaths *lbg_dps;
> + for (size_t i = 0; i < od->n_lb_group_uuids; i++) {
> + lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> + &od->lb_group_uuids[i]);
> + if (lbg_dps) {
> + ovn_lb_group_datapaths_remove_ls(lbg_dps, 1, &od);
> +
> + struct ovn_lb_datapaths *lb_dps;
> + for (size_t j = 0; j < lbg_dps->lb_group->n_lbs; j++) {
> + const struct uuid *lb_uuid =
> + &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid;
> + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
lb_uuid);
> + if (lb_dps) {
> + ovn_lb_datapaths_remove_ls(lb_dps, 1, &od);
> + }
> + }
> + }
> + }
> +
> + associate_ls_lb_groups(od, lb_group_datapaths_map, lb_datapaths_map);
> + ls_change->lbs_changed = true;
> + *updated = true;
> + /* Re-evaluate 'od->has_lb_vip' */
> + init_lb_for_datapath(od);
> + return true;
> +}
>
> /* Return true if changes are handled incrementally, false otherwise.
> * When there are any changes, try to track what's exactly changed and
set
> @@ -5396,6 +5466,15 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
> goto fail;
> }
>
> + if (!ls_check_and_handle_lb_group_changes(changed_ls,
> +
&nd->lb_group_datapaths_map,
> + &nd->lb_datapaths_map,
> + od, ls_change,
&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);
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index d9f3917a3e..d3a5851e35 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -9783,16 +9783,16 @@ check_engine_stats norecompute recompute
>
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl add logical_switch sw0 load_balancer_group $lbg1_uuid
> -check_engine_stats recompute recompute
> +check_engine_stats norecompute recompute
>
> # Update lb and this should result in recompute
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl --wait=sb set load_balancer . options:bar=foo
> -check_engine_stats recompute recompute
> +check_engine_stats norecompute recompute
>
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl clear logical_switch sw0 load_balancer_group
> -check_engine_stats recompute recompute
> +check_engine_stats norecompute recompute
>
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl add logical_router lr0 load_balancer_group $lbg1_uuid
> --
> 2.40.1
>
> _______________________________________________
> 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
