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 1D02DA0679 for ; Fri, 5 Apr 2019 10:03:56 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CED6D1B46A; Fri, 5 Apr 2019 10:03:54 +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 617CB1B469 for ; Fri, 5 Apr 2019 10:03:53 +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-us3.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id A84D29C0059; Fri, 5 Apr 2019 08:03:51 +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; Fri, 5 Apr 2019 09:03:45 +0100 To: Thomas Monjalon , , , CC: , References: <20190331162437.13048-1-pbhagavatula@marvell.com> <12cbd37b9f47b234459892f8374ac95616070638.camel@marvell.com> <3987869.2tQX2zD6ZG@xps> From: Andrew Rybchenko Message-ID: <20921760-a70f-8537-35b3-4e22f4de6a1f@solarflare.com> Date: Fri, 5 Apr 2019 11:03:42 +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: <3987869.2tQX2zD6ZG@xps> 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-24532.003 X-TM-AS-Result: No-35.781900-8.000000-10 X-TMASE-MatchedRID: gTucSmrmRMMOwH4pD14DsPHkpkyUphL9+WzVGPiSY8jIyrowERRwrYe+ CwLSDva2ydnj7RC0aRpObVtJnKvgfNwrRLDk2cbiDB+ErBr0bAMk9Lf/qe8xxzw2Aod4Vc3hwpY 6xWoyv5cgvYm4kFeJvSf0P3B4TwJKifsNgXDdrIbY5KPiokD1BtrBBrGqBGflV6KWY8jugmWoBf Cx2HzmBnF7+YArNpT4P4VUV23QkHotg1fUv6/cnjiPEKUh+xB+ldyb6Gmq6siTMTaQzhvoep2Tm HG0Cs48Uh+vTTCK6MpUFwdbPogKxcK8bCqGv6ntT2Fx8ldJiRU9KBt6tnyFcoAfY3cVWvvJIA6D 0Tft3i/sUYQXO6SsxuLpee4QwI3en+wMjH6Gua6Ua50su1E7WyHAogh2SU5XjTSbSnelUZIj/FE H1XArqBdki+J3a14BgSqZzLl6LMoxFdXyW/P+XbiMC5wdwKqdfkuZtv/FS5rceXQ6q2ggSn9pEr Ozu0Q/BqqLzUHO3wZWQYoAtT7byhO8OBz9xh4fHALNe6wrjbEsL3b83U5aWQeLCIX046iBSrJTO 1VGhMF2+AZuxakJy89fuxSQqqSyyA1ihY42R28vj6wHfIGxyQhRCJFb9cuseVCAkHLwRePFM83S LuKMYTcbGhe9dAj/Fso+F38Vvny6Mkw4ZNOIMgcbMHjYNxGhhZApJAdFDDabKItl61J/ycnjLTA /UDoAoTCA5Efyn8AiEOZmeUqhzzFcShubSG95lqnq0m3UwjtyMxYoJxlC3FrT5k5eD8D2Zg6GuS S4vRY= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--35.781900-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24532.003 X-MDID: 1554451432-g5Ymy0z1onxS 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: <20190405080342.yJhhSaU_7vht_afHEWT_JlKysd9QKxfJ9StXnMdTsn0@z> On 4/5/19 1:23 AM, Thomas Monjalon wrote: > Hi, > > 02/04/2019 10:44, Andrew Rybchenko: >> 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: >>>>>> 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 > [...] >>>>>>> @@ -3604,9 +3604,12 @@ rte_eth_dma_zone_reserve(const struct >>>>>>> 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. > I don't agree that the problem exists. > You are just trying to use a function for a goal which is > documented as not supported. The documentation says nothing about size, alignment and different socket. It is good that the behaviour is documented, but I can't say that it is friendly. Friendly behaviour would guarantee size, alignment and socket_id properties preserved. Otherwise, it is too error-prone. >>>> 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. > I agree. > A function, with "reserve" in the name, should not do any "free". > >>>> 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. > Yes some drivers may rely on the current behaviour. > But if you carefully check every drivers, you can change > this behaviour and return an error. > >>>> 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. > In such case, I refer to the existing documentation. > For rte_eth_dma_zone_reserve, it says: > " > If the memzone is already created, then this function returns a ptr > to the old one. > " Now I'm more confident that an error should be returned if memzone already exists but its properties do not match requested. >>>>> 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. > Yes, using rte_memzone_free looks saner. > Is there an API missing? > A function to check the size of the memzone? Is rte_memzone.len enough? >