On Wed, 26 Aug 2026 11:10:09 -0500 Weijun Pan <[email protected]> wrote:
> Bonding configuration and LACP state are owned by the primary > process. Install a reduced dev_ops table in secondary processes so > ethdev rejects control operations, and reject the bonding control > API when called from a non-primary process. Query and detach remain > available. > > Bugzilla ID: 1900 > > Signed-off-by: Weijun Pan <[email protected]> > --- I would like to see link update use rte_eth_linkstatus_set() helper. And do another check that documentation matches reality after this patch. Overly verbose (sorry) AI review was: Applies cleanly to main (d55ccd4), diffstat matches, and builds with -Dwerror=true for net/bonding. checkpatches.sh is clean. Fixed since v2 -------------- - rte_eth_bond_8023ad_member_info(), _ext_collect_get() and _ext_distrib_get() are now rejected in a secondary, so they no longer return all-zero LACP state read out of the process-local bond_mode_8023ad_ports[]. - eth_dev_priv_dump is no longer in the secondary ops table, so dump_lacp() cannot walk that array from a secondary either. - The two near-identical primary-process helpers are collapsed into bond_check_primary(). - The stray double blank line in rte_eth_bond_member_add() is gone. - The programmer's guide now states the secondary datapath is a stub, which matches bond_ethdev_rx_secondary() and bond_ethdev_tx_secondary(). The set of API gated in this version looks right to me. Every mutator in rte_eth_bond_api.c and rte_eth_bond_8023ad.c is covered, rte_eth_bond_free() is deliberately left open for detach, and the getters that are left ungated (mode_get, primary_get, members_get, xmit_policy_get, 8023ad_conf_get, 8023ad_agg_selection_get, ...) all read bond_dev_private, which is in shared memory. Omitting dev_stop from secondary_dev_ops is also correct: rte_eth_dev_stop() writes dev->data->dev_started, and rte_eth_dev_close() has its own secondary path so detach still works. Warnings -------- 1. link_update is retained in secondary_dev_ops, so dev_link is now written from two processes without using the ethdev accessors. bond_ethdev_link_update() assigns ethdev->data->dev_link.* field by field, and dev_link lives in the shared rte_eth_dev_data. With .link_update present, any rte_eth_link_get() in a secondary republishes the primary's link record: if (dev->data->dev_conf.intr_conf.lsc && dev->data->dev_started) rte_eth_linkstatus_get(dev, eth_link); else { if (dev->dev_ops->link_update == NULL) return -ENOTSUP; dev->dev_ops->link_update(dev, 1); *eth_link = dev->data->dev_link; } Readers on the LSC path use rte_eth_linkstatus_get(), a 64-bit atomic load, so a reader can observe a torn combination of link_status, link_speed and link_duplex. The driver should use rte_eth_linkstatus_set() rather than touching dev->data->dev_link directly. That is one atomic exchange of the whole word, which closes the window on the primary path as well and makes the secondary's recomputation harmless -- it derives the same value from the same shared member state. Build the result locally and publish once: struct rte_eth_link link; rte_eth_linkstatus_get(ethdev, &link); link.link_speed = RTE_ETH_SPEED_NUM_NONE; if (ethdev->data->dev_started == 0 || bond_ctx->active_member_count == 0) { link.link_status = RTE_ETH_LINK_DOWN; goto out; } link.link_status = RTE_ETH_LINK_UP; ... out: rte_eth_linkstatus_set(ethdev, &link); return 0; Seeding from rte_eth_linkstatus_get() keeps link_duplex and link_autoneg for the modes that never set them; the interior "return 0" paths become "goto out". The rest of the driver has the same problem and is worth a preparatory patch: bond_ethdev_start() and bond_ethdev_stop() set link_status directly (rte_eth_bond_pmd.c:2068, 2220), as does bond_ethdev_lsc_event_callback() at 3101, and bond_ethdev_member_link_status_change() writes link_autoneg and link_duplex at 1439. Nothing in bonding currently uses the accessors. 2. The documentation does not match what the code now rejects. The new prog_guide paragraph lists configure, start/stop, queue setup, member changes, mode, RSS, MAC, MTU and rte_flow. It omits everything else the patch turns into an error in a secondary: - all of rte_eth_bond_8023ad_* except conf_get and agg_selection_get, including member_info(), ext_collect_get() and ext_distrib_get(), which are queries - dedicated queue enable/disable - xmit policy, link monitoring interval, link up/down propagation delays, primary member selection - stats reset, promiscuous and allmulticast, VLAN filter, private dump The commit message and the release note both say "query ... remain available", and the guide says "supported query and detach operations", so a reader would reasonably conclude rte_eth_bond_8023ad_member_info() still works in a secondary. It no longer does. Please say explicitly which queries survive (device info, stats, link, RETA and RSS hash config, bonding mode, members, and the LACP configuration) and that LACP runtime state queries do not. Worth stating the supported teardown too: rte_eth_dev_stop() returns -ENOTSUP in a secondary and rte_eth_dev_close() is the detach call. A secondary that does the usual stop-then-close will now see an error from the stop. 3. Release note is in the wrong section. This changes the behaviour of already-exported functions -- calls that previously returned 0 now return -ENOTSUP or -1 -- so it belongs under "API Changes", not "New Features". Same comment as on v2. 4. bond_ethdev_stats_get() ignores the rte_eth_stats_get() return. Pre-existing, but the patch puts .stats_get in secondary_dev_ops, which makes the failure case realistic: struct rte_eth_stats member_stats; ... for (i = 0; i < internals->member_count; i++) { rte_eth_stats_get(internals->members[i].port_id, &member_stats); stats->ipackets += member_stats.ipackets; eth_stats_qstats_get() runs RTE_ETH_VALID_PORTID_OR_ERR_RET before the memset, so on -ENODEV member_stats is left untouched. On the first iteration that is uninitialized stack; on later iterations it is the previous member's counters, double-counted. A secondary that attached to the bonding device but did not probe the member ports (blocklist, or members added by the primary after the secondary started) hits exactly this. ret = rte_eth_stats_get(internals->members[i].port_id, &member_stats); if (ret != 0) continue; Better as a separate fix ahead of this patch, since it is not secondary-specific. Info ---- - Three of the 8023ad functions declare a second int purely for the new check while an existing one is right there: struct port *port; int res; int ret; ret = bond_check_primary(__func__, -ENOTSUP); if (ret != 0) return ret; ext_distrib_get() and ext_collect_get() reuse err instead, which reads better. Reusing res in ext_collect(), ext_distrib() and ext_slowtx() would drop three declarations and make the series consistent. - rte_eth_bond_api.c passes -1 and rte_eth_bond_8023ad.c passes -ENOTSUP. Each matches its own file's existing convention, so this is defensible, but a caller cannot distinguish "wrong process type" from "bad port id" in the api.c cases. -ENOTSUP everywhere would be clearer if you are willing to change those return values. - The <stdbool.h> addition to eth_bond_private.h is unrelated to this change; bool is already used at line 184 and was working by transitive include. It is a real fix, just not this patch's. - Dropping eth_dev_priv_dump entirely costs some debuggability. Only dump_lacp() touches the process-local array; dump_basic() reads bond_dev_private, which is shared. Keeping .eth_dev_priv_dump in secondary_dev_ops and skipping the LACP section in a secondary would let a secondary still dump mode, members and offloads.
