On 1/28/25 8:22 AM, Ales Musil wrote:
> Add new I-P node that will store all the data for IGMP and
> Multicast groups. This node allows to avoid full recompute of lflow
> node when IGMP or Multicast SB table changes.
>
> The node itself still does full recompute for IGMP and Multicast
> changes however this is a compromise between code complexity and
> the time it takes for all lflow to be created. At the same time
> thi brings the benefit of having the data available when there
> is recompute of the lflow node.
>
> As design choice there is only single lflow_ref for all IGMP
> lflows, that makes them not being thread safe and only main thread
> can generate them during full recompute of lflow node. This shouldn't
> be an issue, because the computation of igmp lflow is pretty simple.
>
> Reported-at: https://issues.redhat.com/browse/FDP-756
> Co-authored-by: Jacob Tanenbaum <jtane...@redhat.com>
> Signed-off-by: Jacob Tanenbaum <jtane...@redhat.com>
> Suggested-by: Dumitru Ceara <dce...@redhat.com>
> Signed-off-by: Ales Musil <amu...@redhat.com>
> ---
Hi Ales, Jacob,
> v3: Rebase on top of latest main.
> Add missing _SAFE in table iterator.
> ---
> northd/en-lflow.c | 52 ++++++++-
> northd/en-lflow.h | 1 +
> northd/en-multicast.c | 220 ++++++++++++++++++++++++++++-----------
> northd/en-multicast.h | 24 ++---
> northd/inc-proc-northd.c | 10 +-
> northd/northd.c | 99 +++++++-----------
> northd/northd.h | 10 +-
> tests/ovn-northd.at | 89 ++++++++++++++++
> 8 files changed, 363 insertions(+), 142 deletions(-)
>
[...]
> @@ -17510,6 +17470,40 @@ build_lswitch_and_lrouter_flows(
> free(svc_check_match);
> }
>
> +/* The IGMP flows have to be built in main thread because there is
> + * single lflow_ref for all of them which isn't thread safe.
> + * This shouldn't affect performance as there is a limited how many
> + * IGMP groups can be created. */
> +void
> +build_igmp_lflows(struct hmap *igmp_groups, const struct hmap *ls_datapaths,
> + struct lflow_table *lflows, struct lflow_ref *lflow_ref)
> +{
> + struct ds actions = DS_EMPTY_INITIALIZER;
> + struct ds match = DS_EMPTY_INITIALIZER;
> +
> + struct ovn_datapath *od;
> + HMAP_FOR_EACH (od, key_node, ls_datapaths) {
> + init_mcast_flow_count(od);
> + build_mcast_flood_lswitch(od, lflows, &actions, lflow_ref);
> + }
> +
> + stopwatch_start(LFLOWS_IGMP_STOPWATCH_NAME, time_msec());
> + struct ovn_igmp_group *igmp_group;
> + HMAP_FOR_EACH (igmp_group, hmap_node, igmp_groups) {
> + if (igmp_group->datapath->nbs) {
> + build_lswitch_ip_mcast_igmp_mld(igmp_group, lflows, &actions,
> + &match, lflow_ref);
This all happens now in a single thread, the main thread. However, the
build_lswitch_ip_mcast_igmp_mld() function still increments the atomic
mcast_sw_info->active_v4_flows variable.
We actually have two small "issues" with this:
1. there's no need for an atomic variable
2. we don't need to keep writing to the counter inside the logical
datapath structure; we can just track counts here, locally. That would
also fix an already existing issue: writing to lflow input node data
that should be read-only.
I'd say, let's follow up on this with a new patch. What do you think?
> + } else {
> + build_igmp_flows_for_lrouter(igmp_group, lflows, &actions,
> + &match, lflow_ref);
> + }
> + }
> + stopwatch_stop(LFLOWS_IGMP_STOPWATCH_NAME, time_msec());
> +
> + ds_destroy(&actions);
> + ds_destroy(&match);
> +}
> +
[...]
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index df646ec68..897818b38 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -14445,3 +14445,92 @@ AT_CHECK([ovn-sbctl lflow-list S1 | grep
> ls_out_acl_action | grep priority=500 |
>
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([IGMP incremental processing])
> +
> +check_recompute_counter() {
> + lflow_recomp=$(as northd ovn-appctl -t ovn-northd inc-engine/show-stats
> lflow recompute)
> + AT_CHECK([test x$lflow_recomp = x$1])
> +}
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.11
> +
> +sim_add hv2
> +as hv2
> +
> +check ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.2
> +
> +check ovn-nbctl ls-add sw1
> +check ovn-nbctl ls-add sw2
> +
> +check ovn-nbctl lsp-add sw1 sw1-p11
> +check ovn-nbctl lsp-add sw2 sw2-p21
> +
> +check ovn-nbctl lr-add rtr
> +check ovn-nbctl lrp-add rtr rtr-sw1 00:00:00:00:01:00 10.0.0.254/24
> +check ovn-nbctl lrp-add rtr rtr-sw2 00:00:00:00:02:00 10.0.0.254/24
> +
> +
Nit: one line too many
> +check ovn-nbctl lsp-add sw1 sw1-rtr \
> + -- lsp-set-type sw1-rtr router \
> + -- lsp-set-addresses sw1-rtr 00:00:00:00:01:00 \
> + -- lsp-set-options sw1-rtr router-port=rtr-sw1
> +
> +check ovn-nbctl lsp-add sw2 sw2-rtr \
> + -- lsp-set-type sw2-rtr router \
> + -- lsp-set-addresses sw1-rtr 00:00:00:00:02:00 \
> + -- lsp-set-options sw2-rtr router-port=rtr-sw2
> +
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +
> +# Create IGMP_Group 239.0.1.68 with port sw1-p11
> +ovn-sbctl create IGMP_Group address=239.0.1.68 \
Nit: check_uuid
> + datapath=$(fetch_column Datapath_Binding _uuid external_ids:name=sw1) \
> + chassis=$(fetch_column Chassis _uuid name=hv1) \
> + chassis_name=hv1 \
> + ports=$(fetch_column Port_Binding _uuid logical_port=sw1-p11)
> +igmp_uuid=$(fetch_column IGMP_GROUP _uuid address=239.0.1.68)
s/IGMP_GROUP/IGMP_Group
> +
> +check ovn-nbctl --wait=sb sync
> +wait_row_count Igmp_Group 1 address=239.0.1.68
> +wait_row_count Multicast_Group 1 name="239.0.1.68"
> +wait_row_count Multicast_Group 1 name="239.0.1.68" ports='[['$(fetch_column
> Port_Binding _uuid logical_port=sw1-p11)']]'
> +ovn-sbctl list igmp_group
Nit: not needed.
> +check_recompute_counter 0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +check ovn-nbctl set logical_router rtr \
> + options:mcast_relay="true"
Missing --wait=sb sync here (as discussed offline).
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +# Update IGMP_Group 239.0.1.68 to include sw2-p21
> +ovn-sbctl add IGMP_Group $igmp_uuid ports $(fetch_column Port_Binding _uuid
> logical_port=sw2-p21)
Nit: check
> +
> +check ovn-nbctl --wait=sb sync
> +wait_row_count IGMP_Group 1 address=239.0.1.68
> +
> +# Check that new Multicast_Group is created
> +wait_row_count Multicast_Group 2 name=239.0.1.68
> +check_recompute_counter 0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +# Delete IGMP_Group 239.0.1.68
> +ovn-sbctl destroy IGMP_Group $igmp_uuid
Nit: check.
checkpatch also reported some of this on the ovsrobot run.
> +check ovn-nbctl --wait=sb sync
> +check_recompute_counter 0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +wait_row_count IGMP_Group 0 address=239.0.1.68
> +wait_row_count Multicast_Group 0 name=239.0.1.68
> +
> +OVN_CLEANUP([hv1], [hv2])
> +AT_CLEANUP
> +])
I made the small changes and applied this patch to main.
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
