From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f68.google.com (mail-wm0-f68.google.com [74.125.82.68]) by dpdk.org (Postfix) with ESMTP id 8BBA82C6E for ; Wed, 1 Feb 2017 13:06:04 +0100 (CET) Received: by mail-wm0-f68.google.com with SMTP id u63so5339512wmu.2 for ; Wed, 01 Feb 2017 04:06:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=GdqaSrVXA32TzgCxL+dxlNFI6QEXI4pMtq5x+OyE58k=; b=RgnlSKN7h94ih11aYg0Q76tQ8QTgOcvwKiwo4HdDIIHwIC4yDUYO2IwwtfXxQW/fjt 0PwMwclTP6LwrZAUdVkU8291yhEHJ+P0Em9sefnqtUap0kjhqk8svdfyaRxMPU20tikN /W6UeHJc/REuqSaD4HG9uxFwy01kaO+9m/rrU3rKcxH39cmYqAh6QJ2XxpZmeo5yj/7o vQVn3MTKG4w40fdFA4lf6PZtR9Zc/WZQHKgcKd1IL0h72QS8AQQREonQ1fPSHj+D9wop ejen0u9tHqV/Hs5U4rPjrcyN/DeQIYxmcMebBPGWyoL36YnKyeQY64BVn1cr8sk1boIV hAeA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=GdqaSrVXA32TzgCxL+dxlNFI6QEXI4pMtq5x+OyE58k=; b=trHMvaFhotpBIA9yLvsz7nx3yIk9q2FQ3Ip896zCACHex6DJxL4e1wv2DK5jQSQ3wV A2Vqdn01XyqPOjzsF4OENYw+D6qe0+y8x+9a8979c72gWcZOVssVFgtk4cZWmo/Q/45a W9OlfhZwv3Z0ohPslR7TluDj+Zl33e2x/3BDJhiu/oylSw3mrthL44oSSMUFFsjsrTDd lYasuw+4CNcI5pyZ2BNqoN05vZjj7ZxaLCa7KGyGIJ50UFfaFvgvhHffDH4JNIIQngz8 KR3po35Y72y13/xivPelIISmOqRbQnxfUAo/ekdiVSHCO7Ybqv5ons8vR3KUYNl8WiH3 WleQ== X-Gm-Message-State: AIkVDXKYtu6q+BVKcRDjbylpwSAK+aNGERztYMmUYm5LOPfoWHXC22mGz8/BzGZnLCDE714iu/byVoolF0paoA== X-Received: by 10.28.212.82 with SMTP id l79mr22034472wmg.101.1485950764178; Wed, 01 Feb 2017 04:06:04 -0800 (PST) MIME-Version: 1.0 Sender: jblunck@gmail.com Received: by 10.28.211.20 with HTTP; Wed, 1 Feb 2017 04:06:03 -0800 (PST) In-Reply-To: <20170201105430.GQ10133@6wind.com> References: <1485529023-5486-1-git-send-email-aconole@redhat.com> <20170127093729.5cef9138@xeon-e3> <2880962.uYJx3WqeFl@xps13> <20170201105430.GQ10133@6wind.com> From: Jan Blunck Date: Wed, 1 Feb 2017 13:06:03 +0100 X-Google-Sender-Auth: lmOLnZVe_Aj7F-dIcGriF83Py1o Message-ID: To: Adrien Mazarguil Cc: Thomas Monjalon , Aaron Conole , dev@dpdk.org, Stephen Hemminger , Bruce Richardson Content-Type: text/plain; charset=UTF-8 Subject: Re: [dpdk-dev] [PATCH 25/25] rte_eal_init: add info about rte_errno codes X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 01 Feb 2017 12:06:04 -0000 On Wed, Feb 1, 2017 at 11:54 AM, Adrien Mazarguil wrote: > On Mon, Jan 30, 2017 at 09:19:29PM +0100, Thomas Monjalon wrote: >> 2017-01-30 13:38, Aaron Conole: >> > Stephen Hemminger writes: >> > > Bruce Richardson 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