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 11:44:32 +0300 Message-ID: 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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit 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: <12cbd37b9f47b234459892f8374ac95616070638.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 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.