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

[Shahaf Shuler <shahafs@mellanox.com>]

Wednesday, September 13, 2017 11:13 AM, Andrew Rybchenko:
On 09/13/2017 09:37 AM, Shahaf Shuler wrote:

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

OK.

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

I will add documentation. Offloads can be reported in only one cap – either it is per-port or per-queue.


>If so, could you explain why to enable it should be specified in
>both places.

It is set also in the queue setup to emphasize the queue also have this offload. Logically it can be avoided, however I thought it is good to have, to make it explicit to application and PMDs.

How should be treated configuration when the offload is
>enabled on port, but disabled on queue level.

In that case the queue setup should return with error. As the application tries do a mixed configuration for per-port offload.



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

I think it will complex the documentation. The old API is obsoleted. If PMD developer thinks on how to implement a new feature, and read this doc,  it should implement according to the new API.


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

Right. This can be added on a different series.




[snip]