On 1/31/2024 2:57 AM, Suanming Mou wrote:
> Hi,
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yi...@amd.com>
>> Sent: Wednesday, January 31, 2024 1:19 AM
>> To: Suanming Mou <suanmi...@nvidia.com>; Ori Kam <or...@nvidia.com>;
>> Aman Singh <aman.deep.si...@intel.com>; Yuying Zhang
>> <yuying.zh...@intel.com>; Dariusz Sosnowski <dsosnow...@nvidia.com>; Slava
>> Ovsiienko <viachesl...@nvidia.com>; Matan Azrad <ma...@nvidia.com>; NBU-
>> Contact-Thomas Monjalon (EXTERNAL) <tho...@monjalon.net>; Andrew
>> Rybchenko <andrew.rybche...@oktetlabs.ru>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v3 1/3] ethdev: rename action modify field data structure
>>
>> On 1/15/2024 9:13 AM, Suanming Mou wrote:
>>> Current rte_flow_action_modify_data struct describes the pkt field
>>> perfectly and is used only in action.
>>>
>>> It is planned to be used for item as well. This commit renames it to
>>> "rte_flow_field_data" making it compatible to be used by item.
>>>
>>
>> ack to rename struct to use in pattern.
>>
>>> Signed-off-by: Suanming Mou <suanmi...@nvidia.com>
>>> Acked-by: Ori Kam <or...@nvidia.com>
>>> Acked-by: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
>>> ---
>>> app/test-pmd/cmdline_flow.c | 2 +-
>>> doc/guides/prog_guide/rte_flow.rst | 2 +-
>>> doc/guides/rel_notes/release_24_03.rst | 1 +
>>> drivers/net/mlx5/mlx5_flow.c | 4 ++--
>>> drivers/net/mlx5/mlx5_flow.h | 6 +++---
>>> drivers/net/mlx5/mlx5_flow_dv.c | 10 +++++-----
>>> lib/ethdev/rte_flow.h | 8 ++++----
>>> 7 files changed, 17 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>>> index ce71818705..3725e955c7 100644
>>> --- a/app/test-pmd/cmdline_flow.c
>>> +++ b/app/test-pmd/cmdline_flow.c
>>> @@ -740,7 +740,7 @@ enum index {
>>> #define ITEM_RAW_SIZE \
>>> (sizeof(struct rte_flow_item_raw) + ITEM_RAW_PATTERN_SIZE)
>>>
>>> -/** Maximum size for external pattern in struct
>>> rte_flow_action_modify_data. */
>>> +/** Maximum size for external pattern in struct rte_flow_field_data.
>>> +*/
>>> #define ACTION_MODIFY_PATTERN_SIZE 32
>>>
>>
>> What do you think to update 'ACTION_MODIFY_PATTERN_SIZE' here too, instead
>> of next patch?
>
> Agree.
>
>>
>> <...>
>>
>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
>>> affdc8121b..40f6dcaacd 100644
>>> --- a/lib/ethdev/rte_flow.h
>>> +++ b/lib/ethdev/rte_flow.h
>>> @@ -3910,9 +3910,9 @@ enum rte_flow_field_id {
>>> * @warning
>>> * @b EXPERIMENTAL: this structure may change without prior notice
>>> *
>>> - * Field description for MODIFY_FIELD action.
>>> + * Field description for packet field.
>>>
>>
>> New note is not very helpful, how can we make it more useful?
>>
>> Does it make sense to keep 'MODIFY_FIELD' and add 'COMPARE ITEM' in next
>> patch, to clarify the intended usage for the struct, otherwise it is too
>> generic.
>
> OK, sorry, the purpose is to make it generic. So next time if other ITEM or
> ACTION need that field, it can be used directly.
> Otherwise, it feels like it can only be used by 'MODIFY_FIELD' and
> 'COMPARE_ITEM', what do you think?
>
I don't have an intention to limit its usage, but to clarify usage for
whoever checks the document.
"Field description for packet field." doesn't say what exactly it is and
cause confusion.
Perhaps wording can be changed to say two possible usages are for
'MODIFY_FIELD' and 'COMPARE_ITEM'?
