diff mbox series

[ovs-dev,v2,3/4] northd: Remove the list of groups from mcast info.

Message ID 20250116122532.725005-4-amusil@redhat.com
State Superseded
Headers show
Series Incremental processing of IGMP changes in northd. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Ales Musil Jan. 16, 2025, 12:25 p.m. UTC
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 <jtanenba@redhat.com>
Signed-off-by: Jacob Tanenbaum <jtanenba@redhat.com>
Suggested-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Signed-off-by: Ales Musil <amusil@redhat.com>
---
 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 mbox series

Patch

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 ddd05f7e4..905f19ff1 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);
@@ -9815,39 +9814,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,
@@ -9862,6 +9828,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). */
@@ -9869,11 +9877,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;
 
@@ -9943,7 +9950,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), */
@@ -13546,14 +13553,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);
 
@@ -13567,33 +13610,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.
      */
@@ -17007,6 +17023,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
@@ -17026,8 +17043,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);
@@ -17289,9 +17305,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);
+                    }
                 }
             }
             lsi->thread_lflow_counter = thread_lflow_counter;
@@ -17515,10 +17538,14 @@  build_lswitch_and_lrouter_flows(
         stopwatch_stop(LFLOWS_LS_STATEFUL_STOPWATCH_NAME, time_msec());
         stopwatch_start(LFLOWS_IGMP_STOPWATCH_NAME, time_msec());
         HMAP_FOR_EACH (igmp_group, hmap_node, igmp_groups) {
-            build_lswitch_ip_mcast_igmp_mld(igmp_group,
-                                            lsi.lflows,
-                                            &lsi.actions,
-                                            &lsi.match);
+            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);
+            }
         }
         stopwatch_stop(LFLOWS_IGMP_STOPWATCH_NAME, time_msec());
 
diff --git a/northd/northd.h b/northd/northd.h
index 044704c70..218f0f62d 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -292,7 +292,6 @@  struct mcast_info {
 
     struct hmap group_tnlids;  /* Group tunnel IDs in use on this DP. */
     uint32_t group_tnlid_hint; /* Hint for allocating next group tunnel ID. */
-    struct ovs_list groups;    /* List of groups learnt on this DP. */
 
     union {
         struct mcast_switch_info sw;  /* Switch specific multicast info. */