DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>
To: Weifeng Li <liweifeng96@126.com>
Cc: <dev@dpdk.org>, "Wei Hu (Xavier)" <xavier.huwei@huawei.com>,
	<liyunqi@huawei.com>, <linuxarm@huawei.com>,
	Ferruh Yigit <ferruh.yigit@intel.com>,
	Chas Williams <chas3@att.com>
Subject: Re: [dpdk-dev] [PATCH v4] net/bonding: change the state machine to defaulted
Date: Sat, 18 Jul 2020 15:08:21 +0800	[thread overview]
Message-ID: <4da342e4-431b-d0ee-0f6c-13d1f6080eab@huawei.com> (raw)
In-Reply-To: <20200718043538.2315-1-liweifeng96@126.com>

Hi, Weifeng Li


On 2020/7/18 12:35, Weifeng Li wrote:
> A dpdk bonding 802.3ad network as follows:
> +----------+                        +-----------+
> |dpdk lacp |bond1.1 <------> bond2.1|switch lacp|
> |          |bond1.2 <------> bond2.2|           |
> +----------+                        +-----------+
> If a fiber optic go wrong about single pass during normal running like
> this:
> bond1.2 -----> bond2.2 ok
> bond1.2 <--x-- bond2.2 error: bond1.2 receive no LACPDU Some packets from
>                                switch to dpdk will choose bond2.2 and lost.
>
> DPDK lacp state machine will transits to the expired state if no LACPDU
> is received before the current_while_timer expires. But if no LACPDU is
> received before the current_while_timer expires again, DPDK lacp state
> machine has no change. Bond2.2 can not change to inactive depend on the
> received LACPDU.
> According to IEEE 802.3ad, if no lacpdu is received before the
> current_while_timer expires again, the state machine should transits from
> expired to defaulted. Bond2.2 will change to inactive depend on the LACPDU
> with defaulted state.
>
> This patch adds a state machine change from expired to defaulted when no
> lacpdu is received before the current_while_timer expires again according
> to IEEE 802.3ad:
> If no LACPDU is received before the current_while timer expires again,
> the state machine transits to the DEFAULTED state. The record Default
> function overwrites the current operational parameters for the Partner
> with administratively configured values. This allows configuration of
> aggregations and individual links when no protocol partner is present,
> while still permitting an active partner to override default settings.
> The update_Default_Selected function sets the Selected variable FALSE
> if the Link Aggregation Group has changed. Since all operational parameters
> are now set to locally administered values there can be no disagreement as
> to the Link Aggregation Group, so the Matched variable is set TRUE.
>
> The relevant description is in the chapter 43.4.12 of the link below:
> https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=850426

Fixes: 46fb43683679 ("bond: add mode 4")
Cc: stable@dpdk.org
> Signed-off-by: Weifeng Li <liweifeng96@126.com>

Acked-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>

Thanks you for fixing this issue.

Xavier
> ---
> v1 -> v2: adjust the formate of commit log
> ---
> v2 -> v3:
> 1. adjust coding style issue.
> 2. add description about partner_admin from IEEE spec.
> ---
> v3 -> v4
> 1. adjust coding style issue: Missing Signed-off-by
> ---
>   drivers/net/bonding/eth_bond_8023ad_private.h |  3 +++
>   drivers/net/bonding/rte_eth_bond_8023ad.c     | 21 +++++++++++++++++----
>   2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/bonding/eth_bond_8023ad_private.h b/drivers/net/bonding/eth_bond_8023ad_private.h
> index 6e44ffd..9b5738a 100644
> --- a/drivers/net/bonding/eth_bond_8023ad_private.h
> +++ b/drivers/net/bonding/eth_bond_8023ad_private.h
> @@ -50,6 +50,7 @@
>   #define SM_FLAGS_MOVED                      0x0100
>   #define SM_FLAGS_PARTNER_SHORT_TIMEOUT      0x0200
>   #define SM_FLAGS_NTT                        0x0400
> +#define SM_FLAGS_EXPIRED                    0x0800
>   
>   #define BOND_LINK_FULL_DUPLEX_KEY           0x01
>   #define BOND_LINK_SPEED_KEY_10M             0x02
> @@ -103,6 +104,8 @@ struct port {
>   
>   	/** The operational Partner's port parameters */
>   	struct port_params partner;
> +	/** Partner administrative parameter values */
> +	struct port_params partner_admin;
>   
>   	/* Additional port parameters not listed in documentation */
>   	/** State machine flags */
> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index 3991825..3c2c7bf 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -356,16 +356,28 @@ rx_machine(struct bond_dev_private *internals, uint16_t slave_id,
>   
>   		timer_set(&port->current_while_timer, timeout);
>   		ACTOR_STATE_CLR(port, EXPIRED);
> +		SM_FLAG_CLR(port, EXPIRED);
>   		return; /* No state change */
>   	}
>   
>   	/* If CURRENT state timer is not running (stopped or expired)
>   	 * transit to EXPIRED state from DISABLED or CURRENT */
>   	if (!timer_is_running(&port->current_while_timer)) {
> -		ACTOR_STATE_SET(port, EXPIRED);
> -		PARTNER_STATE_CLR(port, SYNCHRONIZATION);
> -		PARTNER_STATE_SET(port, LACP_SHORT_TIMEOUT);
> -		timer_set(&port->current_while_timer, internals->mode4.short_timeout);
> +		if (SM_FLAG(port, EXPIRED)) {
> +			port->selected = UNSELECTED;
> +			memcpy(&port->partner, &port->partner_admin,
> +				sizeof(struct port_params));
> +			record_default(port);
> +			ACTOR_STATE_CLR(port, EXPIRED);
> +			timer_cancel(&port->current_while_timer);
> +		} else {
> +			SM_FLAG_SET(port, EXPIRED);
> +			ACTOR_STATE_SET(port, EXPIRED);
> +			PARTNER_STATE_CLR(port, SYNCHRONIZATION);
> +			PARTNER_STATE_SET(port, LACP_SHORT_TIMEOUT);
> +			timer_set(&port->current_while_timer,
> +				internals->mode4.short_timeout);
> +		}
>   	}
>   }
>   
> @@ -1021,6 +1033,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
>   	port->actor.port_number = rte_cpu_to_be_16(slave_id + 1);
>   
>   	memcpy(&port->partner, &initial, sizeof(struct port_params));
> +	memcpy(&port->partner_admin, &initial, sizeof(struct port_params));
>   
>   	/* default states */
>   	port->actor_state = STATE_AGGREGATION | STATE_LACP_ACTIVE | STATE_DEFAULTED;


  reply	other threads:[~2020-07-18  7:08 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-07 14:38 [dpdk-dev] [PATCH v2] " Weifeng Li
2020-07-08  9:13 ` Wei Hu (Xavier)
2020-07-18  4:05 ` [dpdk-dev] [PATCH v3] " Weifeng Li
2020-07-18  4:35   ` [dpdk-dev] [PATCH v4] " Weifeng Li
2020-07-18  7:08     ` Wei Hu (Xavier) [this message]
2020-07-20 23:28       ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4da342e4-431b-d0ee-0f6c-13d1f6080eab@huawei.com \
    --to=xavier.huwei@huawei.com \
    --cc=chas3@att.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=linuxarm@huawei.com \
    --cc=liweifeng96@126.com \
    --cc=liyunqi@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).