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

[Neil Horman <nhorman@redhat.com>]

On Wed, Oct 11, 2017 at 03:05:47PM -0400, Aaron Conole wrote:
> 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.
> 
I'll add a +1 here.  The degree to which the community is concerned about ABI
stability is reflected in the fact that no two major releases of DPDK have had
compatible ABI's.  While thats somewhat understandable from a developer
perspective (it really doesn't matter if you have a stable ABI if you always use
the latest version, in fact its a hinderance to doing your job), its potentially
catastrophic from a downstream consumer point of view.  No downstream consumer
is going to keep up with the latest version of DPDK on a consistent basis (I'm
going to guess that there are still users of DPDK 2.2 floating out there).  They
just embed a copy, get it working, and move on with their lives.  Thats
seemingly fine for as long as your foot print is small, but the second that you
hit a major bug or security issue in an old version and can't just upgrade the
DPDK without a major engineering effort, thats going to be a big problem, and
users will look elsewhere for a solution that offers both performance and
compatibility.  XDP and iovisor are already gaining ground on the performance
front, and offer that level of upgradability.

Neil

> > (end of rant, thanks for reading)
> 
> [0]: https://www.complang.tuwien.ac.at/anton/linux-binary-compatibility.html