From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Rybchenko Subject: Re: [EXT] Re: [PATCH] ethdev: fix DMA zone reserve not honoring size Date: Tue, 2 Apr 2019 10:36:12 +0300 Message-ID: <31ff511d-db25-7a64-63cb-97272f363dc1@solarflare.com> References: <20190331162437.13048-1-pbhagavatula@marvell.com> <105f221f-0c64-e950-df9a-2c9eaa0991c3@solarflare.com> <819b2c86516855fda65403a8e94be9d03c4d7eeb.camel@marvell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Cc: "dev@dpdk.org" , "stable@dpdk.org" To: Jerin Jacob Kollanukkaran , "stephen@networkplumber.org" , "thomas@monjalon.net" , Pavan Nikhilesh Bhagavatula , "ferruh.yigit@intel.com" Return-path: In-Reply-To: <819b2c86516855fda65403a8e94be9d03c4d7eeb.camel@marvell.com> Content-Language: en-GB 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 4/2/19 3:47 AM, Jerin Jacob Kollanukkaran wrote: > On Mon, 2019-04-01 at 10:30 +0300, Andrew Rybchenko wrote: >> External Email >> On 3/31/19 7:25 PM, Pavan Nikhilesh Bhagavatula wrote: >>> From: Pavan Nikhilesh >>> >>> The `rte_eth_dma_zone_reserve()` is generally used to create HW >>> rings. >>> In some scenarios when a driver needs to reconfigure the ring size >>> since the named memzone already exists it returns the previous >>> memzone >>> without checking if a different sized ring is requested. >>> >>> Introduce a check to see if the ring size requested is different >>> from the >>> previously created memzone length. >>> >>> Fixes: 719dbebceb81 ("xen: allow determining DOM0 at runtime") >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Pavan Nikhilesh >>> --- >>> lib/librte_ethdev/rte_ethdev.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/librte_ethdev/rte_ethdev.c >>> b/lib/librte_ethdev/rte_ethdev.c >>> index 12b66b68c..4ae12e43b 100644 >>> --- a/lib/librte_ethdev/rte_ethdev.c >>> +++ b/lib/librte_ethdev/rte_ethdev.c >>> @@ -3604,9 +3604,12 @@ rte_eth_dma_zone_reserve(const struct >>> rte_eth_dev *dev, const char *ring_name, >>> } >>> >>> mz = rte_memzone_lookup(z_name); >>> - if (mz) >>> + if (mz && (mz->len == size)) >>> return mz; >>> >>> + if (mz) >>> + rte_memzone_free(mz); >> >> NACK >> I really don't like that API which should reserve does free if >> requested >> size does not match previously allocated. > Why? Is due to API name? 1. The problem really exists. The problem is bad and it very good that you     caught it and came up with a patch. Many thanks. 2. Silently free and reallocate memory is bad. Memory could be used/mapped etc. 3. As an absolute minimum if we accept the behaviour it must be documented     in the function description. > If so, > Can we have rte_eth_dma_zone_reservere_with_resize() then ? > or any another name, You would like to have? 4. I'd prefer an error if different size (or bigger) memzone is requested,     but I understand that it can break existing drivers. Thomas, Ferruh, what do you think? >> I understand the motivation, but I don't think the solution is >> correct. > What you think it has correct solution then? See above plus handling in drivers or dedicated function with better name as you suggest above. > Obviously, We can not allocate max ring size in init time. > If the NIC has support for 64K HW ring, We will be wasting too much as > it is per queue. Yes, I agree that it is an overkill. net/sfc tries to carefully free/reserve on NIC/queues reconfigure. Many thanks, Andrew.