Re: [PATCH next] bonding: Extending LACP MUX State Machine to include a Collecting State.

[Aahil Awatramani]

Thanks for the feedback, I have made the changes you have suggested
moving forward.

On Thu, Dec 21, 2023 at 9:35 AM Simon Horman <horms@xxxxxxxxxx> wrote:
>
> On Thu, Dec 21, 2023 at 02:36:50AM +0000, Aahil Awatramani wrote:
> > Introduces two new states, AD_MUX_COLLECTING and AD_MUX_DISTRIBUTING in
> > the LACP MUX state machine for separated handling of an initial
> > Collecting state before the Collecting and Distributing state. This
> > enables a port to be in a state where it can receive incoming packets
> > while not still distributing. This is useful for reducing packet loss when
> > a port begins distributing before its partner is able to collect.
> > Additionally this also brings the 802.3ad bonding driver's implementation
> > closer to the LACP specification which already predefined this behaviour.
> >
> > With this change, 802.3ad mode will use new
> > bond_set_slave_txrx_{enabled|disabled}_flags() set of functions only
> > instead of the earlier one (bond_set_slave_{active|inactive}_flags).
> > Additionally, it adds new functions such as
> > bond_set_slave_tx_disabled_flags and bond_set_slave_rx_enabled_flags to
> > precisely manage the port's collecting and distributing states.
> > Previously, there was no dedicated method to disable TX while keeping RX
> > enabled, which this patch addresses.
> >
> > Note that the regular flow process in the kernel's bonding driver remains
> > unaffected by this patch. The extension requires explicit opt-in by the
> > user (in order to ensure no disruptions for existing setups) via netlink
> > or sysfs support using the new bonding parameter lacp_extended_mux. The
> > default value for lacp_extended_mux is set to 0 so as to preserve existing
> > behaviour.
> >
> > Signed-off-by: Aahil Awatramani <aahila@xxxxxxxxxx>
>
> ...
>
> > @@ -1906,6 +2005,46 @@ static void ad_initialize_port(struct port *port, int lacp_fast)
> >       }
> >  }
> >
> > +/**
> > + * ad_enable_collecting - enable a port's receive
> > + * @port: the port we're looking at
> > + * @update_slave_arr: Does slave array need update?
>
> The line above documenting @update_slave_arr
> should be removed as the parameter is not in
> the function definition below.
>
> > + *
> > + * Enable @port if it's in an active aggregator
> > + */
> > +static void ad_enable_collecting(struct port *port)
> > +{
> > +     if (port->aggregator->is_active) {
> > +             struct slave *slave = port->slave;
> > +
> > +             slave_dbg(slave->bond->dev, slave->dev,
> > +                       "Enabling collecting on port %d (LAG %d)\n",
> > +                       port->actor_port_number,
> > +                       port->aggregator->aggregator_identifier);
> > +             __enable_collecting_port(port);
> > +     }
> > +}
>
> The above is a pretty minor problem, but the bots are likely
> to complain about it, so please fix it in v2 after waiting
> for feedback from others on this patch.
>
> When posting v2 please target it at the net-next tree.
>
>         Subject: [PATCH net-next v2] ...
>
> --
> pw-bot: changes-requested