[dpdk-stable] [dpdk-dev] [EXT] Re: [PATCH] ethdev: fix DMA zone reserve not honoring size

[Andrew Rybchenko]

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 <pbhagavatula at 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 at dpdk.org
>>>
>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula at marvell.com>
>>> ---
>>>   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.
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.

> 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.