Re: [dpdk-dev] [PATCH v2 1/3] ethdev: add actions to modify TCP header fields

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 4/4/19 4:25 PM, Adrien Mazarguil wrote:
> Hi Ori,
>
> (trimming message down a bit)
>
> On Thu, Apr 04, 2019 at 09:01:52AM +0000, Ori Kam wrote:
>> Hi Adrien,
>>
>> PSB
> <snip>
>>> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> <snip>
>>> On Wed, Apr 03, 2019 at 10:49:09AM +0000, Dekel Peled wrote:
>>>> Thanks, PSB.
> <snip>
>>>>> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> <snip>
>>>>> I still don't agree with the wording as it implies one must combine this
>>> action
>>>>> with the TCP pattern item or else, while one should simply ensure the
>>>>> presence of TCP traffic somehow. This may be done by a prior filtering rule.
>>>>>
>>>>> So here's a generic suggestion which could be used with pretty much all
>>>>> modifying actions (other actions have the same problem and will have to be
>>>>> fixed as well eventually):
>>>>>
>>>>>   Using this action on non-matching traffic results in undefined behavior.
>>>>>
>>>>> This comment applies to all instances in this patch.
>>>> I accept your suggestion, indeed the existing actions have the problematic
>>> condition.
>>>> However I would like to currently leave this patch as-is for consistency.
>>>> I will send a fix patch for next release, applying the updated text to all
>>> modify-header actions.
>>>
>>> Please do it now as it's much more difficult to change an existing API
>>> later (think deprecation notices and endless discussions); even seemingly
>>> minor documentation issues like this one may affect applications.
>>>
>> I agree that changing API is not easy. This is why I think we should keep Dekel patch,
>> there is a number of API and consistency is very important. Also the PMD is based on the current
>> description that such command should fail.
>>
>> So lets keep it this way if you want to change all API then and only then this API should be changed.
> Wait, I'm not asking Delek to modify existing code/APIs right now, only to
> document these new actions properly from the start so we don't have to do it
> later (you even acknowledged it's more difficult that way).
>
> So I fail to understand why it's so important for their documentation to be
> consistent with unrelated and badly documented actions?
>
> Note the change I'm asking for at the API level doesn't affect PMD code,
> which remains free to put extra limitations (namely the presence of TCP
> pattern items). It's just that these limitations have nothing to do in the
> API itself.
>
> <snip>
>>>> It's either 2 actions with 1 parameters, or 1 action with 2 parameters.
>>>> The current implementation is more straight-forward in my opinion.
>>> I generally also prefer the one action per thing to do approach, but seeing
>>> the kind of actions you're adding, I fear we'll soon end up with lots of
>>> similar rte_flow_action_* structures modifying a single 32-bit value in some
>>> way.
>>>
>>> So for the same reasons as above, I think it's the right time to define a
>>> shared structure to rule them all, or maybe even let users provide a
>>> rte_be32_t/uint32_t/whatever pointer directly as a conf pointer (not
>>> as straightforward to document though).
>>>
>>> An object to rule them all would look something like that:
>>>
>>>   union rte_flow_integer {
>>>       rte_be64_t be64;
>>>       rte_le64_t le64;
>>>       uint64_t u64;
>>>       int64_t i64;
>>>       rte_be32_t be32;
>>>       rte_le32_t le32;
>>>       uint32_t u32;
>>>       int32_t i32;
>>>       uint8_t u8;
>>>       int8_t i8;
>>>   };
>>>
>>> Then actions that need a single integer value only have to document which
>>> field is relevant to them. How about that?

I like the idea (plus 16-bit options). We already have too many
rte_flow_action_* structures with single integer in it.

>> Like my previous comment. I understand your idea, but it has no huge advantage compared to the
>> suggested one by Dekel which also match all other API.
>>
>> Currently for each action we have a direct command, which is easy to understand by using your idea we break this concept.
> Yes, although not all actions have a configuration structure. Those that do
> indeed have a rte_flow_action_* counterpart, but it doesn't have to be
> unique, see RTE_FLOW_ITEM_GTP/GTPC/GTPU for instance.
>
> Likewise this patch adds struct rte_flow_action_modify_tcp_seq shared by
> RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ and RTE_FLOW_ACTION_TYPE_DEC_TCP_SEQ
> although they lack a common prefix (inc_tcp/dec_tcp vs. modify_tcp). The
> type to use is covered by documentation and that's fine.
>
> So why not go a little further and share the exact same structure with
> RTE_FLOW_ACTION_TYPE_INC_TCP_ACK and RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK?

Shouldn't these action be RTE_FLOW_ACTION_TYPE_MOD_TCP_{ACK,SEQ}
with singed 32-bit integer parameter (negative to decrement, positive to
increment)? IMHO, having INC and DEC is too artificial and just bloat API
and code.

> And while there, why not plan for subsequent actions that take a single
> integer value of some kind, because modifying existing APIs once upstream is
> complicated... See where I'm going?
>
>> There is no issue with having a large number of actions, it is even easer to read and document if each action is dedicated,
>> as you can also see from OVS.
> I'm actually fine with a large number of actions (rte_flow can support 2^31
> unique actions). Not so much with a large number of identical configuration
> structures that only differ by name associated with them. This is what I'd
> like to avoid before it's too late.

Too many actions bloat the code everywhere: API, PMDs, testpmd, other apps.
If it is possible to decrease number of different actions without
over-complicating, it should be done.

>> So I vote to keep Dekel patch as is.
> I don't, I guess another vote is needed to decide :)
>