[dpdk-dev,v6,1/3] ether: support runtime queue setup

  The patch let etherdev driver expose the capability flag through
rte_eth_dev_info_get when it support runtime queue configuraiton,
then base on the flag rte_eth_[rx|tx]_queue_setup could decide
continue to setup the queue or just return fail when device already
started.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
v6:
- fix tx queue state check in rte_eth_tx_queue_setup 

 doc/guides/nics/features.rst  |  8 ++++++++
 lib/librte_ether/rte_ethdev.c | 30 ++++++++++++++++++------------
 lib/librte_ether/rte_ethdev.h |  7 +++++++
 3 files changed, 33 insertions(+), 12 deletions(-)
  

Hi,

Please replace ether and etherdev by ethdev (in title and text).

08/04/2018 04:42, Qi Zhang:
> The patch let etherdev driver expose the capability flag through
> rte_eth_dev_info_get when it support runtime queue configuraiton,

typo: configuration

> then base on the flag rte_eth_[rx|tx]_queue_setup could decide
> continue to setup the queue or just return fail when device already
> started.

Generally speaking, it is easier to read when broke in several sentences,
and starting with the problem statement.
Example:
"
It is not possible to setup a queue when the port is started
because of a check in ethdev layer.
New capability flags are added in order to relax this check
for devices which support queue setup in runtime.
The functions rte_eth_[rx|tx]_queue_setup will raise an error only
if the port is started and runtime setup of queue is not supported.
"
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -981,6 +981,11 @@ struct rte_eth_conf {
>   */
>  #define DEV_TX_OFFLOAD_SECURITY         0x00020000
>  
> +#define DEV_RUNTIME_RX_QUEUE_SETUP 0x00000001
> +/**< Deferred setup rx queue */
> +#define DEV_RUNTIME_TX_QUEUE_SETUP 0x00000002
> +/**< Deferred setup tx queue */

Please use RTE_ETH_ prefix.

>  /*
>   * If new Tx offload capabilities are defined, they also must be
>   * mentioned in rte_tx_offload_names in rte_ethdev.c file.
> @@ -1029,6 +1034,8 @@ struct rte_eth_dev_info {
>  	/** Configured number of rx/tx queues */
>  	uint16_t nb_rx_queues; /**< Number of RX queues. */
>  	uint16_t nb_tx_queues; /**< Number of TX queues. */
> +	uint64_t runtime_queue_setup_capa;
> +	/**< queues can be setup after dev_start (DEV_DEFERRED_). */

Why using uint64_t for that?
Maybe these flags can find another place, less specific.
What about a field for all setup capabilities? setup_capa?
  

On 4/10/2018 2:59 PM, Thomas Monjalon wrote:
> Hi,
> 
> Please replace ether and etherdev by ethdev (in title and text).
> 
> 08/04/2018 04:42, Qi Zhang:
>> The patch let etherdev driver expose the capability flag through
>> rte_eth_dev_info_get when it support runtime queue configuraiton,
> 
> typo: configuration
> 
>> then base on the flag rte_eth_[rx|tx]_queue_setup could decide
>> continue to setup the queue or just return fail when device already
>> started.
> 
> Generally speaking, it is easier to read when broke in several sentences,
> and starting with the problem statement.
> Example:
> "
> It is not possible to setup a queue when the port is started
> because of a check in ethdev layer.
> New capability flags are added in order to relax this check
> for devices which support queue setup in runtime.
> The functions rte_eth_[rx|tx]_queue_setup will raise an error only
> if the port is started and runtime setup of queue is not supported.
> "
>>
>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>> ---
>> --- a/lib/librte_ether/rte_ethdev.h
>> +++ b/lib/librte_ether/rte_ethdev.h
>> @@ -981,6 +981,11 @@ struct rte_eth_conf {
>>   */
>>  #define DEV_TX_OFFLOAD_SECURITY         0x00020000
>>  
>> +#define DEV_RUNTIME_RX_QUEUE_SETUP 0x00000001
>> +/**< Deferred setup rx queue */
>> +#define DEV_RUNTIME_TX_QUEUE_SETUP 0x00000002
>> +/**< Deferred setup tx queue */
> 
> Please use RTE_ETH_ prefix.
> 
>>  /*
>>   * If new Tx offload capabilities are defined, they also must be
>>   * mentioned in rte_tx_offload_names in rte_ethdev.c file.
>> @@ -1029,6 +1034,8 @@ struct rte_eth_dev_info {
>>  	/** Configured number of rx/tx queues */
>>  	uint16_t nb_rx_queues; /**< Number of RX queues. */
>>  	uint16_t nb_tx_queues; /**< Number of TX queues. */
>> +	uint64_t runtime_queue_setup_capa;
>> +	/**< queues can be setup after dev_start (DEV_DEFERRED_). */
> 
> Why using uint64_t for that?
> Maybe these flags can find another place, less specific.
> What about a field for all setup capabilities? setup_capa?

I was about to make similar comment, why not start a more generic capabilities
variable [1].
And make flag values more generic like: "DEV_CAPA_RUNTIME_RX_QUEUE_SETUP" etc ..


[1]
Previously a few times mentioned that there is no way for application to get
device capabilities dynamically, that is true, with offload capabilities flag
this has been solved for offloading but still remaining as a generic issue.
  

On 4/8/2018 3:42 AM, Qi Zhang wrote:
> The patch let etherdev driver expose the capability flag through
> rte_eth_dev_info_get when it support runtime queue configuraiton,
> then base on the flag rte_eth_[rx|tx]_queue_setup could decide
> continue to setup the queue or just return fail when device already
> started.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> v6:
> - fix tx queue state check in rte_eth_tx_queue_setup 
> 
>  doc/guides/nics/features.rst  |  8 ++++++++
>  lib/librte_ether/rte_ethdev.c | 30 ++++++++++++++++++------------
>  lib/librte_ether/rte_ethdev.h |  7 +++++++
>  3 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> index 1b4fb979f..6983faa4e 100644
> --- a/doc/guides/nics/features.rst
> +++ b/doc/guides/nics/features.rst
> @@ -892,7 +892,15 @@ Documentation describes performance values.
>  
>  See ``dpdk.org/doc/perf/*``.
>  
> +.. _nic_features_queue_runtime_setup_capabilities:
>  
> +Queue runtime setup capabilities
> +---------------------------------
> +
> +Supports queue setup / release after device started.
> +
> +* **[provides] rte_eth_dev_info**: ``runtime_queue_config_capa:DEV_RUNTIME_RX_QUEUE_SETUP,DEV_RUNTIME_TX_QUEUE_SETUP``.
> +* **[related]  API**: ``rte_eth_dev_info_get()``.

New feature added, can you please add this into default.ini file, and is it
possible to shorten the feature name?
  

10/04/2018 15:59, Thomas Monjalon:
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > +#define DEV_RUNTIME_RX_QUEUE_SETUP 0x00000001
> > +/**< Deferred setup rx queue */
> > +#define DEV_RUNTIME_TX_QUEUE_SETUP 0x00000002
> > +/**< Deferred setup tx queue */
> 
> Please use RTE_ETH_ prefix.

Qi, you missed this comment.
It must be fixed by a new patch, please.

And the field must mention the related flags prefix.
  

Hi Thomas:

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, April 25, 2018 3:36 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Lu, Wenzhuo <wenzhuo.lu@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Doherty, Declan <declan.doherty@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v6 1/3] ether: support runtime queue setup
> 
> 10/04/2018 15:59, Thomas Monjalon:
> > > --- a/lib/librte_ether/rte_ethdev.h
> > > +++ b/lib/librte_ether/rte_ethdev.h
> > > +#define DEV_RUNTIME_RX_QUEUE_SETUP 0x00000001 /**< Deferred
> setup
> > > +rx queue */ #define DEV_RUNTIME_TX_QUEUE_SETUP 0x00000002
> /**<
> > > +Deferred setup tx queue */
> >
> > Please use RTE_ETH_ prefix.

Actually I saw all the offload flag is started with DEV_, so
Should I still need rename to RTE_ETH_DEV_CAPA_***? 

> 
> Qi, you missed this comment.
> It must be fixed by a new patch, please.
> 
> And the field must mention the related flags prefix.

OK I will add this. 

Regards
Qi
  

25/04/2018 07:33, Zhang, Qi Z:
> > 10/04/2018 15:59, Thomas Monjalon:
> > > > --- a/lib/librte_ether/rte_ethdev.h
> > > > +++ b/lib/librte_ether/rte_ethdev.h
> > > > +#define DEV_RUNTIME_RX_QUEUE_SETUP 0x00000001 /**< Deferred
> > setup
> > > > +rx queue */ #define DEV_RUNTIME_TX_QUEUE_SETUP 0x00000002
> > /**<
> > > > +Deferred setup tx queue */
> > >
> > > Please use RTE_ETH_ prefix.
> 
> Actually I saw all the offload flag is started with DEV_, so
> Should I still need rename to RTE_ETH_DEV_CAPA_***? 

Yes
The flags starting with DEV_ will be managed later because it is hard
to rename an existing flag. It is in my roadmap.
The idea is to not add new flags with wrong namespace prefix.