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]
