From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) by dpdk.org (Postfix) with ESMTP id 8E9F51B1FD for ; Fri, 5 Apr 2019 00:23:55 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 34DB8231F4; Thu, 4 Apr 2019 18:23:55 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Thu, 04 Apr 2019 18:23:55 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=b6V82jJbbElsIv3gzcfT6ULl36eL3idvg1mP3SxZwF8=; b=P2LRgQhDPcde DVsziX5eTVwHY/rHQoy6cyAp8a/U2rysSBkYHFy/8FiumtmoK836Orst2Vzedwu9 jxLxOscChSEKzOWwGF4wRwwQwZQoIn04M7XOURekBMaRO4hHlvoVpf2bcO6t2siM EWLNpJCwkslOUUry3RcnpScWuQsAmrA= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; bh=b6V82jJbbElsIv3gzcfT6ULl36eL3idvg1mP3SxZw F8=; b=wb1sH9VfmL9gzWXL95s4ZVFJLn2QLaVURCZEyTCA0UcCaf0sLqFMNH0CI uUss80SVO2WDttMuCwN/WFwuuOTbHuZuNmY6g+3XJYmUU0y7c4/lOH+6p+uHs6nr NSQLEvt1L6sDEfsKAL0atpIzBAwWDJJeE9EWqIMjVPI5HDoXNw7iH1coPCRvhrQT uWGe1+JIvUQkBMxSxdY3meC7chMvBo0yQxioLa5Xvgo4VyDaygC4URcol4iROZHi Syp3HjcI79t5wsFSuj1cHUKGaPTO57nCeLeX/X+k+4GY22HAZnQnZYJM7jaANQpH hSbSfO+2mQmkx1rKQC9lT/TK0EwCg== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduuddrtdeigddtjeculddtuddrgedutddrtddtmd cutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdp uffrtefokffrpgfnqfghnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivg hnthhsucdlqddutddtmdenucfjughrpefhvffufffkjghfggfgtgesthfuredttddtvden ucfhrhhomhepvfhhohhmrghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrg hlohhnrdhnvghtqeenucffohhmrghinhepohhvvghrkhhilhhlrdhnvghtnecukfhppeej jedrudefgedrvddtfedrudekgeenucfrrghrrghmpehmrghilhhfrhhomhepthhhohhmrg hssehmohhnjhgrlhhonhdrnhgvthenucevlhhushhtvghrufhiiigvpedt X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 7912710319; Thu, 4 Apr 2019 18:23:53 -0400 (EDT) From: Thomas Monjalon To: Andrew Rybchenko , pbhagavatula@marvell.com, ferruh.yigit@intel.com, jerinj@marvell.com Cc: dev@dpdk.org, stephen@networkplumber.org Date: Fri, 05 Apr 2019 00:23:51 +0200 Message-ID: <3987869.2tQX2zD6ZG@xps> In-Reply-To: References: <20190331162437.13048-1-pbhagavatula@marvell.com> <12cbd37b9f47b234459892f8374ac95616070638.camel@marvell.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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: , X-List-Received-Date: Thu, 04 Apr 2019 22:23:55 -0000 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. > >> 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. " > >>> 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? 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 93FBBA0679 for ; Fri, 5 Apr 2019 00:23:58 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 38B0F1B203; Fri, 5 Apr 2019 00:23:57 +0200 (CEST) Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) by dpdk.org (Postfix) with ESMTP id 8E9F51B1FD for ; Fri, 5 Apr 2019 00:23:55 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 34DB8231F4; Thu, 4 Apr 2019 18:23:55 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Thu, 04 Apr 2019 18:23:55 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=b6V82jJbbElsIv3gzcfT6ULl36eL3idvg1mP3SxZwF8=; b=P2LRgQhDPcde DVsziX5eTVwHY/rHQoy6cyAp8a/U2rysSBkYHFy/8FiumtmoK836Orst2Vzedwu9 jxLxOscChSEKzOWwGF4wRwwQwZQoIn04M7XOURekBMaRO4hHlvoVpf2bcO6t2siM EWLNpJCwkslOUUry3RcnpScWuQsAmrA= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; bh=b6V82jJbbElsIv3gzcfT6ULl36eL3idvg1mP3SxZw F8=; b=wb1sH9VfmL9gzWXL95s4ZVFJLn2QLaVURCZEyTCA0UcCaf0sLqFMNH0CI uUss80SVO2WDttMuCwN/WFwuuOTbHuZuNmY6g+3XJYmUU0y7c4/lOH+6p+uHs6nr NSQLEvt1L6sDEfsKAL0atpIzBAwWDJJeE9EWqIMjVPI5HDoXNw7iH1coPCRvhrQT uWGe1+JIvUQkBMxSxdY3meC7chMvBo0yQxioLa5Xvgo4VyDaygC4URcol4iROZHi Syp3HjcI79t5wsFSuj1cHUKGaPTO57nCeLeX/X+k+4GY22HAZnQnZYJM7jaANQpH hSbSfO+2mQmkx1rKQC9lT/TK0EwCg== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduuddrtdeigddtjeculddtuddrgedutddrtddtmd cutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdp uffrtefokffrpgfnqfghnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivg hnthhsucdlqddutddtmdenucfjughrpefhvffufffkjghfggfgtgesthfuredttddtvden ucfhrhhomhepvfhhohhmrghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrg hlohhnrdhnvghtqeenucffohhmrghinhepohhvvghrkhhilhhlrdhnvghtnecukfhppeej jedrudefgedrvddtfedrudekgeenucfrrghrrghmpehmrghilhhfrhhomhepthhhohhmrg hssehmohhnjhgrlhhonhdrnhgvthenucevlhhushhtvghrufhiiigvpedt X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 7912710319; Thu, 4 Apr 2019 18:23:53 -0400 (EDT) From: Thomas Monjalon To: Andrew Rybchenko , pbhagavatula@marvell.com, ferruh.yigit@intel.com, jerinj@marvell.com Cc: dev@dpdk.org, stephen@networkplumber.org Date: Fri, 05 Apr 2019 00:23:51 +0200 Message-ID: <3987869.2tQX2zD6ZG@xps> In-Reply-To: References: <20190331162437.13048-1-pbhagavatula@marvell.com> <12cbd37b9f47b234459892f8374ac95616070638.camel@marvell.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="UTF-8" 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: <20190404222351.sugSRioZnwscJoAbK_3Z0VtuJyLgntH161GUshyrZrs@z> 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. > >> 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. " > >>> 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?