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

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Thu, Oct 12, 2017 at 05:37:41PM +0100, Ferruh Yigit wrote:
> On 10/12/2017 1:53 PM, Adrien Mazarguil wrote:
> > On Wed, Oct 11, 2017 at 07:07:29PM +0100, Ferruh Yigit wrote:
> >> On 10/11/2017 10:57 AM, Adrien Mazarguil wrote:
> >>> On Tue, Oct 10, 2017 at 07:05:30PM +0100, Ferruh Yigit wrote:
> > <snip>
> >>>> 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.
> > 
> > OK, I will re-submit an updated version later then.
> > 
> >> 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?
> > 
> > Well, that is actually how I originally envisioned this function to be used,
> > but for whatever reason I thought a positive return value was the way to go
> > at the time. Too bad.
> > 
> >> Would you mind updating those drivers as above in a separate patch?
> > 
> > That would be nice but I'm not familiar with most of these PMDs (I'm
> > only updating mlx4 as part of the RSS resurrection series), and there's
> > always a risk of breaking something.
> 
> The change looks mechanical, so I believe it won't break anything.
> 
> Not directly related to this one, but including this one, recently we
> kind of have an agreement that library changes doesn't have to update
> every usage of it, because it is too much work and this is preventing
> improvements in libraries. (Because sometimes library implementer
> doesn't know PMD internals to update it properly.)
> 
> But currently I have list that PMDs needs to reflect some library work,
> and list is growing.
> 
> Let's assume this work go in without updating PMDs, I believe it will
> take some time for some PMDs even to recognize this can be changed.
> 
> So we either need to change our approach, or find an effective way to
> pass this information into PMDs. But I believe even passing this
> information won't be enough, each driver has their own roadmaps,
> resource allocation, some may tend to not apply these unless PMD is broken.
> 
> Any comments?

I'm generally fine with that, it should likely be considered on a case basis
though. Some API changes (e.g. port ID type) also can't avoid modifying all
PMDs at once.

It's more or less optional for those that do not break existing APIs,
depending on their impact. Say, if some new API involves something to be
done on the data path, all PMDs must be validated for performance
non-regression. This can't be done properly by people unfamiliar with
impacted PMDs (or who may benefit from decreasing performance of competing
PMDs :). Either way, if a PMD maintainer disagrees with a change in his/her
PMD for any reason, this should not block the API change from entering. The
patch for that specific PMD should be ignored in the meantime (possibly to
be reworked by one of its maintainers).

-- 
Adrien Mazarguil
6WIND