[v2,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
[1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
doc/guides/prog_guide/rte_flow.rst | 16 ++++++++++++++++
doc/guides/rel_notes/release_21_11.rst | 9 +++++++++
lib/ethdev/rte_flow.h | 17 ++++++++++++++---
3 files changed, 39 insertions(+), 3 deletions(-)
Comments
Hi Slava,
> -----Original Message-----
> From: Slava Ovsiienko <viacheslavo@nvidia.com>
> Sent: Monday, October 11, 2021 2:46 AM
> Subject: [PATCH v2 1/5] ethdev: update modify field flow action
>
> 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
>
> [1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
> doc/guides/prog_guide/rte_flow.rst | 16 ++++++++++++++++
> doc/guides/rel_notes/release_21_11.rst | 9 +++++++++
> lib/ethdev/rte_flow.h | 17 ++++++++++++++---
> 3 files changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 2b42d5ec8c..1ceecb399f 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -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 large
> +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 width as 16, and provide immediate value as sequence of bytes {xxx, xxx, 0x85, xxx, xxx, xxx}.
>
I think you have a typo destination width as 16 should be destination offset as 16.
If fixed you can add my ack to the next version.
Best,
Ori
On 10/11/21 2:45 AM, 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
>
> [1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
> doc/guides/prog_guide/rte_flow.rst | 16 ++++++++++++++++
> doc/guides/rel_notes/release_21_11.rst | 9 +++++++++
> lib/ethdev/rte_flow.h | 17 ++++++++++++++---
> 3 files changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 2b42d5ec8c..1ceecb399f 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -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 large than
large -> larger
> +16 bytes the pattern can be provided as pointer only.
RTE_FLOW_FIELD_MAC_DST, dst, rte_flow_item_eth, RTE_FLOW_FIELD_IPV6_SRC,
rte_flow_item_ipv6 should be ``x``.
> +
> +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 width as 16, and provide immediate
destination width twice above
> +value as sequence of bytes {xxx, xxx, 0x85, xxx, xxx, xxx}.
>
> .. _table_rte_flow_action_modify_field:
pvalue should be added in the "destination/source field
definition".
dst and src members documentation should be improved to
highlight the difference. Destination cannot be "immediate" and
"pointer". In fact, "pointer" is a kind of "immediate". May be
it is better to use "constant value" instead of "immediate".
>
> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index dfc2cbdeed..41a087d7c1 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -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 udpdated, immediate data
udpdated -> updated
> + 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
> -----------
> @@ -222,6 +229,8 @@ ABI Changes
> ``rte_security_ipsec_xform`` to allow applications to configure SA soft
> and hard expiry limits. Limits can be either in number of packets or bytes.
>
> +* ethdev: ``rte_flow_action_modify_data`` structure udpdated.
udpdated -> updated
I'm not sure that it makes sense to duplicate ABI changes if
API is changed.
> +
>
> Known Issues
> ------------
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index 7b1ed7f110..953924d42b 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -3204,6 +3204,9 @@ enum rte_flow_field_id {
> };
>
> /**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
Isn't a separate fix to add missing experimental header?
> * Field description for MODIFY_FIELD action.
> */
"Another packet field" in the next paragraph I read as
a field of another packet which sounds confusing.
I guess it is "Another field of the packet" in fact.
I think it would be nice to clarify as well.
> struct rte_flow_action_modify_data {
> @@ -3217,10 +3220,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];
> + /*
It should be a Doxygen style comment.
> + * 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;
> };
> };
>
>
@@ -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 large 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 width as 16, and provide immediate
+value as sequence of bytes {xxx, xxx, 0x85, xxx, xxx, xxx}.
.. _table_rte_flow_action_modify_field:
@@ -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 udpdated, 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
-----------
@@ -222,6 +229,8 @@ ABI Changes
``rte_security_ipsec_xform`` to allow applications to configure SA soft
and hard expiry limits. Limits can be either in number of packets or bytes.
+* ethdev: ``rte_flow_action_modify_data`` structure udpdated.
+
Known Issues
------------
@@ -3204,6 +3204,9 @@ enum rte_flow_field_id {
};
/**
+ * @warning
+ * @b EXPERIMENTAL: this structure may change without prior notice
+ *
* Field description for MODIFY_FIELD action.
*/
struct rte_flow_action_modify_data {
@@ -3217,10 +3220,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;
};
};