From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 40780A0679 for ; Tue, 2 Apr 2019 10:44:44 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0B01C34F3; Tue, 2 Apr 2019 10:44:44 +0200 (CEST) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id F0F982C36; Tue, 2 Apr 2019 10:44:42 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us1.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id CACBD140069; Tue, 2 Apr 2019 08:44:41 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Tue, 2 Apr 2019 09:44:35 +0100 To: Jerin Jacob Kollanukkaran , "stephen@networkplumber.org" , "thomas@monjalon.net" , Pavan Nikhilesh Bhagavatula , "ferruh.yigit@intel.com" CC: "dev@dpdk.org" , "stable@dpdk.org" References: <20190331162437.13048-1-pbhagavatula@marvell.com> <105f221f-0c64-e950-df9a-2c9eaa0991c3@solarflare.com> <819b2c86516855fda65403a8e94be9d03c4d7eeb.camel@marvell.com> <31ff511d-db25-7a64-63cb-97272f363dc1@solarflare.com> <12cbd37b9f47b234459892f8374ac95616070638.camel@marvell.com> From: Andrew Rybchenko Message-ID: Date: Tue, 2 Apr 2019 11:44:32 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: <12cbd37b9f47b234459892f8374ac95616070638.camel@marvell.com> Content-Language: en-GB X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24526.003 X-TM-AS-Result: No-30.556500-8.000000-10 X-TMASE-MatchedRID: /77LoUQXvQ8OwH4pD14DsPHkpkyUphL9+WzVGPiSY8gbVUVEY6U/r0oG fkhxJr6Wq1SffpAHzg582LSyel7BroBoGyFs+F1M84dsinZ5e1iH7D1bP/FcOhER5MrEGoh5oHM E123LGpOEBoUDDGMnVx6mHHBXlfPgsEBAuoaUqK+iAZ3zAhQYgrqGBW9J0Yqjh8BhJvgqWBkCkF PsvUlhGHILnXUBr+uu+X6JFUGKUhcTZlFtXatzcRWCVBr+Ay98bveZreOw8zZaW2Ktn+I8/vG39 9+Ui/tdanMRc/pu7fpQWd2CSdVLGuJFvnMb8Rr3zY7Fk1CTrPH/lBG+uXYJkBKs4drB12Rx0QxB l5kNTScsrV1+ql6/GC24qsAWZ/7flvnH+pxJKEpjoaO27r+3fTQM9EfFyYyLTErrGhOiMLjSpsi eekGtxDJ0lHjwzUMziSQL5szRvdHOrVNaM4+O1+J28Kqpjndpfuoqrh8mjZmY5NBG7YIbVywpAZ mF/F0tHUP2d91YdOvhhLoXIby/KPJRA2JqAZKz2x/FmlC/aoyb/LTS0T1K1r/DPenuktW9LFlXZ d2HA7Dt80qCYQSwTWqWebqPCncPOHOT3z1mU7a628cXbnOhTw3mvPKZj/z+Oyxd6V963VS48WuR FQVdauLzNWBegCW2RYvisGWbbS+3sNbcHjySQTsAVzN+Ov/sLsfGyLodGYY4tx7U3hS+yIRXHC2 cqPxuAcI6j5I8aKhdUB/58JDjrw== X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--30.556500-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24526.003 X-MDID: 1554194682-MRFraz0vCuRn Content-Type: text/plain; charset="UTF-8"; format="flowed" Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [EXT] Re: [PATCH] ethdev: fix DMA zone reserve not honoring size 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" Message-ID: <20190402084432.nbOtJN9SlIZu25FbA6zZsJ7VMKcWLqO97S88BgsB4yY@z> On 4/2/19 11:25 AM, Jerin Jacob Kollanukkaran wrote: > On Tue, 2019-04-02 at 10:36 +0300, Andrew Rybchenko wrote: >> 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. > If I understand it correctly, Its been used while configuring > the device and it is per queue, If so, Is there any case where > memory in use in parallel in real world case with DPDK? "in real world case with DPDK" is very fragile justification. I simply don't want to dig in this way since it is very easy to make a mistake or simply false assumption. >> 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. > Handling in driver means return error? Yes. > Regarding API, Yes, We can add new API. What we will do that exiting > driver. Is up to driver maintainers to use the new API. I am fine with > either approach, Just asking the opinion. You have mine, but I'd like to know what other ethdev maintainers think about it. >>> 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.