From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Thomas Monjalon <thomas.monjalon@6wind.com>
Cc: Aaron Conole <aconole@redhat.com>,
dev@dpdk.org, Stephen Hemminger <stephen@networkplumber.org>,
Bruce Richardson <bruce.richardson@intel.com>
Subject: Re: [dpdk-dev] [PATCH 25/25] rte_eal_init: add info about rte_errno codes
Date: Wed, 1 Feb 2017 11:54:30 +0100 [thread overview]
Message-ID: <20170201105430.GQ10133@6wind.com> (raw)
In-Reply-To: <2880962.uYJx3WqeFl@xps13>
On Mon, Jan 30, 2017 at 09:19:29PM +0100, Thomas Monjalon wrote:
> 2017-01-30 13:38, Aaron Conole:
> > Stephen Hemminger <stephen@networkplumber.org> writes:
> > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > >> On Fri, Jan 27, 2017 at 08:33:46AM -0800, Stephen Hemminger wrote:
> > >> > Why use rte_errno?
> > >> > Most DPDK calls just return negative value on error which
> > >> > corresponds to error number.
> > >> > Are you trying to keep ABI compatibility? Doesn't make sense
> > >> > because before all these
> > >> > errors were panic's no working application is going to care.
> > >>
> > >> Either will work, but I actually prefer this way. I view using rte_errno
> > >> to be better as it can work in just about all cases, including with
> > >> functions which return pointers. This allows you to have a standard
> > >> method across all functions for returning error codes, and it only
> > >> requires a single sentinal value to indicate error, rather than using a
> > >> whole range of values.
> > >
> > > The problem is DPDK is getting more inconsistent on how this is done.
> > > As long as error returns are always same as kernel/glibc errno's it really doesn't
> > > matter much which way the value is returned from a technical point of view
> > > but the inconsistency is sure to be a usability problem and source of errors.
> >
> > I am using rte_errno here because I assumed it was the preferred
> > method. In fact, looking at some recently contributed modules (for
> > instance pdump), it seems that folks are using it.
> >
> > I'm not really sure the purpose of having rte_errno if it isn't used, so
> > it'd be helpful to know if there's some consensus on reflecting errors
> > via this variable, or on returning error codes. Whichever is the more
> > consistent with the way the DPDK project does things, I'm game :).
>
> I think we can use both return value and rte_errno.
> We could try to enforce rte_errno as mandatory everywhere.
>
> Adrien did the recent rte_flow API.
> Please Adrien, could you give your thought?
Sure, actually as already pointed out in this thread, both approaches have
pros and cons depending on the use-case.
Through return value:
Pros
----
- Most common approach used in DPPK today.
- Used internally by the Linux kernel (negative errno) and in the pthreads
library (positive errno).
- Avoids the need to access an external, global variable requiring its own
thread-local storage.
- Inherently thread-safe and reentrant (i.e. safe with signal handlers).
- Returned value is also the error code, two facts reported at once.
Cons
----
- Difficult to use with functions returning anything other than signed
integers with negative values having no other meaning.
- The returned value must be assigned to a local variable in order not to
discard it and process it later most of the time.
- All function calls must be tested for errors.
Through rte_errno:
Pros
----
- errno-like, well known behavior defined by the C standard and used
everywhere in the C library.
- Testing return values is not mandatory, e.g. rte_errno can be initialized
to zero before calling a group of functions and checking its value
afterward (rte_errno is only updated in case of error).
- Assigning a local variable to store its value is not necessary as long as
another function that may affect rte_errno is not called.
Cons
----
- Not fully reentrant, thread-safety is fine for most purposes but signal
handlers affecting it still cause undefined behavior (they must at least
save and restore its value in case they modify it).
- Accessing non-local storage may affect CPU cycle-sensitive functions such
as TX/RX burst.
My opinion is that rte_errno is best for control path operations while using
the return value makes more sense in the data path. The major issue being
that function returning anything other than int (e.g. TX/RX burst) cannot
describe any kind of error to the application.
I went with both in rte_flow (return + rte_errno) mostly due to the return
type of a few functions (e.g. rte_flow_create()) and wanted to keep the API
consistent while maintaining compatibility with other DPDK APIs. Note there
is little overhead for API functions to set rte_errno _and_ return its
value, it's mostly free.
I think using both is best also because it leaves applications the choice of
error-handling method, however if I had to pick one I'd go with rte_errno
and standardize on -1 as the default error value (as in the C library).
Below are a bunch of use-case examples to illustrate how rte_errno could
be convenient to applications.
Easily creating many flow rules during init in a all-or-nothing fashion:
rte_errno = 0;
for (i = 0; i != num; ++i)
rule[i] = rte_flow_create(port, ...);
if (unlikely(rte_errno)) {
rte_flow_flush(port);
return -1;
}
Complete TX packet burst failure with explanation (could also detect partial
failures by initializing rte_errno to 0):
sent = rte_eth_tx_burst(...);
if (unlikely(!sent)) {
switch (rte_errno) {
case E2BIG:
// too many packets in burst
...
case EMSGSIZE:
// first packet is too large
...
case ENOBUFS:
// TX queue is full
...
}
}
TX burst functions in PMDs could be modified as follows with minimal impact
on their performance and no ABI change:
uint16_t sent = 0;
int error; // new variable
[process burst]
if (unlikely([something went wrong])) { // this check already exists
error = EPROBLEM; // new assignment
goto error; // instead of "return sent"
}
[process burst]
return sent;
error:
rte_errno = error;
return sent;
--
Adrien Mazarguil
6WIND
next prev parent reply other threads:[~2017-02-01 10:54 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-27 14:56 [dpdk-dev] [PATCH 00/24] linux/eal: Remove most causes of panic on init Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 01/25] eal: CPU init will no longer panic Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 02/25] eal: return error instead of panic for cpu init Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 03/25] eal: No panic on hugepages info init Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 04/25] eal: do not panic on failed hugepage query Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 05/25] eal: failure to parse args returns error Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 06/25] eal-common: introduce a way to query cpu support Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 07/25] eal: Signal error when CPU isn't supported Aaron Conole
2017-01-27 16:27 ` Stephen Hemminger
2017-01-30 16:50 ` Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 08/25] eal: do not panic on memzone initialization fails Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 09/25] eal: set errno when exiting for already called Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 10/25] eal: Do not panic on log failures Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 11/25] eal: Do not panic on pci-probe Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 12/25] eal: do not panic on vfio failure Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 13/25] eal: do not panic on memory init Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 14/25] eal: do not panic on tailq init Aaron Conole
2017-01-27 16:30 ` Stephen Hemminger
2017-01-30 16:51 ` Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 15/25] eal: do not panic on alarm init Aaron Conole
2017-01-27 16:31 ` Stephen Hemminger
2017-01-27 16:42 ` Bruce Richardson
2017-01-30 16:52 ` Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 16/25] eal: convert timer_init not to call panic Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 17/25] eal: change the private pipe call to reflect errno Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 18/25] eal: Do not panic on interrupt thread init Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 19/25] eal: do not error if plugins fail to init Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 20/25] eal_pci: Continue probing even on failures Aaron Conole
2017-01-27 14:56 ` [dpdk-dev] [PATCH 21/25] eal: do not panic on failed PCI probe Aaron Conole
2017-01-27 14:57 ` [dpdk-dev] [PATCH 22/25] eal_common_dev: continue initializing vdevs Aaron Conole
2017-01-27 14:57 ` [dpdk-dev] [PATCH 23/25] eal: do not panic (or abort) if vdev init fails Aaron Conole
2017-01-27 14:57 ` [dpdk-dev] [PATCH 24/25] eal: do not panic when bus probe fails Aaron Conole
2017-01-27 14:57 ` [dpdk-dev] [PATCH 25/25] rte_eal_init: add info about rte_errno codes Aaron Conole
2017-01-27 16:33 ` Stephen Hemminger
2017-01-27 16:47 ` Bruce Richardson
2017-01-27 17:37 ` Stephen Hemminger
2017-01-30 18:38 ` Aaron Conole
2017-01-30 20:19 ` Thomas Monjalon
2017-02-01 10:54 ` Adrien Mazarguil [this message]
2017-02-01 12:06 ` Jan Blunck
2017-02-01 14:18 ` Bruce Richardson
2017-01-31 9:33 ` Bruce Richardson
2017-01-31 16:56 ` Stephen Hemminger
2017-01-27 15:48 ` [dpdk-dev] [PATCH 00/24] linux/eal: Remove most causes of panic on init Aaron Conole
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170201105430.GQ10133@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=aconole@redhat.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=stephen@networkplumber.org \
--cc=thomas.monjalon@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).