On Wed, Oct 21, 2026 at 8:32 AM Xavier Simonart <[email protected]> wrote:
> Hi Jacob > > Thanks for the patch. > Here are some comments / questions below. > > On Wed, Oct 14, 2026 at 7:23 PM Jacob Tanenbaum via dev < > [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). >> > Should we not also add some new tests covering the batching boundaries and > the published/unpublished/empty states ? > >> >> Reported-at: https://redhat.atlassian.net/browse/FDP-4438 >> Assisted-by: Qwen3.8-27B-FP8, opencode >> Signed-off-by: Jacob Tanenbaum <[email protected]> >> >> diff --git a/controller/physical.c b/controller/physical.c >> index 8c5d9b940..aef5357bc 100644 >> --- a/controller/physical.c >> +++ b/controller/physical.c >> @@ -2922,6 +2922,23 @@ mc_ofctrl_add_flow(const struct >> sbrec_multicast_group *mc, >> ofpbuf_clear(&ctx->ofpacts); >> } >> >> +/* Appends (port_key, ct_zone) to 'flood_ports' ('flood_ports' must have >> room >> + * for one more entry; 'n_flood_ports' is the current count). */ >> +static void >> +flood_querier_port_add(struct mcast_querier_port *flood_ports, >> + size_t *n_flood_ports, uint32_t port_key, >> + int zone_id) >> +{ >> + if (!flood_ports) { >> + return; >> + } >> + size_t i = *n_flood_ports; >> + >> + flood_ports[i].port_key = port_key; >> + flood_ports[i].ct_zone = zone_id; >> + (*n_flood_ports)++; >> +} >> + >> static void >> consider_mc_group(const struct physical_ctx *ctx, >> const struct sbrec_multicast_group *mc, >> @@ -2930,12 +2947,31 @@ consider_mc_group(const struct physical_ctx *ctx, >> struct local_datapath *ldp = get_local_datapath(ctx->local_datapaths, >> >> mc->datapath->tunnel_key); >> if (!ldp) { >> + /* The datapath has no ports local to this chassis, so its flood >> + * group would not deliver queries locally. Publish an empty >> list >> + * so the querier can tell this apart from a list that has not >> been >> + * published yet. */ >> + if (mc->tunnel_key == OVN_MCAST_FLOOD_TUNNEL_KEY) { >> + pinctrl_mcast_querier_set_ports(mc->datapath->tunnel_key, >> NULL, >> + 0); >> + } >> return; >> } >> >> struct sset remote_chassis = SSET_INITIALIZER(&remote_chassis); >> struct sset vtep_chassis = SSET_INITIALIZER(&vtep_chassis); >> >> + /* The querier delivers the query to each local port of the flood >> group >> + * (in batches of up to MCAST_QUERIER_PORT_BATCH per OpenFlow >> message) >> + * instead of a single query that the flood group fans out per port >> (see >> + * pinctrl_mcast_querier_set_ports). Collect the local ports of the >> flood >> + * group to publish them to the querier. */ >> + struct mcast_querier_port *flood_ports = NULL; >> + size_t n_flood_ports = 0; >> + if (mc->tunnel_key == OVN_MCAST_FLOOD_TUNNEL_KEY) { >> + flood_ports = xzalloc(mc->n_ports * sizeof *flood_ports); >> + } >> + >> /* Go through all of the ports in the multicast group: >> * >> * - For remote ports, add the chassis to 'remote_chassis' or >> @@ -3005,6 +3041,8 @@ consider_mc_group(const struct physical_ctx *ctx, >> if (ldp->is_transit_switch) { >> local_set_ct_zone_and_output_pb(port->tunnel_key, >> zone_id, >> &local_ctx->ofpacts); >> + flood_querier_port_add(flood_ports, &n_flood_ports, >> + port->tunnel_key, zone_id); >> } else { >> local_output_pb(port->tunnel_key, &remote_ctx->ofpacts); >> local_output_pb(port->tunnel_key, &ramp_ctx->ofpacts); >> @@ -3026,9 +3064,13 @@ consider_mc_group(const struct physical_ctx *ctx, >> || type == LP_L3GATEWAY)) { >> local_set_ct_zone_and_output_pb(port->tunnel_key, zone_id, >> &local_ctx->ofpacts); >> + flood_querier_port_add(flood_ports, &n_flood_ports, >> + port->tunnel_key, zone_id); >> } else if (simap_contains(ctx->patch_ofports, >> port->logical_port)) { >> local_set_ct_zone_and_output_pb(port->tunnel_key, zone_id, >> &local_ctx->ofpacts); >> + flood_querier_port_add(flood_ports, &n_flood_ports, >> + port->tunnel_key, zone_id); >> } else if (type == LP_CHASSISREDIRECT >> && port->chassis == ctx->chassis) { >> const char *distributed_port = smap_get(&port->options, >> @@ -3042,6 +3084,9 @@ consider_mc_group(const struct physical_ctx *ctx, >> local_set_ct_zone_and_output_pb( >> distributed_binding->tunnel_key, zone_id, >> &local_ctx->ofpacts); >> + flood_querier_port_add(flood_ports, &n_flood_ports, >> + >> distributed_binding->tunnel_key, >> + zone_id); >> } >> } >> } else if (!get_localnet_port(ctx->local_datapaths, >> @@ -3068,6 +3113,12 @@ consider_mc_group(const struct physical_ctx *ctx, >> } >> } >> >> + if (flood_ports) { >> + pinctrl_mcast_querier_set_ports(mc->datapath->tunnel_key, >> flood_ports, >> + n_flood_ports); >> + free(flood_ports); >> + } >> + >> bool local_lports = local_ctx->ofpacts.size > 0; >> bool remote_ports = remote_ctx->ofpacts.size > 0; >> bool ramp_ports = ramp_ctx->ofpacts.size > 0; >> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >> index 5c9634b24..19f5bb8bf 100644 >> --- a/controller/pinctrl.c >> +++ b/controller/pinctrl.c >> @@ -291,6 +291,9 @@ static void ip_mcast_snoop_run(void) >> static void ip_mcast_querier_run(struct rconn *swconn, >> long long int *query_time); >> static void ip_mcast_querier_wait(long long int query_time); >> +static size_t ip_mcast_querier_get_ports(int64_t dp_key, >> + struct mcast_querier_port >> *ports, >> + size_t *n_ports); >> static void ip_mcast_sync( >> struct ovsdb_idl_txn *ovnsb_idl_txn, >> const struct sbrec_chassis *chassis, >> @@ -5436,6 +5439,33 @@ static struct hmap mcast_snoop_map >> OVS_GUARDED_BY(pinctrl_mutex); >> */ >> static struct ovs_list mcast_query_list; >> >> +/* >> + * Local-port fanout for the multicast querier. >> + * >> + * physical_run() (main thread) records, for each datapath, the local >> ports >> + * the multicast flood group would deliver to. The querier (run in the >> + * pinctrl_handler thread) then sends the query in batches of a bounded >> number >> + * of those ports instead of a single query via the flood group, so that >> OVS >> + * does not translate an unbounded per-port resubmit fan-out (which >> exceeds >> + * OVS's resubmit limit on nodes with many local ports and causes the >> query to >> + * be rejected with OFPFMFC_UNKNOWN). >> + * An entry exists once physical_run() has considered the datapath's >> flood >> + * group at least once, and may be empty when the flood group has no >> local >> + * ports on this chassis. >> + * >> + * 'mcast_querier_ports' (dp_key -> struct mcast_querier_port_list) is >> written >> + * by the main thread and read by the handler thread, both while holding >> + * pinctrl_mutex. >> + */ >> +struct mcast_querier_port_list { >> + struct hmap_node hmap_node; >> + uint32_t dp_key; >> + size_t n_ports; >> + struct mcast_querier_port *ports; >> +}; >> + >> +static struct hmap mcast_querier_ports OVS_GUARDED_BY(pinctrl_mutex); >> + >> /* Multicast config information stored independently by datapath key. >> * Protected by pinctrl_mutex. pinctrl_handler has RO access and >> pinctrl_main >> * has RW access. Read accesses from pinctrl_ip_mcast_handle() can be >> @@ -5754,6 +5784,7 @@ ip_mcast_snoop_init(void) >> hmap_init(&mcast_snoop_map); >> ovs_list_init(&mcast_query_list); >> hmap_init(&mcast_cfg_map); >> + hmap_init(&mcast_querier_ports); >> } >> >> static void >> @@ -5772,6 +5803,14 @@ ip_mcast_snoop_destroy(void) >> HMAP_FOR_EACH_POP (ip_ms_state, hmap_node, &mcast_cfg_map) { >> free(ip_ms_state); >> } >> + >> + struct mcast_querier_port_list *port_list; >> + >> + HMAP_FOR_EACH_POP (port_list, hmap_node, &mcast_querier_ports) { >> + free(port_list->ports); >> + free(port_list); >> + } >> + hmap_destroy(&mcast_querier_ports); >> } >> >> static void >> @@ -6204,10 +6243,12 @@ pinctrl_ip_mcast_handle(struct rconn *swconn, >> } >> } >> >> -static void >> -ip_mcast_querier_send_igmp(struct rconn *swconn, struct ip_mcast_snoop >> *ip_ms) >> +/* Composes an IGMPv3 multicast query into 'buf' (up to 'buf_size' >> bytes) and >> + * returns its length in bytes. */ >> +static size_t >> +ip_mcast_querier_compose_igmp(struct ip_mcast_snoop *ip_ms, void *buf, >> + size_t buf_size) >> { >> - /* Compose a multicast query. */ >> uint64_t packet_stub[128 / 8]; >> struct dp_packet packet; >> >> @@ -6223,35 +6264,21 @@ ip_mcast_querier_send_igmp(struct rconn *swconn, >> struct ip_mcast_snoop *ip_ms) >> uint8_t qqic = max_response; >> packet_set_igmp3_query(&packet, max_response, 0, false, 0, qqic); >> >> - /* Inject multicast query. */ >> - uint64_t ofpacts_stub[4096 / 8]; >> - struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub); >> - enum ofp_version version = rconn_get_version(swconn); >> - put_load(ip_ms->dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts); >> - put_load(OVN_MCAST_FLOOD_TUNNEL_KEY, MFF_LOG_OUTPORT, 0, 32, >> &ofpacts); >> - put_load(1, MFF_LOG_FLAGS, MLF_LOCAL_ONLY, 1, &ofpacts); >> - struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts); >> - resubmit->in_port = OFPP_CONTROLLER; >> - resubmit->table_id = OFTABLE_LOCAL_OUTPUT; >> - >> - struct ofputil_packet_out po = { >> - .packet = dp_packet_data(&packet), >> - .packet_len = dp_packet_size(&packet), >> - .buffer_id = UINT32_MAX, >> - .ofpacts = ofpacts.data, >> - .ofpacts_len = ofpacts.size, >> - }; >> - match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); >> - enum ofputil_protocol proto = >> ofputil_protocol_from_ofp_version(version); >> - queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); >> + size_t size = dp_packet_size(&packet); >> + if (size > buf_size) { >> + size = buf_size; >> > Doing this, are we not sending a corrupted packet? In that case, should we > instead drop the packet and WARN ? > This was actually to protect the memcpy() from a stack overflow. logging and dropping the packet is a better way to do it. > + } >> + memcpy(buf, dp_packet_data(&packet), size); >> dp_packet_uninit(&packet); >> - ofpbuf_uninit(&ofpacts); >> + return size; >> } >> >> -static void >> -ip_mcast_querier_send_mld(struct rconn *swconn, struct ip_mcast_snoop >> *ip_ms) >> +/* Composes an MLDv2 multicast query into 'buf' (up to 'buf_size' bytes) >> and >> + * returns its length in bytes. */ >> +static size_t >> +ip_mcast_querier_compose_mld(struct ip_mcast_snoop *ip_ms, void *buf, >> + size_t buf_size) >> { >> - /* Compose a multicast query. */ >> uint64_t packet_stub[128 / 8]; >> struct dp_packet packet; >> >> @@ -6273,31 +6300,162 @@ ip_mcast_querier_send_mld(struct rconn *swconn, >> struct ip_mcast_snoop *ip_ms) >> struct in6_addr unspecified = { { { 0 } } }; >> packet_set_mld_query(&packet, max_response, &unspecified, false, 0, >> qqic); >> >> - /* Inject multicast query. */ >> - uint64_t ofpacts_stub[4096 / 8]; >> - struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub); >> - enum ofp_version version = rconn_get_version(swconn); >> - put_load(ip_ms->dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts); >> - put_load(OVN_MCAST_FLOOD_TUNNEL_KEY, MFF_LOG_OUTPORT, 0, 32, >> &ofpacts); >> - put_load(1, MFF_LOG_FLAGS, MLF_LOCAL_ONLY, 1, &ofpacts); >> - struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts); >> - resubmit->in_port = OFPP_CONTROLLER; >> - resubmit->table_id = OFTABLE_LOCAL_OUTPUT; >> - >> - struct ofputil_packet_out po = { >> - .packet = dp_packet_data(&packet), >> - .packet_len = dp_packet_size(&packet), >> - .buffer_id = UINT32_MAX, >> - .ofpacts = ofpacts.data, >> - .ofpacts_len = ofpacts.size, >> - }; >> - match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); >> - enum ofputil_protocol proto = >> ofputil_protocol_from_ofp_version(version); >> - queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); >> + size_t size = dp_packet_size(&packet); >> + if (size > buf_size) { >> + size = buf_size; >> + } >> + memcpy(buf, dp_packet_data(&packet), size); >> dp_packet_uninit(&packet); >> - ofpbuf_uninit(&ofpacts); >> + return size; >> +} >> + >> +/* Maximum number of local ports bundled into a single querier >> PACKET_OUT. >> + * >> + * Each port in a batch is emitted as (load CT_ZONE, if set) + load >> OUTPORT + >> + * resubmit(CHECK_LOOPBACK). OVS translates a batch by resubmitting >> once per >> + * port through the egress pipeline, and the translation's resubmit >> counter >> + * accumulates across all ports in the batch against ofproto's >> MAX_RESUBMITS >> + * (4096) budget. The per-port egress path is a fixed ~19 resubmits, so >> a >> + * batch of 100 ports uses ~1900 of the 4096 budget, safely under the >> limit. >> + * Bundling keeps each OVS translation bounded (a single flood-group >> query of >> + * all ~330 ports would use ~6300 resubmits and be rejected) while >> reducing the >> + * number of OpenFlow PACKET_OUT messages to ceil(n_ports / 100). */ >> +#define MCAST_QUERIER_PORT_BATCH 100 >> + >> +/* Sends 'query' ('query_len' bytes) to each port in 'ports' (0 to >> + * 'n_ports'), in batches of at most MCAST_QUERIER_PORT_BATCH ports per >> + * PACKET_OUT. Each port in a batch uses the same per-port actions the >> flood >> + * group would apply for that port (load the port's conntrack zone, set >> the >> + * logical output port, resubmit to the CHECK_LOOPBACK table), so every >> port's >> + * query traverses the per-port egress pipeline exactly like the flood >> group >> + * delivery would. Bounding each PACKET_OUT to a fixed number of ports >> keeps >> + * each OVS translation under OVS's resubmit limit (which a single >> flood-group >> + * query of all local ports would exceed) while reducing the number of >> + * OpenFlow messages. */ >> +static void >> +ip_mcast_querier_send_port_fanout(struct rconn *swconn, >> + struct ip_mcast_snoop *ip_ms, >> + const void *query, size_t query_len, >> + const struct mcast_querier_port *ports, >> + size_t n_ports) >> +{ >> + enum ofputil_protocol proto = ofputil_protocol_from_ofp_version( >> + rconn_get_version(swconn)); >> + >> + for (size_t start = 0; start < n_ports; >> + start += MCAST_QUERIER_PORT_BATCH) { >> + size_t end = MIN(start + MCAST_QUERIER_PORT_BATCH, n_ports); >> + >> + struct ofpbuf ofpacts; >> + ofpbuf_init(&ofpacts, 0); >> + put_load(ip_ms->dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts); >> + for (size_t i = start; i < end; i++) { >> + const struct mcast_querier_port *port = &ports[i]; >> + >> + if (port->ct_zone) { >> + put_load(port->ct_zone, MFF_LOG_CT_ZONE, 0, 16, >> &ofpacts); >> + } >> + put_load(port->port_key, MFF_LOG_OUTPORT, 0, 32, &ofpacts); >> + struct ofpact_resubmit *resubmit = >> ofpact_put_RESUBMIT(&ofpacts); >> + resubmit->in_port = OFPP_CONTROLLER; >> + resubmit->table_id = OFTABLE_CHECK_LOOPBACK; >> + } >> + >> + struct ofputil_packet_out po = { >> + .packet = query, >> + .packet_len = query_len, >> + .buffer_id = UINT32_MAX, >> + .ofpacts = ofpacts.data, >> + .ofpacts_len = ofpacts.size, >> + }; >> + match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); >> + queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); >> + ofpbuf_uninit(&ofpacts); >> + } >> +} >> + >> +/* Fetches the local ports published by physical_run() for datapath >> 'dp_key'. >> + * Returns a newly allocated array of 'n_ports' entries (caller must >> free), or >> + * NULL when the list has not been published yet or is empty. */ >> +static struct mcast_querier_port * >> +ip_mcast_querier_fetch_ports(int64_t dp_key, size_t *n_ports) >> +{ >> + size_t n = ip_mcast_querier_get_ports(dp_key, NULL, NULL); >> + if (!n) { >> + *n_ports = 0; >> + return NULL; >> + } >> + struct mcast_querier_port *ports = xmalloc(n * sizeof *ports); >> + ip_mcast_querier_get_ports(dp_key, ports, &n); >> + *n_ports = n; >> + return ports; >> } >> >> +/* Composes and sends the IGMPv3 and MLDv2 queries (if ipv4 or ipv6 is >> + * enabled) to all local ports of the datapath's multicast flood group, >> in >> + * batches of a set number of ports per OpenFlow message (see >> + * ip_mcast_querier_send_port_fanout). Called only when >> + * ip_mcast_querier_send() has verified that the per-port list published >> by >> + * physical_run() is non-empty; if the list is emptied before the fetch >> below, >> + * the queries are skipped and sent on the next interval. */ >> +static void >> +ip_mcast_querier_send_query(struct rconn *swconn, struct ip_mcast_snoop >> *ip_ms) >> +{ >> + size_t n_ports; >> + struct mcast_querier_port *ports = >> + ip_mcast_querier_fetch_ports(ip_ms->dp_key, &n_ports); >> + if (!ports) { >> + return; >> + } >> + >> + char query_buf[128]; >> + if (ip_ms->cfg.querier_v4_enabled) { >> + size_t query_len = ip_mcast_querier_compose_igmp(ip_ms, >> query_buf, >> + sizeof >> query_buf); >> + ip_mcast_querier_send_port_fanout(swconn, ip_ms, query_buf, >> + query_len, ports, n_ports); >> + } >> + if (ip_ms->cfg.querier_v6_enabled) { >> + size_t query_len = ip_mcast_querier_compose_mld(ip_ms, query_buf, >> + sizeof >> query_buf); >> + ip_mcast_querier_send_port_fanout(swconn, ip_ms, query_buf, >> + query_len, ports, n_ports); >> + } >> + free(ports); >> +} >> + >> +/* Returns whether physical_run() has published the local ports of the >> + * datapath's multicast flood group for datapath 'dp_key' yet (the list >> may >> + * be empty), and, when 'n_ports' is non-NULL, the number of ports in the >> + * list. >> + * >> + * Called from the pinctrl_handler thread. Takes pinctrl_mutex >> internally. >> + */ >> +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) { >> > Could we use HMAP_FOR_EACH_WITH_HASH ? > Yes that would be better, I will change this. > + if (list->dp_key == dp_key) { >> + if (n_ports) { >> + *n_ports = list->n_ports; >> + } >> + published = true; >> + break; >> + } >> + } >> + >> + ovs_mutex_unlock(&pinctrl_mutex); >> + return published; >> +} >> + >> +/* Retry interval (ms) the querier uses while physical_run() has not yet >> + * published the local ports of the datapath's multicast flood group. */ >> +#define MCAST_QUERIER_INITIAL_RETRY_MS 1000 >> + >> static long long int >> ip_mcast_querier_send(struct rconn *swconn, struct ip_mcast_snoop *ip_ms, >> long long int current_time) >> @@ -6306,16 +6464,30 @@ ip_mcast_querier_send(struct rconn *swconn, >> struct ip_mcast_snoop *ip_ms, >> return ip_ms->query_time_ms; >> } >> >> - if (ip_ms->cfg.querier_v4_enabled) { >> - ip_mcast_querier_send_igmp(swconn, ip_ms); >> - } >> + size_t n_ports; >> + bool published = >> + ip_mcast_querier_ports_published(ip_ms->dp_key, &n_ports); >> >> - if (ip_ms->cfg.querier_v6_enabled) { >> - ip_mcast_querier_send_mld(swconn, ip_ms); >> - } >> + /* While the port list has not been published yet, retry at a short >> + * backoff so that the first query is not delayed by a full query >> + * interval; afterwards, retry on the regular query interval. */ >> + long long int interval_ms = >> + published ? ip_ms->cfg.query_interval_s * 1000 >> + : MCAST_QUERIER_INITIAL_RETRY_MS; >> >> /* Set the next query time. */ >> - ip_ms->query_time_ms = current_time + ip_ms->cfg.query_interval_s * >> 1000; >> + ip_ms->query_time_ms = current_time + interval_ms; >> + >> + /* Skip the query if the local ports of the datapath's multicast >> flood >> + * group have not been published by physical_run() yet, or the flood >> + * group has no local ports. A flood-group query would not be >> delivered >> + * locally in either state, so just retry on the next interval. */ >> + if (!published || !n_ports) { >> + return ip_ms->query_time_ms; >> + } >> + >> + ip_mcast_querier_send_query(swconn, ip_ms); >> > Do we really need both ip_mcast_querier_ports_published > and ip_mcast_querier_get_ports ? > It looks to me like we loop 3x times (with each lock, scan, unlock). Can't > we do this in a single loop? > I think you are right and we can do one loop I will change this. > + >> return ip_ms->query_time_ms; >> } >> >> @@ -6349,6 +6521,80 @@ ip_mcast_querier_wait(long long int query_time) >> } >> } >> >> +/* Records, for datapath 'dp_key', the local ports that the multicast >> flood >> + * group delivers to. 'ports' may be NULL, in which case an empty list >> is >> + * stored: this both avoids keeping a stale list from a previous pass and >> + * lets the querier distinguish "the flood group has no local ports on >> this >> + * chassis" from "the list has not been published yet". >> + * >> + * Called from the main thread (physical_run and the mc group changes >> + * handler) while it iterates the multicast groups. Takes pinctrl_mutex >> + * internally, so the caller must not already hold it. >> + */ >> +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)); >> > Is there ever deleted e.g. if the datapath got deleted (I only see it > drained in ip_mcast_snoop_destroy()) ? > Nothing deletes per datapath, this is a good spot and is a leak. I will fix this > + } >> + >> + 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); >> +} >> + >> +/* Returns the number of local ports the querier should fan out to for >> + * datapath 'dp_key' (0 if none are known yet), and, when 'ports' and >> + * 'n_ports' are non-NULL, copies up to *n_ports of them into 'ports' and >> + * updates *n_ports to the number copied. >> + * >> + * Called from the pinctrl_handler thread. Takes pinctrl_mutex >> internally. >> + */ >> +static size_t >> +ip_mcast_querier_get_ports(int64_t dp_key, struct mcast_querier_port >> *ports, >> + size_t *n_ports) >> +{ >> + ovs_mutex_lock(&pinctrl_mutex); >> + >> + size_t n = 0; >> + struct mcast_querier_port_list *list; >> + HMAP_FOR_EACH (list, hmap_node, &mcast_querier_ports) { >> + if (list->dp_key != dp_key) { >> + continue; >> + } >> + n = list->n_ports; >> + if (ports && n_ports) { >> + size_t count = MIN(n, *n_ports); >> + memcpy(ports, list->ports, count * sizeof *ports); >> + *n_ports = count; >> + } >> + break; >> + } >> + >> + ovs_mutex_unlock(&pinctrl_mutex); >> + return n; >> +} >> + >> static void >> send_garp_rarp_wait(long long int send_garp_rarp_time) >> { >> diff --git a/controller/pinctrl.h b/controller/pinctrl.h >> index a638fe29f..bd69539ca 100644 >> --- a/controller/pinctrl.h >> +++ b/controller/pinctrl.h >> @@ -86,4 +86,35 @@ void send_self_originated_neigh_packet(struct rconn >> *swconn, >> struct in6_addr *local, >> struct in6_addr *target, >> uint8_t table_id); >> + >> +/* A local port that the multicast flood group would deliver a query to, >> along >> + * with the conntrack zone that the flood group's per-port action loads. >> */ >> +struct mcast_querier_port { >> + uint32_t port_key; >> + uint16_t ct_zone; >> +}; >> + >> +/* Multicast querier local-port fanout. >> + * >> + * Instead of the querier injecting a single IGMP/MLD query via the >> multicast >> + * flood group (which OVS translates by resubmitting once per local port, >> + * exceeding OVS's resubmit limit on nodes with many local ports and >> causing >> + * the query to be rejected with OFPFMFC_UNKNOWN), the querier injects >> the >> + * query in batches of a bounded number of local ports per OpenFlow >> message. >> + * Each port in a batch is injected the same way a single port of the >> flood >> + * group would be: load the port's conntrack zone, set the logical >> output port, >> + * and resubmit to the CHECK_LOOPBACK table. Bounding each message to a >> fixed >> + * number of ports keeps each OVS translation under OVS's resubmit limit >> while >> + * reducing the number of OpenFlow messages. >> + * >> + * The physical node (physical_run and the mc group changes handler) >> knows >> + * exactly which local ports the flood group would deliver to; it >> publishes >> + * them here so the pinctrl_handler thread can fan the query out per >> port. >> + * >> + * Called from the main thread; read by the handler thread (the querier). >> + * Takes pinctrl_mutex internally. >> + */ >> +void pinctrl_mcast_querier_set_ports(int64_t dp_key, >> + const struct mcast_querier_port >> *ports, >> + size_t n_ports); >> #endif /* controller/pinctrl.h */ >> -- >> 2.56.0 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks > Xavier > Thanks for the review! I will fix these in a V2 Jacob _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
