Re: [dpdk-dev] [PATCH v10] ethdev: new Rx/Tx offloads API

["Dai, Wei" <wei.dai@intel.com>]

Hi, Thomas
Thanks for your feedback and guidance.

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, May 10, 2018 10:36 AM
> To: Dai, Wei <wei.dai@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v10] ethdev: new Rx/Tx offloads API
> 
> Hi,
> 
> I am checking if this patch comply with goals discussed in the survey:
> 	http://dpdk.org/ml/archives/dev/2018-March/094459.html
> 
> - Allow "forgetting" port offloads in queue offloads setup.
> 
> - An offload enabled at port level, cannot be disabled at queue level.
> 
> - Every queue capabilities must be reported as port capabilities.
> 
> - A capability should be reported at queue level
>   only if it can be enabled on queue when it is disabled on port level.
> 
> I think some items must be updated in doxygen comments of rte_ethdev.h.
> Please could you try to do a v11 for doxygen? I will review it quickly.
> 
> Examples:
> 
> 	- in queue offloads:
> 		"No need to repeat flags already enabled at port level.
> 		 A flag enabled at port level, cannot be disabled at queue level."
> 
> 	- in port capabilities: "(include per-queue capabilities)"
> 
> More comments below, thanks.
> 
> 
> 10/05/2018 02:56, Wei Dai:
> > This patch check if a input requested offloading is valid or not.
> > Any reuqested offloading must be supported in the device capabilities.
> > Any offloading is disabled by default if it is not set in the
> > parameter dev_conf->[rt]xmode.offloads to rte_eth_dev_configure( ) and
> > [rt]x_conf->offloads to rte_eth_[rt]x_queue_setup( ).
> > If any offloading is enabled in rte_eth_dev_configure( ) by
> > application, it is enabled on all queues no matter whether it is
> > per-queue or per-port type and no matter whether it is set or cleared
> > in [rt]x_conf->offloads to rte_eth_[rt]x_queue_setup( ).
> > If a per-queue offloading hasn't be enabled in rte_eth_dev_configure(
> > ), it can be enabled or disabled for individual queue in
> > ret_eth_[rt]x_queue_setup( ).
> > A new added offloading is the one which hasn't been enabled in
> > rte_eth_dev_configure( ) and is reuqested to be enabled in
> > rte_eth_[rt]x_queue_setup( ), it must be per-queue type, otherwise
> > triger an error log.
> > The underlying PMD must be aware that the requested offloadings to PMD
> > specific queue_setup( ) function only carries those new added
> > offloadings of per-queue type.
> 
> Good summary.
> Please forget the whitespace inside the parens.
I will remove whitespaces inside the parens.
> 
> > This patch can make above such checking in a common way in rte_ethdev
> > layer to avoid same checking in underlying PMD.
> 
> Good
> 
> > --- a/doc/guides/prog_guide/poll_mode_drv.rst
> > +++ b/doc/guides/prog_guide/poll_mode_drv.rst
> > @@ -297,16 +297,30 @@ Per-Port and Per-Queue Offloads
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> >  In the DPDK offload API, offloads are divided into per-port and per-queue
> offloads.
> > +A per-queue offloading can be enabled on a queue and disabled on
> another queue at the same time.
> > +A pure per-port offloading can't be enabled on a queue and disabled on
> another queue at the same time.
> > +A pure per-port offloading must be enabled or disabled on all queues at
> the same time.
> > +A per-port offloading can be enabled or disabled on all queues at the
> same time.
> 
> What is the difference between pure per-port and per-port here?
> 
Can I add following words to explain more: 
A pure per-port offloads are those supported by device but not per-queue type.
A supported offload can be per-queue or pure per-port,  but can't be both
types at same device. 
Any offloadings are per-port type as it can be enabled or disabled on all queues
at the same time.

> > +It is certain that both per-queue and pure per-port offloading are per-port
> type.
> 
> I don't understand this sentence.
> 
Explained above, I just want to divide all offloading into two distinct types: 
Per-queue and pure per-port.
And per-queue + pure per-port = per-port.


> >  The different offloads capabilities can be queried using
> ``rte_eth_dev_info_get()``.
> > +The dev_info->[rt]x_queue_offload_capa returned from
> ``rte_eth_dev_info_get()`` includes all per-queue offloading capabilities.
> > +The dev_info->[rt]x_offload_capa returned from
> ``rte_eth_dev_info_get()`` includes all per-port and per-queue offloading
> capabilities.
> 
> Yes
> 
> > +Any requested offloading by application must be within the device
> capabilities.
> 
> Yes
> 
> > +Any offloading is disabled by default if it is not set in the
> > +parameter
> 
> Yes
> 
> > +dev_conf->[rt]xmode.offloads to ``rte_eth_dev_configure( )`` and
> > +[rt]x_conf->offloads to ``rte_eth_[rt]x_queue_setup( )``.
> > +If any offloading is enabled in ``rte_eth_dev_configure( )`` by
> > +application, it is enabled on all queues no matter whether it is
> > +per-queue or per-port type and no matter whether it is set or cleared
> > +in [rt]x_conf->offloads to ``rte_eth_[rt]x_queue_setup( )``.
> > +If a per-queue offloading hasn't been enabled in
> > +``rte_eth_dev_configure( )``, it can be enabled or disabled in
> ``rte_eth_[rt]x_queue_setup( )`` for individual queue.
> 
> Yes
> 
> > +A new added offloads in [rt]x_conf->offloads to
> > +``rte_eth_[rt]x_queue_setup( )`` input by application is the one
> > +which hasn't been enabled in ``rte_eth_dev_configure( )`` and is requested
> to be enabled in ``rte_eth_[rt]x_queue_setup( )``, it must be per-queue type,
> otherwise return error.
> 
> Yes
> 
> 
> > --- a/doc/guides/rel_notes/release_18_05.rst
> > +++ b/doc/guides/rel_notes/release_18_05.rst
> > +* **ethdev: changes to offload API**
> 
> No need of bold formatting of title in API changes.
> 
Will revise it as you guide.

> > +
> > +   A pure per-port offloading isn't requested to be repeated in
> [rt]x_conf->offloads to
> > +   ``rte_eth_[rt]x_queue_setup( )``. Now any offloading enabled in
> ``rte_eth_dev_configure( )``
> > +   can't be disabled by ``rte_eth_[rt]x_queue_setup( )``. Any new added
> offloading which has
> > +   not been enabled in ``rte_eth_dev_configure( )`` and is requested to
> be enabled in
> > +   ``rte_eth_[rt]x_queue_setup( )`` must be per-queue type, otherwise
> return error.
> 
> 
>