On 10/26/26 3:02 PM, Xavier Simonart wrote: > Hi Jacob > > Thanks for the v3. > LGTM > Acked-by: Xavier Simonart <[email protected]> > > Thanks > Xavier >
Hi Jacob, Xavier, Thanks for the new revision and for the reviews! > > On Mon, Oct 26, 2026 at 2:14 AM Jacob Tanenbaum <[email protected]> wrote: > >> The IGMP/MLD querier injects a single query through the >> multicast flood group. The flood group's OpenFlow flow >> contains one "load OUTPORT; resubmit(CHECK_LOOPBACK)" >> action per local port, and OVS translates the entire thing >> in a single packet_xlate(). The translation's resubmit >> counter accumulates across all of those ports, so on a node >> with many local ports (~330) the translation exceeds OVS's >> MAX_RESUBMITS (4096) budget, packet_xlate() returns >> OFPFMFC_UNKNOWN, and the query is dropped. >> >> Rather than relying on the flood group, have the querier >> inject the query directly to the flood group's local ports. >> consider_mc_group() already determines exactly which local >> ports the flood group would deliver to (and each port's >> conntrack zone); publish that list to pinctrl so the querier >> can fan the query out per port. Publish an empty list when >> the flood group has no local ports on this chassis (including >> datapaths with no local ports at all), so the querier can >> distinguish that from a list that has not been published >> yet. >> >> The querier sends one OpenFlow PACKET_OUT per batch of up to >> 100 ports. OVS executes every resubmit in an action list >> (the flood group relies on this), and a batch of 100 ports >> uses ~1900 of the 4096-resubmit budget because the per-port >> egress path is a fixed ~19 resubmits. The single flood >> query of ~330 ports used ~6300 resubmits. >> >> Each port's query still traverses the per-port egress >> pipeline, so multicast ACLs still filter which ports >> receive it. If the port list has not been published >> yet (first transient cycles), the querier skips the query >> and retries every second; once published, an empty list >> means the flood group has no local ports, so the query is >> skipped on the regular query interval. A flood-group query >> would not be delivered locally in either state. >> >> Tested with the IGMP/MLD snoop/querier/relay, external >> querier, and multicast relay unit tests (real >> ovn-controller + vswitchd, pcap-validated query delivery). >> >> Reported-at: https://redhat.atlassian.net/browse/FDP-4445 I guess we should maybe also handle other ovn-controller generated broadcast/multicast packets in a similar way, e.g., ipv6_ra_send(), send_arp_nd() but that's a bit out of the scope of the current change. Would you have time to work on this in the future by any chance? >> Assisted-by: Qwen3.8-27B-FP8, opencode >> Signed-off-by: Jacob Tanenbaum <[email protected]> >> >> --- >> >> static void >> @@ -5895,6 +5921,16 @@ ip_mcast_sync(struct ovsdb_idl_txn *ovnsb_idl_txn, >> } >> } >> >> + /* Remove the querier port lists of datapaths that no longer exist */ Nit: missing period at end of sentence. >> + struct mcast_querier_port_list *port_list; >> + HMAP_FOR_EACH_SAFE (port_list, hmap_node, &mcast_querier_ports) { >> + if (!get_local_datapath(local_datapaths, port_list->dp_key)) { >> + hmap_remove(&mcast_querier_ports, &port_list->hmap_node); >> + free(port_list->ports); >> + free(port_list); >> + } >> + } >> + >> const struct sbrec_igmp_group *sbrec_ip_mrouter; >> const struct sbrec_igmp_group *sbrec_igmp; >> >> >> +/* Sets the local ports of the flood group for datapath dp_key. >> + * ports may be NULL to store and empty list which lets the querier Typo: "store and empty list". >> + * distinguish "no local ports" from "not yet published". */ >> +void >> +pinctrl_mcast_querier_set_ports(int64_t dp_key, >> + const struct mcast_querier_port *ports, >> + size_t n_ports) >> +{ >> + ovs_mutex_lock(&pinctrl_mutex); >> + >> + struct mcast_querier_port_list *list; >> + HMAP_FOR_EACH_WITH_HASH (list, hmap_node, ip_mcast_snoop_hash(dp_key), >> + &mcast_querier_ports) { >> + if (list->dp_key == dp_key) { >> + break; >> + } >> + } We're assuming that HMAP_FOR_EACH_WITH_HASH will set 'list' to NULL when it reached the end of the bucket list. That seems a bit risky to me, I'd use a "found" boolean instead. >> + if (!list) { >> + list = xzalloc(sizeof *list); >> + list->dp_key = dp_key; >> + hmap_insert(&mcast_querier_ports, &list->hmap_node, >> + ip_mcast_snoop_hash(dp_key)); >> + } >> + >> + free(list->ports); >> + if (n_ports) { >> + list->ports = xmalloc(n_ports * sizeof *list->ports); >> + memcpy(list->ports, ports, n_ports * sizeof *list->ports); >> + } else { >> + list->ports = NULL; >> + } >> + list->n_ports = n_ports; >> + >> + ovs_mutex_unlock(&pinctrl_mutex); >> +} >> + The rest looks good to me so I took care of the minor nits above and applied the patch to main, 26.09 and 26.03. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
