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

[Adrien Mazarguil <adrien.mazarguil@6wind.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?

Yes, I entirely agree, I must admit the stated reasons behind my previous
message weren't super clear to begin with as I wrote it after some off-list
discussion.

My point is that static inlines must not be considered harmful for what they
are particularly in a project like DPDK where there is almost always a
performance trade-off depending on the use case. They must be considered on
a case basis, and not be forbidden outright with no possible discussion.

> > 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.

Right, nothing against that. I was referring to the fact the .map file in
the case of a shared libray helps with ABI breakage by providing a kind of
run-time error check that ensures an application compiled against an older
version of a symbol is prevented from starting at all, thereby avoiding any
adverse effects later on.

However to benefit from that, some sort of symbol is necessary and that's
only possible on non-inline functions. Since we agree that this, in itself,
doesn't guarantee ABI stability due to other mandatory factors, I think it
cannot be the only ammunition to use against inline functions. Everything
must be considered.

> > 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.

For the specific case of this function, it basically initializes a
structure. Now that's probably just me, but I believe initializers are a
nice thing to have inline so the compiler can optimize them away wherever it
thinks it's worth a few CPU cycles, somewhat like optimized memcpy's (that
was the possible performance gain I was thinking about).

This function should only change if the target structure changes, the ABI
breakage requiring the major ABI version bump in that case comes from the
structure change, not from the inline function itself.

You have a point with the fact the main change from the original code is the
return value sign update due to bad design. Had it been non-inline, only the
resulting symbol version would have changed, not that of the entire ABI.

Fortunately for this specific case again, that function was neither
versioned nor part of the public facing API until this commit. It would have
been an entirely different story otherwise.

> *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.

I believe all the remaining "bare metal" handling code has been removed by
now, however the fact DPDK started as a framework to embed applications and
not as a shared library to link them against surely didn't help. There's a
lot of design leftovers from this era, such as EAL still snatching argv.

> 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.

I do understand your points and the reasons behind them. My goal and the way
I see things as a PMD developer are likely why we're having this discussion,
so I'd like to summarize why the topic came up on my side:

- DPDK is all about performance *because* without much more performance than
  what can be achieved with a vanilla Linux kernel, it's only a pain to use
  compared to the much more comfortable (and stable) socket() API. Since no
  one would use it, there wouldn't be any market for it.

- Every CPU cycle counts. That makes DPDK difficult to use because we cannot
  get away with a friendly socket()-like API. Almost every aspect can be
  configured to tailor it to an application needs. This was historically
  done at compile time through .config options, now through run-time
  parameters with the unfortunate side effect of causing the number of TX/RX
  burst functions grow dangerously in many PMDs to maintain the same level
  of performance.

- ABI concerns mainly apply to shared libraries [1], which happens to be
  what Linux distributions want to use for very good reasons. The problem is
  that shared libraries, by their very nature are not as efficient thanks to
  PIC, relocations and so on. CPU-bound applications may actually prefer to
  link against static libraries to squeeze a handful more pps; DPDK cannot
  ignore this as it has a measurable performance impact for those.

Basically ABI stability is good, unless we want more performance on top of
more features. There is no end in sight to new features and associated API
changes/ABI breakages.

While I don't want to sound pessimistic, I don't think expecting things to
settle without killing DPDK is a realistic goal. That's why I was taking
maintenance/stable releases as an example; without new features, ABI
stability is almost guaranteed. Efforts could be diverted from the pursuit
of ABI stability in the master branch to improve maintenance ones.

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

No worries, I needed to vent a bit myself :)

Note, from the above comments it may not look like it but I'm actually
concerned about API/ABI stability where applicable, that was one of the
reasons behind the design of rte_flow (which can be extended safely without
wrecking ABIs in the process). Unfortunately several bad design choices led
to the need for the obligatory ABI breakage that will have to occur at some
point in the future.

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

[1] Applications are pretty much guaranteed to be recompiled against static
    ones and encounter issues due to API changes first.

-- 
Adrien Mazarguil
6WIND