Re: [PATCH net v3 2/5] bonding: 3ad: fix carrier when no valid slaves

[Jay Vosburgh <jv@jvosburgh.net>]

Louis Scalbert <louis.scalbert@6wind.com> wrote:

>Hello Jay,
>
>Thank you very much for this detailed review.
>
>Le lun. 13 avr. 2026 à 19:01, Jay Vosburgh <jv@jvosburgh.net> a écrit :
>>
>> Louis Scalbert <louis.scalbert@6wind.com> wrote:
>>
>> >Apply the "lacp_fallback" configuration from the previous commit.
>> >
>> >"lacp_fallback" mode "strict" asserts that the bonding master carrier
>> >only when at least 'min_links' slaves are in the collecting/distributing
>> >state (or collecting only if the coupled_control default behavior is
>> >disabled).
>> >
>> >Fixes: 655f8919d549 ("bonding: add min links parameter to 802.3ad")
>> >Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
>> >---
>> > drivers/net/bonding/bond_3ad.c     | 26 ++++++++++++++++++++++++--
>> > drivers/net/bonding/bond_options.c |  1 +
>> > 2 files changed, 25 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> >index af7f74cfdc08..b79a76296966 100644
>> >--- a/drivers/net/bonding/bond_3ad.c
>> >+++ b/drivers/net/bonding/bond_3ad.c
>> >@@ -745,6 +745,22 @@ static void __set_agg_ports_ready(struct aggregator *aggregator, int val)
>> >       }
>> > }
>> >
>> >+static int __agg_valid_ports(struct aggregator *agg)
>> >+{
>> >+      struct port *port;
>> >+      int valid = 0;
>> >+
>> >+      for (port = agg->lag_ports; port;
>> >+           port = port->next_port_in_aggregator) {
>> >+              if (port->actor_oper_port_state & LACP_STATE_COLLECTING &&
>> >+                  (!port->slave->bond->params.coupled_control ||
>> >+                   port->actor_oper_port_state & LACP_STATE_DISTRIBUTING))
>> >+                      valid++;
>>
>>         Do we need to test coupled_control?  I.e., can the test be
>
>With coupled_control enabled (default), the actor allows traffic from
>the partner only when it reaches both the COLLECTING and DISTRIBUTING
>states, i.e., in the AD_MUX_COLLECTING_DISTRIBUTING Mux state.
>
>With coupled_control disabled, the actor allows traffic from the
>partner as soon as it reaches the COLLECTING state, regardless of the
>DISTRIBUTING flag. In this case, COLLECTING is set in the
>AD_MUX_COLLECTING state, while DISTRIBUTING is only set later in the
>AD_MUX_DISTRIBUTING state.
>
>From the perspective of upper-layer processes, a carrier up state
>indicates that the link is fully operational and capable of both
>collecting and distributing traffic. These processes are not aware of
>the distinction between COLLECTING and COLLECTING & DISTRIBUTING
>states.

	Ok, so to the upper layers, carrier up means "can both send and
receive," however...

>>
>>                 if ((port->actor_oper_port_state & LACP_STATE_COLLECTING) &&
>>                     (port->actor_oper_port_state & LACP_STATE_DISTRIBUTING))
>>
>
>If we want to allow collection to start without waiting for
>distribution to be enabled, then the carrier must be asserted as soon
>as the COLLECTING state is reached.
>In that case, this test would not be valid.

	...here, are you saying that the bond should transition to
carrier up as soon as it's able to receive, even if it cannot yet send?
Won't that break the upper layers that expect carrier up to mean that
they can transmit?

	My presumption in suggesting the test logic is that carrier up
is the first meaning, that the interface can both send and receive, and
therefore has both _COLLECTING and _DISTRIBUTING.

	-J

>In practice, I can only test for LACP_STATE_COLLECTING, because
>LACP_STATE_DISTRIBUTING is always set together with
>LACP_STATE_COLLECTING.
>
>>         To my reading, ad_mux_machine will set _COLLECTING and
>> _DISTRIBUTING appropriately regardless of the coupled_control selection.
>
>I don't agree. See:
>https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/bonding/bond_3ad.c?id=v7.0#n1090
>https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/bonding/bond_3ad.c?id=v7.0#n1202
>
>> >+      }
>> >+
>> >+      return valid;
>> >+}
>> >+
>> > static int __agg_active_ports(struct aggregator *agg)
>> > {
>> >       struct port *port;
>> >@@ -2120,6 +2136,7 @@ static void ad_enable_collecting_distributing(struct port *port,
>> >                         port->actor_port_number,
>> >                         port->aggregator->aggregator_identifier);
>> >               __enable_port(port);
>> >+              bond_3ad_set_carrier(port->slave->bond);
>> >               /* Slave array needs update */
>> >               *update_slave_arr = true;
>> >               /* Should notify peers if possible */
>> >@@ -2141,6 +2158,7 @@ static void ad_disable_collecting_distributing(struct port *port,
>> >                         port->actor_port_number,
>> >                         port->aggregator->aggregator_identifier);
>> >               __disable_port(port);
>> >+              bond_3ad_set_carrier(port->slave->bond);
>> >               /* Slave array needs an update */
>> >               *update_slave_arr = true;
>> >       }
>> >@@ -2819,8 +2837,12 @@ int bond_3ad_set_carrier(struct bonding *bond)
>> >       }
>> >       active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
>> >       if (active) {
>> >-              /* are enough slaves available to consider link up? */
>> >-              if (__agg_active_ports(active) < bond->params.min_links) {
>> >+              /* are enough slaves in collecting (and distributing) state to consider
>> >+               * link up?
>> >+               */
>> >+              if ((bond->params.lacp_fallback ? __agg_valid_ports(active)
>> >+                                      : __agg_active_ports(active)) <
>> >+                  bond->params.min_links) {
>>
>>         I think the original comment is better; if the new option is
>> off, it doesn't require collecting / distributing state.
>>
>>         -J
>>
>> >                       if (netif_carrier_ok(bond->dev)) {
>> >                               netif_carrier_off(bond->dev);
>> >                               goto out;
>> >diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>> >index b672b8a881bb..d64a5d2f80b6 100644
>> >--- a/drivers/net/bonding/bond_options.c
>> >+++ b/drivers/net/bonding/bond_options.c
>> >@@ -1706,6 +1706,7 @@ static int bond_option_lacp_fallback_set(struct bonding *bond,
>> >       netdev_dbg(bond->dev, "Setting LACP fallback to %s (%llu)\n",
>> >                  newval->string, newval->value);
>> >       bond->params.lacp_fallback = newval->value;
>> >+      bond_3ad_set_carrier(bond);
>> >
>> >       return 0;
>> > }
>> >--
>> >2.39.2

---
	-Jay Vosburgh, jv@jvosburgh.net