Re: [dpdk-dev] [PATCH v1 1/7] ethdev: expose flow API error helper

[Aaron Conole <aconole@redhat.com>]

Adrien Mazarguil <adrien.mazarguil@6wind.com> writes:

> On Wed, Oct 11, 2017 at 11:23:12AM +0200, Thomas Monjalon wrote:
>> 05/10/2017 11:49, Adrien Mazarguil:
>> > rte_flow_error_set() is a convenient helper to initialize error objects.
>> > 
>> > Since there is no fundamental reason to prevent applications from using it,
>> > expose it through the public interface after modifying its return value
>> > from positive to negative. This is done for consistency with the rest of
>> > the public interface.
>> > 
>> > Documentation is updated accordingly.
>> > 
>> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>> > ---
>> > --- a/lib/librte_ether/rte_flow.h
>> > +++ b/lib/librte_ether/rte_flow.h
>> >  /**
>> > + * Initialize flow error structure.
>> > + *
>> > + * @param[out] error
>> > + *   Pointer to flow error structure (may be NULL).
>> > + * @param code
>> > + *   Related error code (rte_errno).
>> > + * @param type
>> > + *   Cause field and error types.
>> > + * @param cause
>> > + *   Object responsible for the error.
>> > + * @param message
>> > + *   Human-readable error message.
>> > + *
>> > + * @return
>> > + *   Negative error code (errno value) and rte_errno is set.
>> > + */
>> > +static inline int
>> > +rte_flow_error_set(struct rte_flow_error *error,
>> > +		   int code,
>> > +		   enum rte_flow_error_type type,
>> > +		   const void *cause,
>> > +		   const char *message)
>> 
>> When calling this function, the performance is not critical.
>> So it must not be an inline function.
>> Please move it to the .c file and add it to the .map file.
>
> I'll do it as part of moving this patch into my upcoming series for mlx4,
> since it relies on the new return value.
>
> Now for the record, I believe that static inlines, particularly short ones
> that are not expected to be modified any time soon (provided they are
> defined properly that is) are harmless.
>
> Yes, that means modifying them, depending on what they do, may cause a
> global ABI breakage of the underlying library which translates to a major
> version bump, however exactly the same applies to some macros (in particular
> the *_MAX ones), structure and other type definitions and so on.

Sure.  I think if it's small and there's never a chance for it to
change, then it's okay.  But, as you point out - they are similar to
macros in that they are code directly added to the application.  There
is never a way to fix a bug or adjust some semantic behavior without
requiring a recompile.  That's a strong kind of binding.  Certainly we
would want that communicated as ABI, regardless of which .h/.c file the
function exists, yes?

> What I'm getting at is, making this function part of the .map file won't
> help all that much with the goal of reaching a stable ABI. Making it a
> normal function on the other hand successfully prevents PMDs and
> applications from considering its use in their data plane (flow rules
> management from the data plane is likely the next step for rte_flow).

I don't understand this point well.  There are two things - committing
to a stable ABI, and communicating information about that ABI.  Having
.map files doesn't do anything for a stable ABI.  It merely
communicates information.  The stability must come from the developer
community and maintainers.

> In my opinion, performance (even if hypothetical) matters more than ABI
> stability, particularly since the current pattern is for the whole ABI to be
> broken every other release. I think that with DPDK, ABI stability can only
> be properly guaranteed in stable/maintenance releases but that effort is
> vain in the master branch, affected by way too many changes for it to ever
> become reality. This doesn't mean we shouldn't even try, however we
> shouldn't restrict ourselves; when the ABI needs to be broken, so be it.

First, performance isn't hypothetical.  You have code which performs,
and have measured various metrics to show this, or you don't have data.
No voodoo required.  Hypothetical isn't even an argument when it comes
to predictable devices (like computers).  In the case above, you have a
non-performance critical function.  It's an error function.  It passes
strings around.  Performance cannot be a consideration here.

Second, it depends on where you want to view the boundary.  If a change is
internal to DPDK (meaning, the application is never expected to interact
with that area of code), then it shouldn't be part of ABI and changing
it to your hearts content should be fine.

*rant-on*

I see lots of projects which use DPDK, but I only know of three or so
projects which don't directly embed a copy of the DPDK source
tree.  I think this is a side effect of not having a stable ABI - it's
not possible to rely on easily upgrading the library, so people just
pick a version and never upgrade.  That's bad for everyone.  It results
in fewer contributions, and difficulty getting people to participate.
There's too much text like "Install DPDK 1.8.0. With other versions is
not guaranted it works properly" that is embedded in downstream users
(that comes from an actual project README).  Even at DPDK summit in
Dublin, one of the most vocal opponents to ABI stability admitted that
his company takes the code and never contributes it back.  That is
really not friendly.

This even has a different side effect - one of market penetration.  It's
difficult for distributions to support this model.  So, there will be
one or two packages they might pick up, with one or two versions.
Everything else is "roll your own."  And when it's more difficult than
'apt-get|yum|dnf|foo install' then users don't use it, and the cycle
continues.

I'll point out that I can take a number of compiled binaries from 20
years ago[0], and run on a modern kernel.  I wish I could take a
binary linked to DPDK from 5 months ago, and run it with today's DPDK.
I believe that *should* be the goal.  I am confident it's achievable.
But, I recognize it's hard and requires extra effort.

Anyway, apologies if I took some bait here, and sorry that I ranted off
on my own.

> (end of rant, thanks for reading)

[0]: https://www.complang.tuwien.ac.at/anton/linux-binary-compatibility.html