On 10/14/26 7:16 PM, Jacob Tanenbaum via dev 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-4438
> Assisted-by: Qwen3.8-27B-FP8, opencode
> Signed-off-by: Jacob Tanenbaum <[email protected]>
>
Hi, Jacob. Not a full review, I didn't evaluate the design of the solution,
but I ran some LLM with it and it reported a few plausible issues. See below.
>From myself, a general comment: Please, trim down the commit message and
all the comments in the patch. There is too much unnecessary text and text
that make no sense in the context of the code, e.g. mentioning 330 ports
make no sense in the code. A lot of details in those comments will also
inevitably go stale. It is going to be hard to maintain. Only comment on
things that are necessary and not obvious.
Best regards, Ilya Maximets.
The LLM review follows:
This patch stops sending IGMP/MLD querier packets through the _MC_flood
group. Instead, physical_run() publishes each datapath's local flood
ports, and pinctrl sends the queries to those ports in batches of
MCAST_QUERIER_PORT_BATCH ports per PACKET_OUT.
> +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 (list, hmap_node, &mcast_querier_ports) {
> + if (list->dp_key == dp_key) {
> + break;
> + }
> + }
> + 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));
> + }
[...]
Who removes an entry from mcast_querier_ports once a datapath's _MC_flood
group is gone? This function only inserts or overwrites, and no remove
or clear helper is exported.
In physical_handle_mc_group_changes(), the deleted-row branch only does:
if (sbrec_multicast_group_is_deleted(mc)) {
ofctrl_remove_flows(flow_table, &mc->header_.uuid);
physical_run() calls consider_mc_group() only for rows that still exist,
and it does not clear the map first. Entries look like they are freed
only in ip_mcast_snoop_destroy() at shutdown.
That means each deleted logical switch leaves behind its dp_key entry and
port array, and so does each datapath that drops out of conditional
monitoring. northd hands out tunnel keys with a moving hint in
ovn_allocate_tnlid(), so churn keeps adding fresh keys. Can this make
memory grow without bound in a long-running ovn-controller?
If a tunnel key is later reused, the entry would also report
published=true with old port keys and ct zones until consider_mc_group()
refreshes it. Would it make sense to drop the entry in the deleted
branch, and to prune entries during a full recompute?
---
> +static bool
> +ip_mcast_querier_ports_published(int64_t dp_key, size_t *n_ports)
> +{
> + ovs_mutex_lock(&pinctrl_mutex);
> +
> + struct mcast_querier_port_list *list;
> + bool published = false;
> + HMAP_FOR_EACH (list, hmap_node, &mcast_querier_ports) {
> + if (list->dp_key == dp_key) {
[...]
Entries are inserted with ip_mcast_snoop_hash(dp_key), but every lookup
here scans the whole map. The same happens in
pinctrl_mcast_querier_set_ports() and ip_mcast_querier_get_ports().
Could these lookups use HMAP_FOR_EACH_WITH_HASH, the way
ip_mcast_snoop_find() does?
The full scan matters because consider_mc_group() publishes an entry for
every _MC_flood group, including non-local datapaths:
> + if (!ldp) {
[...]
> + if (mc->tunnel_key == OVN_MCAST_FLOOD_TUNNEL_KEY) {
> + pinctrl_mcast_querier_set_ports(mc->datapath->tunnel_key, NULL,
> + 0);
> + }
With monitor-all, the map grows to one entry per logical switch, so a
full physical_run() costs about D^2/2 comparisons under pinctrl_mutex.
For example, 10k switches give about 5*10^7 iterations per recompute.
This cost is paid even when no querier is enabled anywhere. Is that
quadratic cost in pflow_output, and the lock contention with
pinctrl_handler that comes with it, expected?
---
> +#define MCAST_QUERIER_PORT_BATCH 100
[...]
> + for (size_t start = 0; start < n_ports;
> + start += MCAST_QUERIER_PORT_BATCH) {
> + size_t end = MIN(start + MCAST_QUERIER_PORT_BATCH, n_ports);
Could a test be added for the new batching in
ip_mcast_querier_send_port_fanout()? The existing IGMP/MLD tests use only
a few ports. They never reach the original MAX_RESUBMITS failure or the
batch boundaries (for example 100, 101 and 200 ports).
The tests also don't seem to cover the new published/empty/unpublished
states in ip_mcast_querier_send(): the 1 s retry while the list is
unpublished, and the skipped query when a datapath has no local ports.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
