[dpdk-dev] [PATCH] ethdev: fix device info getting

[dpdk-dev] [PATCH] ethdev: fix device info getting

[Wenzhuo Lu]

The device information cannot be gotten correctly before
the configuration is set. Because on some NICs the
information has dependence on the configuration.

Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c | 47 +++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 3d556a8..9d60bea 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1017,28 +1017,6 @@ struct rte_eth_dev *
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
-	dev = &rte_eth_devices[port_id];
-
-	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
-	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
-
-	rte_eth_dev_info_get(port_id, &dev_info);
-
-	/* If number of queues specified by application for both Rx and Tx is
-	 * zero, use driver preferred values. This cannot be done individually
-	 * as it is valid for either Tx or Rx (but not both) to be zero.
-	 * If driver does not provide any preferred valued, fall back on
-	 * EAL defaults.
-	 */
-	if (nb_rx_q == 0 && nb_tx_q == 0) {
-		nb_rx_q = dev_info.default_rxportconf.nb_queues;
-		if (nb_rx_q == 0)
-			nb_rx_q = RTE_ETH_DEV_FALLBACK_RX_NBQUEUES;
-		nb_tx_q = dev_info.default_txportconf.nb_queues;
-		if (nb_tx_q == 0)
-			nb_tx_q = RTE_ETH_DEV_FALLBACK_TX_NBQUEUES;
-	}
-
 	if (nb_rx_q > RTE_MAX_QUEUES_PER_PORT) {
 		RTE_ETHDEV_LOG(ERR,
 			"Number of RX queues requested (%u) is greater than max supported(%d)\n",
@@ -1053,6 +1031,11 @@ struct rte_eth_dev *
 		return -EINVAL;
 	}
 
+	dev = &rte_eth_devices[port_id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
+
 	if (dev->data->dev_started) {
 		RTE_ETHDEV_LOG(ERR,
 			"Port %u must be stopped to allow configuration\n",
@@ -1060,8 +1043,26 @@ struct rte_eth_dev *
 		return -EBUSY;
 	}
 
-	/* Copy the dev_conf parameter into the dev structure */
+	/* Copy the dev_conf parameter into the dev structure,
+	 * then get the info.
+	 */
 	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
+	rte_eth_dev_info_get(port_id, &dev_info);
+
+	/* If number of queues specified by application for both Rx and Tx is
+	 * zero, use driver preferred values. This cannot be done individually
+	 * as it is valid for either Tx or Rx (but not both) to be zero.
+	 * If driver does not provide any preferred valued, fall back on
+	 * EAL defaults.
+	 */
+	if (nb_rx_q == 0 && nb_tx_q == 0) {
+		nb_rx_q = dev_info.default_rxportconf.nb_queues;
+		if (nb_rx_q == 0)
+			nb_rx_q = RTE_ETH_DEV_FALLBACK_RX_NBQUEUES;
+		nb_tx_q = dev_info.default_txportconf.nb_queues;
+		if (nb_tx_q == 0)
+			nb_tx_q = RTE_ETH_DEV_FALLBACK_TX_NBQUEUES;
+	}
 
 	/*
 	 * Check that the numbers of RX and TX queues are not greater
-- 
1.9.3

Re: [dpdk-dev] [PATCH] ethdev: fix device info getting

[Andrew Rybchenko]

On 12.07.2018 08:27, Wenzhuo Lu wrote:
> The device information cannot be gotten correctly before
> the configuration is set. Because on some NICs the
> information has dependence on the configuration.
>
> Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
>   lib/librte_ethdev/rte_ethdev.c | 47 +++++++++++++++++++++---------------------
>   1 file changed, 24 insertions(+), 23 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 3d556a8..9d60bea 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1017,28 +1017,6 @@ struct rte_eth_dev *
>   
>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>   
> -	dev = &rte_eth_devices[port_id];
> -
> -	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
> -	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
> -
> -	rte_eth_dev_info_get(port_id, &dev_info);
> -
> -	/* If number of queues specified by application for both Rx and Tx is
> -	 * zero, use driver preferred values. This cannot be done individually
> -	 * as it is valid for either Tx or Rx (but not both) to be zero.
> -	 * If driver does not provide any preferred valued, fall back on
> -	 * EAL defaults.
> -	 */
> -	if (nb_rx_q == 0 && nb_tx_q == 0) {
> -		nb_rx_q = dev_info.default_rxportconf.nb_queues;
> -		if (nb_rx_q == 0)
> -			nb_rx_q = RTE_ETH_DEV_FALLBACK_RX_NBQUEUES;
> -		nb_tx_q = dev_info.default_txportconf.nb_queues;
> -		if (nb_tx_q == 0)
> -			nb_tx_q = RTE_ETH_DEV_FALLBACK_TX_NBQUEUES;
> -	}
> -
>   	if (nb_rx_q > RTE_MAX_QUEUES_PER_PORT) {
>   		RTE_ETHDEV_LOG(ERR,
>   			"Number of RX queues requested (%u) is greater than max supported(%d)\n",
> @@ -1053,6 +1031,11 @@ struct rte_eth_dev *
>   		return -EINVAL;
>   	}
>   
> +	dev = &rte_eth_devices[port_id];
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
> +
>   	if (dev->data->dev_started) {
>   		RTE_ETHDEV_LOG(ERR,
>   			"Port %u must be stopped to allow configuration\n",
> @@ -1060,8 +1043,26 @@ struct rte_eth_dev *
>   		return -EBUSY;
>   	}
>   
> -	/* Copy the dev_conf parameter into the dev structure */
> +	/* Copy the dev_conf parameter into the dev structure,
> +	 * then get the info.
> +	 */
>   	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
> +	rte_eth_dev_info_get(port_id, &dev_info);
> +
> +	/* If number of queues specified by application for both Rx and Tx is
> +	 * zero, use driver preferred values. This cannot be done individually
> +	 * as it is valid for either Tx or Rx (but not both) to be zero.
> +	 * If driver does not provide any preferred valued, fall back on
> +	 * EAL defaults.
> +	 */
> +	if (nb_rx_q == 0 && nb_tx_q == 0) {
> +		nb_rx_q = dev_info.default_rxportconf.nb_queues;
> +		if (nb_rx_q == 0)
> +			nb_rx_q = RTE_ETH_DEV_FALLBACK_RX_NBQUEUES;
> +		nb_tx_q = dev_info.default_txportconf.nb_queues;
> +		if (nb_tx_q == 0)
> +			nb_tx_q = RTE_ETH_DEV_FALLBACK_TX_NBQUEUES;

Values assigned in this branch are not checked against
RTE_MAX_QUEUES_PER_PORT and RTE_MAX_QUEUES_PER_PORT now

> +	}
>   
>   	/*
>   	 * Check that the numbers of RX and TX queues are not greater

Re: [dpdk-dev] [PATCH] ethdev: fix device info getting

[Lu, Wenzhuo]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> Sent: Thursday, July 12, 2018 4:06 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ethdev: fix device info getting
> 
> On 12.07.2018 08:27, Wenzhuo Lu wrote:
> > The device information cannot be gotten correctly before the
> > configuration is set. Because on some NICs the information has
> > dependence on the configuration.
> >
> > Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > ---
> >   lib/librte_ethdev/rte_ethdev.c | 47 +++++++++++++++++++++------------------
> ---
> >   1 file changed, 24 insertions(+), 23 deletions(-)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.c
> > b/lib/librte_ethdev/rte_ethdev.c index 3d556a8..9d60bea 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -1017,28 +1017,6 @@ struct rte_eth_dev *
> >
> >   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> >
> > -	dev = &rte_eth_devices[port_id];
> > -
> > -	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -
> ENOTSUP);
> > -	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -
> ENOTSUP);
> > -
> > -	rte_eth_dev_info_get(port_id, &dev_info);
> > -
> > -	/* If number of queues specified by application for both Rx and Tx is
> > -	 * zero, use driver preferred values. This cannot be done individually
> > -	 * as it is valid for either Tx or Rx (but not both) to be zero.
> > -	 * If driver does not provide any preferred valued, fall back on
> > -	 * EAL defaults.
> > -	 */
> > -	if (nb_rx_q == 0 && nb_tx_q == 0) {
> > -		nb_rx_q = dev_info.default_rxportconf.nb_queues;
> > -		if (nb_rx_q == 0)
> > -			nb_rx_q = RTE_ETH_DEV_FALLBACK_RX_NBQUEUES;
> > -		nb_tx_q = dev_info.default_txportconf.nb_queues;
> > -		if (nb_tx_q == 0)
> > -			nb_tx_q = RTE_ETH_DEV_FALLBACK_TX_NBQUEUES;
> > -	}
> > -
> >   	if (nb_rx_q > RTE_MAX_QUEUES_PER_PORT) {
> >   		RTE_ETHDEV_LOG(ERR,
> >   			"Number of RX queues requested (%u) is greater
> than max
> > supported(%d)\n", @@ -1053,6 +1031,11 @@ struct rte_eth_dev *
> >   		return -EINVAL;
> >   	}
> >
> > +	dev = &rte_eth_devices[port_id];
> > +
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -
> ENOTSUP);
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -
> ENOTSUP);
> > +
> >   	if (dev->data->dev_started) {
> >   		RTE_ETHDEV_LOG(ERR,
> >   			"Port %u must be stopped to allow configuration\n",
> @@ -1060,8
> > +1043,26 @@ struct rte_eth_dev *
> >   		return -EBUSY;
> >   	}
> >
> > -	/* Copy the dev_conf parameter into the dev structure */
> > +	/* Copy the dev_conf parameter into the dev structure,
> > +	 * then get the info.
> > +	 */
> >   	memcpy(&dev->data->dev_conf, &local_conf,
> > sizeof(dev->data->dev_conf));
> > +	rte_eth_dev_info_get(port_id, &dev_info);
> > +
> > +	/* If number of queues specified by application for both Rx and Tx is
> > +	 * zero, use driver preferred values. This cannot be done individually
> > +	 * as it is valid for either Tx or Rx (but not both) to be zero.
> > +	 * If driver does not provide any preferred valued, fall back on
> > +	 * EAL defaults.
> > +	 */
> > +	if (nb_rx_q == 0 && nb_tx_q == 0) {
> > +		nb_rx_q = dev_info.default_rxportconf.nb_queues;
> > +		if (nb_rx_q == 0)
> > +			nb_rx_q = RTE_ETH_DEV_FALLBACK_RX_NBQUEUES;
> > +		nb_tx_q = dev_info.default_txportconf.nb_queues;
> > +		if (nb_tx_q == 0)
> > +			nb_tx_q = RTE_ETH_DEV_FALLBACK_TX_NBQUEUES;
> 
> Values assigned in this branch are not checked against
> RTE_MAX_QUEUES_PER_PORT and RTE_MAX_QUEUES_PER_PORT now
O, I supposed the default values cannot be larger than RTE_MAX_QUEUES_PER_PORT. But, yes, it's each NIC's decision. My assumption can be wrong. I'll move the check down here.

> 
> > +	}
> >
> >   	/*
> >   	 * Check that the numbers of RX and TX queues are not greater

[dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Wenzhuo Lu]

The device information cannot be gotten correctly before
the configuration is set. Because on some NICs the
information has dependence on the configuration.

Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 3d556a8..6f606c1 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1022,6 +1022,17 @@ struct rte_eth_dev *
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
 
+	if (dev->data->dev_started) {
+		RTE_ETHDEV_LOG(ERR,
+			"Port %u must be stopped to allow configuration\n",
+			port_id);
+		return -EBUSY;
+	}
+
+	/* Copy the dev_conf parameter into the dev structure,
+	 * then get the info.
+	 */
+	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
 	rte_eth_dev_info_get(port_id, &dev_info);
 
 	/* If number of queues specified by application for both Rx and Tx is
@@ -1053,16 +1064,6 @@ struct rte_eth_dev *
 		return -EINVAL;
 	}
 
-	if (dev->data->dev_started) {
-		RTE_ETHDEV_LOG(ERR,
-			"Port %u must be stopped to allow configuration\n",
-			port_id);
-		return -EBUSY;
-	}
-
-	/* Copy the dev_conf parameter into the dev structure */
-	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
-
 	/*
 	 * Check that the numbers of RX and TX queues are not greater
 	 * than the maximum number of RX and TX queues supported by the
-- 
1.9.3

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Andrew Rybchenko]

Hi, Wenzhuo,

I'm sorry, but I have more even harder questions than the previous one.
This questions are rather generic and mainly to ethdev maintainers.

On 13.07.2018 05:42, Wenzhuo Lu wrote:
> The device information cannot be gotten correctly before
> the configuration is set. Because on some NICs the
> information has dependence on the configuration.

Thinking about it I have the following question. Is it valid behaviour
of the dev_info if it changes after configuration?
I always thought that the primary goal of the dev_info is to
provide information to app about device capabilities to allow app
configure device and queues correctly. Now we see the case when
dev_info changes on configure. May be it is acceptable, but it is
really suspicious. If we accept it, it should be documented.
May be dev_info should be split into parts: part which is persistent and
part which may depend on device configuration.

> Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
>   lib/librte_ethdev/rte_ethdev.c | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 3d556a8..6f606c1 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1022,6 +1022,17 @@ struct rte_eth_dev *
>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
>   
> +	if (dev->data->dev_started) {
> +		RTE_ETHDEV_LOG(ERR,
> +			"Port %u must be stopped to allow configuration\n",
> +			port_id);
> +		return -EBUSY;
> +	}
> +
> +	/* Copy the dev_conf parameter into the dev structure,
> +	 * then get the info.
> +	 */
> +	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));

It is not a problem of the patch, but I'd like to highlight it to Ferruh 
and
Thomas. What we have in the case of below failures? State is really
inconsistent in the case of reconfigure. We have applied new dev_conf,
but we still have previous Rx/Tx queues which were setup before.

>   	rte_eth_dev_info_get(port_id, &dev_info);
>   
>   	/* If number of queues specified by application for both Rx and Tx is
> @@ -1053,16 +1064,6 @@ struct rte_eth_dev *
>   		return -EINVAL;
>   	}
>   
> -	if (dev->data->dev_started) {
> -		RTE_ETHDEV_LOG(ERR,
> -			"Port %u must be stopped to allow configuration\n",
> -			port_id);
> -		return -EBUSY;
> -	}
> -
> -	/* Copy the dev_conf parameter into the dev structure */
> -	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
> -
>   	/*
>   	 * Check that the numbers of RX and TX queues are not greater
>   	 * than the maximum number of RX and TX queues supported by the

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Lu, Wenzhuo]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> Sent: Friday, July 13, 2018 4:03 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> 
> Hi, Wenzhuo,
> 
> I'm sorry, but I have more even harder questions than the previous one.
> This questions are rather generic and mainly to ethdev maintainers.
> 
> On 13.07.2018 05:42, Wenzhuo Lu wrote:
> > The device information cannot be gotten correctly before the
> > configuration is set. Because on some NICs the information has
> > dependence on the configuration.
> 
> Thinking about it I have the following question. Is it valid behaviour of the
> dev_info if it changes after configuration?
> I always thought that the primary goal of the dev_info is to provide
> information to app about device capabilities to allow app configure device
> and queues correctly. Now we see the case when dev_info changes on
> configure. May be it is acceptable, but it is really suspicious. If we accept it, it
> should be documented.
> May be dev_info should be split into parts: part which is persistent and part
> which may depend on device configuration.
As I remember, the similar discussion has happened :) I've raised the similar suggestion like this. But we don’t make it happen.
The reason is, you see, this is the rte layer's behavior. So the user doesn't have to know it. From APP's PoV, it inputs the configuration, it calls this API "rte_eth_dev_configure". It doesn't know  the configuration is copied before getting the info or not.
So, to my opinion, we can still keep the behavior. We only need to split it into parts when we do see the case that cannot make it.

> 
> > Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > ---
> >   lib/librte_ethdev/rte_ethdev.c | 21 +++++++++++----------
> >   1 file changed, 11 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.c
> > b/lib/librte_ethdev/rte_ethdev.c index 3d556a8..6f606c1 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -1022,6 +1022,17 @@ struct rte_eth_dev *
> >   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -
> ENOTSUP);
> >   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -
> ENOTSUP);
> >
> > +	if (dev->data->dev_started) {
> > +		RTE_ETHDEV_LOG(ERR,
> > +			"Port %u must be stopped to allow configuration\n",
> > +			port_id);
> > +		return -EBUSY;
> > +	}
> > +
> > +	/* Copy the dev_conf parameter into the dev structure,
> > +	 * then get the info.
> > +	 */
> > +	memcpy(&dev->data->dev_conf, &local_conf,
> > +sizeof(dev->data->dev_conf));
> 
> It is not a problem of the patch, but I'd like to highlight it to Ferruh and
> Thomas. What we have in the case of below failures? State is really
> inconsistent in the case of reconfigure. We have applied new dev_conf, but
> we still have previous Rx/Tx queues which were setup before.
> 
> >   	rte_eth_dev_info_get(port_id, &dev_info);
> >
> >   	/* If number of queues specified by application for both Rx and Tx
> > is @@ -1053,16 +1064,6 @@ struct rte_eth_dev *
> >   		return -EINVAL;
> >   	}
> >
> > -	if (dev->data->dev_started) {
> > -		RTE_ETHDEV_LOG(ERR,
> > -			"Port %u must be stopped to allow configuration\n",
> > -			port_id);
> > -		return -EBUSY;
> > -	}
> > -
> > -	/* Copy the dev_conf parameter into the dev structure */
> > -	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data-
> >dev_conf));
> > -
> >   	/*
> >   	 * Check that the numbers of RX and TX queues are not greater
> >   	 * than the maximum number of RX and TX queues supported by the

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Lu, Wenzhuo]

Hi Andrew,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lu, Wenzhuo
> Sent: Monday, July 16, 2018 9:08 AM
> To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> 
> Hi Andrew,
> 
> > -----Original Message-----
> > From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> > Sent: Friday, July 13, 2018 4:03 PM
> > To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> > <thomas@monjalon.net>
> > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> >
> > Hi, Wenzhuo,
> >
> > I'm sorry, but I have more even harder questions than the previous one.
> > This questions are rather generic and mainly to ethdev maintainers.
> >
> > On 13.07.2018 05:42, Wenzhuo Lu wrote:
> > > The device information cannot be gotten correctly before the
> > > configuration is set. Because on some NICs the information has
> > > dependence on the configuration.
> >
> > Thinking about it I have the following question. Is it valid behaviour
> > of the dev_info if it changes after configuration?
> > I always thought that the primary goal of the dev_info is to provide
> > information to app about device capabilities to allow app configure
> > device and queues correctly. Now we see the case when dev_info changes
> > on configure. May be it is acceptable, but it is really suspicious. If
> > we accept it, it should be documented.
> > May be dev_info should be split into parts: part which is persistent
> > and part which may depend on device configuration.
> As I remember, the similar discussion has happened :) I've raised the similar
> suggestion like this. But we don’t make it happen.
> The reason is, you see, this is the rte layer's behavior. So the user doesn't
> have to know it. From APP's PoV, it inputs the configuration, it calls this API
> "rte_eth_dev_configure". It doesn't know  the configuration is copied before
> getting the info or not.
> So, to my opinion, we can still keep the behavior. We only need to split it
> into parts when we do see the case that cannot make it.
Maybe I talked too much about the patch. Think about it again. Your comments is about how to use the APIs,
rte_eth_dev_info_get, rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is just to get the info. It can be called anywhere, before configuration or after. It's reasonable the info changes with the configuration changing.
But we do have something missing, like, rte_eth_dev_capability_get which should be stable. APP can use this API to get the necessary info before configuration.

A question, maybe a little divergent thinking, that APP should have some intelligence to handle the capability automatically. So getting the capability is not so good and effective, looks like we still need the human involvement. Maybe that the reason currently we suppose APP know the capability from the paper copies, examples...

> 
> >
> > > Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
> > > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > > ---
> > >   lib/librte_ethdev/rte_ethdev.c | 21 +++++++++++----------
> > >   1 file changed, 11 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/lib/librte_ethdev/rte_ethdev.c
> > > b/lib/librte_ethdev/rte_ethdev.c index 3d556a8..6f606c1 100644
> > > --- a/lib/librte_ethdev/rte_ethdev.c
> > > +++ b/lib/librte_ethdev/rte_ethdev.c
> > > @@ -1022,6 +1022,17 @@ struct rte_eth_dev *
> > >   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -
> > ENOTSUP);
> > >   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -
> > ENOTSUP);
> > >
> > > +	if (dev->data->dev_started) {
> > > +		RTE_ETHDEV_LOG(ERR,
> > > +			"Port %u must be stopped to allow configuration\n",
> > > +			port_id);
> > > +		return -EBUSY;
> > > +	}
> > > +
> > > +	/* Copy the dev_conf parameter into the dev structure,
> > > +	 * then get the info.
> > > +	 */
> > > +	memcpy(&dev->data->dev_conf, &local_conf,
> > > +sizeof(dev->data->dev_conf));
> >
> > It is not a problem of the patch, but I'd like to highlight it to
> > Ferruh and Thomas. What we have in the case of below failures? State
> > is really inconsistent in the case of reconfigure. We have applied new
> > dev_conf, but we still have previous Rx/Tx queues which were setup before.
> >
> > >   	rte_eth_dev_info_get(port_id, &dev_info);
> > >
> > >   	/* If number of queues specified by application for both Rx and
> > > Tx is @@ -1053,16 +1064,6 @@ struct rte_eth_dev *
> > >   		return -EINVAL;
> > >   	}
> > >
> > > -	if (dev->data->dev_started) {
> > > -		RTE_ETHDEV_LOG(ERR,
> > > -			"Port %u must be stopped to allow configuration\n",
> > > -			port_id);
> > > -		return -EBUSY;
> > > -	}
> > > -
> > > -	/* Copy the dev_conf parameter into the dev structure */
> > > -	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data-
> > >dev_conf));
> > > -
> > >   	/*
> > >   	 * Check that the numbers of RX and TX queues are not greater
> > >   	 * than the maximum number of RX and TX queues supported by the

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Thomas Monjalon]

16/07/2018 03:58, Lu, Wenzhuo:
> Hi Andrew,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lu, Wenzhuo
> > Sent: Monday, July 16, 2018 9:08 AM
> > To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
> > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> > <thomas@monjalon.net>
> > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> > 
> > Hi Andrew,
> > 
> > > -----Original Message-----
> > > From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> > > Sent: Friday, July 13, 2018 4:03 PM
> > > To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> > > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> > > <thomas@monjalon.net>
> > > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> > >
> > > Hi, Wenzhuo,
> > >
> > > I'm sorry, but I have more even harder questions than the previous one.
> > > This questions are rather generic and mainly to ethdev maintainers.
> > >
> > > On 13.07.2018 05:42, Wenzhuo Lu wrote:
> > > > The device information cannot be gotten correctly before the
> > > > configuration is set. Because on some NICs the information has
> > > > dependence on the configuration.
> > >
> > > Thinking about it I have the following question. Is it valid behaviour
> > > of the dev_info if it changes after configuration?
> > > I always thought that the primary goal of the dev_info is to provide
> > > information to app about device capabilities to allow app configure
> > > device and queues correctly. Now we see the case when dev_info changes
> > > on configure. May be it is acceptable, but it is really suspicious. If
> > > we accept it, it should be documented.
> > > May be dev_info should be split into parts: part which is persistent
> > > and part which may depend on device configuration.
> > As I remember, the similar discussion has happened :) I've raised the similar
> > suggestion like this. But we don’t make it happen.
> > The reason is, you see, this is the rte layer's behavior. So the user doesn't
> > have to know it. From APP's PoV, it inputs the configuration, it calls this API
> > "rte_eth_dev_configure". It doesn't know  the configuration is copied before
> > getting the info or not.
> > So, to my opinion, we can still keep the behavior. We only need to split it
> > into parts when we do see the case that cannot make it.
> Maybe I talked too much about the patch. Think about it again. Your comments is about how to use the APIs,
> rte_eth_dev_info_get, rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is just to get the info. It can be called anywhere, before configuration or after. It's reasonable the info changes with the configuration changing.
> But we do have something missing, like, rte_eth_dev_capability_get which should be stable. APP can use this API to get the necessary info before configuration.
> 
> A question, maybe a little divergent thinking, that APP should have some intelligence to handle the capability automatically. So getting the capability is not so good and effective, looks like we still need the human involvement. Maybe that the reason currently we suppose APP know the capability from the paper copies, examples...

I am not sure to understand all the sentences.
But I agree that we should take a decision about the stability
of these infos.
Either infos cannot change after probing,
or we must document that the app must request infos regularly (when?).

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Lu, Wenzhuo]

Hi Thomas,


> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, August 1, 2018 11:37 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> 
> 16/07/2018 03:58, Lu, Wenzhuo:
> > Hi Andrew,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lu, Wenzhuo
> > > Sent: Monday, July 16, 2018 9:08 AM
> > > To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
> > > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> > > <thomas@monjalon.net>
> > > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> > >
> > > Hi Andrew,
> > >
> > > > -----Original Message-----
> > > > From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> > > > Sent: Friday, July 13, 2018 4:03 PM
> > > > To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> > > > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> > > > <thomas@monjalon.net>
> > > > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> > > >
> > > > Hi, Wenzhuo,
> > > >
> > > > I'm sorry, but I have more even harder questions than the previous one.
> > > > This questions are rather generic and mainly to ethdev maintainers.
> > > >
> > > > On 13.07.2018 05:42, Wenzhuo Lu wrote:
> > > > > The device information cannot be gotten correctly before the
> > > > > configuration is set. Because on some NICs the information has
> > > > > dependence on the configuration.
> > > >
> > > > Thinking about it I have the following question. Is it valid
> > > > behaviour of the dev_info if it changes after configuration?
> > > > I always thought that the primary goal of the dev_info is to
> > > > provide information to app about device capabilities to allow app
> > > > configure device and queues correctly. Now we see the case when
> > > > dev_info changes on configure. May be it is acceptable, but it is
> > > > really suspicious. If we accept it, it should be documented.
> > > > May be dev_info should be split into parts: part which is
> > > > persistent and part which may depend on device configuration.
> > > As I remember, the similar discussion has happened :) I've raised
> > > the similar suggestion like this. But we don’t make it happen.
> > > The reason is, you see, this is the rte layer's behavior. So the
> > > user doesn't have to know it. From APP's PoV, it inputs the
> > > configuration, it calls this API "rte_eth_dev_configure". It doesn't
> > > know  the configuration is copied before getting the info or not.
> > > So, to my opinion, we can still keep the behavior. We only need to
> > > split it into parts when we do see the case that cannot make it.
> > Maybe I talked too much about the patch. Think about it again. Your
> > comments is about how to use the APIs, rte_eth_dev_info_get,
> rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is just to get
> the info. It can be called anywhere, before configuration or after. It's
> reasonable the info changes with the configuration changing.
> > But we do have something missing, like, rte_eth_dev_capability_get which
> should be stable. APP can use this API to get the necessary info before
> configuration.
> >
> > A question, maybe a little divergent thinking, that APP should have some
> intelligence to handle the capability automatically. So getting the capability
> is not so good and effective, looks like we still need the human involvement.
> Maybe that the reason currently we suppose APP know the capability from
> the paper copies, examples...
> 
> I am not sure to understand all the sentences.
> But I agree that we should take a decision about the stability of these infos.
> Either infos cannot change after probing, or we must document that the app
> must request infos regularly (when?).
Sorry, I missed this mail.

I have the concern that different NICs have different behavior. One info can be stable on a NIC but dynamic on another. Considering this, we may better not splitting the rte_eth_dev_info_get to 2 APIs. And comparing with handling this in rte layer, maybe we can let every NIC has its own decision.
I have an idea. Maybe we can add a parameter for potential dynamic fields. Like,
Changing 
uint16_t nb_rx_queues;
to
struct nb_rx_queues {
uint16_t value;
bool stable;
}
By default, the stable is false. Then every NIC can maintain its own behavior.

Some fileds that must be stable can be left unchanged, like, driver_name, max_rx_queues.

As this patch is just reversing a bad commit to fix a bug, if my idea sounds good or worth discussing, I can send another RFC mail for it.

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Andrew Rybchenko]

On 13.08.2018 05:50, Lu, Wenzhuo wrote:
> Hi Thomas,
>
>
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>> Sent: Wednesday, August 1, 2018 11:37 PM
>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Andrew Rybchenko
>> <arybchenko@solarflare.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>
>> 16/07/2018 03:58, Lu, Wenzhuo:
>>> Hi Andrew,
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lu, Wenzhuo
>>>> Sent: Monday, July 16, 2018 9:08 AM
>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
>>>> <thomas@monjalon.net>
>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>
>>>> Hi Andrew,
>>>>
>>>>> -----Original Message-----
>>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
>>>>> Sent: Friday, July 13, 2018 4:03 PM
>>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
>>>>> <thomas@monjalon.net>
>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>>
>>>>> Hi, Wenzhuo,
>>>>>
>>>>> I'm sorry, but I have more even harder questions than the previous one.
>>>>> This questions are rather generic and mainly to ethdev maintainers.
>>>>>
>>>>> On 13.07.2018 05:42, Wenzhuo Lu wrote:
>>>>>> The device information cannot be gotten correctly before the
>>>>>> configuration is set. Because on some NICs the information has
>>>>>> dependence on the configuration.
>>>>> Thinking about it I have the following question. Is it valid
>>>>> behaviour of the dev_info if it changes after configuration?
>>>>> I always thought that the primary goal of the dev_info is to
>>>>> provide information to app about device capabilities to allow app
>>>>> configure device and queues correctly. Now we see the case when
>>>>> dev_info changes on configure. May be it is acceptable, but it is
>>>>> really suspicious. If we accept it, it should be documented.
>>>>> May be dev_info should be split into parts: part which is
>>>>> persistent and part which may depend on device configuration.
>>>> As I remember, the similar discussion has happened :) I've raised
>>>> the similar suggestion like this. But we don’t make it happen.
>>>> The reason is, you see, this is the rte layer's behavior. So the
>>>> user doesn't have to know it. From APP's PoV, it inputs the
>>>> configuration, it calls this API "rte_eth_dev_configure". It doesn't
>>>> know  the configuration is copied before getting the info or not.
>>>> So, to my opinion, we can still keep the behavior. We only need to
>>>> split it into parts when we do see the case that cannot make it.
>>> Maybe I talked too much about the patch. Think about it again. Your
>>> comments is about how to use the APIs, rte_eth_dev_info_get,
>> rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is just to get
>> the info. It can be called anywhere, before configuration or after. It's
>> reasonable the info changes with the configuration changing.
>>> But we do have something missing, like, rte_eth_dev_capability_get which
>> should be stable. APP can use this API to get the necessary info before
>> configuration.
>>> A question, maybe a little divergent thinking, that APP should have some
>> intelligence to handle the capability automatically. So getting the capability
>> is not so good and effective, looks like we still need the human involvement.
>> Maybe that the reason currently we suppose APP know the capability from
>> the paper copies, examples...
>>
>> I am not sure to understand all the sentences.
>> But I agree that we should take a decision about the stability of these infos.
>> Either infos cannot change after probing, or we must document that the app
>> must request infos regularly (when?).
> Sorry, I missed this mail.
>
> I have the concern that different NICs have different behavior. One info can be stable on a NIC but dynamic on another. Considering this, we may better not splitting the rte_eth_dev_info_get to 2 APIs. And comparing with handling this in rte layer, maybe we can let every NIC has its own decision.
> I have an idea. Maybe we can add a parameter for potential dynamic fields. Like,
> Changing
> uint16_t nb_rx_queues;
> to
> struct nb_rx_queues {
> uint16_t value;
> bool stable;
> }

May be it is just very bad example, but as I understand nb_rx_queues is 
mainly required to configure the device properly. Or should app 
configure, get new value, reconfigure again, get new value and so on and 
stop when previous is equal to the new one. Yes, I dramatise and it 
sounds really bad. In any case it would over-complicate interface and no 
single app will do it correctly.

Stable dev_info is simple. If there are real cases when something can't 
be stable (and may be recommended Rx/Tx ring sizes is good example, it 
should at least documented in dev_info structure description or may be 
moved to separate API.

> By default, the stable is false. Then every NIC can maintain its own behavior.
>
> Some fileds that must be stable can be left unchanged, like, driver_name, max_rx_queues.
>
> As this patch is just reversing a bad commit to fix a bug, if my idea sounds good or worth discussing, I can send another RFC mail for it.
>

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Lu, Wenzhuo]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> Sent: Monday, August 13, 2018 4:39 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> 
> On 13.08.2018 05:50, Lu, Wenzhuo wrote:
> > Hi Thomas,
> >
> >
> >> -----Original Message-----
> >> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> >> Sent: Wednesday, August 1, 2018 11:37 PM
> >> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Andrew Rybchenko
> >> <arybchenko@solarflare.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> >>
> >> 16/07/2018 03:58, Lu, Wenzhuo:
> >>> Hi Andrew,
> >>>
> >>>> -----Original Message-----
> >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lu, Wenzhuo
> >>>> Sent: Monday, July 16, 2018 9:08 AM
> >>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
> >>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> >>>> <thomas@monjalon.net>
> >>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> >>>>
> >>>> Hi Andrew,
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> >>>>> Sent: Friday, July 13, 2018 4:03 PM
> >>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> >>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> >>>>> <thomas@monjalon.net>
> >>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> >>>>>
> >>>>> Hi, Wenzhuo,
> >>>>>
> >>>>> I'm sorry, but I have more even harder questions than the previous
> one.
> >>>>> This questions are rather generic and mainly to ethdev maintainers.
> >>>>>
> >>>>> On 13.07.2018 05:42, Wenzhuo Lu wrote:
> >>>>>> The device information cannot be gotten correctly before the
> >>>>>> configuration is set. Because on some NICs the information has
> >>>>>> dependence on the configuration.
> >>>>> Thinking about it I have the following question. Is it valid
> >>>>> behaviour of the dev_info if it changes after configuration?
> >>>>> I always thought that the primary goal of the dev_info is to
> >>>>> provide information to app about device capabilities to allow app
> >>>>> configure device and queues correctly. Now we see the case when
> >>>>> dev_info changes on configure. May be it is acceptable, but it is
> >>>>> really suspicious. If we accept it, it should be documented.
> >>>>> May be dev_info should be split into parts: part which is
> >>>>> persistent and part which may depend on device configuration.
> >>>> As I remember, the similar discussion has happened :) I've raised
> >>>> the similar suggestion like this. But we don’t make it happen.
> >>>> The reason is, you see, this is the rte layer's behavior. So the
> >>>> user doesn't have to know it. From APP's PoV, it inputs the
> >>>> configuration, it calls this API "rte_eth_dev_configure". It
> >>>> doesn't know  the configuration is copied before getting the info or not.
> >>>> So, to my opinion, we can still keep the behavior. We only need to
> >>>> split it into parts when we do see the case that cannot make it.
> >>> Maybe I talked too much about the patch. Think about it again. Your
> >>> comments is about how to use the APIs, rte_eth_dev_info_get,
> >> rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is just to
> >> get the info. It can be called anywhere, before configuration or
> >> after. It's reasonable the info changes with the configuration changing.
> >>> But we do have something missing, like, rte_eth_dev_capability_get
> >>> which
> >> should be stable. APP can use this API to get the necessary info
> >> before configuration.
> >>> A question, maybe a little divergent thinking, that APP should have
> >>> some
> >> intelligence to handle the capability automatically. So getting the
> >> capability is not so good and effective, looks like we still need the human
> involvement.
> >> Maybe that the reason currently we suppose APP know the capability
> >> from the paper copies, examples...
> >>
> >> I am not sure to understand all the sentences.
> >> But I agree that we should take a decision about the stability of these
> infos.
> >> Either infos cannot change after probing, or we must document that
> >> the app must request infos regularly (when?).
> > Sorry, I missed this mail.
> >
> > I have the concern that different NICs have different behavior. One info
> can be stable on a NIC but dynamic on another. Considering this, we may
> better not splitting the rte_eth_dev_info_get to 2 APIs. And comparing with
> handling this in rte layer, maybe we can let every NIC has its own decision.
> > I have an idea. Maybe we can add a parameter for potential dynamic
> > fields. Like, Changing uint16_t nb_rx_queues; to struct nb_rx_queues {
> > uint16_t value; bool stable; }
> 
> May be it is just very bad example, but as I understand nb_rx_queues is
> mainly required to configure the device properly. Or should app configure,
> get new value, reconfigure again, get new value and so on and stop when
> previous is equal to the new one. Yes, I dramatise and it sounds really bad.
> In any case it would over-complicate interface and no single app will do it
> correctly.
I  think you're talking about max_rx_queues. APP can get that info before configuration. Then configure rx queue number which is not larger than it. That's enough.
nb_rx_queues should be the number which is configured by APP and how many queues are actually used. To my opinion, it's mainly used by the GUI to show the value to human being.

BTW, max_rx_queues could be an good example that shows that some parameters are stable on some NICs but not on other NICs.
Take Intel NICs for example (I don’t familiar with others.), normally max_rx_queues is stable on PF. But on VF, as the max number is decided by PF, it could be dynamic. When VF starts, it can get an default value from PF. If it not enough, it can request a larger one from PF. If the number works, VF can get a new number.

> 
> Stable dev_info is simple. If there are real cases when something can't be
> stable (and may be recommended Rx/Tx ring sizes is good example, it should
> at least documented in dev_info structure description or may be moved to
> separate API.
> 
> > By default, the stable is false. Then every NIC can maintain its own
> behavior.
> >
> > Some fileds that must be stable can be left unchanged, like, driver_name,
> max_rx_queues.
> >
> > As this patch is just reversing a bad commit to fix a bug, if my idea sounds
> good or worth discussing, I can send another RFC mail for it.
> >

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Ferruh Yigit]

On 8/14/2018 1:57 AM, Lu, Wenzhuo wrote:
> Hi Andrew,
> 
>> -----Original Message-----
>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
>> Sent: Monday, August 13, 2018 4:39 PM
>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Thomas Monjalon
>> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>
>> On 13.08.2018 05:50, Lu, Wenzhuo wrote:
>>> Hi Thomas,
>>>
>>>
>>>> -----Original Message-----
>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>>>> Sent: Wednesday, August 1, 2018 11:37 PM
>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Andrew Rybchenko
>>>> <arybchenko@solarflare.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>>>> Cc: dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>
>>>> 16/07/2018 03:58, Lu, Wenzhuo:
>>>>> Hi Andrew,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lu, Wenzhuo
>>>>>> Sent: Monday, July 16, 2018 9:08 AM
>>>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
>>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
>>>>>> <thomas@monjalon.net>
>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>>>
>>>>>> Hi Andrew,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
>>>>>>> Sent: Friday, July 13, 2018 4:03 PM
>>>>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
>>>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
>>>>>>> <thomas@monjalon.net>
>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>>>>
>>>>>>> Hi, Wenzhuo,
>>>>>>>
>>>>>>> I'm sorry, but I have more even harder questions than the previous
>> one.
>>>>>>> This questions are rather generic and mainly to ethdev maintainers.
>>>>>>>
>>>>>>> On 13.07.2018 05:42, Wenzhuo Lu wrote:
>>>>>>>> The device information cannot be gotten correctly before the
>>>>>>>> configuration is set. Because on some NICs the information has
>>>>>>>> dependence on the configuration.
>>>>>>> Thinking about it I have the following question. Is it valid
>>>>>>> behaviour of the dev_info if it changes after configuration?
>>>>>>> I always thought that the primary goal of the dev_info is to
>>>>>>> provide information to app about device capabilities to allow app
>>>>>>> configure device and queues correctly. Now we see the case when
>>>>>>> dev_info changes on configure. May be it is acceptable, but it is
>>>>>>> really suspicious. If we accept it, it should be documented.
>>>>>>> May be dev_info should be split into parts: part which is
>>>>>>> persistent and part which may depend on device configuration.
>>>>>> As I remember, the similar discussion has happened :) I've raised
>>>>>> the similar suggestion like this. But we don’t make it happen.
>>>>>> The reason is, you see, this is the rte layer's behavior. So the
>>>>>> user doesn't have to know it. From APP's PoV, it inputs the
>>>>>> configuration, it calls this API "rte_eth_dev_configure". It
>>>>>> doesn't know  the configuration is copied before getting the info or not.
>>>>>> So, to my opinion, we can still keep the behavior. We only need to
>>>>>> split it into parts when we do see the case that cannot make it.
>>>>> Maybe I talked too much about the patch. Think about it again. Your
>>>>> comments is about how to use the APIs, rte_eth_dev_info_get,
>>>> rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is just to
>>>> get the info. It can be called anywhere, before configuration or
>>>> after. It's reasonable the info changes with the configuration changing.
>>>>> But we do have something missing, like, rte_eth_dev_capability_get
>>>>> which
>>>> should be stable. APP can use this API to get the necessary info
>>>> before configuration.
>>>>> A question, maybe a little divergent thinking, that APP should have
>>>>> some
>>>> intelligence to handle the capability automatically. So getting the
>>>> capability is not so good and effective, looks like we still need the human
>> involvement.
>>>> Maybe that the reason currently we suppose APP know the capability
>>>> from the paper copies, examples...
>>>>
>>>> I am not sure to understand all the sentences.
>>>> But I agree that we should take a decision about the stability of these
>> infos.
>>>> Either infos cannot change after probing, or we must document that
>>>> the app must request infos regularly (when?).
>>> Sorry, I missed this mail.
>>>
>>> I have the concern that different NICs have different behavior. One info
>> can be stable on a NIC but dynamic on another. Considering this, we may
>> better not splitting the rte_eth_dev_info_get to 2 APIs. And comparing with
>> handling this in rte layer, maybe we can let every NIC has its own decision.
>>> I have an idea. Maybe we can add a parameter for potential dynamic
>>> fields. Like, Changing uint16_t nb_rx_queues; to struct nb_rx_queues {
>>> uint16_t value; bool stable; }
>>
>> May be it is just very bad example, but as I understand nb_rx_queues is
>> mainly required to configure the device properly. Or should app configure,
>> get new value, reconfigure again, get new value and so on and stop when
>> previous is equal to the new one. Yes, I dramatise and it sounds really bad.
>> In any case it would over-complicate interface and no single app will do it
>> correctly.
> I  think you're talking about max_rx_queues. APP can get that info before configuration. Then configure rx queue number which is not larger than it. That's enough.
> nb_rx_queues should be the number which is configured by APP and how many queues are actually used. To my opinion, it's mainly used by the GUI to show the value to human being.
> 
> BTW, max_rx_queues could be an good example that shows that some parameters are stable on some NICs but not on other NICs.
> Take Intel NICs for example (I don’t familiar with others.), normally max_rx_queues is stable on PF. But on VF, as the max number is decided by PF, it could be dynamic. When VF starts, it can get an default value from PF. If it not enough, it can request a larger one from PF. If the number works, VF can get a new number.

"struct rte_eth_dev_info" is a little overloaded, it has:
- static info, like *device
- device limitations, max_*, *_lim
- device capabilities, *_capa
- suggested configurations, default_*conf
- device configuration, nb_[r/t]x_queues
- other, switch_info

There is a concern that some values are dynamic, but this is not new, for
example nb_rx/tx_queues can be changed by rte_eth_dev_rx/tx_queue_config() API
and rte_eth_dev_info() output will be changed.
For this patch suggested configuration changes based on some other config values
looks ok as concept.
So I think we can say after every configuration related API dev info can be
changed.

But rte_eth_dev_info_get() has been called within rte_eth_dev_configure()
creating a cyclic dependency, this is forcing copy the user provided config
before rte_eth_dev_info().

This case the concern of copying user provided config to device config in early
stages cause inconsistent data in error case, this is valid concern and
unfortunately already an issue with the current implementation.

What would you think keep the logic in this patch but improve it with save and
restore existing device config for error cases?

> 
>>
>> Stable dev_info is simple. If there are real cases when something can't be
>> stable (and may be recommended Rx/Tx ring sizes is good example, it should
>> at least documented in dev_info structure description or may be moved to
>> separate API.
>>
>>> By default, the stable is false. Then every NIC can maintain its own
>> behavior.
>>>
>>> Some fileds that must be stable can be left unchanged, like, driver_name,
>> max_rx_queues.
>>>
>>> As this patch is just reversing a bad commit to fix a bug, if my idea sounds
>> good or worth discussing, I can send another RFC mail for it.
>>>
> 

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Andrew Rybchenko]

On 22.08.2018 19:55, Ferruh Yigit wrote:
> On 8/14/2018 1:57 AM, Lu, Wenzhuo wrote:
>> Hi Andrew,
>>
>>> -----Original Message-----
>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
>>> Sent: Monday, August 13, 2018 4:39 PM
>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Thomas Monjalon
>>> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>
>>> Cc: dev@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>
>>> On 13.08.2018 05:50, Lu, Wenzhuo wrote:
>>>> Hi Thomas,
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>>>>> Sent: Wednesday, August 1, 2018 11:37 PM
>>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Andrew Rybchenko
>>>>> <arybchenko@solarflare.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>> Cc: dev@dpdk.org
>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>>
>>>>> 16/07/2018 03:58, Lu, Wenzhuo:
>>>>>> Hi Andrew,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lu, Wenzhuo
>>>>>>> Sent: Monday, July 16, 2018 9:08 AM
>>>>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
>>>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
>>>>>>> <thomas@monjalon.net>
>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>>>>
>>>>>>> Hi Andrew,
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
>>>>>>>> Sent: Friday, July 13, 2018 4:03 PM
>>>>>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
>>>>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
>>>>>>>> <thomas@monjalon.net>
>>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>>>>>
>>>>>>>> Hi, Wenzhuo,
>>>>>>>>
>>>>>>>> I'm sorry, but I have more even harder questions than the previous
>>> one.
>>>>>>>> This questions are rather generic and mainly to ethdev maintainers.
>>>>>>>>
>>>>>>>> On 13.07.2018 05:42, Wenzhuo Lu wrote:
>>>>>>>>> The device information cannot be gotten correctly before the
>>>>>>>>> configuration is set. Because on some NICs the information has
>>>>>>>>> dependence on the configuration.
>>>>>>>> Thinking about it I have the following question. Is it valid
>>>>>>>> behaviour of the dev_info if it changes after configuration?
>>>>>>>> I always thought that the primary goal of the dev_info is to
>>>>>>>> provide information to app about device capabilities to allow app
>>>>>>>> configure device and queues correctly. Now we see the case when
>>>>>>>> dev_info changes on configure. May be it is acceptable, but it is
>>>>>>>> really suspicious. If we accept it, it should be documented.
>>>>>>>> May be dev_info should be split into parts: part which is
>>>>>>>> persistent and part which may depend on device configuration.
>>>>>>> As I remember, the similar discussion has happened :) I've raised
>>>>>>> the similar suggestion like this. But we don’t make it happen.
>>>>>>> The reason is, you see, this is the rte layer's behavior. So the
>>>>>>> user doesn't have to know it. From APP's PoV, it inputs the
>>>>>>> configuration, it calls this API "rte_eth_dev_configure". It
>>>>>>> doesn't know  the configuration is copied before getting the info or not.
>>>>>>> So, to my opinion, we can still keep the behavior. We only need to
>>>>>>> split it into parts when we do see the case that cannot make it.
>>>>>> Maybe I talked too much about the patch. Think about it again. Your
>>>>>> comments is about how to use the APIs, rte_eth_dev_info_get,
>>>>> rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is just to
>>>>> get the info. It can be called anywhere, before configuration or
>>>>> after. It's reasonable the info changes with the configuration changing.
>>>>>> But we do have something missing, like, rte_eth_dev_capability_get
>>>>>> which
>>>>> should be stable. APP can use this API to get the necessary info
>>>>> before configuration.
>>>>>> A question, maybe a little divergent thinking, that APP should have
>>>>>> some
>>>>> intelligence to handle the capability automatically. So getting the
>>>>> capability is not so good and effective, looks like we still need the human
>>> involvement.
>>>>> Maybe that the reason currently we suppose APP know the capability
>>>>> from the paper copies, examples...
>>>>>
>>>>> I am not sure to understand all the sentences.
>>>>> But I agree that we should take a decision about the stability of these
>>> infos.
>>>>> Either infos cannot change after probing, or we must document that
>>>>> the app must request infos regularly (when?).
>>>> Sorry, I missed this mail.
>>>>
>>>> I have the concern that different NICs have different behavior. One info
>>> can be stable on a NIC but dynamic on another. Considering this, we may
>>> better not splitting the rte_eth_dev_info_get to 2 APIs. And comparing with
>>> handling this in rte layer, maybe we can let every NIC has its own decision.
>>>> I have an idea. Maybe we can add a parameter for potential dynamic
>>>> fields. Like, Changing uint16_t nb_rx_queues; to struct nb_rx_queues {
>>>> uint16_t value; bool stable; }
>>> May be it is just very bad example, but as I understand nb_rx_queues is
>>> mainly required to configure the device properly. Or should app configure,
>>> get new value, reconfigure again, get new value and so on and stop when
>>> previous is equal to the new one. Yes, I dramatise and it sounds really bad.
>>> In any case it would over-complicate interface and no single app will do it
>>> correctly.
>> I  think you're talking about max_rx_queues. APP can get that info before configuration. Then configure rx queue number which is not larger than it. That's enough.
>> nb_rx_queues should be the number which is configured by APP and how many queues are actually used. To my opinion, it's mainly used by the GUI to show the value to human being.
>>
>> BTW, max_rx_queues could be an good example that shows that some parameters are stable on some NICs but not on other NICs.
>> Take Intel NICs for example (I don’t familiar with others.), normally max_rx_queues is stable on PF. But on VF, as the max number is decided by PF, it could be dynamic. When VF starts, it can get an default value from PF. If it not enough, it can request a larger one from PF. If the number works, VF can get a new number.
> "struct rte_eth_dev_info" is a little overloaded, it has:
> - static info, like *device
> - device limitations, max_*, *_lim
> - device capabilities, *_capa
> - suggested configurations, default_*conf
> - device configuration, nb_[r/t]x_queues
> - other, switch_info
>
> There is a concern that some values are dynamic, but this is not new, for
> example nb_rx/tx_queues can be changed by rte_eth_dev_rx/tx_queue_config() API
> and rte_eth_dev_info() output will be changed.

The example looks different to me. It is explicit changes directly
requested by the application. So, it is not a surprise that it changes.

> For this patch suggested configuration changes based on some other config values
> looks ok as concept.
> So I think we can say after every configuration related API dev info can be
> changed.

I think that saying that any configuration changes may result in any
changes in dev_info is hardly helpful. I'd suggest to be more specific.
Yes, it is harder and will have bugs, but at least it is helpful.

> But rte_eth_dev_info_get() has been called within rte_eth_dev_configure()
> creating a cyclic dependency, this is forcing copy the user provided config
> before rte_eth_dev_info().
>
> This case the concern of copying user provided config to device config in early
> stages cause inconsistent data in error case, this is valid concern and
> unfortunately already an issue with the current implementation.
>
> What would you think keep the logic in this patch but improve it with save and
> restore existing device config for error cases?
>
>>> Stable dev_info is simple. If there are real cases when something can't be
>>> stable (and may be recommended Rx/Tx ring sizes is good example, it should
>>> at least documented in dev_info structure description or may be moved to
>>> separate API.
>>>
>>>> By default, the stable is false. Then every NIC can maintain its own
>>> behavior.
>>>> Some fileds that must be stable can be left unchanged, like, driver_name,
>>> max_rx_queues.
>>>> As this patch is just reversing a bad commit to fix a bug, if my idea sounds
>>> good or worth discussing, I can send another RFC mail for it.

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Ferruh Yigit]

On 8/23/2018 9:58 AM, Andrew Rybchenko wrote:
> On 22.08.2018 19:55, Ferruh Yigit wrote:
>> On 8/14/2018 1:57 AM, Lu, Wenzhuo wrote:
>>> Hi Andrew,
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
>>>> Sent: Monday, August 13, 2018 4:39 PM
>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Thomas Monjalon
>>>> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>
>>>> Cc: dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>
>>>> On 13.08.2018 05:50, Lu, Wenzhuo wrote:
>>>>> Hi Thomas,
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>>>>>> Sent: Wednesday, August 1, 2018 11:37 PM
>>>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Andrew Rybchenko
>>>>>> <arybchenko@solarflare.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>>> Cc: dev@dpdk.org
>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>>>
>>>>>> 16/07/2018 03:58, Lu, Wenzhuo:
>>>>>>> Hi Andrew,
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lu, Wenzhuo
>>>>>>>> Sent: Monday, July 16, 2018 9:08 AM
>>>>>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
>>>>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
>>>>>>>> <thomas@monjalon.net>
>>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>>>>>
>>>>>>>> Hi Andrew,
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
>>>>>>>>> Sent: Friday, July 13, 2018 4:03 PM
>>>>>>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
>>>>>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
>>>>>>>>> <thomas@monjalon.net>
>>>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>>>>>>
>>>>>>>>> Hi, Wenzhuo,
>>>>>>>>>
>>>>>>>>> I'm sorry, but I have more even harder questions than the previous
>>>> one.
>>>>>>>>> This questions are rather generic and mainly to ethdev maintainers.
>>>>>>>>>
>>>>>>>>> On 13.07.2018 05:42, Wenzhuo Lu wrote:
>>>>>>>>>> The device information cannot be gotten correctly before the
>>>>>>>>>> configuration is set. Because on some NICs the information has
>>>>>>>>>> dependence on the configuration.
>>>>>>>>> Thinking about it I have the following question. Is it valid
>>>>>>>>> behaviour of the dev_info if it changes after configuration?
>>>>>>>>> I always thought that the primary goal of the dev_info is to
>>>>>>>>> provide information to app about device capabilities to allow app
>>>>>>>>> configure device and queues correctly. Now we see the case when
>>>>>>>>> dev_info changes on configure. May be it is acceptable, but it is
>>>>>>>>> really suspicious. If we accept it, it should be documented.
>>>>>>>>> May be dev_info should be split into parts: part which is
>>>>>>>>> persistent and part which may depend on device configuration.
>>>>>>>> As I remember, the similar discussion has happened :) I've raised
>>>>>>>> the similar suggestion like this. But we don’t make it happen.
>>>>>>>> The reason is, you see, this is the rte layer's behavior. So the
>>>>>>>> user doesn't have to know it. From APP's PoV, it inputs the
>>>>>>>> configuration, it calls this API "rte_eth_dev_configure". It
>>>>>>>> doesn't know  the configuration is copied before getting the info or not.
>>>>>>>> So, to my opinion, we can still keep the behavior. We only need to
>>>>>>>> split it into parts when we do see the case that cannot make it.
>>>>>>> Maybe I talked too much about the patch. Think about it again. Your
>>>>>>> comments is about how to use the APIs, rte_eth_dev_info_get,
>>>>>> rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is just to
>>>>>> get the info. It can be called anywhere, before configuration or
>>>>>> after. It's reasonable the info changes with the configuration changing.
>>>>>>> But we do have something missing, like, rte_eth_dev_capability_get
>>>>>>> which
>>>>>> should be stable. APP can use this API to get the necessary info
>>>>>> before configuration.
>>>>>>> A question, maybe a little divergent thinking, that APP should have
>>>>>>> some
>>>>>> intelligence to handle the capability automatically. So getting the
>>>>>> capability is not so good and effective, looks like we still need the human
>>>> involvement.
>>>>>> Maybe that the reason currently we suppose APP know the capability
>>>>>> from the paper copies, examples...
>>>>>>
>>>>>> I am not sure to understand all the sentences.
>>>>>> But I agree that we should take a decision about the stability of these
>>>> infos.
>>>>>> Either infos cannot change after probing, or we must document that
>>>>>> the app must request infos regularly (when?).
>>>>> Sorry, I missed this mail.
>>>>>
>>>>> I have the concern that different NICs have different behavior. One info
>>>> can be stable on a NIC but dynamic on another. Considering this, we may
>>>> better not splitting the rte_eth_dev_info_get to 2 APIs. And comparing with
>>>> handling this in rte layer, maybe we can let every NIC has its own decision.
>>>>> I have an idea. Maybe we can add a parameter for potential dynamic
>>>>> fields. Like, Changing uint16_t nb_rx_queues; to struct nb_rx_queues {
>>>>> uint16_t value; bool stable; }
>>>> May be it is just very bad example, but as I understand nb_rx_queues is
>>>> mainly required to configure the device properly. Or should app configure,
>>>> get new value, reconfigure again, get new value and so on and stop when
>>>> previous is equal to the new one. Yes, I dramatise and it sounds really bad.
>>>> In any case it would over-complicate interface and no single app will do it
>>>> correctly.
>>> I  think you're talking about max_rx_queues. APP can get that info before configuration. Then configure rx queue number which is not larger than it. That's enough.
>>> nb_rx_queues should be the number which is configured by APP and how many queues are actually used. To my opinion, it's mainly used by the GUI to show the value to human being.
>>>
>>> BTW, max_rx_queues could be an good example that shows that some parameters are stable on some NICs but not on other NICs.
>>> Take Intel NICs for example (I don’t familiar with others.), normally max_rx_queues is stable on PF. But on VF, as the max number is decided by PF, it could be dynamic. When VF starts, it can get an default value from PF. If it not enough, it can request a larger one from PF. If the number works, VF can get a new number.
>> "struct rte_eth_dev_info" is a little overloaded, it has:
>> - static info, like *device
>> - device limitations, max_*, *_lim
>> - device capabilities, *_capa
>> - suggested configurations, default_*conf
>> - device configuration, nb_[r/t]x_queues
>> - other, switch_info
>>
>> There is a concern that some values are dynamic, but this is not new, for
>> example nb_rx/tx_queues can be changed by rte_eth_dev_rx/tx_queue_config() API
>> and rte_eth_dev_info() output will be changed.
> 
> The example looks different to me. It is explicit changes directly
> requested by the application. So, it is not a surprise that it changes.
> 
>> For this patch suggested configuration changes based on some other config values
>> looks ok as concept.
>> So I think we can say after every configuration related API dev info can be
>> changed.
> 
> I think that saying that any configuration changes may result in any
> changes in dev_info is hardly helpful. I'd suggest to be more specific.
> Yes, it is harder and will have bugs, but at least it is helpful.

Hi Andrew, Wenzhuo,

Back to this patch, which fixes an actual defect,

What do you think about:
1- Keep existing patch but extend it as, save the original "dev->data" and
revert it back to this original data on all error path.
2- Update rte_eth_dev_info() API document and say default configuration can be
changed based on other config fields. So this reduces the scope of things can
change in dev_info.

Thanks,
ferruh

> 
>> But rte_eth_dev_info_get() has been called within rte_eth_dev_configure()
>> creating a cyclic dependency, this is forcing copy the user provided config
>> before rte_eth_dev_info().
>>
>> This case the concern of copying user provided config to device config in early
>> stages cause inconsistent data in error case, this is valid concern and
>> unfortunately already an issue with the current implementation.
>>
>> What would you think keep the logic in this patch but improve it with save and
>> restore existing device config for error cases?
>>
>>>> Stable dev_info is simple. If there are real cases when something can't be
>>>> stable (and may be recommended Rx/Tx ring sizes is good example, it should
>>>> at least documented in dev_info structure description or may be moved to
>>>> separate API.
>>>>
>>>>> By default, the stable is false. Then every NIC can maintain its own
>>>> behavior.
>>>>> Some fileds that must be stable can be left unchanged, like, driver_name,
>>>> max_rx_queues.
>>>>> As this patch is just reversing a bad commit to fix a bug, if my idea sounds
>>>> good or worth discussing, I can send another RFC mail for it.
> 
> 

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Thomas Monjalon]

22/10/2018 14:01, Ferruh Yigit:
> On 8/23/2018 9:58 AM, Andrew Rybchenko wrote:
> > On 22.08.2018 19:55, Ferruh Yigit wrote:
> >> On 8/14/2018 1:57 AM, Lu, Wenzhuo wrote:
> >>> Hi Andrew,
> >>>
> >>>> -----Original Message-----
> >>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> >>>> Sent: Monday, August 13, 2018 4:39 PM
> >>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Thomas Monjalon
> >>>> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>
> >>>> Cc: dev@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> >>>>
> >>>> On 13.08.2018 05:50, Lu, Wenzhuo wrote:
> >>>>> Hi Thomas,
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> >>>>>> Sent: Wednesday, August 1, 2018 11:37 PM
> >>>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Andrew Rybchenko
> >>>>>> <arybchenko@solarflare.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> >>>>>> Cc: dev@dpdk.org
> >>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> >>>>>>
> >>>>>> 16/07/2018 03:58, Lu, Wenzhuo:
> >>>>>>> Hi Andrew,
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lu, Wenzhuo
> >>>>>>>> Sent: Monday, July 16, 2018 9:08 AM
> >>>>>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
> >>>>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> >>>>>>>> <thomas@monjalon.net>
> >>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> >>>>>>>>
> >>>>>>>> Hi Andrew,
> >>>>>>>>
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> >>>>>>>>> Sent: Friday, July 13, 2018 4:03 PM
> >>>>>>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> >>>>>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> >>>>>>>>> <thomas@monjalon.net>
> >>>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> >>>>>>>>>
> >>>>>>>>> Hi, Wenzhuo,
> >>>>>>>>>
> >>>>>>>>> I'm sorry, but I have more even harder questions than the previous
> >>>> one.
> >>>>>>>>> This questions are rather generic and mainly to ethdev maintainers.
> >>>>>>>>>
> >>>>>>>>> On 13.07.2018 05:42, Wenzhuo Lu wrote:
> >>>>>>>>>> The device information cannot be gotten correctly before the
> >>>>>>>>>> configuration is set. Because on some NICs the information has
> >>>>>>>>>> dependence on the configuration.
> >>>>>>>>> Thinking about it I have the following question. Is it valid
> >>>>>>>>> behaviour of the dev_info if it changes after configuration?
> >>>>>>>>> I always thought that the primary goal of the dev_info is to
> >>>>>>>>> provide information to app about device capabilities to allow app
> >>>>>>>>> configure device and queues correctly. Now we see the case when
> >>>>>>>>> dev_info changes on configure. May be it is acceptable, but it is
> >>>>>>>>> really suspicious. If we accept it, it should be documented.
> >>>>>>>>> May be dev_info should be split into parts: part which is
> >>>>>>>>> persistent and part which may depend on device configuration.
> >>>>>>>> As I remember, the similar discussion has happened :) I've raised
> >>>>>>>> the similar suggestion like this. But we don’t make it happen.
> >>>>>>>> The reason is, you see, this is the rte layer's behavior. So the
> >>>>>>>> user doesn't have to know it. From APP's PoV, it inputs the
> >>>>>>>> configuration, it calls this API "rte_eth_dev_configure". It
> >>>>>>>> doesn't know  the configuration is copied before getting the info or not.
> >>>>>>>> So, to my opinion, we can still keep the behavior. We only need to
> >>>>>>>> split it into parts when we do see the case that cannot make it.
> >>>>>>> Maybe I talked too much about the patch. Think about it again. Your
> >>>>>>> comments is about how to use the APIs, rte_eth_dev_info_get,
> >>>>>> rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is just to
> >>>>>> get the info. It can be called anywhere, before configuration or
> >>>>>> after. It's reasonable the info changes with the configuration changing.
> >>>>>>> But we do have something missing, like, rte_eth_dev_capability_get
> >>>>>>> which
> >>>>>> should be stable. APP can use this API to get the necessary info
> >>>>>> before configuration.
> >>>>>>> A question, maybe a little divergent thinking, that APP should have
> >>>>>>> some
> >>>>>> intelligence to handle the capability automatically. So getting the
> >>>>>> capability is not so good and effective, looks like we still need the human
> >>>> involvement.
> >>>>>> Maybe that the reason currently we suppose APP know the capability
> >>>>>> from the paper copies, examples...
> >>>>>>
> >>>>>> I am not sure to understand all the sentences.
> >>>>>> But I agree that we should take a decision about the stability of these
> >>>> infos.
> >>>>>> Either infos cannot change after probing, or we must document that
> >>>>>> the app must request infos regularly (when?).
> >>>>> Sorry, I missed this mail.
> >>>>>
> >>>>> I have the concern that different NICs have different behavior. One info
> >>>> can be stable on a NIC but dynamic on another. Considering this, we may
> >>>> better not splitting the rte_eth_dev_info_get to 2 APIs. And comparing with
> >>>> handling this in rte layer, maybe we can let every NIC has its own decision.
> >>>>> I have an idea. Maybe we can add a parameter for potential dynamic
> >>>>> fields. Like, Changing uint16_t nb_rx_queues; to struct nb_rx_queues {
> >>>>> uint16_t value; bool stable; }
> >>>> May be it is just very bad example, but as I understand nb_rx_queues is
> >>>> mainly required to configure the device properly. Or should app configure,
> >>>> get new value, reconfigure again, get new value and so on and stop when
> >>>> previous is equal to the new one. Yes, I dramatise and it sounds really bad.
> >>>> In any case it would over-complicate interface and no single app will do it
> >>>> correctly.
> >>> I  think you're talking about max_rx_queues. APP can get that info before configuration. Then configure rx queue number which is not larger than it. That's enough.
> >>> nb_rx_queues should be the number which is configured by APP and how many queues are actually used. To my opinion, it's mainly used by the GUI to show the value to human being.
> >>>
> >>> BTW, max_rx_queues could be an good example that shows that some parameters are stable on some NICs but not on other NICs.
> >>> Take Intel NICs for example (I don’t familiar with others.), normally max_rx_queues is stable on PF. But on VF, as the max number is decided by PF, it could be dynamic. When VF starts, it can get an default value from PF. If it not enough, it can request a larger one from PF. If the number works, VF can get a new number.
> >> "struct rte_eth_dev_info" is a little overloaded, it has:
> >> - static info, like *device
> >> - device limitations, max_*, *_lim
> >> - device capabilities, *_capa
> >> - suggested configurations, default_*conf
> >> - device configuration, nb_[r/t]x_queues
> >> - other, switch_info
> >>
> >> There is a concern that some values are dynamic, but this is not new, for
> >> example nb_rx/tx_queues can be changed by rte_eth_dev_rx/tx_queue_config() API
> >> and rte_eth_dev_info() output will be changed.
> > 
> > The example looks different to me. It is explicit changes directly
> > requested by the application. So, it is not a surprise that it changes.
> > 
> >> For this patch suggested configuration changes based on some other config values
> >> looks ok as concept.
> >> So I think we can say after every configuration related API dev info can be
> >> changed.
> > 
> > I think that saying that any configuration changes may result in any
> > changes in dev_info is hardly helpful. I'd suggest to be more specific.
> > Yes, it is harder and will have bugs, but at least it is helpful.
> 
> Hi Andrew, Wenzhuo,
> 
> Back to this patch, which fixes an actual defect,
> 
> What do you think about:
> 1- Keep existing patch but extend it as, save the original "dev->data" and
> revert it back to this original data on all error path.
> 2- Update rte_eth_dev_info() API document and say default configuration can be
> changed based on other config fields. So this reduces the scope of things can
> change in dev_info.

I think we are doing too much juggling with data in ethdev layer.
All these things should be the responsibility of the PMD.
My radical proposal would be to remove rte_eth_dev_info and integrate
all the data into rte_eth_dev_data.

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Lu, Wenzhuo]

Hi Thomas, Ferruh, Andrew,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, October 22, 2018 8:13 PM
> To: dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> 
> 22/10/2018 14:01, Ferruh Yigit:
> > On 8/23/2018 9:58 AM, Andrew Rybchenko wrote:
> > > On 22.08.2018 19:55, Ferruh Yigit wrote:
> > >> On 8/14/2018 1:57 AM, Lu, Wenzhuo wrote:
> > >>> Hi Andrew,
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> > >>>> Sent: Monday, August 13, 2018 4:39 PM
> > >>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Thomas Monjalon
> > >>>> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>
> > >>>> Cc: dev@dpdk.org
> > >>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info
> > >>>> getting
> > >>>>
> > >>>> On 13.08.2018 05:50, Lu, Wenzhuo wrote:
> > >>>>> Hi Thomas,
> > >>>>>
> > >>>>>
> > >>>>>> -----Original Message-----
> > >>>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > >>>>>> Sent: Wednesday, August 1, 2018 11:37 PM
> > >>>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Andrew Rybchenko
> > >>>>>> <arybchenko@solarflare.com>; Yigit, Ferruh
> > >>>>>> <ferruh.yigit@intel.com>
> > >>>>>> Cc: dev@dpdk.org
> > >>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info
> > >>>>>> getting
> > >>>>>>
> > >>>>>> 16/07/2018 03:58, Lu, Wenzhuo:
> > >>>>>>> Hi Andrew,
> > >>>>>>>
> > >>>>>>>> -----Original Message-----
> > >>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lu,
> > >>>>>>>> Wenzhuo
> > >>>>>>>> Sent: Monday, July 16, 2018 9:08 AM
> > >>>>>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>;
> > >>>>>>>> dev@dpdk.org
> > >>>>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> > >>>>>>>> <thomas@monjalon.net>
> > >>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info
> > >>>>>>>> getting
> > >>>>>>>>
> > >>>>>>>> Hi Andrew,
> > >>>>>>>>
> > >>>>>>>>> -----Original Message-----
> > >>>>>>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> > >>>>>>>>> Sent: Friday, July 13, 2018 4:03 PM
> > >>>>>>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> > >>>>>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> > >>>>>>>>> <thomas@monjalon.net>
> > >>>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info
> > >>>>>>>>> getting
> > >>>>>>>>>
> > >>>>>>>>> Hi, Wenzhuo,
> > >>>>>>>>>
> > >>>>>>>>> I'm sorry, but I have more even harder questions than the
> > >>>>>>>>> previous
> > >>>> one.
> > >>>>>>>>> This questions are rather generic and mainly to ethdev
> maintainers.
> > >>>>>>>>>
> > >>>>>>>>> On 13.07.2018 05:42, Wenzhuo Lu wrote:
> > >>>>>>>>>> The device information cannot be gotten correctly before
> > >>>>>>>>>> the configuration is set. Because on some NICs the
> > >>>>>>>>>> information has dependence on the configuration.
> > >>>>>>>>> Thinking about it I have the following question. Is it valid
> > >>>>>>>>> behaviour of the dev_info if it changes after configuration?
> > >>>>>>>>> I always thought that the primary goal of the dev_info is to
> > >>>>>>>>> provide information to app about device capabilities to
> > >>>>>>>>> allow app configure device and queues correctly. Now we see
> > >>>>>>>>> the case when dev_info changes on configure. May be it is
> > >>>>>>>>> acceptable, but it is really suspicious. If we accept it, it should
> be documented.
> > >>>>>>>>> May be dev_info should be split into parts: part which is
> > >>>>>>>>> persistent and part which may depend on device configuration.
> > >>>>>>>> As I remember, the similar discussion has happened :) I've
> > >>>>>>>> raised the similar suggestion like this. But we don’t make it
> happen.
> > >>>>>>>> The reason is, you see, this is the rte layer's behavior. So
> > >>>>>>>> the user doesn't have to know it. From APP's PoV, it inputs
> > >>>>>>>> the configuration, it calls this API "rte_eth_dev_configure".
> > >>>>>>>> It doesn't know  the configuration is copied before getting the
> info or not.
> > >>>>>>>> So, to my opinion, we can still keep the behavior. We only
> > >>>>>>>> need to split it into parts when we do see the case that cannot
> make it.
> > >>>>>>> Maybe I talked too much about the patch. Think about it again.
> > >>>>>>> Your comments is about how to use the APIs,
> > >>>>>>> rte_eth_dev_info_get,
> > >>>>>> rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is
> > >>>>>> just to get the info. It can be called anywhere, before
> > >>>>>> configuration or after. It's reasonable the info changes with the
> configuration changing.
> > >>>>>>> But we do have something missing, like,
> > >>>>>>> rte_eth_dev_capability_get which
> > >>>>>> should be stable. APP can use this API to get the necessary
> > >>>>>> info before configuration.
> > >>>>>>> A question, maybe a little divergent thinking, that APP should
> > >>>>>>> have some
> > >>>>>> intelligence to handle the capability automatically. So getting
> > >>>>>> the capability is not so good and effective, looks like we
> > >>>>>> still need the human
> > >>>> involvement.
> > >>>>>> Maybe that the reason currently we suppose APP know the
> > >>>>>> capability from the paper copies, examples...
> > >>>>>>
> > >>>>>> I am not sure to understand all the sentences.
> > >>>>>> But I agree that we should take a decision about the stability
> > >>>>>> of these
> > >>>> infos.
> > >>>>>> Either infos cannot change after probing, or we must document
> > >>>>>> that the app must request infos regularly (when?).
> > >>>>> Sorry, I missed this mail.
> > >>>>>
> > >>>>> I have the concern that different NICs have different behavior.
> > >>>>> One info
> > >>>> can be stable on a NIC but dynamic on another. Considering this,
> > >>>> we may better not splitting the rte_eth_dev_info_get to 2 APIs.
> > >>>> And comparing with handling this in rte layer, maybe we can let every
> NIC has its own decision.
> > >>>>> I have an idea. Maybe we can add a parameter for potential
> > >>>>> dynamic fields. Like, Changing uint16_t nb_rx_queues; to struct
> > >>>>> nb_rx_queues { uint16_t value; bool stable; }
> > >>>> May be it is just very bad example, but as I understand
> > >>>> nb_rx_queues is mainly required to configure the device properly.
> > >>>> Or should app configure, get new value, reconfigure again, get
> > >>>> new value and so on and stop when previous is equal to the new one.
> Yes, I dramatise and it sounds really bad.
> > >>>> In any case it would over-complicate interface and no single app
> > >>>> will do it correctly.
> > >>> I  think you're talking about max_rx_queues. APP can get that info
> before configuration. Then configure rx queue number which is not larger
> than it. That's enough.
> > >>> nb_rx_queues should be the number which is configured by APP and
> how many queues are actually used. To my opinion, it's mainly used by the
> GUI to show the value to human being.
> > >>>
> > >>> BTW, max_rx_queues could be an good example that shows that
> some parameters are stable on some NICs but not on other NICs.
> > >>> Take Intel NICs for example (I don’t familiar with others.), normally
> max_rx_queues is stable on PF. But on VF, as the max number is decided by
> PF, it could be dynamic. When VF starts, it can get an default value from PF.
> If it not enough, it can request a larger one from PF. If the number works, VF
> can get a new number.
> > >> "struct rte_eth_dev_info" is a little overloaded, it has:
> > >> - static info, like *device
> > >> - device limitations, max_*, *_lim
> > >> - device capabilities, *_capa
> > >> - suggested configurations, default_*conf
> > >> - device configuration, nb_[r/t]x_queues
> > >> - other, switch_info
> > >>
> > >> There is a concern that some values are dynamic, but this is not
> > >> new, for example nb_rx/tx_queues can be changed by
> > >> rte_eth_dev_rx/tx_queue_config() API and rte_eth_dev_info() output
> will be changed.
> > >
> > > The example looks different to me. It is explicit changes directly
> > > requested by the application. So, it is not a surprise that it changes.
> > >
> > >> For this patch suggested configuration changes based on some other
> > >> config values looks ok as concept.
> > >> So I think we can say after every configuration related API dev
> > >> info can be changed.
> > >
> > > I think that saying that any configuration changes may result in any
> > > changes in dev_info is hardly helpful. I'd suggest to be more specific.
> > > Yes, it is harder and will have bugs, but at least it is helpful.
> >
> > Hi Andrew, Wenzhuo,
> >
> > Back to this patch, which fixes an actual defect,
> >
> > What do you think about:
> > 1- Keep existing patch but extend it as, save the original "dev->data"
> > and revert it back to this original data on all error path.
> > 2- Update rte_eth_dev_info() API document and say default
> > configuration can be changed based on other config fields. So this
> > reduces the scope of things can change in dev_info.
> 
> I think we are doing too much juggling with data in ethdev layer.
> All these things should be the responsibility of the PMD.
> My radical proposal would be to remove rte_eth_dev_info and integrate all
> the data into rte_eth_dev_data.
> 
Sorry for missing this discussion. It's a good discussion about how to optimize the rte_eth.
But I have to say that above discussion can reach a huge reconsitution of the rte_eth and impact every PMD. Is that fair? 
This patch is only try to revert a bad commit as we already find bug. As I remember, at the beginning, Andrew said the discussion may not about the patch but generic. So could we just tell if this patch itself OK at first?  Thanks.

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Thomas Monjalon]

23/10/2018 03:25, Lu, Wenzhuo:
> Hi Thomas, Ferruh, Andrew,
> 
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 22/10/2018 14:01, Ferruh Yigit:
> > > On 8/23/2018 9:58 AM, Andrew Rybchenko wrote:
> > > > On 22.08.2018 19:55, Ferruh Yigit wrote:
> > > >> On 8/14/2018 1:57 AM, Lu, Wenzhuo wrote:
> > > >>> Hi Andrew,
> > > >>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> > > >>>> On 13.08.2018 05:50, Lu, Wenzhuo wrote:
> > > >>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > >>>>>> 16/07/2018 03:58, Lu, Wenzhuo:
> > > >>>>>>> Hi Andrew,
> > > >>>>>>> From: Lu, Wenzhuo
> > > >>>>>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> > > >>>>>>>>>
> > > >>>>>>>>> Hi, Wenzhuo,
> > > >>>>>>>>>
> > > >>>>>>>>> I'm sorry, but I have more even harder questions than the
> > > >>>>>>>>> previous
> > > >>>> one.
> > > >>>>>>>>> This questions are rather generic and mainly to ethdev
> > maintainers.
> > > >>>>>>>>>
> > > >>>>>>>>> On 13.07.2018 05:42, Wenzhuo Lu wrote:
> > > >>>>>>>>>> The device information cannot be gotten correctly before
> > > >>>>>>>>>> the configuration is set. Because on some NICs the
> > > >>>>>>>>>> information has dependence on the configuration.
> > > >>>>>>>>> Thinking about it I have the following question. Is it valid
> > > >>>>>>>>> behaviour of the dev_info if it changes after configuration?
> > > >>>>>>>>> I always thought that the primary goal of the dev_info is to
> > > >>>>>>>>> provide information to app about device capabilities to
> > > >>>>>>>>> allow app configure device and queues correctly. Now we see
> > > >>>>>>>>> the case when dev_info changes on configure. May be it is
> > > >>>>>>>>> acceptable, but it is really suspicious. If we accept it, it should
> > be documented.
> > > >>>>>>>>> May be dev_info should be split into parts: part which is
> > > >>>>>>>>> persistent and part which may depend on device configuration.
> > > >>>>>>>> As I remember, the similar discussion has happened :) I've
> > > >>>>>>>> raised the similar suggestion like this. But we don’t make it
> > happen.
> > > >>>>>>>> The reason is, you see, this is the rte layer's behavior. So
> > > >>>>>>>> the user doesn't have to know it. From APP's PoV, it inputs
> > > >>>>>>>> the configuration, it calls this API "rte_eth_dev_configure".
> > > >>>>>>>> It doesn't know  the configuration is copied before getting the
> > info or not.
> > > >>>>>>>> So, to my opinion, we can still keep the behavior. We only
> > > >>>>>>>> need to split it into parts when we do see the case that cannot
> > make it.
> > > >>>>>>> Maybe I talked too much about the patch. Think about it again.
> > > >>>>>>> Your comments is about how to use the APIs,
> > > >>>>>>> rte_eth_dev_info_get,
> > > >>>>>> rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is
> > > >>>>>> just to get the info. It can be called anywhere, before
> > > >>>>>> configuration or after. It's reasonable the info changes with the
> > configuration changing.
> > > >>>>>>> But we do have something missing, like,
> > > >>>>>>> rte_eth_dev_capability_get which
> > > >>>>>> should be stable. APP can use this API to get the necessary
> > > >>>>>> info before configuration.
> > > >>>>>>> A question, maybe a little divergent thinking, that APP should
> > > >>>>>>> have some
> > > >>>>>> intelligence to handle the capability automatically. So getting
> > > >>>>>> the capability is not so good and effective, looks like we
> > > >>>>>> still need the human
> > > >>>> involvement.
> > > >>>>>> Maybe that the reason currently we suppose APP know the
> > > >>>>>> capability from the paper copies, examples...
> > > >>>>>>
> > > >>>>>> I am not sure to understand all the sentences.
> > > >>>>>> But I agree that we should take a decision about the stability
> > > >>>>>> of these
> > > >>>> infos.
> > > >>>>>> Either infos cannot change after probing, or we must document
> > > >>>>>> that the app must request infos regularly (when?).
> > > >>>>> Sorry, I missed this mail.
> > > >>>>>
> > > >>>>> I have the concern that different NICs have different behavior.
> > > >>>>> One info
> > > >>>> can be stable on a NIC but dynamic on another. Considering this,
> > > >>>> we may better not splitting the rte_eth_dev_info_get to 2 APIs.
> > > >>>> And comparing with handling this in rte layer, maybe we can let every
> > NIC has its own decision.
> > > >>>>> I have an idea. Maybe we can add a parameter for potential
> > > >>>>> dynamic fields. Like, Changing uint16_t nb_rx_queues; to struct
> > > >>>>> nb_rx_queues { uint16_t value; bool stable; }
> > > >>>> May be it is just very bad example, but as I understand
> > > >>>> nb_rx_queues is mainly required to configure the device properly.
> > > >>>> Or should app configure, get new value, reconfigure again, get
> > > >>>> new value and so on and stop when previous is equal to the new one.
> > Yes, I dramatise and it sounds really bad.
> > > >>>> In any case it would over-complicate interface and no single app
> > > >>>> will do it correctly.
> > > >>> I  think you're talking about max_rx_queues. APP can get that info
> > before configuration. Then configure rx queue number which is not larger
> > than it. That's enough.
> > > >>> nb_rx_queues should be the number which is configured by APP and
> > how many queues are actually used. To my opinion, it's mainly used by the
> > GUI to show the value to human being.
> > > >>>
> > > >>> BTW, max_rx_queues could be an good example that shows that
> > some parameters are stable on some NICs but not on other NICs.
> > > >>> Take Intel NICs for example (I don’t familiar with others.), normally
> > max_rx_queues is stable on PF. But on VF, as the max number is decided by
> > PF, it could be dynamic. When VF starts, it can get an default value from PF.
> > If it not enough, it can request a larger one from PF. If the number works, VF
> > can get a new number.
> > > >> "struct rte_eth_dev_info" is a little overloaded, it has:
> > > >> - static info, like *device
> > > >> - device limitations, max_*, *_lim
> > > >> - device capabilities, *_capa
> > > >> - suggested configurations, default_*conf
> > > >> - device configuration, nb_[r/t]x_queues
> > > >> - other, switch_info
> > > >>
> > > >> There is a concern that some values are dynamic, but this is not
> > > >> new, for example nb_rx/tx_queues can be changed by
> > > >> rte_eth_dev_rx/tx_queue_config() API and rte_eth_dev_info() output
> > will be changed.
> > > >
> > > > The example looks different to me. It is explicit changes directly
> > > > requested by the application. So, it is not a surprise that it changes.
> > > >
> > > >> For this patch suggested configuration changes based on some other
> > > >> config values looks ok as concept.
> > > >> So I think we can say after every configuration related API dev
> > > >> info can be changed.
> > > >
> > > > I think that saying that any configuration changes may result in any
> > > > changes in dev_info is hardly helpful. I'd suggest to be more specific.
> > > > Yes, it is harder and will have bugs, but at least it is helpful.
> > >
> > > Hi Andrew, Wenzhuo,
> > >
> > > Back to this patch, which fixes an actual defect,
> > >
> > > What do you think about:
> > > 1- Keep existing patch but extend it as, save the original "dev->data"
> > > and revert it back to this original data on all error path.
> > > 2- Update rte_eth_dev_info() API document and say default
> > > configuration can be changed based on other config fields. So this
> > > reduces the scope of things can change in dev_info.
> > 
> > I think we are doing too much juggling with data in ethdev layer.
> > All these things should be the responsibility of the PMD.
> > My radical proposal would be to remove rte_eth_dev_info and integrate all
> > the data into rte_eth_dev_data.
> > 
> Sorry for missing this discussion. It's a good discussion about how to optimize the rte_eth.
> But I have to say that above discussion can reach a huge reconsitution of the rte_eth and impact every PMD. Is that fair? 
> This patch is only try to revert a bad commit as we already find bug. As I remember, at the beginning, Andrew said the discussion may not about the patch but generic. So could we just tell if this patch itself OK at first?  Thanks.

I have no strong opinion about your patch.
Sorry, I am more interested by a possible future big clean-up.
About a temporary fix, I am OK if Ferruh and Andrew are OK.

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Lu, Wenzhuo]

Hi Ferruh, Andrew,


> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, October 23, 2018 3:29 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> Rybchenko <arybchenko@solarflare.com>
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
> 
> 23/10/2018 03:25, Lu, Wenzhuo:
> > Hi Thomas, Ferruh, Andrew,
> >
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 22/10/2018 14:01, Ferruh Yigit:
> > > > On 8/23/2018 9:58 AM, Andrew Rybchenko wrote:
> > > > > On 22.08.2018 19:55, Ferruh Yigit wrote:
> > > > >> On 8/14/2018 1:57 AM, Lu, Wenzhuo wrote:
> > > > >>> Hi Andrew,
> > > > >>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> > > > >>>> On 13.08.2018 05:50, Lu, Wenzhuo wrote:
> > > > >>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > >>>>>> 16/07/2018 03:58, Lu, Wenzhuo:
> > > > >>>>>>> Hi Andrew,
> > > > >>>>>>> From: Lu, Wenzhuo
> > > > >>>>>>>> From: Andrew Rybchenko
> [mailto:arybchenko@solarflare.com]
> > > > >>>>>>>>>
> > > > >>>>>>>>> Hi, Wenzhuo,
> > > > >>>>>>>>>
> > > > >>>>>>>>> I'm sorry, but I have more even harder questions than
> > > > >>>>>>>>> the previous
> > > > >>>> one.
> > > > >>>>>>>>> This questions are rather generic and mainly to ethdev
> > > maintainers.
> > > > >>>>>>>>>
> > > > >>>>>>>>> On 13.07.2018 05:42, Wenzhuo Lu wrote:
> > > > >>>>>>>>>> The device information cannot be gotten correctly
> > > > >>>>>>>>>> before the configuration is set. Because on some NICs
> > > > >>>>>>>>>> the information has dependence on the configuration.
> > > > >>>>>>>>> Thinking about it I have the following question. Is it
> > > > >>>>>>>>> valid behaviour of the dev_info if it changes after
> configuration?
> > > > >>>>>>>>> I always thought that the primary goal of the dev_info
> > > > >>>>>>>>> is to provide information to app about device
> > > > >>>>>>>>> capabilities to allow app configure device and queues
> > > > >>>>>>>>> correctly. Now we see the case when dev_info changes on
> > > > >>>>>>>>> configure. May be it is acceptable, but it is really
> > > > >>>>>>>>> suspicious. If we accept it, it should
> > > be documented.
> > > > >>>>>>>>> May be dev_info should be split into parts: part which
> > > > >>>>>>>>> is persistent and part which may depend on device
> configuration.
> > > > >>>>>>>> As I remember, the similar discussion has happened :)
> > > > >>>>>>>> I've raised the similar suggestion like this. But we
> > > > >>>>>>>> don’t make it
> > > happen.
> > > > >>>>>>>> The reason is, you see, this is the rte layer's behavior.
> > > > >>>>>>>> So the user doesn't have to know it. From APP's PoV, it
> > > > >>>>>>>> inputs the configuration, it calls this API
> "rte_eth_dev_configure".
> > > > >>>>>>>> It doesn't know  the configuration is copied before
> > > > >>>>>>>> getting the
> > > info or not.
> > > > >>>>>>>> So, to my opinion, we can still keep the behavior. We
> > > > >>>>>>>> only need to split it into parts when we do see the case
> > > > >>>>>>>> that cannot
> > > make it.
> > > > >>>>>>> Maybe I talked too much about the patch. Think about it again.
> > > > >>>>>>> Your comments is about how to use the APIs,
> > > > >>>>>>> rte_eth_dev_info_get,
> > > > >>>>>> rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get
> > > > >>>>>> is just to get the info. It can be called anywhere, before
> > > > >>>>>> configuration or after. It's reasonable the info changes
> > > > >>>>>> with the
> > > configuration changing.
> > > > >>>>>>> But we do have something missing, like,
> > > > >>>>>>> rte_eth_dev_capability_get which
> > > > >>>>>> should be stable. APP can use this API to get the necessary
> > > > >>>>>> info before configuration.
> > > > >>>>>>> A question, maybe a little divergent thinking, that APP
> > > > >>>>>>> should have some
> > > > >>>>>> intelligence to handle the capability automatically. So
> > > > >>>>>> getting the capability is not so good and effective, looks
> > > > >>>>>> like we still need the human
> > > > >>>> involvement.
> > > > >>>>>> Maybe that the reason currently we suppose APP know the
> > > > >>>>>> capability from the paper copies, examples...
> > > > >>>>>>
> > > > >>>>>> I am not sure to understand all the sentences.
> > > > >>>>>> But I agree that we should take a decision about the
> > > > >>>>>> stability of these
> > > > >>>> infos.
> > > > >>>>>> Either infos cannot change after probing, or we must
> > > > >>>>>> document that the app must request infos regularly (when?).
> > > > >>>>> Sorry, I missed this mail.
> > > > >>>>>
> > > > >>>>> I have the concern that different NICs have different behavior.
> > > > >>>>> One info
> > > > >>>> can be stable on a NIC but dynamic on another. Considering
> > > > >>>> this, we may better not splitting the rte_eth_dev_info_get to 2
> APIs.
> > > > >>>> And comparing with handling this in rte layer, maybe we can
> > > > >>>> let every
> > > NIC has its own decision.
> > > > >>>>> I have an idea. Maybe we can add a parameter for potential
> > > > >>>>> dynamic fields. Like, Changing uint16_t nb_rx_queues; to
> > > > >>>>> struct nb_rx_queues { uint16_t value; bool stable; }
> > > > >>>> May be it is just very bad example, but as I understand
> > > > >>>> nb_rx_queues is mainly required to configure the device properly.
> > > > >>>> Or should app configure, get new value, reconfigure again,
> > > > >>>> get new value and so on and stop when previous is equal to the
> new one.
> > > Yes, I dramatise and it sounds really bad.
> > > > >>>> In any case it would over-complicate interface and no single
> > > > >>>> app will do it correctly.
> > > > >>> I  think you're talking about max_rx_queues. APP can get that
> > > > >>> info
> > > before configuration. Then configure rx queue number which is not
> > > larger than it. That's enough.
> > > > >>> nb_rx_queues should be the number which is configured by APP
> > > > >>> and
> > > how many queues are actually used. To my opinion, it's mainly used
> > > by the GUI to show the value to human being.
> > > > >>>
> > > > >>> BTW, max_rx_queues could be an good example that shows that
> > > some parameters are stable on some NICs but not on other NICs.
> > > > >>> Take Intel NICs for example (I don’t familiar with others.),
> > > > >>> normally
> > > max_rx_queues is stable on PF. But on VF, as the max number is
> > > decided by PF, it could be dynamic. When VF starts, it can get an default
> value from PF.
> > > If it not enough, it can request a larger one from PF. If the number
> > > works, VF can get a new number.
> > > > >> "struct rte_eth_dev_info" is a little overloaded, it has:
> > > > >> - static info, like *device
> > > > >> - device limitations, max_*, *_lim
> > > > >> - device capabilities, *_capa
> > > > >> - suggested configurations, default_*conf
> > > > >> - device configuration, nb_[r/t]x_queues
> > > > >> - other, switch_info
> > > > >>
> > > > >> There is a concern that some values are dynamic, but this is
> > > > >> not new, for example nb_rx/tx_queues can be changed by
> > > > >> rte_eth_dev_rx/tx_queue_config() API and rte_eth_dev_info()
> > > > >> output
> > > will be changed.
> > > > >
> > > > > The example looks different to me. It is explicit changes
> > > > > directly requested by the application. So, it is not a surprise that it
> changes.
> > > > >
> > > > >> For this patch suggested configuration changes based on some
> > > > >> other config values looks ok as concept.
> > > > >> So I think we can say after every configuration related API dev
> > > > >> info can be changed.
> > > > >
> > > > > I think that saying that any configuration changes may result in
> > > > > any changes in dev_info is hardly helpful. I'd suggest to be more
> specific.
> > > > > Yes, it is harder and will have bugs, but at least it is helpful.
> > > >
> > > > Hi Andrew, Wenzhuo,
> > > >
> > > > Back to this patch, which fixes an actual defect,
> > > >
> > > > What do you think about:
> > > > 1- Keep existing patch but extend it as, save the original "dev->data"
> > > > and revert it back to this original data on all error path.
> > > > 2- Update rte_eth_dev_info() API document and say default
> > > > configuration can be changed based on other config fields. So this
> > > > reduces the scope of things can change in dev_info.
> > >
> > > I think we are doing too much juggling with data in ethdev layer.
> > > All these things should be the responsibility of the PMD.
> > > My radical proposal would be to remove rte_eth_dev_info and
> > > integrate all the data into rte_eth_dev_data.
> > >
> > Sorry for missing this discussion. It's a good discussion about how to
> optimize the rte_eth.
> > But I have to say that above discussion can reach a huge reconsitution of
> the rte_eth and impact every PMD. Is that fair?
> > This patch is only try to revert a bad commit as we already find bug. As I
> remember, at the beginning, Andrew said the discussion may not about the
> patch but generic. So could we just tell if this patch itself OK at first?  Thanks.
> 
> I have no strong opinion about your patch.
> Sorry, I am more interested by a possible future big clean-up.
> About a temporary fix, I am OK if Ferruh and Andrew are OK.
About this patch itself, is it OK? If so, would you like ack it? Thanks.

Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting

[Andrew Rybchenko]

On 10/22/18 3:13 PM, Thomas Monjalon wrote:
> 22/10/2018 14:01, Ferruh Yigit:
>> On 8/23/2018 9:58 AM, Andrew Rybchenko wrote:
>>> On 22.08.2018 19:55, Ferruh Yigit wrote:
>>>> On 8/14/2018 1:57 AM, Lu, Wenzhuo wrote:
>>>>> Hi Andrew,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
>>>>>> Sent: Monday, August 13, 2018 4:39 PM
>>>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Thomas Monjalon
>>>>>> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>>> Cc: dev@dpdk.org
>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>>>
>>>>>> On 13.08.2018 05:50, Lu, Wenzhuo wrote:
>>>>>>> Hi Thomas,
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>>>>>>>> Sent: Wednesday, August 1, 2018 11:37 PM
>>>>>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Andrew Rybchenko
>>>>>>>> <arybchenko@solarflare.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>>>>> Cc: dev@dpdk.org
>>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>>>>>
>>>>>>>> 16/07/2018 03:58, Lu, Wenzhuo:
>>>>>>>>> Hi Andrew,
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lu, Wenzhuo
>>>>>>>>>> Sent: Monday, July 16, 2018 9:08 AM
>>>>>>>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
>>>>>>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
>>>>>>>>>> <thomas@monjalon.net>
>>>>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>>>>>>>
>>>>>>>>>> Hi Andrew,
>>>>>>>>>>
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
>>>>>>>>>>> Sent: Friday, July 13, 2018 4:03 PM
>>>>>>>>>>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
>>>>>>>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
>>>>>>>>>>> <thomas@monjalon.net>
>>>>>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix device info getting
>>>>>>>>>>>
>>>>>>>>>>> Hi, Wenzhuo,
>>>>>>>>>>>
>>>>>>>>>>> I'm sorry, but I have more even harder questions than the previous
>>>>>> one.
>>>>>>>>>>> This questions are rather generic and mainly to ethdev maintainers.
>>>>>>>>>>>
>>>>>>>>>>> On 13.07.2018 05:42, Wenzhuo Lu wrote:
>>>>>>>>>>>> The device information cannot be gotten correctly before the
>>>>>>>>>>>> configuration is set. Because on some NICs the information has
>>>>>>>>>>>> dependence on the configuration.
>>>>>>>>>>> Thinking about it I have the following question. Is it valid
>>>>>>>>>>> behaviour of the dev_info if it changes after configuration?
>>>>>>>>>>> I always thought that the primary goal of the dev_info is to
>>>>>>>>>>> provide information to app about device capabilities to allow app
>>>>>>>>>>> configure device and queues correctly. Now we see the case when
>>>>>>>>>>> dev_info changes on configure. May be it is acceptable, but it is
>>>>>>>>>>> really suspicious. If we accept it, it should be documented.
>>>>>>>>>>> May be dev_info should be split into parts: part which is
>>>>>>>>>>> persistent and part which may depend on device configuration.
>>>>>>>>>> As I remember, the similar discussion has happened :) I've raised
>>>>>>>>>> the similar suggestion like this. But we don’t make it happen.
>>>>>>>>>> The reason is, you see, this is the rte layer's behavior. So the
>>>>>>>>>> user doesn't have to know it. From APP's PoV, it inputs the
>>>>>>>>>> configuration, it calls this API "rte_eth_dev_configure". It
>>>>>>>>>> doesn't know  the configuration is copied before getting the info or not.
>>>>>>>>>> So, to my opinion, we can still keep the behavior. We only need to
>>>>>>>>>> split it into parts when we do see the case that cannot make it.
>>>>>>>>> Maybe I talked too much about the patch. Think about it again. Your
>>>>>>>>> comments is about how to use the APIs, rte_eth_dev_info_get,
>>>>>>>> rte_eth_dev_configure. To my opinion, rte_eth_dev_info_get is just to
>>>>>>>> get the info. It can be called anywhere, before configuration or
>>>>>>>> after. It's reasonable the info changes with the configuration changing.
>>>>>>>>> But we do have something missing, like, rte_eth_dev_capability_get
>>>>>>>>> which
>>>>>>>> should be stable. APP can use this API to get the necessary info
>>>>>>>> before configuration.
>>>>>>>>> A question, maybe a little divergent thinking, that APP should have
>>>>>>>>> some
>>>>>>>> intelligence to handle the capability automatically. So getting the
>>>>>>>> capability is not so good and effective, looks like we still need the human
>>>>>> involvement.
>>>>>>>> Maybe that the reason currently we suppose APP know the capability
>>>>>>>> from the paper copies, examples...
>>>>>>>>
>>>>>>>> I am not sure to understand all the sentences.
>>>>>>>> But I agree that we should take a decision about the stability of these
>>>>>> infos.
>>>>>>>> Either infos cannot change after probing, or we must document that
>>>>>>>> the app must request infos regularly (when?).
>>>>>>> Sorry, I missed this mail.
>>>>>>>
>>>>>>> I have the concern that different NICs have different behavior. One info
>>>>>> can be stable on a NIC but dynamic on another. Considering this, we may
>>>>>> better not splitting the rte_eth_dev_info_get to 2 APIs. And comparing with
>>>>>> handling this in rte layer, maybe we can let every NIC has its own decision.
>>>>>>> I have an idea. Maybe we can add a parameter for potential dynamic
>>>>>>> fields. Like, Changing uint16_t nb_rx_queues; to struct nb_rx_queues {
>>>>>>> uint16_t value; bool stable; }
>>>>>> May be it is just very bad example, but as I understand nb_rx_queues is
>>>>>> mainly required to configure the device properly. Or should app configure,
>>>>>> get new value, reconfigure again, get new value and so on and stop when
>>>>>> previous is equal to the new one. Yes, I dramatise and it sounds really bad.
>>>>>> In any case it would over-complicate interface and no single app will do it
>>>>>> correctly.
>>>>> I  think you're talking about max_rx_queues. APP can get that info before configuration. Then configure rx queue number which is not larger than it. That's enough.
>>>>> nb_rx_queues should be the number which is configured by APP and how many queues are actually used. To my opinion, it's mainly used by the GUI to show the value to human being.
>>>>>
>>>>> BTW, max_rx_queues could be an good example that shows that some parameters are stable on some NICs but not on other NICs.
>>>>> Take Intel NICs for example (I don’t familiar with others.), normally max_rx_queues is stable on PF. But on VF, as the max number is decided by PF, it could be dynamic. When VF starts, it can get an default value from PF. If it not enough, it can request a larger one from PF. If the number works, VF can get a new number.
>>>> "struct rte_eth_dev_info" is a little overloaded, it has:
>>>> - static info, like *device
>>>> - device limitations, max_*, *_lim
>>>> - device capabilities, *_capa
>>>> - suggested configurations, default_*conf
>>>> - device configuration, nb_[r/t]x_queues
>>>> - other, switch_info
>>>>
>>>> There is a concern that some values are dynamic, but this is not new, for
>>>> example nb_rx/tx_queues can be changed by rte_eth_dev_rx/tx_queue_config() API
>>>> and rte_eth_dev_info() output will be changed.
>>> The example looks different to me. It is explicit changes directly
>>> requested by the application. So, it is not a surprise that it changes.
>>>
>>>> For this patch suggested configuration changes based on some other config values
>>>> looks ok as concept.
>>>> So I think we can say after every configuration related API dev info can be
>>>> changed.
>>> I think that saying that any configuration changes may result in any
>>> changes in dev_info is hardly helpful. I'd suggest to be more specific.
>>> Yes, it is harder and will have bugs, but at least it is helpful.
>> Hi Andrew, Wenzhuo,
>>
>> Back to this patch, which fixes an actual defect,
>>
>> What do you think about:
>> 1- Keep existing patch but extend it as, save the original "dev->data" and
>> revert it back to this original data on all error path.

I guess you mean dev->data->dev_conf here. If so, OK.

>> 2- Update rte_eth_dev_info() API document and say default configuration can be
>> changed based on other config fields. So this reduces the scope of things can
>> change in dev_info.

Yes, it looks like many items in rte_eth_conf may change defaults
(Rx/Tx modes, offloads, loopback mode, intr_conf). However, it should be
highlighted that it is device configuration (struct rte_eth_conf), not 
per-queue
configuration.

> I think we are doing too much juggling with data in ethdev layer.
> All these things should be the responsibility of the PMD.
> My radical proposal would be to remove rte_eth_dev_info and integrate
> all the data into rte_eth_dev_data.

I think it is a bad idea. It is too error-prone since it will require from
PMD to spread the logic and update rte_eth_dev_data in each place
which should change it. I think existing functional interface is the
right approach here. However, we cannot say that dev_info may
change after any configuration action - it could make application
impossible or very hard to configure the device properly (get dev_info,
find the best settings, try to configure starting from the most important,
rollback in the case of failure and retry other ways).

[dpdk-dev] [PATCH v3 1/2] ethdev: fix device info getting

[Wenzhuo Lu]

The device information cannot be gotten correctly before
the configuration is set. Because on some NICs the
information has dependence on the configuration.

Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 8eaa5fc..f181c21 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1102,6 +1102,17 @@ struct rte_eth_dev *
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
 
+	if (dev->data->dev_started) {
+		RTE_ETHDEV_LOG(ERR,
+			"Port %u must be stopped to allow configuration\n",
+			port_id);
+		return -EBUSY;
+	}
+
+	/* Copy the dev_conf parameter into the dev structure,
+	 * then get the info.
+	 */
+	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
 	rte_eth_dev_info_get(port_id, &dev_info);
 
 	/* If number of queues specified by application for both Rx and Tx is
@@ -1133,16 +1144,6 @@ struct rte_eth_dev *
 		return -EINVAL;
 	}
 
-	if (dev->data->dev_started) {
-		RTE_ETHDEV_LOG(ERR,
-			"Port %u must be stopped to allow configuration\n",
-			port_id);
-		return -EBUSY;
-	}
-
-	/* Copy the dev_conf parameter into the dev structure */
-	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
-
 	/*
 	 * Check that the numbers of RX and TX queues are not greater
 	 * than the maximum number of RX and TX queues supported by the
-- 
1.9.3

[dpdk-dev] [PATCH v3 2/2] ethdev: device configuration enhancement

[Wenzhuo Lu]

The new configuration is stored during the process.
But the process may fail. We better rolling the
configuration back as the new one doesn't take effect.

Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c | 59 ++++++++++++++++++++++++++++++------------
 1 file changed, 43 insertions(+), 16 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index f181c21..e2e7b04 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1092,8 +1092,10 @@ struct rte_eth_dev *
 {
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
+	struct rte_eth_conf orig_conf;
 	struct rte_eth_conf local_conf = *dev_conf;
 	int diag;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -1106,12 +1108,16 @@ struct rte_eth_dev *
 		RTE_ETHDEV_LOG(ERR,
 			"Port %u must be stopped to allow configuration\n",
 			port_id);
-		return -EBUSY;
+		ret = -EBUSY;
+		goto fail;
 	}
 
 	/* Copy the dev_conf parameter into the dev structure,
 	 * then get the info.
+	 * But restore the original configure first, as rollback is needed
+	 * when failure happens.
 	 */
+	memcpy(&orig_conf, &dev->data->dev_conf, sizeof(dev->data->dev_conf));
 	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
 	rte_eth_dev_info_get(port_id, &dev_info);
 
@@ -1134,14 +1140,16 @@ struct rte_eth_dev *
 		RTE_ETHDEV_LOG(ERR,
 			"Number of RX queues requested (%u) is greater than max supported(%d)\n",
 			nb_rx_q, RTE_MAX_QUEUES_PER_PORT);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	if (nb_tx_q > RTE_MAX_QUEUES_PER_PORT) {
 		RTE_ETHDEV_LOG(ERR,
 			"Number of TX queues requested (%u) is greater than max supported(%d)\n",
 			nb_tx_q, RTE_MAX_QUEUES_PER_PORT);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	/*
@@ -1152,13 +1160,15 @@ struct rte_eth_dev *
 	if (nb_rx_q > dev_info.max_rx_queues) {
 		RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%u nb_rx_queues=%u > %u\n",
 			port_id, nb_rx_q, dev_info.max_rx_queues);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	if (nb_tx_q > dev_info.max_tx_queues) {
 		RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%u nb_tx_queues=%u > %u\n",
 			port_id, nb_tx_q, dev_info.max_tx_queues);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	/* Check that the device supports requested interrupts */
@@ -1166,13 +1176,15 @@ struct rte_eth_dev *
 			(!(dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC))) {
 		RTE_ETHDEV_LOG(ERR, "Driver %s does not support lsc\n",
 			dev->device->driver->name);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 	if ((dev_conf->intr_conf.rmv == 1) &&
 			(!(dev->data->dev_flags & RTE_ETH_DEV_INTR_RMV))) {
 		RTE_ETHDEV_LOG(ERR, "Driver %s does not support rmv\n",
 			dev->device->driver->name);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	/*
@@ -1185,13 +1197,15 @@ struct rte_eth_dev *
 				"Ethdev port_id=%u max_rx_pkt_len %u > max valid value %u\n",
 				port_id, dev_conf->rxmode.max_rx_pkt_len,
 				dev_info.max_rx_pktlen);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto rollback;
 		} else if (dev_conf->rxmode.max_rx_pkt_len < ETHER_MIN_LEN) {
 			RTE_ETHDEV_LOG(ERR,
 				"Ethdev port_id=%u max_rx_pkt_len %u < min valid value %u\n",
 				port_id, dev_conf->rxmode.max_rx_pkt_len,
 				(unsigned)ETHER_MIN_LEN);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto rollback;
 		}
 	} else {
 		if (dev_conf->rxmode.max_rx_pkt_len < ETHER_MIN_LEN ||
@@ -1210,7 +1224,8 @@ struct rte_eth_dev *
 			port_id, local_conf.rxmode.offloads,
 			dev_info.rx_offload_capa,
 			__func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 	if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) !=
 	     local_conf.txmode.offloads) {
@@ -1220,7 +1235,8 @@ struct rte_eth_dev *
 			port_id, local_conf.txmode.offloads,
 			dev_info.tx_offload_capa,
 			__func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	/* Check that device supports requested rss hash functions. */
@@ -1231,7 +1247,8 @@ struct rte_eth_dev *
 			"Ethdev port_id=%u invalid rss_hf: 0x%"PRIx64", valid value: 0x%"PRIx64"\n",
 			port_id, dev_conf->rx_adv_conf.rss_conf.rss_hf,
 			dev_info.flow_type_rss_offloads);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	/*
@@ -1242,7 +1259,8 @@ struct rte_eth_dev *
 		RTE_ETHDEV_LOG(ERR,
 			"Port%u rte_eth_dev_rx_queue_config = %d\n",
 			port_id, diag);
-		return diag;
+		ret = diag;
+		goto rollback;
 	}
 
 	diag = rte_eth_dev_tx_queue_config(dev, nb_tx_q);
@@ -1251,7 +1269,8 @@ struct rte_eth_dev *
 			"Port%u rte_eth_dev_tx_queue_config = %d\n",
 			port_id, diag);
 		rte_eth_dev_rx_queue_config(dev, 0);
-		return diag;
+		ret = diag;
+		goto rollback;
 	}
 
 	diag = (*dev->dev_ops->dev_configure)(dev);
@@ -1260,7 +1279,8 @@ struct rte_eth_dev *
 			port_id, diag);
 		rte_eth_dev_rx_queue_config(dev, 0);
 		rte_eth_dev_tx_queue_config(dev, 0);
-		return eth_err(port_id, diag);
+		ret = eth_err(port_id, diag);
+		goto rollback;
 	}
 
 	/* Initialize Rx profiling if enabled at compilation time. */
@@ -1270,10 +1290,17 @@ struct rte_eth_dev *
 			port_id, diag);
 		rte_eth_dev_rx_queue_config(dev, 0);
 		rte_eth_dev_tx_queue_config(dev, 0);
-		return eth_err(port_id, diag);
+		ret = eth_err(port_id, diag);
+		goto rollback;
 	}
 
 	return 0;
+
+rollback:
+	memcpy(&dev->data->dev_conf, &orig_conf, sizeof(dev->data->dev_conf));
+
+fail:
+	return ret;
 }
 
 void
-- 
1.9.3

Re: [dpdk-dev] [PATCH v3 2/2] ethdev: device configuration enhancement

[Andrew Rybchenko]

On 11/8/18 5:09 AM, Wenzhuo Lu wrote:
> The new configuration is stored during the process.
> But the process may fail. We better rolling the
> configuration back as the new one doesn't take effect.
>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

I would say that the order is wrong. We should fix this bug first and
the changeset should have appropriate Fixes tags.
I think this bug is older and should be fixed first.
Then the second bug should be fixed without this one present.

Re: [dpdk-dev] [PATCH v3 2/2] ethdev: device configuration enhancement

[Ferruh Yigit]

On 11/8/2018 6:25 AM, Andrew Rybchenko wrote:
> On 11/8/18 5:09 AM, Wenzhuo Lu wrote:
>> The new configuration is stored during the process.
>> But the process may fail. We better rolling the
>> configuration back as the new one doesn't take effect.
>>
>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> 
> I would say that the order is wrong. We should fix this bug first and
> the changeset should have appropriate Fixes tags.
> I think this bug is older and should be fixed first.
> Then the second bug should be fixed without this one present.

Logically suggested order make sense I agree, but both patches are fixing defect
and order won't help backporting them [1], so no strong opinion about order.

Overall this patch should be converted into fix defect with proper Fixes tag
independent from order.

Wenzhuo, what do you think? I would like to get this one for rc3!


[1]
This is older defect but I believe can't be backported cleanly into older stable
trees because of "PMD-tuned Tx/Rx parameters" patches in the middle. Downside
having this first prevents other patch to backported to closer stable trees.

Also having this patch first will require additional return value update in some
checks (nb_tx_q && nb_rx_q checks) in next patch, so for separation fixes this
order is clearer.

Re: [dpdk-dev] [PATCH v3 2/2] ethdev: device configuration enhancement

[Lu, Wenzhuo]

Hi Ferruh,


> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Saturday, November 10, 2018 5:10 AM
> To: Andrew Rybchenko <arybchenko@solarflare.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] ethdev: device configuration
> enhancement
> 
> On 11/8/2018 6:25 AM, Andrew Rybchenko wrote:
> > On 11/8/18 5:09 AM, Wenzhuo Lu wrote:
> >> The new configuration is stored during the process.
> >> But the process may fail. We better rolling the configuration back as
> >> the new one doesn't take effect.
> >>
> >> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >
> > I would say that the order is wrong. We should fix this bug first and
> > the changeset should have appropriate Fixes tags.
> > I think this bug is older and should be fixed first.
> > Then the second bug should be fixed without this one present.
> 
> Logically suggested order make sense I agree, but both patches are fixing
> defect and order won't help backporting them [1], so no strong opinion
> about order.
> 
> Overall this patch should be converted into fix defect with proper Fixes tag
> independent from order.
> 
> Wenzhuo, what do you think? I would like to get this one for rc3!
> 
> 
> [1]
> This is older defect but I believe can't be backported cleanly into older stable
> trees because of "PMD-tuned Tx/Rx parameters" patches in the middle.
> Downside having this first prevents other patch to backported to closer
> stable trees.
> 
> Also having this patch first will require additional return value update in
> some checks (nb_tx_q && nb_rx_q checks) in next patch, so for separation
> fixes this order is clearer.
Yes, to my opinion, these 2 are separate patches. Actually there's no order between them. I put them together only because we have had a mixed discussion.
I didn't put a fix prefix because it's hard to add a fix tag for it. We know it has the problem from the beginning, so after some changes this patch cannot  be backported.

Re: [dpdk-dev] [PATCH v3 2/2] ethdev: device configuration enhancement

[Ferruh Yigit]

On 11/13/2018 12:46 AM, Lu, Wenzhuo wrote:
> Hi Ferruh,
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Saturday, November 10, 2018 5:10 AM
>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v3 2/2] ethdev: device configuration
>> enhancement
>>
>> On 11/8/2018 6:25 AM, Andrew Rybchenko wrote:
>>> On 11/8/18 5:09 AM, Wenzhuo Lu wrote:
>>>> The new configuration is stored during the process.
>>>> But the process may fail. We better rolling the configuration back as
>>>> the new one doesn't take effect.
>>>>
>>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>>
>>> I would say that the order is wrong. We should fix this bug first and
>>> the changeset should have appropriate Fixes tags.
>>> I think this bug is older and should be fixed first.
>>> Then the second bug should be fixed without this one present.
>>
>> Logically suggested order make sense I agree, but both patches are fixing
>> defect and order won't help backporting them [1], so no strong opinion
>> about order.
>>
>> Overall this patch should be converted into fix defect with proper Fixes tag
>> independent from order.
>>
>> Wenzhuo, what do you think? I would like to get this one for rc3!
>>
>>
>> [1]
>> This is older defect but I believe can't be backported cleanly into older stable
>> trees because of "PMD-tuned Tx/Rx parameters" patches in the middle.
>> Downside having this first prevents other patch to backported to closer
>> stable trees.
>>
>> Also having this patch first will require additional return value update in
>> some checks (nb_tx_q && nb_rx_q checks) in next patch, so for separation
>> fixes this order is clearer.
> Yes, to my opinion, these 2 are separate patches. Actually there's no order between them. I put them together only because we have had a mixed discussion.

Yes they are not depends each other. Thinking twice adding first patch will
leave the code in a state more open the defect fixed in second patch. But by
fixing defect first second fix can be applied without having that open.

I will send a new version of the set.

> I didn't put a fix prefix because it's hard to add a fix tag for it. We know it has the problem from the beginning, so after some changes this patch cannot  be backported.
> 

Re: [dpdk-dev] [PATCH v3 2/2] ethdev: device configuration enhancement

[Lu, Wenzhuo]

Hi Ferruh,

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, November 13, 2018 5:41 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] ethdev: device configuration
> enhancement
> 
> On 11/13/2018 12:46 AM, Lu, Wenzhuo wrote:
> > Hi Ferruh,
> >
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Saturday, November 10, 2018 5:10 AM
> >> To: Andrew Rybchenko <arybchenko@solarflare.com>; Lu, Wenzhuo
> >> <wenzhuo.lu@intel.com>; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v3 2/2] ethdev: device configuration
> >> enhancement
> >>
> >> On 11/8/2018 6:25 AM, Andrew Rybchenko wrote:
> >>> On 11/8/18 5:09 AM, Wenzhuo Lu wrote:
> >>>> The new configuration is stored during the process.
> >>>> But the process may fail. We better rolling the configuration back
> >>>> as the new one doesn't take effect.
> >>>>
> >>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >>>
> >>> I would say that the order is wrong. We should fix this bug first
> >>> and the changeset should have appropriate Fixes tags.
> >>> I think this bug is older and should be fixed first.
> >>> Then the second bug should be fixed without this one present.
> >>
> >> Logically suggested order make sense I agree, but both patches are
> >> fixing defect and order won't help backporting them [1], so no strong
> >> opinion about order.
> >>
> >> Overall this patch should be converted into fix defect with proper
> >> Fixes tag independent from order.
> >>
> >> Wenzhuo, what do you think? I would like to get this one for rc3!
> >>
> >>
> >> [1]
> >> This is older defect but I believe can't be backported cleanly into
> >> older stable trees because of "PMD-tuned Tx/Rx parameters" patches in
> the middle.
> >> Downside having this first prevents other patch to backported to
> >> closer stable trees.
> >>
> >> Also having this patch first will require additional return value
> >> update in some checks (nb_tx_q && nb_rx_q checks) in next patch, so
> >> for separation fixes this order is clearer.
> > Yes, to my opinion, these 2 are separate patches. Actually there's no order
> between them. I put them together only because we have had a mixed
> discussion.
> 
> Yes they are not depends each other. Thinking twice adding first patch will
> leave the code in a state more open the defect fixed in second patch. But by
> fixing defect first second fix can be applied without having that open.
> 
> I will send a new version of the set.
Thanks a lot! Very appreciate for your help!

> 
> > I didn't put a fix prefix because it's hard to add a fix tag for it. We know it
> has the problem from the beginning, so after some changes this patch
> cannot  be backported.
> >

[dpdk-dev] [PATCH v4 1/3] ethdev: fix invalid device configuration after failure

[Ferruh Yigit]

From: Wenzhuo Lu <wenzhuo.lu@intel.com>

The new configuration is stored during the rte_eth_dev_configure() API
but the API may fail. After failure stored configuration will be
invalid since it is not fully applied to the device.

We better roll the configuration back after failure.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.c | 49 +++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 13 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 8eaa5fcc7..04dff1f5e 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1092,8 +1092,10 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 {
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
+	struct rte_eth_conf orig_conf;
 	struct rte_eth_conf local_conf = *dev_conf;
 	int diag;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -1140,6 +1142,9 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		return -EBUSY;
 	}
 
+	 /* Store original config, as rollback required on failure */
+	memcpy(&orig_conf, &dev->data->dev_conf, sizeof(dev->data->dev_conf));
+
 	/* Copy the dev_conf parameter into the dev structure */
 	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
 
@@ -1151,13 +1156,15 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	if (nb_rx_q > dev_info.max_rx_queues) {
 		RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%u nb_rx_queues=%u > %u\n",
 			port_id, nb_rx_q, dev_info.max_rx_queues);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	if (nb_tx_q > dev_info.max_tx_queues) {
 		RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%u nb_tx_queues=%u > %u\n",
 			port_id, nb_tx_q, dev_info.max_tx_queues);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	/* Check that the device supports requested interrupts */
@@ -1165,13 +1172,15 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			(!(dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC))) {
 		RTE_ETHDEV_LOG(ERR, "Driver %s does not support lsc\n",
 			dev->device->driver->name);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 	if ((dev_conf->intr_conf.rmv == 1) &&
 			(!(dev->data->dev_flags & RTE_ETH_DEV_INTR_RMV))) {
 		RTE_ETHDEV_LOG(ERR, "Driver %s does not support rmv\n",
 			dev->device->driver->name);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	/*
@@ -1184,13 +1193,15 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 				"Ethdev port_id=%u max_rx_pkt_len %u > max valid value %u\n",
 				port_id, dev_conf->rxmode.max_rx_pkt_len,
 				dev_info.max_rx_pktlen);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto rollback;
 		} else if (dev_conf->rxmode.max_rx_pkt_len < ETHER_MIN_LEN) {
 			RTE_ETHDEV_LOG(ERR,
 				"Ethdev port_id=%u max_rx_pkt_len %u < min valid value %u\n",
 				port_id, dev_conf->rxmode.max_rx_pkt_len,
 				(unsigned)ETHER_MIN_LEN);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto rollback;
 		}
 	} else {
 		if (dev_conf->rxmode.max_rx_pkt_len < ETHER_MIN_LEN ||
@@ -1209,7 +1220,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			port_id, local_conf.rxmode.offloads,
 			dev_info.rx_offload_capa,
 			__func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 	if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) !=
 	     local_conf.txmode.offloads) {
@@ -1219,7 +1231,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			port_id, local_conf.txmode.offloads,
 			dev_info.tx_offload_capa,
 			__func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	/* Check that device supports requested rss hash functions. */
@@ -1230,7 +1243,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			"Ethdev port_id=%u invalid rss_hf: 0x%"PRIx64", valid value: 0x%"PRIx64"\n",
 			port_id, dev_conf->rx_adv_conf.rss_conf.rss_hf,
 			dev_info.flow_type_rss_offloads);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	/*
@@ -1241,7 +1255,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		RTE_ETHDEV_LOG(ERR,
 			"Port%u rte_eth_dev_rx_queue_config = %d\n",
 			port_id, diag);
-		return diag;
+		ret = diag;
+		goto rollback;
 	}
 
 	diag = rte_eth_dev_tx_queue_config(dev, nb_tx_q);
@@ -1250,7 +1265,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			"Port%u rte_eth_dev_tx_queue_config = %d\n",
 			port_id, diag);
 		rte_eth_dev_rx_queue_config(dev, 0);
-		return diag;
+		ret = diag;
+		goto rollback;
 	}
 
 	diag = (*dev->dev_ops->dev_configure)(dev);
@@ -1259,7 +1275,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			port_id, diag);
 		rte_eth_dev_rx_queue_config(dev, 0);
 		rte_eth_dev_tx_queue_config(dev, 0);
-		return eth_err(port_id, diag);
+		ret = eth_err(port_id, diag);
+		goto rollback;
 	}
 
 	/* Initialize Rx profiling if enabled at compilation time. */
@@ -1269,10 +1286,16 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			port_id, diag);
 		rte_eth_dev_rx_queue_config(dev, 0);
 		rte_eth_dev_tx_queue_config(dev, 0);
-		return eth_err(port_id, diag);
+		ret = eth_err(port_id, diag);
+		goto rollback;
 	}
 
 	return 0;
+
+rollback:
+	memcpy(&dev->data->dev_conf, &orig_conf, sizeof(dev->data->dev_conf));
+
+	return ret;
 }
 
 void
-- 
2.17.2

[dpdk-dev] [PATCH v4 2/3] ethdev: fix device info getting

[Ferruh Yigit]

From: Wenzhuo Lu <wenzhuo.lu@intel.com>

The device information cannot be gotten correctly before
the configuration is set. Because on some NICs the
information has dependence on the configuration.

Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
Cc: stable@dpdk.org

Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.c | 35 +++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 04dff1f5e..0f01138ea 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1104,6 +1104,22 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
 
+	if (dev->data->dev_started) {
+		RTE_ETHDEV_LOG(ERR,
+			"Port %u must be stopped to allow configuration\n",
+			port_id);
+		return -EBUSY;
+	}
+
+	 /* Store original config, as rollback required on failure */
+	memcpy(&orig_conf, &dev->data->dev_conf, sizeof(dev->data->dev_conf));
+
+	/*
+	 * Copy the dev_conf parameter into the dev structure.
+	 * rte_eth_dev_info_get() requires dev_conf, copy it before dev_info get
+	 */
+	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
+
 	rte_eth_dev_info_get(port_id, &dev_info);
 
 	/* If number of queues specified by application for both Rx and Tx is
@@ -1125,29 +1141,18 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		RTE_ETHDEV_LOG(ERR,
 			"Number of RX queues requested (%u) is greater than max supported(%d)\n",
 			nb_rx_q, RTE_MAX_QUEUES_PER_PORT);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
 	if (nb_tx_q > RTE_MAX_QUEUES_PER_PORT) {
 		RTE_ETHDEV_LOG(ERR,
 			"Number of TX queues requested (%u) is greater than max supported(%d)\n",
 			nb_tx_q, RTE_MAX_QUEUES_PER_PORT);
-		return -EINVAL;
-	}
-
-	if (dev->data->dev_started) {
-		RTE_ETHDEV_LOG(ERR,
-			"Port %u must be stopped to allow configuration\n",
-			port_id);
-		return -EBUSY;
+		ret = -EINVAL;
+		goto rollback;
 	}
 
-	 /* Store original config, as rollback required on failure */
-	memcpy(&orig_conf, &dev->data->dev_conf, sizeof(dev->data->dev_conf));
-
-	/* Copy the dev_conf parameter into the dev structure */
-	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
-
 	/*
 	 * Check that the numbers of RX and TX queues are not greater
 	 * than the maximum number of RX and TX queues supported by the
-- 
2.17.2

Re: [dpdk-dev] [PATCH v4 2/3] ethdev: fix device info getting

[Andrew Rybchenko]

On 11/13/18 2:12 PM, Ferruh Yigit wrote:
> From: Wenzhuo Lu <wenzhuo.lu@intel.com>
>
> The device information cannot be gotten correctly before
> the configuration is set. Because on some NICs the
> information has dependence on the configuration.
>
> Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
> Cc: stable@dpdk.org
>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

[dpdk-dev] [PATCH v4 3/3] ethdev: eliminate interim variable

[Ferruh Yigit]

`local_conf` variable was needed for offload conversions but no more
required. No functional difference, only interim variable eliminated.

Fixes: ab3ce1e0c193 ("ethdev: remove old offload API")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Andrew Rybchenko <arybchenko@solarflare.com>
Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 0f01138ea..5f858174b 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1093,7 +1093,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
 	struct rte_eth_conf orig_conf;
-	struct rte_eth_conf local_conf = *dev_conf;
 	int diag;
 	int ret;
 
@@ -1118,7 +1117,7 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	 * Copy the dev_conf parameter into the dev structure.
 	 * rte_eth_dev_info_get() requires dev_conf, copy it before dev_info get
 	 */
-	memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
+	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
 
 	rte_eth_dev_info_get(port_id, &dev_info);
 
@@ -1192,7 +1191,7 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	 * If jumbo frames are enabled, check that the maximum RX packet
 	 * length is supported by the configured device.
 	 */
-	if (local_conf.rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {
+	if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {
 		if (dev_conf->rxmode.max_rx_pkt_len > dev_info.max_rx_pktlen) {
 			RTE_ETHDEV_LOG(ERR,
 				"Ethdev port_id=%u max_rx_pkt_len %u > max valid value %u\n",
@@ -1217,23 +1216,23 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	}
 
 	/* Any requested offloading must be within its device capabilities */
-	if ((local_conf.rxmode.offloads & dev_info.rx_offload_capa) !=
-	     local_conf.rxmode.offloads) {
+	if ((dev_conf->rxmode.offloads & dev_info.rx_offload_capa) !=
+	     dev_conf->rxmode.offloads) {
 		RTE_ETHDEV_LOG(ERR,
 			"Ethdev port_id=%u requested Rx offloads 0x%"PRIx64" doesn't match Rx offloads "
 			"capabilities 0x%"PRIx64" in %s()\n",
-			port_id, local_conf.rxmode.offloads,
+			port_id, dev_conf->rxmode.offloads,
 			dev_info.rx_offload_capa,
 			__func__);
 		ret = -EINVAL;
 		goto rollback;
 	}
-	if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) !=
-	     local_conf.txmode.offloads) {
+	if ((dev_conf->txmode.offloads & dev_info.tx_offload_capa) !=
+	     dev_conf->txmode.offloads) {
 		RTE_ETHDEV_LOG(ERR,
 			"Ethdev port_id=%u requested Tx offloads 0x%"PRIx64" doesn't match Tx offloads "
 			"capabilities 0x%"PRIx64" in %s()\n",
-			port_id, local_conf.txmode.offloads,
+			port_id, dev_conf->txmode.offloads,
 			dev_info.tx_offload_capa,
 			__func__);
 		ret = -EINVAL;
-- 
2.17.2

Re: [dpdk-dev] [PATCH v4 3/3] ethdev: eliminate interim variable

[Andrew Rybchenko]

On 11/13/18 2:12 PM, Ferruh Yigit wrote:
> `local_conf` variable was needed for offload conversions but no more
> required. No functional difference, only interim variable eliminated.
>
> Fixes: ab3ce1e0c193 ("ethdev: remove old offload API")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

I guess it will be easier to backport the fix if it is the first fix in 
the series.

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [dpdk-dev] [PATCH v4 3/3] ethdev: eliminate interim variable

[Ferruh Yigit]

On 11/13/2018 11:22 AM, Andrew Rybchenko wrote:
> On 11/13/18 2:12 PM, Ferruh Yigit wrote:
>> `local_conf` variable was needed for offload conversions but no more
>> required. No functional difference, only interim variable eliminated.
>>
>> Fixes: ab3ce1e0c193 ("ethdev: remove old offload API")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> I guess it will be easier to backport the fix if it is the first fix in the series.

I thought this is the less important one and that is why put it as last. Didn't
want this patch cause any conflict while getting other fixes.

Which patch do you mention to be easier to backport if this is the first, this
one or others?

> 
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 

Re: [dpdk-dev] [PATCH v4 3/3] ethdev: eliminate interim variable

[Andrew Rybchenko]

On 11/13/18 2:51 PM, Ferruh Yigit wrote:
> On 11/13/2018 11:22 AM, Andrew Rybchenko wrote:
>> On 11/13/18 2:12 PM, Ferruh Yigit wrote:
>>> `local_conf` variable was needed for offload conversions but no more
>>> required. No functional difference, only interim variable eliminated.
>>>
>>> Fixes: ab3ce1e0c193 ("ethdev: remove old offload API")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> I guess it will be easier to backport the fix if it is the first fix in the series.
> I thought this is the less important one and that is why put it as last. Didn't
> want this patch cause any conflict while getting other fixes.
>
> Which patch do you mention to be easier to backport if this is the first, this
> one or others?

This one. It touches lines which are moved in the previous patch.
Not really that important.

>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>

Re: [dpdk-dev] [PATCH v4 1/3] ethdev: fix invalid device configuration after failure

[Andrew Rybchenko]

On 11/13/18 2:12 PM, Ferruh Yigit wrote:
> From: Wenzhuo Lu <wenzhuo.lu@intel.com>
>
> The new configuration is stored during the rte_eth_dev_configure() API
> but the API may fail. After failure stored configuration will be
> invalid since it is not fully applied to the device.
>
> We better roll the configuration back after failure.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [dpdk-dev] [PATCH v4 1/3] ethdev: fix invalid device configuration after failure

[Ferruh Yigit]

On 11/13/2018 11:19 AM, Andrew Rybchenko wrote:
> On 11/13/18 2:12 PM, Ferruh Yigit wrote:
>> From: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>
>> The new configuration is stored during the rte_eth_dev_configure() API
>> but the API may fail. After failure stored configuration will be
>> invalid since it is not fully applied to the device.
>>
>> We better roll the configuration back after failure.
>>
>> Fixes: af75078fece3 ("first public release")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 

Series applied to dpdk-next-net/master, thanks.