[RFC] ethdev: add new offload flag to keep CRC
Checks
Commit Message
DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
default behavior in PMDs is to keep the CRC until this flag removed
Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
- Setting both KEEP_CRC & CRC_STRIP is INVALID
- Setting only CRC_STRIP PMD should strip the CRC
- Setting only KEEP_CRC PMD should keep the CRC
- Not setting both PMD should keep the CRC
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
doc/guides/rel_notes/deprecation.rst | 10 ----------
lib/librte_ethdev/rte_ethdev.h | 4 ++++
2 files changed, 4 insertions(+), 10 deletions(-)
Comments
On 06/09/2018 01:57 AM, Ferruh Yigit wrote:
> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
>
> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
> default behavior in PMDs is to keep the CRC until this flag removed
>
> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
> - Setting both KEEP_CRC & CRC_STRIP is INVALID
ethdev layer should enforce it.
> - Setting only CRC_STRIP PMD should strip the CRC
> - Setting only KEEP_CRC PMD should keep the CRC
> - Not setting both PMD should keep the CRC
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
- rte_rx_offload_names should be updated as well
- testpmd should be updated
I'm not sure that I understand the transition plan. I think PMDs which
support
KEEP_CRC should be updated by the patch to advertise the offload. Otherwise,
generic checks in ethdev will not allow to enable it. Other option is to
simply
drop it on ethdev layer (since basically it changes nothing for PMD now) and
encourage PMD maintainers to advertise and take it into account if the
feature
is supported (however, if the bit is dropped on ethdev layer, the code
would
be unused/dead regardless application request). Too complicated. I guess
there are not so many PMDs which support KEEP_CRC. Better to update them.
The patch should encourage applications which would like to keep CRC to
use the offload since the next release will drop CRC_STRIP and it will
change behaviour (no KEEP_CRC => strip it)
On 6/9/2018 11:11 AM, Andrew Rybchenko wrote:
> On 06/09/2018 01:57 AM, Ferruh Yigit wrote:
>> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
>>
>> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
>> default behavior in PMDs is to keep the CRC until this flag removed
>>
>> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
>> - Setting both KEEP_CRC & CRC_STRIP is INVALID
>
> ethdev layer should enforce it.
OK, will add this check.
>
>> - Setting only CRC_STRIP PMD should strip the CRC
>> - Setting only KEEP_CRC PMD should keep the CRC
>> - Not setting both PMD should keep the CRC
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>
> - rte_rx_offload_names should be updated as well
> - testpmd should be updated
>
> I'm not sure that I understand the transition plan. I think PMDs which support
> KEEP_CRC should be updated by the patch to advertise the offload. Otherwise,
> generic checks in ethdev will not allow to enable it.
Right, PMDs should be updated to advertise this offload, will do it.
> Other option is to simply
> drop it on ethdev layer (since basically it changes nothing for PMD now) and
> encourage PMD maintainers to advertise and take it into account if the feature
> is supported (however, if the bit is dropped on ethdev layer, the code would
> be unused/dead regardless application request). Too complicated. I guess
> there are not so many PMDs which support KEEP_CRC. Better to update them
> The patch should encourage applications which would like to keep CRC to
> use the offload since the next release will drop CRC_STRIP and it will
> change behaviour (no KEEP_CRC => strip it)
+1, will check apps.
On Fri, 8 Jun 2018 23:57:09 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
>
> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
> default behavior in PMDs is to keep the CRC until this flag removed
This won't work for virtual devices that never keep CRC.
Maybe wording should be changed.
On 6/11/2018 4:25 PM, Stephen Hemminger wrote:
> On Fri, 8 Jun 2018 23:57:09 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
>> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
>>
>> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
>> default behavior in PMDs is to keep the CRC until this flag removed
>
> This won't work for virtual devices that never keep CRC.
> Maybe wording should be changed.
Yes, it won't work for virtual devices, but it is already the case, no offload
flag means keep CRC.
This task is to replace that behavior gradually. What will be replaced is:
flag: DEV_RX_OFFLOAD_CRC_STRIP => DEV_RX_OFFLOAD_KEEP_CRC
default, no flag, behavior : KEEP_CRC => STRIP_CRC
But because of gradually switch, this release, 18.08, both flags will exits and
no flag default still will be KEEP_CRC
For virtual devices, for this release, since virtual PMDs don't announce any CRC
related offload capability, applications can't explicitly request to keep the CRC.
But no flag case (default keep CRC) will be wrong and ignored by virtual PMDs.
on 18.11 when DEV_RX_OFFLOAD_CRC_STRIP is removed and default behavior is STRIP
CRC, it will be correct for virtual device.
@@ -67,16 +67,6 @@ Deprecation Notices
- removal of ``txq_flags`` field from ``rte_eth_txconf`` struct.
- removal of the offloads bit-field from ``rte_eth_rxmode`` struct.
-* ethdev: A new offloading flag ``DEV_RX_OFFLOAD_KEEP_CRC`` will be added in v18.08,
- This will replace the usage of not setting ``DEV_RX_OFFLOAD_CRC_STRIP`` flag
- and will be implemented in PMDs accordingly.
- In v18.08 both ``DEV_RX_OFFLOAD_KEEP_CRC`` and ``DEV_RX_OFFLOAD_CRC_STRIP`` flags
- will be available, usage will be:
- ``CRC_STRIP``: Strip CRC from packet
- ``KEEP_CRC``: Keep CRC in packet
- Both ``CRC_STRIP`` & ``KEEP_CRC``: Invalid
- No flag: Keep CRC in packet
-
* ethdev: In v18.11 ``DEV_RX_OFFLOAD_CRC_STRIP`` offload flag will be removed, default
behavior without any flag will be changed to CRC strip.
To keep CRC ``DEV_RX_OFFLOAD_KEEP_CRC`` flag is required.
@@ -939,6 +939,10 @@ struct rte_eth_conf {
#define DEV_RX_OFFLOAD_SCATTER 0x00002000
#define DEV_RX_OFFLOAD_TIMESTAMP 0x00004000
#define DEV_RX_OFFLOAD_SECURITY 0x00008000
+
+/* Invalid to set both DEV_RX_OFFLOAD_CRC_STRIP and DEV_RX_OFFLOAD_KEEP_CRC
+ * No DEV_RX_OFFLOAD_CRC_STRIP flag means keep CRC */
+#define DEV_RX_OFFLOAD_KEEP_CRC 0x00010000
#define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
DEV_RX_OFFLOAD_UDP_CKSUM | \
DEV_RX_OFFLOAD_TCP_CKSUM)