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

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

On 10/11/2017 10:57 AM, Adrien Mazarguil wrote:
> On Tue, Oct 10, 2017 at 07:05:30PM +0100, Ferruh Yigit wrote:
>> 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.
> 
> Yes, I generally agree with that.
> 
>>> 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?
> 
> Right, actually rte_flow_copy() was missing the EXPERIMENTAL tag. It's been
> present for a single release and wasn't documented as being officially part
> of the public rte_flow interface, unlike this series with rte_flow_conv().
> 
> For various reasons including its lack of flexibility (enforced by the
> struct rte_flow_desc format), I honestly believe rte_flow_copy() is only
> presently used by the fail-safe PMD. I may be wrong, and in the case of
> complaints the plan is to re-add rte_flow_copy() as a wrapper to
> rte_flow_conv() later as a standard maintenance fix.
> 
>>> 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 can drop the script from this series to speed up inclusion if it there's
> any concern about it. It's only a helper to update rte_flow_conv.h after
> modifying rte_flow.h, I thought it could be useful to anyone, hence I've
> included it but it's pretty much optional.
> 
>> I would like to see more comment on this, specially from PMD maintainers.
> 
> Me too. I don't even mind negative ones!
> 
> Here's what I plan to do regardless, seeing most concerns so far are with
> rte_flow_copy()/rte_flow_conv():
> 
> - Whether this series is included for 17.11 or later, a v2 is already
>   necessary.
> 
> - I will drop the rte_flow_error() change to submit it instead along another
>   upcoming series for mlx4 where it's the most needed.
> 
> - We'll then continue to discuss rte_flow_conv() as a something nice to have
>   but not super urgent to integrate and I'll keep trying to convince
>   everyone it's safe enough.
> 
> - Once it becomes clear there's no way to have it for 17.11, I'll update
>   this series as a somewhat late deprecation notice for rte_flow_copy().
> 
> Sounds good?

OK. Lets get rte_flow_error() change and not block mlx4 changes.

And I still suggest waiting beginning of the release for rest of the
patch. So it can come with optional header auto generation.



Related to the rte_flow_error_set() modification, as far as I can see it
doesn't effect drivers but following drivers are now using it:

drivers/net/bnxt/
drivers/net/e1000/
drivers/net/enic/
drivers/net/i40e/
drivers/net/ixgbe/
drivers/net/mlx4/
drivers/net/mlx5/
drivers/net/sfc/
drivers/net/tap/

There is already tap and mlx4 fixes in the patch to fix
"return -rte_flow_error_set(...)" kind of usage.

Rest uses rte_flow_error_set() as:

"
rte_flow_error_set(...);
return -rte_errno;
"

But now it can directly be used as:
"
return rte_flow_error_set(...);
"

What do you think updating to that usage?
Would you mind updating those drivers as above in a separate patch?

Thanks,
ferruh