[v2,4/8] net/bonding: support enabling LACP short timeout
Checks
Commit Message
- 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(-)
Comments
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
@@ -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;
};
/**
@@ -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;
}
@@ -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 {