Re: [PATCH v2 4/8] net/bonding: support enabling LACP short timeout

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 12/21/2021 7:57 PM, Robert Sanford wrote:
> - Add support for enabling LACP short timeout, i.e., link partner can
>    use fast periodic time interval between transmits.
> 
> Signed-off-by: Robert Sanford <rsanford@akamai.com>
> ---
>   drivers/net/bonding/eth_bond_8023ad_private.h |  3 ++-
>   drivers/net/bonding/rte_eth_bond_8023ad.c     | 28 +++++++++++++++++++++++----
>   drivers/net/bonding/rte_eth_bond_8023ad.h     |  3 +++
>   3 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/bonding/eth_bond_8023ad_private.h b/drivers/net/bonding/eth_bond_8023ad_private.h
> index 60db31e..bfde03c 100644
> --- a/drivers/net/bonding/eth_bond_8023ad_private.h
> +++ b/drivers/net/bonding/eth_bond_8023ad_private.h
> @@ -159,7 +159,6 @@ struct mode8023ad_private {
>   	uint64_t rx_marker_timeout;
>   	uint64_t update_timeout_us;
>   	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
> -	uint8_t external_sm;
>   	struct rte_ether_addr mac_addr;
>   
>   	struct rte_eth_link slave_link;
> @@ -178,6 +177,8 @@ struct mode8023ad_private {
>   		uint16_t tx_qid;
>   	} dedicated_queues;
>   	enum rte_bond_8023ad_agg_selection agg_selection;
> +	uint8_t short_timeout_enabled : 1;
> +	uint8_t short_timeout_updated : 1;
>   };
>   
>   /**
> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index 9ed2a46..5c175e7 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -868,10 +868,10 @@ bond_mode_8023ad_periodic_cb(void *arg)
>   	struct rte_eth_link link_info;
>   	struct rte_ether_addr slave_addr;
>   	struct rte_mbuf *lacp_pkt = NULL;
> +	uint8_t short_timeout_updated = internals->mode4.short_timeout_updated;
>   	uint16_t slave_id;
>   	uint16_t i;
>   
> -
>   	/* Update link status on each port */
>   	for (i = 0; i < internals->active_slave_count; i++) {
>   		uint16_t key;
> @@ -916,6 +916,13 @@ bond_mode_8023ad_periodic_cb(void *arg)
>   		slave_id = internals->active_slaves[i];
>   		port = &bond_mode_8023ad_ports[slave_id];
>   
> +		if (short_timeout_updated) {
> +			if (internals->mode4.short_timeout_enabled)
> +				ACTOR_STATE_SET(port, LACP_SHORT_TIMEOUT);
> +			else
> +				ACTOR_STATE_CLR(port, LACP_SHORT_TIMEOUT);
> +		}
> +
>   		if ((port->actor.key &
>   				rte_cpu_to_be_16(BOND_LINK_FULL_DUPLEX_KEY)) == 0) {
>   
> @@ -960,6 +967,9 @@ bond_mode_8023ad_periodic_cb(void *arg)
>   		show_warnings(slave_id);
>   	}
>   
> +	if (short_timeout_updated)
> +		internals->mode4.short_timeout_updated = 0;
> +
>   	rte_eal_alarm_set(internals->mode4.update_timeout_us,
>   			bond_mode_8023ad_periodic_cb, arg);
>   }
> @@ -1054,7 +1064,6 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
>   	/* Given slave must not be in active list. */
>   	RTE_ASSERT(find_slave_by_id(internals->active_slaves,
>   	internals->active_slave_count, slave_id) == internals->active_slave_count);
> -	RTE_SET_USED(internals); /* used only for assert when enabled */
>   
>   	memcpy(&port->actor, &initial, sizeof(struct port_params));
>   	/* Standard requires that port ID must be greater than 0.
> @@ -1065,7 +1074,9 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
>   	memcpy(&port->partner_admin, &initial, sizeof(struct port_params));
>   
>   	/* default states */
> -	port->actor_state = STATE_AGGREGATION | STATE_LACP_ACTIVE | STATE_DEFAULTED;
> +	port->actor_state = STATE_AGGREGATION | STATE_LACP_ACTIVE |
> +		STATE_DEFAULTED | (internals->mode4.short_timeout_enabled ?
> +		STATE_LACP_SHORT_TIMEOUT : 0);
>   	port->partner_state = STATE_LACP_ACTIVE | STATE_AGGREGATION;
>   	port->sm_flags = SM_FLAGS_BEGIN;
>   
> @@ -1209,6 +1220,7 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
>   	struct mode8023ad_private *mode4 = &internals->mode4;
>   	uint64_t ms_ticks = rte_get_tsc_hz() / 1000;
>   
> +	memset(conf, 0, sizeof(*conf));
>   	conf->fast_periodic_ms = mode4->fast_periodic_timeout / ms_ticks;
>   	conf->slow_periodic_ms = mode4->slow_periodic_timeout / ms_ticks;
>   	conf->short_timeout_ms = mode4->short_timeout / ms_ticks;
> @@ -1219,6 +1231,7 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev,
>   	conf->rx_marker_period_ms = mode4->rx_marker_timeout / ms_ticks;
>   	conf->slowrx_cb = mode4->slowrx_cb;
>   	conf->agg_selection = mode4->agg_selection;
> +	conf->lacp_timeout_control = mode4->short_timeout_enabled;
>   }
>   
>   static void
> @@ -1234,6 +1247,7 @@ bond_mode_8023ad_conf_get_default(struct rte_eth_bond_8023ad_conf *conf)
>   	conf->update_timeout_ms = BOND_MODE_8023AX_UPDATE_TIMEOUT_MS;
>   	conf->slowrx_cb = NULL;
>   	conf->agg_selection = AGG_STABLE;
> +	conf->lacp_timeout_control = 0;
>   }
>   
>   static void
> @@ -1274,6 +1288,11 @@ bond_mode_8023ad_setup(struct rte_eth_dev *dev,
>   	mode4->slowrx_cb = conf->slowrx_cb;
>   	mode4->agg_selection = AGG_STABLE;
>   
> +	if (mode4->short_timeout_enabled != conf->lacp_timeout_control) {
> +		mode4->short_timeout_enabled = conf->lacp_timeout_control;
> +		mode4->short_timeout_updated = 1;
> +	}
> +
>   	if (dev->data->dev_started)
>   		bond_mode_8023ad_start(dev);
>   }
> @@ -1478,7 +1497,8 @@ bond_8023ad_setup_validate(uint16_t port_id,
>   				conf->aggregate_wait_timeout_ms == 0 ||
>   				conf->tx_period_ms == 0 ||
>   				conf->rx_marker_period_ms == 0 ||
> -				conf->update_timeout_ms == 0) {
> +				conf->update_timeout_ms == 0 ||
> +				conf->lacp_timeout_control > 1) {
>   			RTE_BOND_LOG(ERR, "given mode 4 configuration is invalid");
>   			return -EINVAL;
>   		}
> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.h b/drivers/net/bonding/rte_eth_bond_8023ad.h
> index 7e9a018..87f6b2f 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.h
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.h
> @@ -139,6 +139,9 @@ struct rte_eth_bond_8023ad_conf {
>   	uint32_t update_timeout_ms;
>   	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
>   	enum rte_bond_8023ad_agg_selection agg_selection;
> +	uint8_t lacp_timeout_control;
> +	/**< LACPDU.Actor_State.LACP_Timeout flag: 0=Long 1=Short. */
> +	uint8_t reserved_8s[3];
>   };
>   
>   struct rte_eth_bond_8023ad_slave_info {

The changes gives ABI warning [1], it increases size of struct that is
parameter to the public API.

So old applications will send a smaller struct, but new DPDK library
will check beyond the size of the struct, most probably some unrelated
memory in the stack, this looks an ABI break to me.
@Ray can you please check if I am missing anything?



[1]
   [C] 'function int rte_eth_bond_8023ad_conf_get(uint16_t, rte_eth_bond_8023ad_conf*)' at rte_eth_bond_8023ad.c:1423:1 has some indirect sub-type changes:
     parameter 2 of type 'rte_eth_bond_8023ad_conf*' has sub-type changes:
       in pointed to type 'struct rte_eth_bond_8023ad_conf' at rte_eth_bond_8023ad.h:131:1:
Error: ABI issue reported for 'abidiff --suppr devtools/libabigail.abignore --no-added-syms --headers-dir1 reference/usr/local/include --headers-dir2 install/usr/local/include reference/dump/librte_net_bond.dump install/dump/librte_net_bond.dump'
ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue).
         type size hasn't changed
         2 data member insertions:
           'uint8_t rte_eth_bond_8023ad_conf::lacp_timeout_control', at offset 352 (in bits) at rte_eth_bond_8023ad.h:142:1
           'uint8_t rte_eth_bond_8023ad_conf::reserved_8s[3]', at offset 360 (in bits) at rte_eth_bond_8023ad.h:144:1