Re: [dpdk-dev] [PATCH v9 02/10] bond: replace rte_panic instances in bonding driver

[Kevin Traynor <ktraynor@redhat.com>]

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 <arnon@qwilt.com>
> ---
>  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
>