Hi, Thomas Thanks for your feedback and guidance. > -----Original Message----- > From: Thomas Monjalon [mailto:tho...@monjalon.net] > Sent: Thursday, May 10, 2018 10:36 AM > To: Dai, Wei <wei....@intel.com> > Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yi...@intel.com>; Zhang, Qi Z > <qi.z.zh...@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. > > >
