[v3,1/3] ethdev: rename action modify field data structure
Checks
Commit Message
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.
Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@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(-)
Comments
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 <suanmingm@nvidia.com>
> Acked-by: Ori Kam <orika@nvidia.com>
> Acked-by: Andrew Rybchenko <andrew.rybchenko@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?
<...>
> 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.
Hi,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Wednesday, January 31, 2024 1:19 AM
> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
> <yuying.zhang@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>; Slava
> Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>; NBU-
> Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; Andrew
> Rybchenko <andrew.rybchenko@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 <suanmingm@nvidia.com>
> > Acked-by: Ori Kam <orika@nvidia.com>
> > Acked-by: Andrew Rybchenko <andrew.rybchenko@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?
On 1/31/2024 2:57 AM, Suanming Mou wrote:
> Hi,
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Wednesday, January 31, 2024 1:19 AM
>> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
>> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
>> <yuying.zhang@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>; Slava
>> Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>; NBU-
>> Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; Andrew
>> Rybchenko <andrew.rybchenko@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 <suanmingm@nvidia.com>
>>> Acked-by: Ori Kam <orika@nvidia.com>
>>> Acked-by: Andrew Rybchenko <andrew.rybchenko@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'?
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, February 1, 2024 6:56 PM
> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
> <yuying.zhang@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>; Slava
> Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>; NBU-
> Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v3 1/3] ethdev: rename action modify field data structure
>
> On 1/31/2024 2:57 AM, Suanming Mou wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >> Sent: Wednesday, January 31, 2024 1:19 AM
> >> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
> >> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
> >> <yuying.zhang@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>;
> >> Slava Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad
> >> <matan@nvidia.com>; NBU- Contact-Thomas Monjalon (EXTERNAL)
> >> <thomas@monjalon.net>; Andrew Rybchenko
> >> <andrew.rybchenko@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 <suanmingm@nvidia.com>
> >>> Acked-by: Ori Kam <orika@nvidia.com>
> >>> Acked-by: Andrew Rybchenko <andrew.rybchenko@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'?
Sounds good, OK, I will update.
BTW, I saw the patch apply failed, seems it is due to Raslan's branch has some extra features than your branch.
So I just want to know is it OK? Or should I still base on your branch? When will the branches be synced.
On 2/1/2024 11:09 AM, Suanming Mou wrote:
>
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Thursday, February 1, 2024 6:56 PM
>> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
>> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
>> <yuying.zhang@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>; Slava
>> Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>; NBU-
>> Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; Andrew
>> Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v3 1/3] ethdev: rename action modify field data structure
>>
>> On 1/31/2024 2:57 AM, Suanming Mou wrote:
>>> Hi,
>>>
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>>>> Sent: Wednesday, January 31, 2024 1:19 AM
>>>> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
>>>> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
>>>> <yuying.zhang@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>;
>>>> Slava Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad
>>>> <matan@nvidia.com>; NBU- Contact-Thomas Monjalon (EXTERNAL)
>>>> <thomas@monjalon.net>; Andrew Rybchenko
>>>> <andrew.rybchenko@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 <suanmingm@nvidia.com>
>>>>> Acked-by: Ori Kam <orika@nvidia.com>
>>>>> Acked-by: Andrew Rybchenko <andrew.rybchenko@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'?
>
> Sounds good, OK, I will update.
>
> BTW, I saw the patch apply failed, seems it is due to Raslan's branch has some extra features than your branch.
> So I just want to know is it OK? Or should I still base on your branch? When will the branches be synced.
>
Thanks.
Can you please rebase next version on next-net, this way we can benefit
from CI checks?
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, February 1, 2024 7:21 PM
> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
> <yuying.zhang@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>; Slava
> Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>; NBU-
> Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v3 1/3] ethdev: rename action modify field data structure
>
> On 2/1/2024 11:09 AM, Suanming Mou wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >> Sent: Thursday, February 1, 2024 6:56 PM
> >> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
> >> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
> >> <yuying.zhang@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>;
> >> Slava Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad
> >> <matan@nvidia.com>; NBU- Contact-Thomas Monjalon (EXTERNAL)
> >> <thomas@monjalon.net>; Andrew Rybchenko
> >> <andrew.rybchenko@oktetlabs.ru>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [PATCH v3 1/3] ethdev: rename action modify field data
> >> structure
> >>
> >> On 1/31/2024 2:57 AM, Suanming Mou wrote:
> >>> Hi,
> >>>
> >>>> -----Original Message-----
> >>>> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >>>> Sent: Wednesday, January 31, 2024 1:19 AM
> >>>> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam
> >>>> <orika@nvidia.com>; Aman Singh <aman.deep.singh@intel.com>; Yuying
> >>>> Zhang <yuying.zhang@intel.com>; Dariusz Sosnowski
> >>>> <dsosnowski@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>;
> >>>> Matan Azrad <matan@nvidia.com>; NBU- Contact-Thomas Monjalon
> >>>> (EXTERNAL) <thomas@monjalon.net>; Andrew Rybchenko
> >>>> <andrew.rybchenko@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 <suanmingm@nvidia.com>
> >>>>> Acked-by: Ori Kam <orika@nvidia.com>
> >>>>> Acked-by: Andrew Rybchenko <andrew.rybchenko@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'?
> >
> > Sounds good, OK, I will update.
> >
> > BTW, I saw the patch apply failed, seems it is due to Raslan's branch has some
> extra features than your branch.
> > So I just want to know is it OK? Or should I still base on your branch? When will
> the branches be synced.
> >
>
> Thanks.
>
> Can you please rebase next version on next-net, this way we can benefit from CI
> checks?
OK, got it.
@@ -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
/** Storage size for struct rte_flow_action_modify_field including pattern. */
@@ -3171,7 +3171,7 @@ destination offset as ``48``, and provide immediate value ``0xXXXX85XX``.
| ``width`` | number of bits to use |
+---------------+-------------------------+
-.. _table_rte_flow_action_modify_data:
+.. _table_rte_flow_field_data:
.. table:: destination/source field definition
@@ -106,6 +106,7 @@ ABI Changes
* No ABI change that would break compatibility with 23.11.
+* ethdev: Rename the experimental ``struct rte_flow_action_modify_data`` to be ``struct rte_flow_field_data``
Known Issues
------------
@@ -2493,7 +2493,7 @@ mlx5_validate_action_ct(struct rte_eth_dev *dev,
* Validate the level value for modify field action.
*
* @param[in] data
- * Pointer to the rte_flow_action_modify_data structure either src or dst.
+ * Pointer to the rte_flow_field_data structure either src or dst.
* @param[out] error
* Pointer to error structure.
*
@@ -2501,7 +2501,7 @@ mlx5_validate_action_ct(struct rte_eth_dev *dev,
* 0 on success, a negative errno value otherwise and rte_errno is set.
*/
int
-flow_validate_modify_field_level(const struct rte_flow_action_modify_data *data,
+flow_validate_modify_field_level(const struct rte_flow_field_data *data,
struct rte_flow_error *error)
{
if (data->level == 0)
@@ -1121,7 +1121,7 @@ flow_items_to_tunnel(const struct rte_flow_item items[])
* Tag array index.
*/
static inline uint8_t
-flow_tag_index_get(const struct rte_flow_action_modify_data *data)
+flow_tag_index_get(const struct rte_flow_field_data *data)
{
return data->tag_index ? data->tag_index : data->level;
}
@@ -2523,7 +2523,7 @@ int mlx5_flow_validate_action_default_miss(uint64_t action_flags,
const struct rte_flow_attr *attr,
struct rte_flow_error *error);
int flow_validate_modify_field_level
- (const struct rte_flow_action_modify_data *data,
+ (const struct rte_flow_field_data *data,
struct rte_flow_error *error);
int mlx5_flow_item_acceptable(const struct rte_flow_item *item,
const uint8_t *mask,
@@ -2828,7 +2828,7 @@ size_t flow_dv_get_item_hdr_len(const enum rte_flow_item_type item_type);
int flow_dv_convert_encap_data(const struct rte_flow_item *items, uint8_t *buf,
size_t *size, struct rte_flow_error *error);
void mlx5_flow_field_id_to_modify_info
- (const struct rte_flow_action_modify_data *data,
+ (const struct rte_flow_field_data *data,
struct field_modify_info *info, uint32_t *mask,
uint32_t width, struct rte_eth_dev *dev,
const struct rte_flow_attr *attr, struct rte_flow_error *error);
@@ -1446,7 +1446,7 @@ flow_modify_info_mask_32_masked(uint32_t length, uint32_t off, uint32_t post_mas
}
static __rte_always_inline enum mlx5_modification_field
-mlx5_mpls_modi_field_get(const struct rte_flow_action_modify_data *data)
+mlx5_mpls_modi_field_get(const struct rte_flow_field_data *data)
{
return MLX5_MODI_IN_MPLS_LABEL_0 + data->tag_index;
}
@@ -1454,7 +1454,7 @@ mlx5_mpls_modi_field_get(const struct rte_flow_action_modify_data *data)
static void
mlx5_modify_flex_item(const struct rte_eth_dev *dev,
const struct mlx5_flex_item *flex,
- const struct rte_flow_action_modify_data *data,
+ const struct rte_flow_field_data *data,
struct field_modify_info *info,
uint32_t *mask, uint32_t width)
{
@@ -1578,7 +1578,7 @@ mlx5_modify_flex_item(const struct rte_eth_dev *dev,
void
mlx5_flow_field_id_to_modify_info
- (const struct rte_flow_action_modify_data *data,
+ (const struct rte_flow_field_data *data,
struct field_modify_info *info, uint32_t *mask,
uint32_t width, struct rte_eth_dev *dev,
const struct rte_flow_attr *attr, struct rte_flow_error *error)
@@ -5329,8 +5329,8 @@ flow_dv_validate_action_modify_field(struct rte_eth_dev *dev,
struct mlx5_sh_config *config = &priv->sh->config;
struct mlx5_hca_attr *hca_attr = &priv->sh->cdev->config.hca_attr;
const struct rte_flow_action_modify_field *conf = action->conf;
- const struct rte_flow_action_modify_data *src_data = &conf->src;
- const struct rte_flow_action_modify_data *dst_data = &conf->dst;
+ const struct rte_flow_field_data *src_data = &conf->src;
+ const struct rte_flow_field_data *dst_data = &conf->dst;
uint32_t dst_width, src_width, width = conf->width;
ret = flow_dv_validate_action_modify_hdr(action_flags, action, error);
@@ -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.
*/
-struct rte_flow_action_modify_data {
+struct rte_flow_field_data {
enum rte_flow_field_id field; /**< Field or memory type ID. */
union {
struct {
@@ -4021,8 +4021,8 @@ enum rte_flow_modify_op {
*/
struct rte_flow_action_modify_field {
enum rte_flow_modify_op operation; /**< Operation to perform. */
- struct rte_flow_action_modify_data dst; /**< Destination field. */
- struct rte_flow_action_modify_data src; /**< Source field. */
+ struct rte_flow_field_data dst; /**< Destination field. */
+ struct rte_flow_field_data src; /**< Source field. */
uint32_t width; /**< Number of bits to use from a source field. */
};