From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id D2A1F2935 for ; Wed, 9 Mar 2016 15:59:50 +0100 (CET) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84) (envelope-from ) id 1adfc2-0005yL-WA; Wed, 09 Mar 2016 16:01:23 +0100 To: "Hunt, David" , Panu Matilainen , dev@dpdk.org References: <1455634095-4183-1-git-send-email-david.hunt@intel.com> <1457517037-71693-1-git-send-email-david.hunt@intel.com> <1457517037-71693-5-git-send-email-david.hunt@intel.com> <56DFFF06.3080209@redhat.com> <56E00971.8040905@intel.com> From: Olivier MATZ X-Enigmail-Draft-Status: N1110 Message-ID: <56E03A60.6010807@6wind.com> Date: Wed, 9 Mar 2016 15:59:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.6.0 MIME-Version: 1.0 In-Reply-To: <56E00971.8040905@intel.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 4/4] mempool: add in the RTE_NEXT_ABI for ABI breakages X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Mar 2016 14:59:50 -0000 Hi David, On 03/09/2016 12:30 PM, Hunt, David wrote: > Hi Panu, > > On 3/9/2016 10:46 AM, Panu Matilainen wrote: >> On 03/09/2016 11:50 AM, David Hunt wrote: >>> This patch is for those people who want to be easily able to switch >>> between the new mempool layout and the old. Change the value of >>> RTE_NEXT_ABI in common_base config file >> >> I guess the idea here is to document how to switch between the ABIs >> but to me this reads as if this patch is supposed to change the value >> in common_base. Of course there's no such change included (nor should >> there be) here, but the description could use some fine-tuning perhaps. >> > > You're right, I'll clarify the comments. v4 due soon. > >>> >>> v3: Updated to take re-work of file layouts into consideration >>> >>> v2: Kept all the NEXT_ABI defs to this patch so as to make the >>> previous patches easier to read, and also to imake it clear what >>> code is necessary to keep ABI compatibility when NEXT_ABI is >>> disabled. >> >> Maybe its just me, but: >> I can see why NEXT_ABI is in a separate patch for review purposes but >> for final commit this split doesn't seem right to me. In any case its >> quite a large change for NEXT_ABI. >> > > The patch basically re-introduces the old (pre-mempool) code as the > refactoring of the code would have made the NEXT_ABI additions totally > unreadable. I think this way is the lesser of two evils. > >> In any case, you should add a deprecation notice for the oncoming ABI >> break in 16.07. >> > > Sure, I'll add that in v4. Sorry, maybe I wasn't very clear in my previous messages. For me, the NEXT_ABI is not the proper solution because, as Panu stated, it makes the patch hard to read. My understanding of NEXT_ABI is that it should only be used if the changes are small enough. Duplicating the code with a big #ifdef NEXT_ABI is not an option to me either. So that's why the deprecation notice should be used instead. But in this case, it means that this patch won't be present in 16.04, but will be added in 16.07. Regards, Olivier