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

Message ID 00db12469380e587ac434bfc5c63795e32cfddef.1507193185.git.adrien.mazarguil@6wind.com (mailing list archive)
State Not Applicable, archived
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Adrien Mazarguil Oct. 5, 2017, 9:49 a.m. UTC
  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>
---
 doc/guides/prog_guide/rte_flow.rst | 23 +++++++++++++++++---
 drivers/net/mlx4/mlx4_flow.c       |  6 +++---
 drivers/net/tap/tap_flow.c         |  2 +-
 lib/librte_ether/rte_flow.c        | 30 +++++++++++++-------------
 lib/librte_ether/rte_flow.h        | 36 +++++++++++++++++++++++++++++++
 lib/librte_ether/rte_flow_driver.h | 38 ---------------------------------
 6 files changed, 75 insertions(+), 60 deletions(-)
  

Comments

Thomas Monjalon Oct. 11, 2017, 9:23 a.m. UTC | #1
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.
  
Adrien Mazarguil Oct. 11, 2017, 11:56 a.m. UTC | #2
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.

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

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.

(end of rant, thanks for reading)
  
Aaron Conole Oct. 11, 2017, 7:05 p.m. UTC | #3
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
  
Neil Horman Oct. 12, 2017, 1:37 p.m. UTC | #4
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
  
Adrien Mazarguil Oct. 12, 2017, 2:02 p.m. UTC | #5
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.
  

Patch

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 662a912..565a809 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -1678,6 +1678,25 @@  freed by the application, however its pointer can be considered valid only
 as long as its associated DPDK port remains configured. Closing the
 underlying device or unloading the PMD invalidates it.
 
+Helpers
+-------
+
+Error initializer
+~~~~~~~~~~~~~~~~~
+
+.. code-block:: c
+
+   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);
+
+This function initializes ``error`` (if non-NULL) with the provided
+parameters and sets ``rte_errno`` to ``code``. A negative error ``code`` is
+then returned.
+
 Caveats
 -------
 
@@ -1743,13 +1762,11 @@  the legacy filtering framework, which should eventually disappear.
   whatsoever). They only make sure these callbacks are non-NULL or return
   the ``ENOSYS`` (function not supported) error.
 
-This interface additionally defines the following helper functions:
+This interface additionally defines the following helper function:
 
 - ``rte_flow_ops_get()``: get generic flow operations structure from a
   port.
 
-- ``rte_flow_error_set()``: initialize generic flow error structure.
-
 More will be added over time.
 
 Device compatibility
diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
index 0885a91..018843b 100644
--- a/drivers/net/mlx4/mlx4_flow.c
+++ b/drivers/net/mlx4/mlx4_flow.c
@@ -955,9 +955,9 @@  mlx4_flow_isolate(struct rte_eth_dev *dev,
 		mlx4_mac_addr_del(priv);
 	} else if (mlx4_mac_addr_add(priv) < 0) {
 		priv->isolated = 1;
-		return -rte_flow_error_set(error, rte_errno,
-					   RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
-					   NULL, "cannot leave isolated mode");
+		return rte_flow_error_set(error, rte_errno,
+					  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+					  NULL, "cannot leave isolated mode");
 	}
 	return 0;
 }
diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index eefa868..a790946 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -1447,7 +1447,7 @@  tap_flow_isolate(struct rte_eth_dev *dev,
 	return 0;
 error:
 	pmd->flow_isolate = 0;
-	return -rte_flow_error_set(
+	return rte_flow_error_set(
 		error, ENOTSUP, RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
 		"TC rule creation failed");
 }
diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c
index 2001fbb..34ce516 100644
--- a/lib/librte_ether/rte_flow.c
+++ b/lib/librte_ether/rte_flow.c
@@ -145,9 +145,9 @@  rte_flow_validate(uint8_t port_id,
 		return -rte_errno;
 	if (likely(!!ops->validate))
 		return ops->validate(dev, attr, pattern, actions, error);
-	return -rte_flow_error_set(error, ENOSYS,
-				   RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
-				   NULL, rte_strerror(ENOSYS));
+	return rte_flow_error_set(error, ENOSYS,
+				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+				  NULL, rte_strerror(ENOSYS));
 }
 
 /* Create a flow rule on a given port. */
@@ -183,9 +183,9 @@  rte_flow_destroy(uint8_t port_id,
 		return -rte_errno;
 	if (likely(!!ops->destroy))
 		return ops->destroy(dev, flow, error);
-	return -rte_flow_error_set(error, ENOSYS,
-				   RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
-				   NULL, rte_strerror(ENOSYS));
+	return rte_flow_error_set(error, ENOSYS,
+				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+				  NULL, rte_strerror(ENOSYS));
 }
 
 /* Destroy all flow rules associated with a port. */
@@ -200,9 +200,9 @@  rte_flow_flush(uint8_t port_id,
 		return -rte_errno;
 	if (likely(!!ops->flush))
 		return ops->flush(dev, error);
-	return -rte_flow_error_set(error, ENOSYS,
-				   RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
-				   NULL, rte_strerror(ENOSYS));
+	return rte_flow_error_set(error, ENOSYS,
+				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+				  NULL, rte_strerror(ENOSYS));
 }
 
 /* Query an existing flow rule. */
@@ -220,9 +220,9 @@  rte_flow_query(uint8_t port_id,
 		return -rte_errno;
 	if (likely(!!ops->query))
 		return ops->query(dev, flow, action, data, error);
-	return -rte_flow_error_set(error, ENOSYS,
-				   RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
-				   NULL, rte_strerror(ENOSYS));
+	return rte_flow_error_set(error, ENOSYS,
+				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+				  NULL, rte_strerror(ENOSYS));
 }
 
 /* Restrict ingress traffic to the defined flow rules. */
@@ -238,9 +238,9 @@  rte_flow_isolate(uint8_t port_id,
 		return -rte_errno;
 	if (likely(!!ops->isolate))
 		return ops->isolate(dev, set, error);
-	return -rte_flow_error_set(error, ENOSYS,
-				   RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
-				   NULL, rte_strerror(ENOSYS));
+	return rte_flow_error_set(error, ENOSYS,
+				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+				  NULL, rte_strerror(ENOSYS));
 }
 
 /** Compute storage space needed by item specification. */
diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
index bba6169..ec42f02 100644
--- a/lib/librte_ether/rte_flow.h
+++ b/lib/librte_ether/rte_flow.h
@@ -43,6 +43,7 @@ 
  */
 
 #include <rte_arp.h>
+#include <rte_errno.h>
 #include <rte_ether.h>
 #include <rte_icmp.h>
 #include <rte_ip.h>
@@ -1270,6 +1271,41 @@  int
 rte_flow_isolate(uint8_t port_id, int set, struct rte_flow_error *error);
 
 /**
+ * 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)
+{
+	if (error) {
+		*error = (struct rte_flow_error){
+			.type = type,
+			.cause = cause,
+			.message = message,
+		};
+	}
+	rte_errno = code;
+	return -code;
+}
+
+/**
  * Generic flow representation.
  *
  * This form is sufficient to describe an rte_flow independently from any
diff --git a/lib/librte_ether/rte_flow_driver.h b/lib/librte_ether/rte_flow_driver.h
index 4d95391..ea92fe7 100644
--- a/lib/librte_ether/rte_flow_driver.h
+++ b/lib/librte_ether/rte_flow_driver.h
@@ -45,7 +45,6 @@ 
 
 #include <stdint.h>
 
-#include <rte_errno.h>
 #include "rte_ethdev.h"
 #include "rte_flow.h"
 
@@ -128,43 +127,6 @@  struct rte_flow_ops {
 };
 
 /**
- * Initialize generic flow error structure.
- *
- * This function also sets rte_errno to a given value.
- *
- * @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
- *   Error code.
- */
-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)
-{
-	if (error) {
-		*error = (struct rte_flow_error){
-			.type = type,
-			.cause = cause,
-			.message = message,
-		};
-	}
-	rte_errno = code;
-	return code;
-}
-
-/**
  * Get generic flow operations structure from a port.
  *
  * @param port_id