From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v4 1/2] mbuf: support attaching external buffer to mbuf Date: Wed, 25 Apr 2018 00:15:39 +0200 Message-ID: <1852150.zQNNKj7iQK@xps> References: <20180310012532.15809-1-yskoh@mellanox.com> <1963503.R6hUdDjUTB@xps> <20180424215340.GA88208@yongseok-MBP.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: Yongseok Koh , wenzhuo.lu@intel.com, jingjing.wu@intel.com, dev@dpdk.org, konstantin.ananyev@intel.com, adrien.mazarguil@6wind.com, nelio.laranjeiro@6wind.com To: Olivier Matz , Andrew Rybchenko Return-path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 729751C01 for ; Wed, 25 Apr 2018 00:15:42 +0200 (CEST) In-Reply-To: <20180424215340.GA88208@yongseok-MBP.local> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 24/04/2018 23:53, Yongseok Koh: > On Tue, Apr 24, 2018 at 10:22:45PM +0200, Thomas Monjalon wrote: > > 24/04/2018 21:15, Olivier Matz: > > > On Tue, Apr 24, 2018 at 09:21:00PM +0300, Andrew Rybchenko wrote: > > > > On 04/24/2018 07:02 PM, Olivier Matz wrote: > > > > > On Tue, Apr 24, 2018 at 03:28:33PM +0300, Andrew Rybchenko wrote: > > > > > > On 04/24/2018 04:38 AM, Yongseok Koh wrote: > > > > > > > + * Returns TRUE if given mbuf is cloned by mbuf indirection, or FALSE > > > > > > > + * otherwise. > > > > > > > + * > > > > > > > + * If a mbuf has its data in another mbuf and references it by mbuf > > > > > > > + * indirection, this mbuf can be defined as a cloned mbuf. > > > > > > > + */ > > > > > > > +#define RTE_MBUF_CLONED(mb) ((mb)->ol_flags & IND_ATTACHED_MBUF) > > > > > > > + > > > > > > > +/** > > > > > > > * Returns TRUE if given mbuf is indirect, or FALSE otherwise. > > > > > > > */ > > > > > > > -#define RTE_MBUF_INDIRECT(mb) ((mb)->ol_flags & IND_ATTACHED_MBUF) > > > > > > > +#define RTE_MBUF_INDIRECT(mb) RTE_MBUF_CLONED(mb) > > > > > > It is still confusing that INDIRECT != !DIRECT. > > > > > > May be we have no good options right now, but I'd suggest to at least > > > > > > deprecate > > > > > > RTE_MBUF_INDIRECT() and completely remove it in the next release. > > > > > Agree. I may have missed something, but is my previous suggestion > > > > > not doable? > > > > > > > > > > - direct = embeds its own data (and indirect = !direct) > > > > > - clone (or another name) = data is another mbuf > > > > > - extbuf = data is in an external buffer > > > > > > > > I guess the problem that it changes INDIRECT semantics since EXTBUF > > > > is added as well. I think strictly speaking it is an API change. > > > > Is it OK to make it without announcement? > > > > > > In any case, there will be an ABI change, because an application > > > compiled for 18.02 will not be able to handle these new kind of > > > mbuf. > > > > > > So unfortunatly yes, I think this kind of changes should first be > > > announced. > > > > > > Thomas, what do you think? > > > > What is the impact for the application developer? > > Is there something to change in the application after this patch? > > Let me address two concerns discussed here. > > 1) API breakage of RTE_MBUF_DIRECT() > Previously, direct == !indirect but now direct == !indirect && !extbuf. But to > set the new flag (EXT_ATTACHED_MBUF), the new API, rte_pktmbuf_attach_extbuf() > should be used and it is experimental. If application isn't compiled without > allowing experimental API or application doesn't use the new API, it is always > true that direct == !indirect. It looks logically okay to me. And FYI, it passed > the mbuf_autotest. > > 2) ABI breakage of mlx5's new Multi-Packet RQ (a.k.a MPRQ) feature > It's right that it could breadk ABI if the PMD delivers packets with external > buffer attached. But, the MPRQ feature is disabled by default and it can be > enabled only by the newly introduced PMD parameter (mprq_en). So, there's no > possibility that 18.02-based application receives a mbuf having an external > buffer. And, like Olivier mentioned, there's another ABI breakage by removing > control mbuf anyway. > > So, I don't think there's need for developers to change their application after > this patch unless they want to use the new feature. To summarize, this a feature addition, there is no breakage. So I don't see what should be announced. I think it could be integrated as experimental with a first PMD implementation in 18.05. It will allow to test the feature in the field, and have more feedbacks about how to improve the API.