[v6,1/5] ethdev: update modify field flow action
Checks
Commit Message
The generic modify field flow action introduced in [1] has
some issues related to the immediate source operand:
- immediate source can be presented either as an unsigned
64-bit integer or pointer to data pattern in memory.
There was no explicit pointer field defined in the union.
- the byte ordering for 64-bit integer was not specified.
Many fields have shorter lengths and byte ordering
is crucial.
- how the bit offset is applied to the immediate source
field was not defined and documented.
- 64-bit integer size is not enough to provide IPv6
addresses.
In order to cover the issues and exclude any ambiguities
the following is done:
- introduce the explicit pointer field
in rte_flow_action_modify_data structure
- replace the 64-bit unsigned integer with 16-byte array
- update the modify field flow action documentation
Appropriate deprecation notice has been removed.
[1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
Fixes: 2ba49b5f3721 ("doc: announce change to ethdev modify action data")
Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
doc/guides/prog_guide/rte_flow.rst | 24 +++++++++++++++++++++++-
doc/guides/rel_notes/deprecation.rst | 4 ----
doc/guides/rel_notes/release_21_11.rst | 7 +++++++
lib/ethdev/rte_flow.h | 16 ++++++++++++----
4 files changed, 42 insertions(+), 9 deletions(-)
Comments
On 10/13/2021 7:45 PM, Viacheslav Ovsiienko wrote:
> The generic modify field flow action introduced in [1] has
> some issues related to the immediate source operand:
>
> - immediate source can be presented either as an unsigned
> 64-bit integer or pointer to data pattern in memory.
> There was no explicit pointer field defined in the union.
>
> - the byte ordering for 64-bit integer was not specified.
> Many fields have shorter lengths and byte ordering
> is crucial.
>
> - how the bit offset is applied to the immediate source
> field was not defined and documented.
>
> - 64-bit integer size is not enough to provide IPv6
> addresses.
>
> In order to cover the issues and exclude any ambiguities
> the following is done:
>
> - introduce the explicit pointer field
> in rte_flow_action_modify_data structure
>
> - replace the 64-bit unsigned integer with 16-byte array
>
> - update the modify field flow action documentation
>
> Appropriate deprecation notice has been removed.
>
> [1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
>
> Fixes: 2ba49b5f3721 ("doc: announce change to ethdev modify action data")
>
Hi Viacheslav,
The commit in the 'Fixes' line is the commit that announces the deprecation
notice, sure this patch is not fixing it.
I am removing the above 'Fixes' line.
This patch is breaking the ABI and it can't be backported anyway, but
the 'Fixes' line still can be useful for documentation purpose, so if there
is valid fix commits please share them and I can squash them in next-net.
> Signed-off-by: Viacheslav Ovsiienko<viacheslavo@nvidia.com>
> Acked-by: Ori Kam<orika@nvidia.com>
> Acked-by: Andrew Rybchenko<andrew.rybchenko@oktetlabs.ru>
There is no actual fixing commits.
I've just followed the practice of removing the deprecation notice in dedicated commit.
So, no extra steps needed, thank you for the patch correction.
With best regards,
Slava
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, October 14, 2021 15:09
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
> Cc: Raslan Darawsheh <rasland@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Ori Kam
> <orika@nvidia.com>; Gregory Etelson <getelson@nvidia.com>; NBU-Contact-
> Thomas Monjalon <thomas@monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH v6 1/5] ethdev: update modify field flow
> action
>
> On 10/13/2021 7:45 PM, Viacheslav Ovsiienko wrote:
> > The generic modify field flow action introduced in [1] has some issues
> > related to the immediate source operand:
> >
> > - immediate source can be presented either as an unsigned
> > 64-bit integer or pointer to data pattern in memory.
> > There was no explicit pointer field defined in the union.
> >
> > - the byte ordering for 64-bit integer was not specified.
> > Many fields have shorter lengths and byte ordering
> > is crucial.
> >
> > - how the bit offset is applied to the immediate source
> > field was not defined and documented.
> >
> > - 64-bit integer size is not enough to provide IPv6
> > addresses.
> >
> > In order to cover the issues and exclude any ambiguities the following
> > is done:
> >
> > - introduce the explicit pointer field
> > in rte_flow_action_modify_data structure
> >
> > - replace the 64-bit unsigned integer with 16-byte array
> >
> > - update the modify field flow action documentation
> >
> > Appropriate deprecation notice has been removed.
> >
> > [1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow
> > action")
> >
> > Fixes: 2ba49b5f3721 ("doc: announce change to ethdev modify action
> > data")
> >
>
> Hi Viacheslav,
>
> The commit in the 'Fixes' line is the commit that announces the deprecation
> notice, sure this patch is not fixing it.
>
> I am removing the above 'Fixes' line.
>
> This patch is breaking the ABI and it can't be backported anyway, but the
> 'Fixes' line still can be useful for documentation purpose, so if there is valid fix
> commits please share them and I can squash them in next-net.
>
> > Signed-off-by: Viacheslav Ovsiienko<viacheslavo@nvidia.com>
> > Acked-by: Ori Kam<orika@nvidia.com>
> > Acked-by: Andrew Rybchenko<andrew.rybchenko@oktetlabs.ru>
@@ -2835,6 +2835,22 @@ a packet to any other part of it.
``value`` sets an immediate value to be used as a source or points to a
location of the value in memory. It is used instead of ``level`` and ``offset``
for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` respectively.
+The data in memory should be presented exactly in the same byte order and
+length as in the relevant flow item, i.e. data for field with type
+``RTE_FLOW_FIELD_MAC_DST`` should follow the conventions of ``dst`` field
+in ``rte_flow_item_eth`` structure, with type ``RTE_FLOW_FIELD_IPV6_SRC`` -
+``rte_flow_item_ipv6`` conventions, and so on. If the field size is larger than
+16 bytes the pattern can be provided as pointer only.
+
+The bitfield extracted from the memory being applied as second operation
+parameter is defined by action width and by the destination field offset.
+Application should provide the data in immediate value memory (either as
+buffer or by pointer) exactly as item field without any applied explicit offset,
+and destination packet field (with specified width and bit offset) will be
+replaced by immediate source bits from the same bit offset. For example,
+to replace the third byte of MAC address with value 0x85, application should
+specify destination width as 8, destination offset as 16, and provide immediate
+value as sequence of bytes {xxx, xxx, 0x85, xxx, xxx, xxx}.
.. _table_rte_flow_action_modify_field:
@@ -2865,7 +2881,13 @@ for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` respectively.
+---------------+----------------------------------------------------------+
| ``offset`` | number of bits to skip at the beginning |
+---------------+----------------------------------------------------------+
- | ``value`` | immediate value or a pointer to this value |
+ | ``value`` | immediate value buffer (source field only, not |
+ | | applicable to destination) for RTE_FLOW_FIELD_VALUE |
+ | | field type |
+ +---------------+----------------------------------------------------------+
+ | ``pvalue`` | pointer to immediate value data (source field only, not |
+ | | applicable to destination) for RTE_FLOW_FIELD_POINTER |
+ | | field type |
+---------------+----------------------------------------------------------+
Action: ``CONNTRACK``
@@ -120,10 +120,6 @@ Deprecation Notices
* ethdev: Announce moving from dedicated modify function for each field,
to using the general ``rte_flow_modify_field`` action.
-* ethdev: The struct ``rte_flow_action_modify_data`` will be modified
- to support modifying fields larger than 64 bits.
- In addition, documentation will be updated to clarify byte order.
-
* ethdev: Attribute ``shared`` of the ``struct rte_flow_action_count``
is deprecated and will be removed in DPDK 21.11. Shared counters should
be managed using shared actions API (``rte_flow_shared_action_create`` etc).
@@ -187,6 +187,13 @@ API Changes
the crypto/security operation. This field will be used to communicate
events such as soft expiry with IPsec in lookaside mode.
+* ethdev: ``rte_flow_action_modify_data`` structure updated, immediate data
+ array is extended, data pointer field is explicitly added to union, the
+ action behavior is defined in more strict fashion and documentation updated.
+ The immediate value behavior has been changed, the entire immediate field
+ should be provided, and offset for immediate source bitfield is assigned
+ from destination one.
+
ABI Changes
-----------
@@ -3217,10 +3217,18 @@ struct rte_flow_action_modify_data {
uint32_t offset;
};
/**
- * Immediate value for RTE_FLOW_FIELD_VALUE or
- * memory address for RTE_FLOW_FIELD_POINTER.
+ * Immediate value for RTE_FLOW_FIELD_VALUE, presented in the
+ * same byte order and length as in relevant rte_flow_item_xxx.
+ * The immediate source bitfield offset is inherited from
+ * the destination's one.
*/
- uint64_t value;
+ uint8_t value[16];
+ /**
+ * Memory address for RTE_FLOW_FIELD_POINTER, memory layout
+ * should be the same as for relevant field in the
+ * rte_flow_item_xxx structure.
+ */
+ void *pvalue;
};
};
@@ -3240,7 +3248,7 @@ enum rte_flow_modify_op {
* RTE_FLOW_ACTION_TYPE_MODIFY_FIELD
*
* Modify a destination header field according to the specified
- * operation. Another packet field can be used as a source as well
+ * operation. Another field of the packet can be used as a source as well
* as tag, mark, metadata, immediate value or a pointer to it.
*/
struct rte_flow_action_modify_field {