From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 278CBA04F9; Thu, 9 Jan 2020 15:58:26 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id ED0AB1DE6C; Thu, 9 Jan 2020 15:58:25 +0100 (CET) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 467261DE42; Thu, 9 Jan 2020 15:58:24 +0100 (CET) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jan 2020 06:58:23 -0800 X-IronPort-AV: E=Sophos;i="5.69,414,1571727600"; d="scan'208";a="223328998" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.26]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 09 Jan 2020 06:58:20 -0800 Date: Thu, 9 Jan 2020 14:58:17 +0000 From: Bruce Richardson To: "Burakov, Anatoly" Cc: Olivier Matz , dev@dpdk.org, Andrew Rybchenko , stable@dpdk.org Message-ID: <20200109145817.GB255@bricha3-MOBL.ger.corp.intel.com> References: <20200109132720.15664-1-olivier.matz@6wind.com> <8b59b3c9-ac1a-f448-e38d-063a6cb8ba7a@intel.com> <20200109142351.GJ22738@platinum> <6e43ec20-4fa5-55fa-6ca9-2ffca9bcf7dc@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6e43ec20-4fa5-55fa-6ca9-2ffca9bcf7dc@intel.com> User-Agent: Mutt/1.12.1 (2019-06-15) Subject: Re: [dpdk-dev] [PATCH] mempool: fix mempool virt populate with small chunks 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Jan 09, 2020 at 02:29:08PM +0000, Burakov, Anatoly wrote: > On 09-Jan-20 2:23 PM, Olivier Matz wrote: > > On Thu, Jan 09, 2020 at 01:52:41PM +0000, Burakov, Anatoly wrote: > > > On 09-Jan-20 1:27 PM, Olivier Matz wrote: > > > > To populate a mempool with a virtual area, the mempool code calls > > > > rte_mempool_populate_iova() for each iova-contiguous area. It happens > > > > (rarely) that this area is too small to store one object. In this case, > > > > rte_mempool_populate_iova() returns an error, which is forwarded by > > > > rte_mempool_populate_virt(). > > > > > > > > This case should not throw an error in > > > > rte_mempool_populate_virt(). Instead, the area that is too small should > > > > just be ignored. > > > > > > > > To fix this issue, change the return value of > > > > rte_mempool_populate_iova() to -ENOBUFS when no object can be populated, > > > > so it can be ignored by the caller. As this would be an API change, add > > > > a compat wrapper to keep the current API unchanged. The wrapper will be > > > > removed for 20.11. > > > > > > > > Fixes: 354788b60cfd ("mempool: allow populating with unaligned virtual area") > > > > Cc: stable@dpdk.org > > > > > > > > Signed-off-by: Olivier Matz > > > > --- > > > > > > > > > > The approach fixes the issue on my end, so > > > > > > Tested-by: Anatoly Burakov > > > > > > > Is there a simple way to ensure that we won't forget to remove the > > > > wrapper for 20.11? Anatoly suggested me to use versioned symbols, but > > > > it's not clear to me how. > > > > > > > > > > Yes, i'd like to do better than "ah shur we won't forget pinky swear". > > > > > > Can't we do this with ABI versioning? E.g. > > > > > > rte_populate_iova_v20() ... returns EINVAL > > > > > > rte_populate_iova_v21() ... returns ENOBUFS > > > > > > I'm pretty sure, even if it doesn't break, it will still be more likely to > > > not be forgotten because there's almost a guarantee that someone will grep > > > for symbol versioning macros across the codebase around 20.11 timeframe. > > > > Without using symbol versionning, would this be ok too? > > > > int > > rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr, > > rte_iova_t iova, size_t len, rte_mempool_memchunk_free_cb_t *free_cb, > > void *opaque) > > { > > int ret; > > > > ret = __rte_mempool_populate_iova(mp, vaddr, iova, len, free_cb, opaque); > > > > #if RTE_VERSION < RTE_VERSION_NUM(20, 11, 0, 0) > > if (ret == -ENOBUFS) > > ret = -EINVAL; > > #endif > > > > return ret; > > } > > > > > > Well it would certainly work :) it just makes it more likely that this will > be missed. > Depends on your definition of "work". The difference vs having the ABI versioned code is that any application linked against 20.02...20.08 will have the old behaviour and have to change to use 20.11. Using proper versioning, any newly linked apps immediately get the old behaviour and the compatiblity is provided only for those apps linked against 19.11. Therefore, I think the ABI compatibility approach is still better, and should not be that bad, given that I think all that needs to be done for v20 is to call v21 and then convert an -ENOBUFS to -EINVAL as you do here. __mempool_populate_iova_v20(....) { ret = __mempool_populate_iova_v21(...); return ret == -ENOBUFS ? -EINVAL : ret; } Regards, /Bruce