On 1/28/25 8:22 AM, Ales Musil wrote:
> The removal is needed for IGMP I-P node, because the ovn_datapath
> shouldn't hold reference to struct that might have different lifetime
> and might be freed during different node run. The same logic applies
> in the opposite direction, the list holds pointer to list inside
> ovn_datapath.
>
> This also allows to better isolate logical flows that are related
> to igmp in functions that can be reused for the IGMP handler.
>
> Co-authored-by: Jacob Tanenbaum <jtane...@redhat.com>
> Signed-off-by: Jacob Tanenbaum <jtane...@redhat.com>
> Suggested-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
> Signed-off-by: Ales Musil <amu...@redhat.com>
> ---
> v3: Rebase on top of latest main.
> ---
Hi Ales,
> northd/en-multicast.c | 68 ++++++----------
> northd/en-multicast.h | 1 -
> northd/northd.c | 185 ++++++++++++++++++++++++------------------
> northd/northd.h | 1 -
> 4 files changed, 132 insertions(+), 123 deletions(-)
>
> diff --git a/northd/en-multicast.c b/northd/en-multicast.c
> index deb192a82..0f07cf2fe 100644
> --- a/northd/en-multicast.c
> +++ b/northd/en-multicast.c
> @@ -207,18 +207,24 @@ build_mcast_groups(const struct sbrec_igmp_group_table
> *sbrec_igmp_group_table,
>
> /* Add the extracted ports to the IGMP group. */
> ovn_igmp_group_add_entry(igmp_group, igmp_ports, n_igmp_ports);
> - }
>
> - /* Build IGMP groups for multicast routers with relay enabled. The router
> - * IGMP groups are based on the groups learnt by their multicast enabled
> - * peers.
> - */
> - HMAP_FOR_EACH (od, key_node, ls_datapaths) {
> + /* Skip mrouter entries. */
> + if (!strcmp(igmp_group->mcgroup.name, OVN_IGMP_GROUP_MROUTERS)) {
> + continue;
> + }
>
> - if (ovs_list_is_empty(&od->mcast_info.groups)) {
> + /* For IPv6 only relay routable multicast groups
> + * (RFC 4291 2.7).
> + */
> + if (!IN6_IS_ADDR_V4MAPPED(&group_address) &&
> + !ipv6_addr_is_routable_multicast(&group_address)) {
> continue;
> }
>
> + /* Build IGMP groups for multicast routers with relay enabled.
> + * The router IGMP groups are based on the groups learnt by their
> + * multicast enabled peers.
> + */
> for (size_t i = 0; i < od->n_router_ports; i++) {
> struct ovn_port *router_port = od->router_ports[i]->peer;
>
> @@ -232,38 +238,19 @@ build_mcast_groups(const struct sbrec_igmp_group_table
> *sbrec_igmp_group_table,
> continue;
> }
>
> - struct ovn_igmp_group *igmp_group;
> - LIST_FOR_EACH (igmp_group, list_node, &od->mcast_info.groups) {
> - struct in6_addr *address = &igmp_group->address;
> -
> - /* Skip mrouter entries. */
> - if (!strcmp(igmp_group->mcgroup.name,
> - OVN_IGMP_GROUP_MROUTERS)) {
> - continue;
> - }
> -
> - /* For IPv6 only relay routable multicast groups
> - * (RFC 4291 2.7).
> - */
> - if (!IN6_IS_ADDR_V4MAPPED(address) &&
> - !ipv6_addr_is_routable_multicast(address)) {
> - continue;
> - }
> -
> - struct ovn_igmp_group *igmp_group_rtr =
> - ovn_igmp_group_add(sbrec_mcast_group_by_name_dp,
> - igmp_groups, router_port->od,
> - address, igmp_group->mcgroup.name);
> - struct ovn_port **router_igmp_ports =
> - xmalloc(sizeof *router_igmp_ports);
> - /* Store the chassis redirect port otherwise traffic will
> not
> - * be tunneled properly.
> - */
> - router_igmp_ports[0] = router_port->cr_port
> - ? router_port->cr_port
> - : router_port;
> - ovn_igmp_group_add_entry(igmp_group_rtr, router_igmp_ports,
> 1);
> - }
> + struct ovn_igmp_group *igmp_group_rtr =
> + ovn_igmp_group_add(sbrec_mcast_group_by_name_dp,
> + igmp_groups, router_port->od,
> + &group_address, igmp_group->mcgroup.name);
> + struct ovn_port **router_igmp_ports =
> + xmalloc(sizeof *router_igmp_ports);
> + /* Store the chassis redirect port otherwise traffic will not
> + * be tunneled properly.
> + */
> + router_igmp_ports[0] = router_port->cr_port
> + ? router_port->cr_port
> + : router_port;
> + ovn_igmp_group_add_entry(igmp_group_rtr, router_igmp_ports, 1);
> }
> }
>
> @@ -522,8 +509,6 @@ ovn_igmp_group_add(struct ovsdb_idl_index
> *sbrec_mcast_group_by_name_dp,
>
> hmap_insert(igmp_groups, &igmp_group->hmap_node,
> ovn_igmp_group_hash(datapath, address));
> - ovs_list_push_back(&datapath->mcast_info.groups,
> - &igmp_group->list_node);
> }
>
> return igmp_group;
> @@ -656,7 +641,6 @@ ovn_igmp_group_destroy(struct hmap *igmp_groups,
> free(entry);
> }
> hmap_remove(igmp_groups, &igmp_group->hmap_node);
> - ovs_list_remove(&igmp_group->list_node);
> free(igmp_group);
> }
> }
> diff --git a/northd/en-multicast.h b/northd/en-multicast.h
> index 5fa4d8976..9a6848f78 100644
> --- a/northd/en-multicast.h
> +++ b/northd/en-multicast.h
> @@ -62,7 +62,6 @@ struct ovn_igmp_group_entry {
> */
> struct ovn_igmp_group {
> struct hmap_node hmap_node; /* Index on 'datapath' and 'address'. */
> - struct ovs_list list_node; /* Linkage in the per-dp igmp group list. */
>
> struct ovn_datapath *datapath;
> struct in6_addr address; /* Multicast IPv6-mapped-IPv4 or IPv4 address.
> */
> diff --git a/northd/northd.c b/northd/northd.c
> index 59495717d..db62a2bfc 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -668,7 +668,6 @@ init_mcast_info_for_datapath(struct ovn_datapath *od)
> hmap_init(&od->mcast_info.group_tnlids);
> /* allocations start from hint + 1 */
> od->mcast_info.group_tnlid_hint = OVN_MIN_IP_MULTICAST - 1;
> - ovs_list_init(&od->mcast_info.groups);
>
> if (od->nbs) {
> init_mcast_info_for_switch_datapath(od);
> @@ -9768,39 +9767,6 @@ build_lswitch_destination_lookup_bmcast(struct
> ovn_datapath *od,
> "ip6.mcast_flood",
> "outport = \""MC_FLOOD"\"; output;",
> lflow_ref);
> -
> - /* Forward uregistered IP multicast to routers with relay enabled
> - * and to any ports configured to flood IP multicast traffic.
> - * If configured to flood unregistered traffic this will be
> - * handled by the L2 multicast flow.
> - */
> - if (!mcast_sw_info->flood_unregistered) {
> - ds_clear(actions);
> -
> - if (mcast_sw_info->flood_relay) {
> - ds_put_cstr(actions,
> - "clone { "
> - "outport = \""MC_MROUTER_FLOOD"\"; "
> - "output; "
> - "}; ");
> - }
> -
> - if (mcast_sw_info->flood_static) {
> - ds_put_cstr(actions, "outport =\""MC_STATIC"\"; output;");
> - }
> -
> - /* Explicitly drop the traffic if relay or static flooding
> - * is not configured.
> - */
> - if (!mcast_sw_info->flood_relay &&
> - !mcast_sw_info->flood_static) {
> - ds_put_cstr(actions, debug_drop_action());
> - }
> -
> - ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 80,
> - "ip4.mcast || ip6.mcast",
> - ds_cstr(actions), lflow_ref);
> - }
> }
>
> if (!smap_get_bool(&od->nbs->other_config,
> @@ -9815,6 +9781,48 @@ build_lswitch_destination_lookup_bmcast(struct
> ovn_datapath *od,
> "outport = \""MC_FLOOD"\"; output;", lflow_ref);
> }
>
> +/* Ingress table destination lookup, multicast handling (priority 80). */
> +static void
> +build_mcast_flood_lswitch(struct ovn_datapath *od, struct lflow_table
> *lflows,
> + struct ds *actions, struct lflow_ref *lflow_ref)
> +{
> + ovs_assert(od->nbs);
> + struct mcast_switch_info *mcast_sw_info = &od->mcast_info.sw;
> + if (!mcast_sw_info->enabled || mcast_sw_info->flood_unregistered) {
> + return;
> + }
> +
> + ds_clear(actions);
> +
> + /* Forward unregistered IP multicast to routers with relay enabled
> + * and to any ports configured to flood IP multicast traffic.
> + * If configured to flood unregistered traffic this will be
> + * handled by the L2 multicast flow.
> + */
> + if (mcast_sw_info->flood_relay) {
> + ds_put_cstr(actions,
> + "clone { "
> + "outport = \""MC_MROUTER_FLOOD"\"; "
> + "output; "
> + "}; ");
> + }
> +
> + if (mcast_sw_info->flood_static) {
> + ds_put_cstr(actions, "outport =\""MC_STATIC"\"; output;");
> + }
> +
> + /* Explicitly drop the traffic if relay or static flooding
> + * is not configured.
> + */
> + if (!mcast_sw_info->flood_relay &&
> + !mcast_sw_info->flood_static) {
> + ds_put_cstr(actions, debug_drop_action());
> + }
> +
> + ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 80,
> + "ip4.mcast || ip6.mcast", ds_cstr(actions), lflow_ref);
> +}
> +
>
> /* Ingress table 25: Add IP multicast flows learnt from IGMP/MLD
> * (priority 90). */
> @@ -9822,11 +9830,10 @@ static void
> build_lswitch_ip_mcast_igmp_mld(struct ovn_igmp_group *igmp_group,
> struct lflow_table *lflows,
> struct ds *actions,
> - struct ds *match)
> + struct ds *match,
> + struct lflow_ref *lflow_ref)
> {
> - if (!(igmp_group->datapath && igmp_group->datapath->nbs)) {
> - return;
> - }
> + ovs_assert(igmp_group->datapath->nbs);
>
> uint64_t dummy;
>
> @@ -9896,7 +9903,7 @@ build_lswitch_ip_mcast_igmp_mld(struct ovn_igmp_group
> *igmp_group,
> igmp_group->mcgroup.name);
>
> ovn_lflow_add(lflows, igmp_group->datapath, S_SWITCH_IN_L2_LKUP,
> - 90, ds_cstr(match), ds_cstr(actions), NULL);
> + 90, ds_cstr(match), ds_cstr(actions), lflow_ref);
> }
>
> /* Ingress table 25: Destination lookup, unicast handling (priority 50), */
> @@ -13499,14 +13506,50 @@ build_route_flows_for_lrouter(
> unique_routes_destroy(&unique_routes);
> }
>
> +static void
> +build_igmp_flows_for_lrouter(struct ovn_igmp_group *igmp_group,
> + struct lflow_table *lflows,
> + struct ds *match, struct ds *actions,
> + struct lflow_ref *lflow_ref)
> +{
> + ovs_assert(igmp_group->datapath->nbr);
> +
> + if (!igmp_group->datapath->mcast_info.rtr.relay) {
> + return;
> + }
> +
> + ds_clear(match);
> + ds_clear(actions);
> + if (IN6_IS_ADDR_V4MAPPED(&igmp_group->address)) {
> + ds_put_format(match, "ip4 && ip4.dst == %s ",
> + igmp_group->mcgroup.name);
> + } else {
> + ds_put_format(match, "ip6 && ip6.dst == %s ",
> + igmp_group->mcgroup.name);
> + }
> + if (igmp_group->datapath->mcast_info.rtr.flood_static) {
> + ds_put_cstr(actions,
> + "clone { "
> + "outport = \""MC_STATIC"\"; "
> + "ip.ttl--; "
> + "next; "
> + "};");
> + }
> + ds_put_format(actions, "outport = \"%s\"; ip.ttl--; next;",
> + igmp_group->mcgroup.name);
> + ovn_lflow_add(lflows, igmp_group->datapath, S_ROUTER_IN_IP_ROUTING,
> 10500,
> + ds_cstr(match), ds_cstr(actions),
> + lflow_ref);
> +}
> +
> /* IP Multicast lookup. Here we set the output port, adjust TTL and
> * advance to next table (priority 500).
> */
> static void
> -build_mcast_lookup_flows_for_lrouter(
> - struct ovn_datapath *od, struct lflow_table *lflows,
> - struct ds *match, struct ds *actions,
> - struct lflow_ref *lflow_ref)
> +build_mcast_lookup_flows_for_lrouter(struct ovn_datapath *od,
> + struct lflow_table *lflows,
> + struct ds *match,
> + struct lflow_ref *lflow_ref)
> {
> ovs_assert(od->nbr);
>
> @@ -13520,33 +13563,6 @@ build_mcast_lookup_flows_for_lrouter(
> return;
> }
>
> - struct ovn_igmp_group *igmp_group;
> -
> - LIST_FOR_EACH (igmp_group, list_node, &od->mcast_info.groups) {
> - ds_clear(match);
> - ds_clear(actions);
> - if (IN6_IS_ADDR_V4MAPPED(&igmp_group->address)) {
> - ds_put_format(match, "ip4 && ip4.dst == %s ",
> - igmp_group->mcgroup.name);
> - } else {
> - ds_put_format(match, "ip6 && ip6.dst == %s ",
> - igmp_group->mcgroup.name);
> - }
> - if (od->mcast_info.rtr.flood_static) {
> - ds_put_cstr(actions,
> - "clone { "
> - "outport = \""MC_STATIC"\"; "
> - "ip.ttl--; "
> - "next; "
> - "};");
> - }
> - ds_put_format(actions, "outport = \"%s\"; ip.ttl--; next;",
> - igmp_group->mcgroup.name);
> - ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 10500,
> - ds_cstr(match), ds_cstr(actions),
> - lflow_ref);
> - }
> -
> /* If needed, flood unregistered multicast on statically configured
> * ports. Otherwise drop any multicast traffic.
> */
> @@ -16961,6 +16977,7 @@ build_lswitch_and_lrouter_iterate_by_ls(struct
> ovn_datapath *od,
> build_lswitch_output_port_sec_od(od, lsi->lflows, NULL);
> build_lswitch_lb_affinity_default_flows(od, lsi->lflows, NULL);
> build_lswitch_lflows_l2_unknown(od, lsi->lflows, NULL);
> + build_mcast_flood_lswitch(od, lsi->lflows, &lsi->actions, NULL);
> }
>
> /* Helper function to combine all lflow generation which is iterated by
> @@ -16980,8 +16997,7 @@ build_lswitch_and_lrouter_iterate_by_lr(struct
> ovn_datapath *od,
> build_route_flows_for_lrouter(od, lsi->lflows,
> lsi->parsed_routes, lsi->route_tables,
> lsi->bfd_ports, NULL);
> - build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match,
> - &lsi->actions, NULL);
> + build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match, NULL);
> build_ingress_policy_flows_for_lrouter(od, lsi->lflows, lsi->lr_ports,
> lsi->route_policies, NULL);
> build_arp_resolve_flows_for_lrouter(od, lsi->lflows, NULL);
> @@ -17243,9 +17259,16 @@ build_lflows_thread(void *arg)
> if (stop_parallel_processing()) {
> return NULL;
> }
> - build_lswitch_ip_mcast_igmp_mld(igmp_group, lsi->lflows,
> - &lsi->match,
> - &lsi->actions);
> + if (igmp_group->datapath->nbs) {
> + build_lswitch_ip_mcast_igmp_mld(igmp_group,
> + lsi->lflows,
> + &lsi->actions,
> + &lsi->match, NULL);
> + } else {
> + build_igmp_flows_for_lrouter(igmp_group, lsi->lflows,
> + &lsi->actions,
> + &lsi->match, NULL);
Nit: The "name mismatch" between the switch and router case is not that
nice.
I renamed this function to build_lrouter_ip_mcast_igmp_mld().
With this small modification, I applied the patch to main.
Thanks,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
