Re: [PATCH] bonding: don't force LACPDU tx to ~333 ms boundaries

[Jay Vosburgh <jv@jvosburgh.net>]

Seth Forshee (DigitalOcean) <sforshee@kernel.org> wrote:

>The timer which ensures that no more than 3 LACPDUs are transmitted in
>a second rearms itself every 333ms regardless of whether an LACPDU is
>transmitted when the timer expires. This causes LACPDU tx to be delayed
>until the next expiration of the timer, which effectively aligns LACPDUs
>to ~333ms boundaries. This results in a variable amount of jitter in the
>timing of periodic LACPDUs.

	To be clear, the "3 per second" limitation that all of this
should to conform to is from IEEE 802.1AX-2014, 6.4.16 Transmit machine:

	"When the LACP_Enabled variable is TRUE and the NTT (6.4.7)
	variable is TRUE, the Transmit machine shall ensure that a
	properly formatted LACPDU (6.4.2) is transmitted [i.e., issue a
	CtrlMuxN:M_UNITDATA.Request(LACPDU) service primitive], subject
	to the restriction that no more than three LACPDUs may be
	transmitted in any Fast_Periodic_Time interval. If NTT is set to
	TRUE when this limit is in force, the transmission shall be
	delayed until such a time as the restriction is no longer in
	force. The NTT variable shall be set to FALSE when the Transmit
	machine has transmitted a LACPDU."

	The current implementation conforms to this as you describe: by
aligning transmission to 1/3 second boundaries, no more than 3 can ever
be sent in one second.

	If, hypothetically, the state machine were to transition, or a
user updates port settings (either of which would set NTT each time)
more than 3 times in a second, would your patched code obey this
restriction?

	For completeness, and to make this email as complicated as
possible, I'll note that 802.1AX-2020 removes this particular
restriction in favor of incorporating the 802.3 generic limit on
transmission rates for Slow Protocols (of which LACP is one) to 10 per
second (802.3-2022, 30.3.1.1.38) into the state machine (802.1AX-2020,
6.4.7, see "txOpportunity" and 6.4.14 LACP Transmit machine).  Linux
bonding doesn't implement the 802.1AX-2020 state machines, though, so I
don't think we can reasonably pick and choose arbitrary pieces from two
differing editions of a standard.

	-J

>Change this to only rearm the timer when an LACPDU is actually sent,
>allowing tx at any point after the timer has expired.
>
>Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>
>---
> drivers/net/bonding/bond_3ad.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index c6807e473ab706afed9560bcdb5e6eca1934f5b7..a8d8aaa169fc09d7d5c201ff298b37b3f11a7ded 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -1378,7 +1378,7 @@ static void ad_tx_machine(struct port *port)
> 	/* check if tx timer expired, to verify that we do not send more than
> 	 * 3 packets per second
> 	 */
>-	if (port->sm_tx_timer_counter && !(--port->sm_tx_timer_counter)) {
>+	if (!port->sm_tx_timer_counter || !(--port->sm_tx_timer_counter)) {
> 		/* check if there is something to send */
> 		if (port->ntt && (port->sm_vars & AD_PORT_LACP_ENABLED)) {
> 			__update_lacpdu_from_port(port);
>@@ -1393,12 +1393,13 @@ static void ad_tx_machine(struct port *port)
> 				 * again until demanded
> 				 */
> 				port->ntt = false;
>+
>+				/* restart tx timer(to verify that we will not
>+				 * exceed AD_MAX_TX_IN_SECOND
>+				 */
>+				port->sm_tx_timer_counter = ad_ticks_per_sec / AD_MAX_TX_IN_SECOND;
> 			}
> 		}
>-		/* restart tx timer(to verify that we will not exceed
>-		 * AD_MAX_TX_IN_SECOND
>-		 */
>-		port->sm_tx_timer_counter = ad_ticks_per_sec/AD_MAX_TX_IN_SECOND;
> 	}
> }
> 
>
>---
>base-commit: 86731a2a651e58953fc949573895f2fa6d456841
>change-id: 20250625-fix-lacpdu-jitter-1554d9f600ab
>
>Best regards,
>-- 
>Seth Forshee (DigitalOcean) <sforshee@kernel.org>

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