Re: [dpdk-dev] [PATCH v3 1/2] ethdev: introduce Rx queue offloads API

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 09/13/2017 09:37 AM, Shahaf Shuler wrote:
> Introduce a new API to configure Rx offloads.
>
> In the new API, offloads are divided into per-port and per-queue
> offloads. The PMD reports capability for each of them.
> Offloads are enabled using the existing DEV_RX_OFFLOAD_* flags.
> To enable per-port offload, the offload should be set on both device
> configuration and queue configuration. To enable per-queue offload, the
> offloads can be set only on queue configuration.
>
> Applications should set the ignore_offload_bitfield bit on rxmode
> structure in order to move to the new API.

I think it would be useful to have the description in the documentation.
It is really important topic on how per-port and per-queue offloads coexist
and rules should be 100% clear for PMD maintainers and application
developers.

Please, also highlight how per-port and per-queue capabilities should be
advertised. I mean if per-queue capability should be reported as per-port
as well. I'd say no to avoid duplication of per-queue capabilities in two
places. If so, could you explain why to enable it should be specified in
both places. How should be treated configuration when the offload is
enabled on port, but disabled on queue level.

> The old Rx offloads API is kept for the meanwhile, in order to enable a
> smooth transition for PMDs and application to the new API.
>
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
>   doc/guides/nics/features.rst  |  33 ++++----
>   lib/librte_ether/rte_ethdev.c | 156 +++++++++++++++++++++++++++++++++----
>   lib/librte_ether/rte_ethdev.h |  51 +++++++++++-
>   3 files changed, 210 insertions(+), 30 deletions(-)
>
> diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> index 37ffbc68c..4e68144ef 100644
> --- a/doc/guides/nics/features.rst
> +++ b/doc/guides/nics/features.rst
> @@ -179,7 +179,7 @@ Jumbo frame
>   
>   Supports Rx jumbo frames.
>   
> -* **[uses]    user config**: ``dev_conf.rxmode.jumbo_frame``,

May be it should be removed from documentation when it is removed from 
sources?
I have no strong opinion, but it would be more clear to find it in the 
documentation
with its status specified (obsolete)
The note is applicable to all similar cases below.

[snip]

> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 0adf3274a..ba7a2b2dc 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h

[snip]

> @@ -907,6 +934,18 @@ struct rte_eth_conf {
>   #define DEV_RX_OFFLOAD_QINQ_STRIP  0x00000020
>   #define DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM 0x00000040
>   #define DEV_RX_OFFLOAD_MACSEC_STRIP     0x00000080
> +#define DEV_RX_OFFLOAD_HEADER_SPLIT	0x00000100
> +#define DEV_RX_OFFLOAD_VLAN_FILTER	0x00000200
> +#define DEV_RX_OFFLOAD_VLAN_EXTEND	0x00000400
> +#define DEV_RX_OFFLOAD_JUMBO_FRAME	0x00000800
> +#define DEV_RX_OFFLOAD_CRC_STRIP	0x00001000
> +#define DEV_RX_OFFLOAD_SCATTER		0x00002000
> +#define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
> +				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> +				 DEV_RX_OFFLOAD_TCP_CKSUM)
> +#define DEV_RX_OFFLOAD_VLAN (DEV_RX_OFFLOAD_VLAN_STRIP | \
> +			     DEV_RX_OFFLOAD_VLAN_FILTER | \
> +			     DEV_RX_OFFLOAD_VLAN_EXTEND)

It is not directly related to the patch, but I'd like to highlight that 
Rx/Tx are asymmetric here
since SCTP is missing for Rx, but present for Tx.

[snip]