[dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev functions.
Hi Konstantin, > -Original Message- > From: Ananyev, Konstantin > Sent: Monday, December 21, 2015 12:02 PM > To: Iremonger, Bernard ; Qiu, Michael > ; dev at dpdk.org > Subject: RE: [dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev > functions. > > > > > -Original Message- > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Iremonger, > > Bernard > > Sent: Monday, December 21, 2015 11:40 AM > > To: Qiu, Michael; dev at dpdk.org > > Subject: Re: [dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev > functions. > > > > Hi Michael, > > > > > -Original Message- > > > From: Qiu, Michael > > > Sent: Monday, December 21, 2015 9:03 AM > > > To: Iremonger, Bernard ; dev at dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH] librte_ether: fix crashes in > > > rte_ethdev functions. > > > > > > On 2015/12/18 1:24, Bernard Iremonger wrote: > > > > The nb_rx_queues and nb_tx_queues are initialised before the > > > > tx_queue and rx_queue arrays are allocated. The arrays are > > > > allocated when the ethdev port is started. > > > > > > > > If any of the following functions are called before the ethdev > > > > port is started there is a segmentation fault: > > > > > > > > rte_eth_stats_get > > > > rte_eth_stats_reset > > > > rte_eth_xstats_get > > > > rte_eth_xstats_reset > > > > > > > > Fixes: af75078fece3 ("first public release") > > > > Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended > > > > statistics") > > > > Fixes: d4fef8b0d5e5 ("ethdev: expose generic and driver specific > > > > stats in xstats") > > > > Signed-off-by: Bernard Iremonger > > > > --- > > > > lib/librte_ether/rte_ethdev.c | 16 > > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/lib/librte_ether/rte_ethdev.c > > > > b/lib/librte_ether/rte_ethdev.c index ed971b4..a0ee84d 100644 > > > > --- a/lib/librte_ether/rte_ethdev.c > > > > +++ b/lib/librte_ether/rte_ethdev.c > > > > @@ -1441,7 +1441,10 @@ rte_eth_stats_get(uint8_t port_id, struct > > > rte_eth_stats *stats) > > > > memset(stats, 0, sizeof(*stats)); > > > > > > > > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, - > > > ENOTSUP); > > > > - (*dev->dev_ops->stats_get)(dev, stats); > > > > + > > > > + if (dev->data->dev_started) > > > > + (*dev->dev_ops->stats_get)(dev, stats); > > > > + > > So why it would be no possible now to get statistics on the stopped device? > Konstantin I did not consider this scenario. I need to rethink this patch. Self NAK Regards, Bernard.
[dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev functions.
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Iremonger, Bernard > Sent: Monday, December 21, 2015 11:40 AM > To: Qiu, Michael; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev > functions. > > Hi Michael, > > > -Original Message- > > From: Qiu, Michael > > Sent: Monday, December 21, 2015 9:03 AM > > To: Iremonger, Bernard ; dev at dpdk.org > > Subject: Re: [dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev > > functions. > > > > On 2015/12/18 1:24, Bernard Iremonger wrote: > > > The nb_rx_queues and nb_tx_queues are initialised before the tx_queue > > > and rx_queue arrays are allocated. The arrays are allocated when the > > > ethdev port is started. > > > > > > If any of the following functions are called before the ethdev port is > > > started there is a segmentation fault: > > > > > > rte_eth_stats_get > > > rte_eth_stats_reset > > > rte_eth_xstats_get > > > rte_eth_xstats_reset > > > > > > Fixes: af75078fece3 ("first public release") > > > Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended > > > statistics") > > > Fixes: d4fef8b0d5e5 ("ethdev: expose generic and driver specific stats > > > in xstats") > > > Signed-off-by: Bernard Iremonger > > > --- > > > lib/librte_ether/rte_ethdev.c | 16 > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > > > diff --git a/lib/librte_ether/rte_ethdev.c > > > b/lib/librte_ether/rte_ethdev.c index ed971b4..a0ee84d 100644 > > > --- a/lib/librte_ether/rte_ethdev.c > > > +++ b/lib/librte_ether/rte_ethdev.c > > > @@ -1441,7 +1441,10 @@ rte_eth_stats_get(uint8_t port_id, struct > > rte_eth_stats *stats) > > > memset(stats, 0, sizeof(*stats)); > > > > > > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, - > > ENOTSUP); > > > - (*dev->dev_ops->stats_get)(dev, stats); > > > + > > > + if (dev->data->dev_started) > > > + (*dev->dev_ops->stats_get)(dev, stats); > > > + So why it would be no possible now to get statistics on the stopped device? Konstantin > > > > My question is should we mark an error or a warning here and return an > > error so that the caller knows what happens? > > > > Thanks, > > Michael > > > > In other cases in rte_ethdev.c where there is a check on > "dev->data->dev_started" there is a RTE_PMD_DEBUG_TRACE() line. I will > add a RTE_PMD_DEBUG_TRACE() line. > The rte_eth_stats_reset() and rte_eth_xstats_reset() functions return void. > Not sure if an error is required for the rte_eth_stats_get() and > rte_eth_xstats_get() functions as the stats information returned is all > zero's at present. > > Regards, > > Bernard.
[dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev functions.
Hi Michael, > -Original Message- > From: Qiu, Michael > Sent: Monday, December 21, 2015 9:03 AM > To: Iremonger, Bernard ; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev > functions. > > On 2015/12/18 1:24, Bernard Iremonger wrote: > > The nb_rx_queues and nb_tx_queues are initialised before the tx_queue > > and rx_queue arrays are allocated. The arrays are allocated when the > > ethdev port is started. > > > > If any of the following functions are called before the ethdev port is > > started there is a segmentation fault: > > > > rte_eth_stats_get > > rte_eth_stats_reset > > rte_eth_xstats_get > > rte_eth_xstats_reset > > > > Fixes: af75078fece3 ("first public release") > > Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended > > statistics") > > Fixes: d4fef8b0d5e5 ("ethdev: expose generic and driver specific stats > > in xstats") > > Signed-off-by: Bernard Iremonger > > --- > > lib/librte_ether/rte_ethdev.c | 16 > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/lib/librte_ether/rte_ethdev.c > > b/lib/librte_ether/rte_ethdev.c index ed971b4..a0ee84d 100644 > > --- a/lib/librte_ether/rte_ethdev.c > > +++ b/lib/librte_ether/rte_ethdev.c > > @@ -1441,7 +1441,10 @@ rte_eth_stats_get(uint8_t port_id, struct > rte_eth_stats *stats) > > memset(stats, 0, sizeof(*stats)); > > > > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, - > ENOTSUP); > > - (*dev->dev_ops->stats_get)(dev, stats); > > + > > + if (dev->data->dev_started) > > + (*dev->dev_ops->stats_get)(dev, stats); > > + > > My question is should we mark an error or a warning here and return an > error so that the caller knows what happens? > > Thanks, > Michael In other cases in rte_ethdev.c where there is a check on "dev->data->dev_started" there is a RTE_PMD_DEBUG_TRACE() line. I will add a RTE_PMD_DEBUG_TRACE() line. The rte_eth_stats_reset() and rte_eth_xstats_reset() functions return void. Not sure if an error is required for the rte_eth_stats_get() and rte_eth_xstats_get() functions as the stats information returned is all zero's at present. Regards, Bernard.
[dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev functions.
On 2015/12/18 1:24, Bernard Iremonger wrote: > The nb_rx_queues and nb_tx_queues are initialised before > the tx_queue and rx_queue arrays are allocated. The arrays > are allocated when the ethdev port is started. > > If any of the following functions are called before the ethdev > port is started there is a segmentation fault: > > rte_eth_stats_get > rte_eth_stats_reset > rte_eth_xstats_get > rte_eth_xstats_reset > > Fixes: af75078fece3 ("first public release") > Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended statistics") > Fixes: d4fef8b0d5e5 ("ethdev: expose generic and driver specific stats in > xstats") > Signed-off-by: Bernard Iremonger > --- > lib/librte_ether/rte_ethdev.c | 16 > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c > index ed971b4..a0ee84d 100644 > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -1441,7 +1441,10 @@ rte_eth_stats_get(uint8_t port_id, struct > rte_eth_stats *stats) > memset(stats, 0, sizeof(*stats)); > > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP); > - (*dev->dev_ops->stats_get)(dev, stats); > + > + if (dev->data->dev_started) > + (*dev->dev_ops->stats_get)(dev, stats); > + My question is should we mark an error or a warning here and return an error so that the caller knows what happens? Thanks, Michael > stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed; > return 0; > } > @@ -1455,7 +1458,10 @@ rte_eth_stats_reset(uint8_t port_id) > dev = _eth_devices[port_id]; > > RTE_FUNC_PTR_OR_RET(*dev->dev_ops->stats_reset); > - (*dev->dev_ops->stats_reset)(dev); > + > + if (dev->data->dev_started) > + (*dev->dev_ops->stats_reset)(dev); > + > dev->data->rx_mbuf_alloc_failed = 0; > } > > @@ -1479,7 +1485,8 @@ rte_eth_xstats_get(uint8_t port_id, struct > rte_eth_xstats *xstats, > (dev->data->nb_tx_queues * RTE_NB_TXQ_STATS); > > /* implemented by the driver */ > - if (dev->dev_ops->xstats_get != NULL) { > + if ((dev->dev_ops->xstats_get != NULL) && > + (dev->data->dev_started)) { > /* Retrieve the xstats from the driver at the end of the >* xstats struct. >*/ > @@ -1548,7 +1555,8 @@ rte_eth_xstats_reset(uint8_t port_id) > dev = _eth_devices[port_id]; > > /* implemented by the driver */ > - if (dev->dev_ops->xstats_reset != NULL) { > + if ((dev->dev_ops->xstats_reset != NULL) && > + (dev->data->dev_started)) { > (*dev->dev_ops->xstats_reset)(dev); > return; > }
[dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev functions.
The nb_rx_queues and nb_tx_queues are initialised before the tx_queue and rx_queue arrays are allocated. The arrays are allocated when the ethdev port is started. If any of the following functions are called before the ethdev port is started there is a segmentation fault: rte_eth_stats_get rte_eth_stats_reset rte_eth_xstats_get rte_eth_xstats_reset Fixes: af75078fece3 ("first public release") Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended statistics") Fixes: d4fef8b0d5e5 ("ethdev: expose generic and driver specific stats in xstats") Signed-off-by: Bernard Iremonger --- lib/librte_ether/rte_ethdev.c | 16 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index ed971b4..a0ee84d 100644 --- a/lib/librte_ether/rte_ethdev.c +++ b/lib/librte_ether/rte_ethdev.c @@ -1441,7 +1441,10 @@ rte_eth_stats_get(uint8_t port_id, struct rte_eth_stats *stats) memset(stats, 0, sizeof(*stats)); RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP); - (*dev->dev_ops->stats_get)(dev, stats); + + if (dev->data->dev_started) + (*dev->dev_ops->stats_get)(dev, stats); + stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed; return 0; } @@ -1455,7 +1458,10 @@ rte_eth_stats_reset(uint8_t port_id) dev = _eth_devices[port_id]; RTE_FUNC_PTR_OR_RET(*dev->dev_ops->stats_reset); - (*dev->dev_ops->stats_reset)(dev); + + if (dev->data->dev_started) + (*dev->dev_ops->stats_reset)(dev); + dev->data->rx_mbuf_alloc_failed = 0; } @@ -1479,7 +1485,8 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats, (dev->data->nb_tx_queues * RTE_NB_TXQ_STATS); /* implemented by the driver */ - if (dev->dev_ops->xstats_get != NULL) { + if ((dev->dev_ops->xstats_get != NULL) && + (dev->data->dev_started)) { /* Retrieve the xstats from the driver at the end of the * xstats struct. */ @@ -1548,7 +1555,8 @@ rte_eth_xstats_reset(uint8_t port_id) dev = _eth_devices[port_id]; /* implemented by the driver */ - if (dev->dev_ops->xstats_reset != NULL) { + if ((dev->dev_ops->xstats_reset != NULL) && + (dev->data->dev_started)) { (*dev->dev_ops->xstats_reset)(dev); return; } -- 2.6.3