Re: [dpdk-dev] [PATCH v7 0/5] runtime queue setup

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 4/22/2018 12:58 PM, Qi Zhang wrote:
> From: Qi Zhang <Qi.z.zhang@intel.com>
> 
> v7:
> - update default.ini and i40e.ini.
> - rename runtime_queue_setup_capa to dev_capa for generic. 
> - testpmd queue setup command be moved to "ports" command group.
> - remove ring_size and offload from queue setup command in testpmd.
> - enable per queue config in testpmd.
> - enable queue ring size configure command in testpmd.
> - fix couple typo.
> 
> TODO:
>   queue offload config commmand is not implemented yet, but per queue 
>   configure data structure is already supported in PATCH 3
> 
> v6:
> - fix tx queue state check in rte_eth_rx_queue_setup
> - fix error message in testpmd.
> 
> v5:
> - fix first tx queue check in i40e.
> 
> v4:
> - fix i40e rx/tx funciton conflict handle.
> - no need conflict check for first rx/tx queue at runtime setup.
> - fix missing offload paramter in testpmd cmdline.
> 
> v3:
> - not overload deferred start.
> - rename deferred setup to runtime setup.
> - remove unecessary testpmd parameters (patch 2/4 of v2)
> - add offload support to testpmd queue setup command line
> - i40e fix: return fail when required rx/tx function conflict with
>   exist setup.
> 
> v2:
> - enhance comment in rte_ethdev.h
> 
> According to exist implementation,rte_eth_[rx|tx]_queue_setup will
> always return fail if device is already started(rte_eth_dev_start).
> 
> This can't satisfied the usage when application want to deferred setup
> part of the queues while keep traffic running on those queues already
> be setup.
> 
> example:
> rte_eth_dev_config(nb_rxq = 2, nb_txq =2)
> rte_eth_rx_queue_setup(idx = 0 ...)
> rte_eth_rx_queue_setup(idx = 0 ...)
> rte_eth_dev_start(...) /* [rx|tx]_burst is ready to start on queue 0 */
> rte_eth_rx_queue_setup(idx=1 ...) /* fail*/
> 
> Basically this is not a general hardware limitation, because for NIC
> like i40e, ixgbe, it is not necessary to stop the whole device before
> configure a fresh queue or reconfigure an exist queue with no traffic
> on it.
> 
> The patch let etherdev driver expose the capability flag through
> rte_eth_dev_info_get when it support deferred queue configuraiton,
> then base on these flag, rte_eth_[rx|tx]_queue_setup could decide
> continue to setup the queue or just return fail when device already
> started.
> 
> 
> Qi Zhang (1):
>   net/i40e: enable runtime queue setup
> 
> qi Zhang (4):
>   ethdev: support runtime queue setup
>   app/testpmd: add command for queue setup
>   app/testpmd: enable per queue configure
>   app/testpmd: enable queue ring size configure

Overall looks good to me. There are a few minor comments/questions on individual
patches.

For series,
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>


Please feel free to keep the ack for next version of the set based on comments
in patches.