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)
Message-ID: <20190405080342.yJhhSaU_7vht_afHEWT_JlKysd9QKxfJ9StXnMdTsn0@z> (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?
>
next prev 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).