For ixgbe, when queue start failure, for example, mbuf allocate failure, the device will still start success, which could be an issue. Add return status check of queue start to avoid this issue. Signed-off-by: Michael Qiu <michael.qiu@intel.com> --- lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 6 +++++- lib/librte_pmd_ixgbe/ixgbe_ethdev.h | 2 +- lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 22 +++++++++++++++++----- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c index 3fc3738..59e3321 100644 --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c @@ -1491,7 +1491,11 @@ ixgbe_dev_start(struct rte_eth_dev *dev) goto error; } - ixgbe_dev_rxtx_start(dev); + err = ixgbe_dev_rxtx_start(dev); + if (err < 0) { + PMD_INIT_LOG(ERR, "Unable to start rxtx queues\n"); + goto error; + } if (ixgbe_is_sfp(hw) && hw->phy.multispeed_fiber) { err = hw->mac.ops.setup_sfp(hw); diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.h b/lib/librte_pmd_ixgbe/ixgbe_ethdev.h index ca99170..7461450 100644 --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.h +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.h @@ -251,7 +251,7 @@ int ixgbe_dev_rx_init(struct rte_eth_dev *dev); void ixgbe_dev_tx_init(struct rte_eth_dev *dev); -void ixgbe_dev_rxtx_start(struct rte_eth_dev *dev); +int ixgbe_dev_rxtx_start(struct rte_eth_dev *dev); int ixgbe_dev_rx_queue_start(struct rte_eth_dev *dev, uint16_t rx_queue_id); diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c index e10d6a2..41a930e 100644 --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c @@ -3744,7 +3744,7 @@ ixgbe_setup_loopback_link_82599(struct ixgbe_hw *hw) /* * Start Transmit and Receive Units. */ -void +int ixgbe_dev_rxtx_start(struct rte_eth_dev *dev) { struct ixgbe_hw *hw; @@ -3754,6 +3754,7 @@ ixgbe_dev_rxtx_start(struct rte_eth_dev *dev) uint32_t dmatxctl; uint32_t rxctrl; uint16_t i; + int ret = 0; PMD_INIT_FUNC_TRACE(); hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); @@ -3776,14 +3777,24 @@ ixgbe_dev_rxtx_start(struct rte_eth_dev *dev) for (i = 0; i < dev->data->nb_tx_queues; i++) { txq = dev->data->tx_queues[i]; - if (!txq->tx_deferred_start) - ixgbe_dev_tx_queue_start(dev, i); + if (!txq->tx_deferred_start) { + ret = ixgbe_dev_tx_queue_start(dev, i); + if (ret < 0) { + PMD_INIT_LOG(ERR, "Start tx queue failed\n"); + return ret; + } + } } for (i = 0; i < dev->data->nb_rx_queues; i++) { rxq = dev->data->rx_queues[i]; - if (!rxq->rx_deferred_start) - ixgbe_dev_rx_queue_start(dev, i); + if (!rxq->rx_deferred_start) { + ret = ixgbe_dev_rx_queue_start(dev, i); + if (ret < 0) { + PMD_INIT_LOG(ERR, "Start rx queue failed\n"); + return ret; + } + } } /* Enable Receive engine */ @@ -3798,6 +3809,7 @@ ixgbe_dev_rxtx_start(struct rte_eth_dev *dev) dev->data->dev_conf.lpbk_mode == IXGBE_LPBK_82599_TX_RX) ixgbe_setup_loopback_link_82599(hw); + return 0; } /* -- 1.9.3
Any comments?
On 2015/1/15 22:45, Qiu, Michael wrote:
> For ixgbe, when queue start failure, for example, mbuf allocate
> failure, the device will still start success, which could be
> an issue.
>
> Add return status check of queue start to avoid this issue.
>
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> ---
> lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 6 +++++-
> lib/librte_pmd_ixgbe/ixgbe_ethdev.h | 2 +-
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 22 +++++++++++++++++-----
> 3 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> index 3fc3738..59e3321 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> @@ -1491,7 +1491,11 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> goto error;
> }
>
> - ixgbe_dev_rxtx_start(dev);
> + err = ixgbe_dev_rxtx_start(dev);
> + if (err < 0) {
> + PMD_INIT_LOG(ERR, "Unable to start rxtx queues\n");
> + goto error;
> + }
>
> if (ixgbe_is_sfp(hw) && hw->phy.multispeed_fiber) {
> err = hw->mac.ops.setup_sfp(hw);
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.h b/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
> index ca99170..7461450 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
> +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
> @@ -251,7 +251,7 @@ int ixgbe_dev_rx_init(struct rte_eth_dev *dev);
>
> void ixgbe_dev_tx_init(struct rte_eth_dev *dev);
>
> -void ixgbe_dev_rxtx_start(struct rte_eth_dev *dev);
> +int ixgbe_dev_rxtx_start(struct rte_eth_dev *dev);
>
> int ixgbe_dev_rx_queue_start(struct rte_eth_dev *dev, uint16_t rx_queue_id);
>
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> index e10d6a2..41a930e 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> @@ -3744,7 +3744,7 @@ ixgbe_setup_loopback_link_82599(struct ixgbe_hw *hw)
> /*
> * Start Transmit and Receive Units.
> */
> -void
> +int
> ixgbe_dev_rxtx_start(struct rte_eth_dev *dev)
> {
> struct ixgbe_hw *hw;
> @@ -3754,6 +3754,7 @@ ixgbe_dev_rxtx_start(struct rte_eth_dev *dev)
> uint32_t dmatxctl;
> uint32_t rxctrl;
> uint16_t i;
> + int ret = 0;
>
> PMD_INIT_FUNC_TRACE();
> hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> @@ -3776,14 +3777,24 @@ ixgbe_dev_rxtx_start(struct rte_eth_dev *dev)
>
> for (i = 0; i < dev->data->nb_tx_queues; i++) {
> txq = dev->data->tx_queues[i];
> - if (!txq->tx_deferred_start)
> - ixgbe_dev_tx_queue_start(dev, i);
> + if (!txq->tx_deferred_start) {
> + ret = ixgbe_dev_tx_queue_start(dev, i);
> + if (ret < 0) {
> + PMD_INIT_LOG(ERR, "Start tx queue failed\n");
> + return ret;
> + }
> + }
> }
>
> for (i = 0; i < dev->data->nb_rx_queues; i++) {
> rxq = dev->data->rx_queues[i];
> - if (!rxq->rx_deferred_start)
> - ixgbe_dev_rx_queue_start(dev, i);
> + if (!rxq->rx_deferred_start) {
> + ret = ixgbe_dev_rx_queue_start(dev, i);
> + if (ret < 0) {
> + PMD_INIT_LOG(ERR, "Start rx queue failed\n");
> + return ret;
> + }
> + }
> }
>
> /* Enable Receive engine */
> @@ -3798,6 +3809,7 @@ ixgbe_dev_rxtx_start(struct rte_eth_dev *dev)
> dev->data->dev_conf.lpbk_mode == IXGBE_LPBK_82599_TX_RX)
> ixgbe_setup_loopback_link_82599(hw);
>
> + return 0;
> }
>
> /*
Hi Michael, I'm clearly not the maintainer of ixgbe, so I'd prefer someone else reviewing this patch. However I have few comments. 2015-01-15 22:45, Michael Qiu: > - ixgbe_dev_rxtx_start(dev); > + err = ixgbe_dev_rxtx_start(dev); > + if (err < 0) { > + PMD_INIT_LOG(ERR, "Unable to start rxtx queues\n"); \n is not needed in PMD_INIT_LOG. Is this useful to print a log here, given that there already has some logs in ixgbe_dev_rxtx_start? > + PMD_INIT_LOG(ERR, "Start tx queue failed\n"); [...] > + PMD_INIT_LOG(ERR, "Start rx queue failed\n"); Please remove \n. Except these minor comments, it looks good. Thanks -- Thomas
On 1/27/2015 6:02 PM, Thomas Monjalon wrote: > Hi Michael, > > I'm clearly not the maintainer of ixgbe, so I'd prefer someone else > reviewing this patch. However I have few comments. Thanks Thomas, I will send v2 with your comments. But who maintains ixgbe? I would like add him(or she) to the cc list. > 2015-01-15 22:45, Michael Qiu: >> - ixgbe_dev_rxtx_start(dev); >> + err = ixgbe_dev_rxtx_start(dev); >> + if (err < 0) { >> + PMD_INIT_LOG(ERR, "Unable to start rxtx queues\n"); > \n is not needed in PMD_INIT_LOG. > > Is this useful to print a log here, given that there already has > some logs in ixgbe_dev_rxtx_start? You are right, what I'm opinion is to show more details about the error, but seems duplicated. I will remove it. Thanks, Michael > >> + PMD_INIT_LOG(ERR, "Start tx queue failed\n"); > [...] >> + PMD_INIT_LOG(ERR, "Start rx queue failed\n"); > Please remove \n. > > Except these minor comments, it looks good. > Thanks
For ixgbe, when queue start failure, for example, mbuf allocate failure, the device will still start success, which could be an issue. Add return status check of queue start to avoid this issue. Signed-off-by: Michael Qiu <michael.qiu@intel.com> --- v2 --> v1 . remove duplicated error message in ixgbe_dev_rxtx_start() . remove '\n' in PMD_INIT_LOG() lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 6 +++++- lib/librte_pmd_ixgbe/ixgbe_ethdev.h | 2 +- lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 20 +++++++++++++++----- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c index b58ec45..ede8706 100644 --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c @@ -1495,7 +1495,11 @@ ixgbe_dev_start(struct rte_eth_dev *dev) goto error; } - ixgbe_dev_rxtx_start(dev); + err = ixgbe_dev_rxtx_start(dev); + if (err < 0) { + PMD_INIT_LOG(ERR, "Unable to start rxtx queues"); + goto error; + } if (ixgbe_is_sfp(hw) && hw->phy.multispeed_fiber) { err = hw->mac.ops.setup_sfp(hw); diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.h b/lib/librte_pmd_ixgbe/ixgbe_ethdev.h index 677c257..1383194 100644 --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.h +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.h @@ -265,7 +265,7 @@ int ixgbe_dev_rx_init(struct rte_eth_dev *dev); void ixgbe_dev_tx_init(struct rte_eth_dev *dev); -void ixgbe_dev_rxtx_start(struct rte_eth_dev *dev); +int ixgbe_dev_rxtx_start(struct rte_eth_dev *dev); int ixgbe_dev_rx_queue_start(struct rte_eth_dev *dev, uint16_t rx_queue_id); diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c index 840bc07..0224ed0 100644 --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c @@ -3806,7 +3806,7 @@ ixgbe_setup_loopback_link_82599(struct ixgbe_hw *hw) /* * Start Transmit and Receive Units. */ -void +int ixgbe_dev_rxtx_start(struct rte_eth_dev *dev) { struct ixgbe_hw *hw; @@ -3816,6 +3816,7 @@ ixgbe_dev_rxtx_start(struct rte_eth_dev *dev) uint32_t dmatxctl; uint32_t rxctrl; uint16_t i; + int ret = 0; PMD_INIT_FUNC_TRACE(); hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); @@ -3838,14 +3839,22 @@ ixgbe_dev_rxtx_start(struct rte_eth_dev *dev) for (i = 0; i < dev->data->nb_tx_queues; i++) { txq = dev->data->tx_queues[i]; - if (!txq->tx_deferred_start) - ixgbe_dev_tx_queue_start(dev, i); + if (!txq->tx_deferred_start) { + ret = ixgbe_dev_tx_queue_start(dev, i); + if (ret < 0) { + return ret; + } + } } for (i = 0; i < dev->data->nb_rx_queues; i++) { rxq = dev->data->rx_queues[i]; - if (!rxq->rx_deferred_start) - ixgbe_dev_rx_queue_start(dev, i); + if (!rxq->rx_deferred_start) { + ret = ixgbe_dev_rx_queue_start(dev, i); + if (ret < 0) { + return ret; + } + } } /* Enable Receive engine */ @@ -3860,6 +3869,7 @@ ixgbe_dev_rxtx_start(struct rte_eth_dev *dev) dev->data->dev_conf.lpbk_mode == IXGBE_LPBK_82599_TX_RX) ixgbe_setup_loopback_link_82599(hw); + return 0; } /* -- 1.9.3
2015-01-27 20:16, Michael Qiu: > For ixgbe, when queue start failure, for example, mbuf allocate > failure, the device will still start success, which could be > an issue. > > Add return status check of queue start to avoid this issue. > > Signed-off-by: Michael Qiu <michael.qiu@intel.com> > --- > v2 --> v1 > . remove duplicated error message in ixgbe_dev_rxtx_start() > . remove '\n' in PMD_INIT_LOG() So the braces are not needed anymore (reported by checkpatch): > + if (ret < 0) { > + return ret; > + } Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com> Applied with removed braces Thanks -- Thomas
2015-01-27 12:00, Qiu, Michael:
> On 1/27/2015 6:02 PM, Thomas Monjalon wrote:
> > Hi Michael,
> >
> > I'm clearly not the maintainer of ixgbe, so I'd prefer someone else
> > reviewing this patch. However I have few comments.
>
> Thanks Thomas,
>
> I will send v2 with your comments.
>
> But who maintains ixgbe? I would like add him(or she) to the cc list.
I don't know clearly. I hope we'll collect some maintainers name in next days.
I was able to do the review for this simple patch.
--
Thomas