From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <3chas3@gmail.com> Received: from mail-it0-f68.google.com (mail-it0-f68.google.com [209.85.214.68]) by dpdk.org (Postfix) with ESMTP id 923C65F48 for ; Sat, 2 Jun 2018 23:23:26 +0200 (CEST) Received: by mail-it0-f68.google.com with SMTP id j186-v6so5894960ita.5 for ; Sat, 02 Jun 2018 14:23:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=uWPIwbBoQB8ubvFE8RZTNqwACWz4CegKQCFzHDpiNes=; b=HZauCigL/MOLsq6GwpzBBfziGM6uHCnrDP3QIgYKkEj/dC+Gl/jf5gc9XpgROd18/B JXAK2mGutgRaGditK7LZT/9SrlMztIx0N/kU0MWNdopdHeYYw9O++ibWoYy0G029kzWM t9c6Y0MUrotSt+HpXj3dAR3DwoVQSl8GjTE2CwnXi5V28D/bUaGIHPzwBorrC+pkoSp4 pWTxgUBJ9FYE2Sv/zEhu6kmDsMbfLJYcil0xNo0i4P+3j1GZICOGZlWI7nPMWh5vhXbP lim9Vlu4mkAvbfBaE4r9UNiYdptRv/rHeFyenLZHu8RXEm7NvbAlqnsweyR+pqIFwKJy HGiw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=uWPIwbBoQB8ubvFE8RZTNqwACWz4CegKQCFzHDpiNes=; b=Fwtf0KQ115eiuAU4yZ/YW+pxqgV0n9X8Fg8uCEybywRSo3uD2MrpSDaO1laCGdQEi8 Z40RLnfhEYzfsQQVLS00TaHkZaW7dxiwPmO3TrX0pr2HfC62wiURuhU69PemYo2+Hmto Thilw9gj2UTLg9HF2/ESP0IeEX7LxC0C3ve3BJ81fwJGpbt0WWTrbgJgXAy7CXUoFvvz tCvQPkX0UvdIHuAERqFIGhmXHjImFXLZPgg+RNgVakFhrLTvFOVrUhrpImEaUoTd2nN7 c94K0IPffmZy/pQYxQUeGEVOQe2hclqj6RhRb2+jLWNYb3OFxSsQ2kvk1eDQSghGKzd2 rKeA== X-Gm-Message-State: APt69E3+ZAEAzjWcU5qhfCQ9LC+P7xmeZbvnqoVljsmsGd0FpJAi4Utc 77SIb/V1dS3JVdZsSTdm++Z6wmZ0+waELZArCe8= X-Google-Smtp-Source: ADUXVKJJwplJf4x4M1Gk0jNVHOzqQYeNI20IlhYpG4PQ2IwP0CtAb7eUsnZKVyFiSpI8PhKtQ5SLo08/Od+pOmMCMto= X-Received: by 2002:a24:8b81:: with SMTP id g123-v6mr9430076ite.67.1527974605975; Sat, 02 Jun 2018 14:23:25 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:e90e:0:0:0:0:0 with HTTP; Sat, 2 Jun 2018 14:23:25 -0700 (PDT) In-Reply-To: <1527847501-14713-1-git-send-email-radu.nicolau@intel.com> References: <1525867586-23328-1-git-send-email-radu.nicolau@intel.com> <1527847501-14713-1-git-send-email-radu.nicolau@intel.com> From: Chas Williams <3chas3@gmail.com> Date: Sat, 2 Jun 2018 17:23:25 -0400 Message-ID: To: Radu Nicolau Cc: dev@dpdk.org, Declan Doherty , Ferruh Yigit Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v4] net/bonding: fix slave add for mode 4 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 02 Jun 2018 21:23:26 -0000 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 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 > --- > 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 > >