Re: [PATCH v3] ethdev: deprecate header fields and metadata flow actions

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/25/2021 1:06 PM, Thomas Monjalon wrote:
> 25/11/2021 12:53, Ferruh Yigit:
>> On 11/24/2021 3:37 PM, Viacheslav Ovsiienko wrote:
>>> The generic RTE_FLOW_ACTION_TYPE_MODIFY_FIELD action was
>>> introduced by [1]. This action provides an unified way
>>> to perform various arithmetic and transfer operations over
>>> packet network header fields and packet metadata.
>>>
>>> [1] commit 641dbe4fb053 ("net/mlx5: support modify field flow action")
>>>
>>> On other side there are a bunch of multiple legacy actions,
>>> that can be superseded by the generic modify field action:
>>>
>>> RTE_FLOW_ACTION_TYPE_OF_SET_MPLS_TTL
>>> RTE_FLOW_ACTION_TYPE_OF_DEC_MPLS_TTL
>>> RTE_FLOW_ACTION_TYPE_OF_SET_NW_TTL
>>> RTE_FLOW_ACTION_TYPE_OF_DEC_NW_TTL      sfc
>>> RTE_FLOW_ACTION_TYPE_OF_COPY_TTL_OUT
>>> RTE_FLOW_ACTION_TYPE_OF_COPY_TTL_IN
>>> RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC       bnxt, cxgbe, mlx5
>>> RTE_FLOW_ACTION_TYPE_SET_IPV4_DST       bnxt, cxgbe, mlx5
>>> RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC       cxgbe, mlx5
>>> RTE_FLOW_ACTION_TYPE_SET_IPV6_DST       cxgbe, mlx5
>>> RTE_FLOW_ACTION_TYPE_SET_TP_SRC         cxgbe, mlx5
>>> RTE_FLOW_ACTION_TYPE_SET_TP_DST         cxgbe, mlx5
>>> RTE_FLOW_ACTION_TYPE_DEC_TTL            mlx5, sfc
>>> RTE_FLOW_ACTION_TYPE_SET_TTL            mlx5
>>> RTE_FLOW_ACTION_TYPE_SET_MAC_SRC        cxgbe, mlx5
>>> RTE_FLOW_ACTION_TYPE_SET_MAC_DST        cxgbe, mlx5
>>> RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ        mlx5
>>> RTE_FLOW_ACTION_TYPE_DEC_TCP_SEQ        mlx5
>>> RTE_FLOW_ACTION_TYPE_INC_TCP_ACK        mlx5
>>> RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK        mlx5
>>> RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP      mlx5
>>> RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP      mlx5
>>> RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID    bnxt, cnxk, cxgbe, enic,
>>>                                           mlx5, octeontx2, sfc
>>> RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP    bnxt, cnxk, cxgbe, enic,
>>>                                           mlx5, octeontx2, sfc
>>> RTE_FLOW_ACTION_TYPE_SET_TAG            mlx5
>>> RTE_FLOW_ACTION_TYPE_SET_META           mlx5
>>>
>>> This note deprecates the following RTE Flow actions:
>>> 1. As not supported by any of PMDs:
>>>
>>> RTE_FLOW_ACTION_TYPE_OF_SET_MPLS_TTL
>>> RTE_FLOW_ACTION_TYPE_OF_DEC_MPLS_TTL
>>> RTE_FLOW_ACTION_TYPE_OF_SET_NW_TTL
>>> RTE_FLOW_ACTION_TYPE_OF_COPY_TTL_OUT
>>> RTE_FLOW_ACTION_TYPE_OF_COPY_TTL_IN
>>>
>>> 2. As supposed to be replaced by generig field modify action:
>>> RTE_FLOW_ACTION_TYPE_OF_DEC_NW_TTL
>>> RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC
>>> RTE_FLOW_ACTION_TYPE_SET_IPV4_DST
>>> RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC
>>> RTE_FLOW_ACTION_TYPE_SET_IPV6_DST
>>> RTE_FLOW_ACTION_TYPE_SET_TP_SRC
>>> RTE_FLOW_ACTION_TYPE_SET_TP_DST
>>> RTE_FLOW_ACTION_TYPE_DEC_TTL
>>> RTE_FLOW_ACTION_TYPE_SET_TTL
>>> RTE_FLOW_ACTION_TYPE_SET_MAC_SRC
>>> RTE_FLOW_ACTION_TYPE_SET_MAC_DST
>>> RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ
>>> RTE_FLOW_ACTION_TYPE_DEC_TCP_SEQ
>>> RTE_FLOW_ACTION_TYPE_INC_TCP_ACK
>>> RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK
>>> RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP
>>> RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP
>>> RTE_FLOW_ACTION_TYPE_SET_TAG
>>> RTE_FLOW_ACTION_TYPE_SET_META
>>>
>>> The VLAN set actions are interrelated to VLAN header insertion/removal
>>> and supported by multiple PMDs and supposed to be just deprecated but
>>> not be removed in 22.11.
>>>
>>
>> Why not remove them for v22.11? Do you think PMDs can't change the
>> existing implementation until 22.11?
>>
>>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>>>
>>> --
>>> v2  - deprecation.rst is updated
>>> v3  - doc comments addressed
>>>       - commit message comments addressed
>>>       - SET_VLAN_VID and SET_VLAN_PCP actions deprecated, but will not
>>>         be removed in 22.11
>>
>> Deprecated symbols are to prevent new code using them, but for this case
>> there is no alternative, since PMDs still don't support
>> 'RTE_FLOW_ACTION_TYPE_MODIFY_FIELD' yet.
> 
> This patch is not preventing new code using old actions,
> there are just comments to point to the new direction.
> 

Since the 'deprecated' implementation is done as documentation, it doesn't
practically prevent anything, that is true but
if user applications wants to use 'SET_IPV4_SRC' action, she will be have
to use a *deprecated* action for any driver other than mlx.

I just suggest updating the wording so that it still points the direction,
but also clarifies that 'SET_IPV4_SRC' action is still can be used.

Btw, isn't the 'deprecation.rst' to document the long term direction.

>> This patch is forcing users to use deprecated actions (except from mlx).
> 
> I don't get it.
> It is encouraging to use the new generic action,
> which is supported only by mlx5 for now.
> 

That is the problem, we are deprecating old method and new generic
action is only supported by single PMD. And we didn't give enough
time to other PMDs for the implementation.

And it is a headache for application that a feature is supported in two
different ways in different PMDs.
For some PMDs applications should use 'SET_IPV4_SRC' action, for other
it should use 'MODIFY_FIELD' action, to have same rule.

Wouldn't be nice to move slowly:
- add 'MODIFY_FIELD' as experimental
- give time to PMDs to implement it while old ones are still default
- deprecate old actions and make 'MODIFY_FIELD' default
- remove old actions

In this case single PMD is implementing an action and we are making it
default immediately.


> 
>> What about a slight change:
>> 1- In this release, update header/document as 'RTE_FLOW_ACTION_TYPE_MODIFY_FIELD'
>>      is preferred way if supported. Instead of deprecating old ones.
> 
> Deprecation is just a comment, clearly showing that it may be removed in future.
> In my opinion, it makes the message simple and clear.
> 

What does '@deprecated' exactly mean?
What is the expectation from application for a '@deprecated' symbol, keep using
it with new code?

> 
>> 2- Have an agreement with PMD maintainers to switch to new action before v22.11,
>>      and don't accept old action implementation in PMDs anymore.
>>      Based on agreement update 'deprecation.rst' in this release to note that
>>      old actions will be removed on v22.11.
>>      (It would be good to have a check to prevent old actions merged during that time.)
> 
> Not sure I get it.
> You want to remove VLAN actions? I think it is premature.
> 

Nothing specific to VLAN here.
Main point is having an agreement with other PMD maintainers on the direction,
since they will need to do some update in their code.

>> 3- In v22.11, remove old actions, the PMDs that don't support MODIFY_FIELD
>>      action will lose the feature.
> 
> The VLAN actions are probably already used a lot in the field.
> I would consider removing them only if it becomes a burden to maintain.
> 

I am not saying anything specific to VLAN.
But for all actions, are you suggesting to keep both 'MODIFY_FIELD' and old
actions support in PMDs?
Since this patch has a deprecation note to remove old actions on v22.11.