From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 927551B1F5 for ; Wed, 11 Oct 2017 21:05:52 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D7CC5C0587ED; Wed, 11 Oct 2017 19:05:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D7CC5C0587ED Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=aconole@redhat.com Received: from dhcp-25-97.bos.redhat.com (unknown [10.18.25.172]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6DFA677A3F; Wed, 11 Oct 2017 19:05:48 +0000 (UTC) From: Aaron Conole To: Adrien Mazarguil Cc: Thomas Monjalon , dev@dpdk.org, Ferruh Yigit , Gaetan Rivet , fbl@redhat.com, Neil Horman References: <00db12469380e587ac434bfc5c63795e32cfddef.1507193185.git.adrien.mazarguil@6wind.com> <2075344.22PATxnhkA@xps> <20171011115642.GI13551@6wind.com> Date: Wed, 11 Oct 2017 15:05:47 -0400 In-Reply-To: <20171011115642.GI13551@6wind.com> (Adrien Mazarguil's message of "Wed, 11 Oct 2017 13:56:42 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 11 Oct 2017 19:05:52 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH v1 1/7] ethdev: expose flow API error helper 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, 11 Oct 2017 19:05:53 -0000 Adrien Mazarguil 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 >> > --- >> > --- 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