DPDK patches and discussions
 help / color / mirror / Atom feed
From: Andrew Rybchenko <arybchenko@solarflare.com>
To: Thomas Monjalon <thomas@monjalon.net>, <pbhagavatula@marvell.com>,
	<ferruh.yigit@intel.com>, <jerinj@marvell.com>
Cc: <dev@dpdk.org>, <stephen@networkplumber.org>
Subject: Re: [dpdk-dev] [EXT] Re: [PATCH] ethdev: fix DMA zone reserve not honoring size
Date: Fri, 5 Apr 2019 11:03:42 +0300	[thread overview]
Message-ID: <20921760-a70f-8537-35b3-4e22f4de6a1f@solarflare.com> (raw)
In-Reply-To: <3987869.2tQX2zD6ZG@xps>

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 <pbhagavatula@marvell.com>
>>>>>>>
>>>>>>> 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 <pbhagavatula@marvell.com>
> [...]
>>>>>>> @@ -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?
>

  parent reply	other threads:[~2019-04-05  8:03 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-31 16:25 [dpdk-dev] " Pavan Nikhilesh Bhagavatula
2019-03-31 16:25 ` Pavan Nikhilesh Bhagavatula
2019-04-01  7:30 ` Andrew Rybchenko
2019-04-01  7:30   ` Andrew Rybchenko
2019-04-01  9:28   ` Burakov, Anatoly
2019-04-01  9:28     ` Burakov, Anatoly
2019-04-01  9:40     ` Burakov, Anatoly
2019-04-01  9:40       ` Burakov, Anatoly
2019-04-01 12:12       ` [dpdk-dev] [EXT] " Pavan Nikhilesh Bhagavatula
2019-04-01 12:12         ` Pavan Nikhilesh Bhagavatula
2019-04-02  0:47   ` Jerin Jacob Kollanukkaran
2019-04-02  0:47     ` Jerin Jacob Kollanukkaran
2019-04-02  7:36     ` Andrew Rybchenko
2019-04-02  7:36       ` Andrew Rybchenko
2019-04-02  8:25       ` Jerin Jacob Kollanukkaran
2019-04-02  8:25         ` Jerin Jacob Kollanukkaran
2019-04-02  8:44         ` Andrew Rybchenko
2019-04-02  8:44           ` Andrew Rybchenko
2019-04-04 22:23           ` Thomas Monjalon
2019-04-04 22:23             ` Thomas Monjalon
2019-04-05  8:03             ` Andrew Rybchenko [this message]
2019-04-05  8:03               ` Andrew Rybchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20921760-a70f-8537-35b3-4e22f4de6a1f@solarflare.com \
    --to=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jerinj@marvell.com \
    --cc=pbhagavatula@marvell.com \
    --cc=stephen@networkplumber.org \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).