[dpdk-dev] [PATCH v1 0/7] Flow API helpers enhancements

[Ferruh Yigit]

On 10/6/2017 9:05 AM, Adrien Mazarguil wrote:
> On Fri, Oct 06, 2017 at 02:13:14AM +0100, Ferruh Yigit wrote:
>> On 10/5/2017 10:49 AM, Adrien Mazarguil wrote:
>>> This series brings enhancements to various rte_flow helpers:
>>>
>>> - Allow applications to use rte_flow_error_set() by making it part of the
>>>   public interface and documenting it as such.
>>>
>>> - Address rte_flow_copy()'s limitations by replacing it with the more
>>>   versatile rte_flow_conv(). This new function allows retrieving other
>>>   properties such as item/action names, enabling testpmd to finally use it
>>>   and get rid of duplicated code.
>>>
>>> - Add a script (gen-rte_flow_conv-h.sh) to help with generating the
>>>   resources used by rte_flow_conv(). Developers should run it when adding or
>>>   modifying pattern items or actions (done as part of this series to add the
>>>   missing "fuzzy" pattern item).
>>>
>>> - Future plans for rte_flow_conv() include translating error codes to
>>>   human-readable messages, so applications do not have to make their own.
>>>
>>> All these changes address concerns raised a couple of months ago [1]. Work
>>> on these patches actually started at the time but I was unable to complete
>>> and clean them up until recently.
>>>
>>> [1] http://dpdk.org/ml/archives/dev/2017-July/070492.html
>>>
>>> Adrien Mazarguil (7):
>>>   ethdev: expose flow API error helper
>>>   ethdev: replace flow API object copy function
>>>   ethdev: add flow API item/action name conversion
>>>   app/testpmd: rely on flow API conversion function
>>>   ethdev: enhance flow API item/action descriptions
>>>   ethdev: generate flow API conversion header
>>>   ethdev: update flow API conversion header
>>
>> Hi Adrien,
>>
>> This received too late for this release cycle, and changes in rte_flow
>> library may effect PMDs.
>>
>> I suggest deferring the set to next release, what do you think?
> 
> Hi Ferruh,
> 
> My opinion as the author (since you're asking :) is that it would be nice to
> have it in this release assuming reviewers don't find blocker issues with
> it.

Review part may be the problem, since this is very short notice before
release, relevant parties may not review this on time.
And they will be right to not expect a new feature like this after
proposal deadline.

> 
> To summarize the changes from a PMD standpoint:
> 
> - rte_flow_error() (previously not public) switching from positive to
>   negative return value like the other rte_flow_*() functions. The only PMDs
>   relying on its return value so far are mlx4 and tap.
> 
> - rte_flow_copy() disappearing. This function was temporary pending a better
>   solution, and so far is only used by fail-safe PMD (modified as part of
>   this series). Besides fail-safe, PMDs did not a have a use case for this
>   function.

Although you update all rte_flow_copy() usage in the DPDK, this is
public API right, and technically a user code may be using this, can we
remove this without notice?

> 
> These patches were originally targeted at 17.08, and since the "fuzzy" item
> is missing from rte_flow_copy() (GTP/GTPU/GTPC are now also missing by the
> way) and there is currently a lot of redundancy between this function and
> testpmd's internals, I thought it would be a good time to have everything in
> a single place. I was also considering using rte_flow_conv() in upcoming
> mlx4 patches in case it was included.
> 
> So here's my suggestion: I can track all rte_flow-related changes in PMDs
> and in rte_flow itself and update this series accordingly until things have
> settled (e.g. I'll re-submit to rebase and include GTP). Once applied, I
> will check all new code that relies on these two functions and update it if
> necessary until the release. How about that?

I have no concern that you will do the necessary updates, and agreed it
is good to get updates to help maintenance, and it would be nice to have
these in the this LTS release.

After above said, API changes one week before integration deadline, a
new script and make target for automated header file, I am a little
scared :), I will be much relieved to get this in the beginning of the
next release cycle.

I would like to see more comment on this, specially from PMD maintainers.