From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id B47468D97 for ; Thu, 26 Apr 2018 18:06:33 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 39F4C406C745; Thu, 26 Apr 2018 16:06:33 +0000 (UTC) Received: from ktraynor.remote.csb (unknown [10.36.118.53]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6CEF2215CDC8; Thu, 26 Apr 2018 16:06:31 +0000 (UTC) To: Arnon Warshavsky , thomas@monjalon.net, anatoly.burakov@intel.com, wenzhuo.lu@intel.com, declan.doherty@intel.com, jerin.jacob@caviumnetworks.com, bruce.richardson@intel.com, ferruh.yigit@intel.com Cc: dev@dpdk.org References: <1524663944-30376-11-git-send-email-arnon@qwilt.com> <1524723664-30510-1-git-send-email-arnon@qwilt.com> <1524723664-30510-3-git-send-email-arnon@qwilt.com> From: Kevin Traynor Organization: Red Hat Message-ID: <9a982432-aec7-a3b5-12b2-e3f9f799aa76@redhat.com> Date: Thu, 26 Apr 2018 17:06:30 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <1524723664-30510-3-git-send-email-arnon@qwilt.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 26 Apr 2018 16:06:33 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 26 Apr 2018 16:06:33 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'ktraynor@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [PATCH v9 02/10] bond: replace rte_panic instances in bonding driver 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: Thu, 26 Apr 2018 16:06:34 -0000 On 04/26/2018 07:20 AM, Arnon Warshavsky wrote: > Replace panic calls with log and return value. > Local functions to this file, > changing from void to int are non-abi-breaking > > Signed-off-by: Arnon Warshavsky > --- > drivers/net/bonding/rte_eth_bond_8023ad.c | 29 ++++++++++++++--------- > drivers/net/bonding/rte_eth_bond_8023ad_private.h | 2 +- > drivers/net/bonding/rte_eth_bond_api.c | 22 ++++++++++++----- > drivers/net/bonding/rte_eth_bond_pmd.c | 9 ++++--- > drivers/net/bonding/rte_eth_bond_private.h | 2 +- > 5 files changed, 42 insertions(+), 22 deletions(-) > > diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c > index c452318..308e623 100644 > --- a/drivers/net/bonding/rte_eth_bond_8023ad.c > +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c > @@ -893,7 +893,7 @@ > bond_mode_8023ad_periodic_cb, arg); > } > > -void > +int > bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev, > uint16_t slave_id) > { > @@ -939,7 +939,7 @@ > timer_cancel(&port->warning_timer); > > if (port->mbuf_pool != NULL) > - return; > + return 0; > > RTE_ASSERT(port->rx_ring == NULL); > RTE_ASSERT(port->tx_ring == NULL); > @@ -968,8 +968,9 @@ > /* Any memory allocation failure in initialization is critical because > * resources can't be free, so reinitialization is impossible. */ What about this comment? Not sure if it is just stale or if there is something else you need to do. > if (port->mbuf_pool == NULL) { > - rte_panic("Slave %u: Failed to create memory pool '%s': %s\n", > - slave_id, mem_name, rte_strerror(rte_errno)); > + RTE_BOND_LOG(ERR, "%s() Slave %u: Failed to create memory pool '%s': %s\n", > + __func__, slave_id, mem_name, rte_strerror(rte_errno)); > + return -1; > } > > snprintf(mem_name, RTE_DIM(mem_name), "slave_%u_rx", slave_id); > @@ -977,8 +978,9 @@ > rte_align32pow2(BOND_MODE_8023AX_SLAVE_RX_PKTS), socket_id, 0); > > if (port->rx_ring == NULL) { > - rte_panic("Slave %u: Failed to create rx ring '%s': %s\n", slave_id, > - mem_name, rte_strerror(rte_errno)); > + RTE_BOND_LOG(ERR, "%s() Slave %u: Failed to create rx ring '%s': %s\n", > + __func__, slave_id, mem_name, rte_strerror(rte_errno)); > + return -1; > } > > /* TX ring is at least one pkt longer to make room for marker packet. */ > @@ -987,9 +989,12 @@ > rte_align32pow2(BOND_MODE_8023AX_SLAVE_TX_PKTS + 1), socket_id, 0); > > if (port->tx_ring == NULL) { > - rte_panic("Slave %u: Failed to create tx ring '%s': %s\n", slave_id, > - mem_name, rte_strerror(rte_errno)); > + RTE_BOND_LOG(ERR, "%s() Slave %u: Fail to create tx ring '%s': %s\n", > + __func__, slave_id, mem_name, rte_strerror(rte_errno)); > + return -1; > } > + > + return 0; > } > > int > @@ -1143,9 +1148,11 @@ > struct bond_dev_private *internals = bond_dev->data->dev_private; > uint8_t i; > > - for (i = 0; i < internals->active_slave_count; i++) > - bond_mode_8023ad_activate_slave(bond_dev, > - internals->active_slaves[i]); > + for (i = 0; i < internals->active_slave_count; i++) { > + if (!bond_mode_8023ad_activate_slave(bond_dev, > + internals->active_slaves[i])) > + return -1; > + } > > return 0; > } > diff --git a/drivers/net/bonding/rte_eth_bond_8023ad_private.h b/drivers/net/bonding/rte_eth_bond_8023ad_private.h > index 0f490a5..96a42f2 100644 > --- a/drivers/net/bonding/rte_eth_bond_8023ad_private.h > +++ b/drivers/net/bonding/rte_eth_bond_8023ad_private.h > @@ -263,7 +263,7 @@ struct mode8023ad_private { > * @return > * 0 on success, negative value otherwise. ^^^ you inadvertently fixed a doxygen bug :-) > */ > -void > +int > bond_mode_8023ad_activate_slave(struct rte_eth_dev *dev, uint16_t port_id); > > /** > diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c > index aa89425..657fd74 100644 > --- a/drivers/net/bonding/rte_eth_bond_api.c > +++ b/drivers/net/bonding/rte_eth_bond_api.c > @@ -69,14 +69,15 @@ > return 0; > } > > -void > +int > activate_slave(struct rte_eth_dev *eth_dev, uint16_t port_id) > { > struct bond_dev_private *internals = eth_dev->data->dev_private; > uint8_t active_count = internals->active_slave_count; > > if (internals->mode == BONDING_MODE_8023AD) > - bond_mode_8023ad_activate_slave(eth_dev, port_id); > + if (bond_mode_8023ad_activate_slave(eth_dev, port_id) != 0) > + return -1; > > if (internals->mode == BONDING_MODE_TLB > || internals->mode == BONDING_MODE_ALB) { > @@ -94,6 +95,8 @@ > bond_tlb_activate_slave(internals); > if (internals->mode == BONDING_MODE_ALB) > bond_mode_alb_client_list_upd(eth_dev); > + > + return 0; > } > > void > @@ -357,10 +360,17 @@ > bond_ethdev_primary_set(internals, > slave_port_id); > > - if (find_slave_by_id(internals->active_slaves, > - internals->active_slave_count, > - slave_port_id) == internals->active_slave_count) > - activate_slave(bonded_eth_dev, slave_port_id); > + int rc = > + find_slave_by_id(internals->active_slaves, > + internals->active_slave_count, > + slave_port_id); > + > + if (rc == internals->active_slave_count) { > + int rc = activate_slave(bonded_eth_dev, > + slave_port_id); > + if (rc != 0) > + return -1; > + } Not functionally wrong, but commented about no need for variables here in v4 and seems to have been missed > } > } > > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c > index 09696ea..d2dbe4a 100644 > --- a/drivers/net/bonding/rte_eth_bond_pmd.c > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c > @@ -1741,8 +1741,10 @@ struct bwg_slave { > /* Any memory allocation failure in initialization is critical because > * resources can't be free, so reinitialization is impossible. */ > if (port->slow_pool == NULL) { > - rte_panic("Slave %u: Failed to create memory pool '%s': %s\n", > - slave_id, mem_name, rte_strerror(rte_errno)); > + RTE_BOND_LOG(ERR, "%s() Slave %u: Failed to create memory pool '%s': %s\n", > + __func__, slave_id, > + mem_name, rte_strerror(rte_errno)); > + return -1; > } > } > > @@ -2673,7 +2675,8 @@ struct bwg_slave { > mac_address_slaves_update(bonded_eth_dev); > } > > - activate_slave(bonded_eth_dev, port_id); > + if (activate_slave(bonded_eth_dev, port_id) != 0) > + return -1; > > /* If user has defined the primary port then default to using it */ > if (internals->user_defined_primary_port && > diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h > index 94eca88..d99d42c 100644 > --- a/drivers/net/bonding/rte_eth_bond_private.h > +++ b/drivers/net/bonding/rte_eth_bond_private.h > @@ -187,7 +187,7 @@ struct bond_dev_private { > void > deactivate_slave(struct rte_eth_dev *eth_dev, uint16_t port_id); > > -void > +int > activate_slave(struct rte_eth_dev *eth_dev, uint16_t port_id); > > void >