From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f42.google.com (mail-wm0-f42.google.com [74.125.82.42]) by dpdk.org (Postfix) with ESMTP id EE1671B3AE for ; Thu, 12 Oct 2017 16:02:39 +0200 (CEST) Received: by mail-wm0-f42.google.com with SMTP id t69so13509837wmt.2 for ; Thu, 12 Oct 2017 07:02:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=DfaLFBxM5pcqH4kbeAw9L28r1Jmu01EpgUmIUzz6YJ0=; b=GMbsvSXryTzM62AjNv+C27bqoobRDi1i+TStNhF0l4wHVbbruPUkO6ocYq/9tIv9mh 7tpSckhEDINhnMBiCXaoBcLiis9bIq7uYZJUm92XHvorXU1ya8KZ6CAJLlb0YYlO/OwU haFtl2VKehkGBNQ7KIk62up35bvIfa+57GSsEiF7yJSCdleDdc8YCH9oC0ciLSh10gM5 Nu2WMUtEB9+/4FhElC4ToElWBtQqTKrjv1figrpl3jTNcp44SlZbI++hWk891b032tUt mCHaChZFQpjyALvNM5JUWzPoRg9yUGOhZE3CgtGPKfjTjaeUUAtrAw73PVD18G4tJwHS syOw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=DfaLFBxM5pcqH4kbeAw9L28r1Jmu01EpgUmIUzz6YJ0=; b=CGw/bJgBgxE7sKs6c5cFgyL2d2lREVpwjZtC3ZihXCR7AbF9FsyXSuMk+08PPadjHp FzXBpXl0XGcpE1cdcf6qK+z5tymu4tQLBUUmu8HMwgpKw8iPd2y7PsxwA5cmtogbOX9I fQYaTJ6b2ondr1r2O/bZAPNFHkypnXONmV1aEaIkq6uQNMcCeyGQmcvwcImwxyxK9Cdt JMJOhtp1+qP6041ka92ax8Zjb+WSm69nvv+hdJXuTGKybmjh432raaSGj5wxYGMD5wlB AGAjZwCRKF6d2mQhBySl3gC0f8VbMwsNWhy1ldmwFWIAA4TqfLyIi35VqW+bE5lwTftP gLrg== X-Gm-Message-State: AMCzsaXcrigXrTS4uavZ13LFvEw7kJQ5m57f500XhvL6jtkSbUDe2WyK UfLTssEz/Jbxwr7TNiMa96oUQl5J X-Google-Smtp-Source: AOwi7QCPjjUJSF8+U2JZL0qg+s3v5fERPDo7ci1nZYwUvFhLsmysu2u31TYZWHpVdjdEly/g7F091A== X-Received: by 10.223.153.130 with SMTP id y2mr2116875wrb.165.1507816959364; Thu, 12 Oct 2017 07:02:39 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id c45sm8554783wrg.97.2017.10.12.07.02.38 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 12 Oct 2017 07:02:38 -0700 (PDT) Date: Thu, 12 Oct 2017 16:02:27 +0200 From: Adrien Mazarguil To: Aaron Conole Cc: Thomas Monjalon , dev@dpdk.org, Ferruh Yigit , Gaetan Rivet , fbl@redhat.com, Neil Horman Message-ID: <20171012140227.GU13551@6wind.com> References: <00db12469380e587ac434bfc5c63795e32cfddef.1507193185.git.adrien.mazarguil@6wind.com> <2075344.22PATxnhkA@xps> <20171011115642.GI13551@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: Thu, 12 Oct 2017 14:02:40 -0000 On Wed, Oct 11, 2017 at 03:05:47PM -0400, Aaron Conole wrote: > 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? 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. -- Adrien Mazarguil 6WIND