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

[Thomas Monjalon <thomas@monjalon.net>]

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.

> 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?

> +It is certain that both per-queue and pure per-port offloading are per-port type.

I don't understand this sentence.

>  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.

> +
> +   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.