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

[Thomas Monjalon <thomas@monjalon.net>]

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.

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