From: Chas Williams <3chas3@gmail.com>
To: Radu Nicolau <radu.nicolau@intel.com>
Cc: dev@dpdk.org, Declan Doherty <declan.doherty@intel.com>,
Ferruh Yigit <ferruh.yigit@intel.com>
Subject: Re: [dpdk-dev] [PATCH v4] net/bonding: fix slave add for mode 4
Date: Sat, 2 Jun 2018 17:23:25 -0400 [thread overview]
Message-ID: <CAG2-GkkpN8c0A-H-J-pQvD1OMixOxhmTANGUVex2UyafOUOo3Q@mail.gmail.com> (raw)
In-Reply-To: <1527847501-14713-1-git-send-email-radu.nicolau@intel.com>
This certainly seems more correct to me. I can't think of a reason that
you shouldn't be able to enslave whatever ports you like. Whether those
ports can become active is a different question. I need to think about
this a bit more before approval but I don't think it makes anything worse.
The current problems I see:
- There's still an activate_slave() at the bottom of
__eth_bond_slave_add_lock_free() -- We should be able to avoid this if
we fix LSC change to not miss slaves being added to the configuration (make
sure we get last_link_status right). That section is a little racy with
respect to stop/start and timer. If we don't do that, that activate still
need the guard against mis-matched link properties.
- What happens if you enslave multiple link down ports? The link speed is
taken from the first slave. What's speed of down link? How do other
interfaces match this property when they come up?
- We need to make sure deactivate/active happens on the LSC for the enslaved
interfaces all the time. A link may fail to activate, but if the down goes
down and comes back the "right" link speed, we need it to then activate.
- If all the links go down, should the "learned" default link properties be
cleared? We are a blank slate at this point, there's no reason not to
learn a new default from the first interface that comes up.
On Fri, Jun 1, 2018 at 6:05 AM, Radu Nicolau <radu.nicolau@intel.com> wrote:
> Moved the link status validity check from the slave add to the slave
> activation step. Otherwise slave add will fail for mode 4 if
> the ports are all stopped but only one of them checked.
>
> Fixes: b77d21cc2364 ("ethdev: add link status get/set helper functions")
> Bugzilla ID: 52
>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
> v4: reworked patch
> v3: updated commit msg
> v2: add fix and Bugzilla references
>
> drivers/net/bonding/rte_eth_bond_api.c | 8 --------
> drivers/net/bonding/rte_eth_bond_pmd.c | 9 ++++++++-
> 2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/bonding/rte_eth_bond_api.c
> b/drivers/net/bonding/rte_eth_bond_api.c
> index d558df8..853b23b 100644
> --- a/drivers/net/bonding/rte_eth_bond_api.c
> +++ b/drivers/net/bonding/rte_eth_bond_api.c
> @@ -345,14 +345,6 @@ __eth_bond_slave_add_lock_free(uint16_t
> bonded_port_id, uint16_t slave_port_id)
> internals->tx_queue_offload_capa &=
> dev_info.tx_queue_offload_capa;
> internals->flow_type_rss_offloads &=
> dev_info.flow_type_rss_offloads;
>
> - if (link_properties_valid(bonded_eth_dev,
> - &slave_eth_dev->data->dev_link) != 0) {
> - RTE_BOND_LOG(ERR, "Invalid link properties for
> slave %d"
> - " in bonding mode %d",
> slave_port_id,
> - internals->mode);
> - return -1;
> - }
> -
> /* RETA size is GCD of all slaves RETA sizes, so, if all
> sizes will be
> * the power of 2, the lower one is GCD
> */
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 02d94b1..b84af43 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -2679,7 +2679,14 @@ bond_ethdev_lsc_event_callback(uint16_t port_id,
> enum rte_eth_event_type type,
> mac_address_slaves_update(bonded_eth_dev);
> }
>
> - activate_slave(bonded_eth_dev, port_id);
> + /* check link state properties */
> + if (link_properties_valid(bonded_eth_dev, &link)) {
> + activate_slave(bonded_eth_dev, port_id);
> + } else {
> + RTE_BOND_LOG(ERR, "Invalid link properties for
> slave %d"
> + " in bonding mode %d", port_id,
> + internals->mode);
> + }
>
> /* If user has defined the primary port then default to
> using it */
> if (internals->user_defined_primary_port &&
> --
> 2.7.5
>
>
next prev parent reply other threads:[~2018-06-02 21:23 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-09 12:06 [dpdk-dev] [PATCH] net/bonding: update link status on slave add Radu Nicolau
2018-05-31 14:07 ` Ferruh Yigit
2018-05-31 16:10 ` [dpdk-dev] [PATCH v3] net/bonding: fix slave add for mode 4 Radu Nicolau
2018-06-01 0:05 ` Chas Williams
2018-06-01 9:59 ` Radu Nicolau
2018-06-01 10:05 ` [dpdk-dev] [PATCH v4] " Radu Nicolau
2018-06-02 21:23 ` Chas Williams [this message]
2018-07-18 13:21 ` [dpdk-dev] [PATCH v5] " Radu Nicolau
2018-08-02 13:42 ` Doherty, Declan
2018-08-05 0:15 ` Thomas Monjalon
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=CAG2-GkkpN8c0A-H-J-pQvD1OMixOxhmTANGUVex2UyafOUOo3Q@mail.gmail.com \
--to=3chas3@gmail.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=radu.nicolau@intel.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).