On 5/13/23 02:03, Han Zhou wrote:
> This patch introduces a change handler for 'northd' input within the
> 'lflow' node. It specifically handles cases when VIFs are created, which
> is an easier start for lflow incremental processing. Support for
> update/delete will be added later.
>
> Below are the performance test results simulating an ovn-k8s topology of 500
> nodes x 50 lsp per node:
>
> Before:
> ovn-nbctl --wait=hv --print-wait-time lsp-add ls_1_0001 lsp_1_0001_01 --
> lsp-set-addresses lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101" --
> lsp-set-port-security lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101"
> ovn-northd completion: 773ms
>
> After:
> ovn-nbctl --wait=hv --print-wait-time lsp-add ls_1_0001 lsp_1_0001_01 --
> lsp-set-addresses lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101" --
> lsp-set-port-security lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101"
> ovn-northd completion: 30ms
>
> It is more than 95% reduction (or 20x faster).
>
> Signed-off-by: Han Zhou <[email protected]>
> ---
> northd/en-lflow.c | 82 ++++++++-----
> northd/en-lflow.h | 1 +
> northd/inc-proc-northd.c | 2 +-
> northd/northd.c | 245 +++++++++++++++++++++++++++++++++------
> northd/northd.h | 6 +-
> tests/ovn-northd.at | 4 +-
> 6 files changed, 277 insertions(+), 63 deletions(-)
>
<snip>
> @@ -5685,7 +5687,18 @@ ovn_dp_group_add_with_reference(struct ovn_lflow
> *lflow_ref,
>
> /* 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;
> 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,
> @@ -5702,12 +5715,17 @@ 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);
>
> - old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, match,
> - actions, ctrl_meter, hash);
> - if (old_lflow) {
> - ovn_dp_group_add_with_reference(old_lflow, od, dp_bitmap,
> - bitmap_len);
> - return;
> + if (add_lflow_to_temp_list) {
> + ovs_assert(od);
> + ovs_assert(!dp_bitmap);
> + } else {
> + old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, match,
> + actions, ctrl_meter, hash);
> + if (old_lflow) {
> + ovn_dp_group_add_with_reference(old_lflow, od, dp_bitmap,
> + bitmap_len);
> + return;
> + }
> }
Hi, Han. Not a full review, but I'm a bit concerned that this code doesn't
support datapath groups. IIUC, if some flows can be aggregated with datapath
groups, I-P will just add them separately. Later a full re-compute will
delete all these flows and replace them with a single grouped one. Might
cause some unnecessary churn in the cluster.
Is my understanding correct? And can it be a performance issue? E.g. if the
database grows too much because of that. Or how hard it is to add datapath
group support to I-P ?
I suppose, DP-groups is one of the reasons you didn't implement deletions
here, right?
Best regards, Ilya Maxiemts.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
