From: Jan Blunck <jblunck@infradead.org>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: Thomas Monjalon <thomas.monjalon@6wind.com>,
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 13:06:03 +0100 [thread overview]
Message-ID: <CALe+Z02jX22VEoH55JTKuNPJbe+99-1oWdA_goZkz_y4cZGs=Q@mail.gmail.com> (raw)
In-Reply-To: <20170201105430.GQ10133@6wind.com>
On Wed, Feb 1, 2017 at 11:54 AM, Adrien Mazarguil
<adrien.mazarguil@6wind.com> wrote:
> 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.
Caller can decide to ignore return value if no error handling is wanted.
>
> 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.
I believe this is Pro since the rte_errno even needs to assign to a
thread-local variable even.
> - All function calls must be tested for errors.
The rte_errno needs to do this too to decide if it needs to assign a
value to rte_errno.
>
> 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.
Actually testing for errors mean you also have to reset the rte_errno
variable before. That also means you have to access thread-local
storage twice.
Besides that the problem of rte_errno is that you do error handling
twice because the implementation still needs to check for the error
condition before assigning a meaningful error value to rte_errno.
After that again the user code needs to check for the return value to
decide if looking at rte_errno makes any sense.
> 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 12:06 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
2017-02-01 12:06 ` Jan Blunck [this message]
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='CALe+Z02jX22VEoH55JTKuNPJbe+99-1oWdA_goZkz_y4cZGs=Q@mail.gmail.com' \
--to=jblunck@infradead.org \
--cc=aconole@redhat.com \
--cc=adrien.mazarguil@6wind.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).