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

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>]

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Monday, April 1, 2019 3:11 PM
> To: Andrew Rybchenko <arybchenko@solarflare.com>; Pavan Nikhilesh
> Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; thomas@monjalon.net; ferruh.yigit@intel.com;
> stephen@networkplumber.org
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [EXT] Re: [dpdk-dev] [PATCH] ethdev: fix DMA zone reserve not
> honoring size
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 01-Apr-19 10:28 AM, Burakov, Anatoly wrote:
> > On 01-Apr-19 8:30 AM, 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>
> >>> ---
> >>>   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.
> >> I understand the motivation, but I don't think the solution is correct.
> >
> > Why does size change in the first place?
> 
> Never mind, i forgot that NICs can be reconfigured :)
> 

😊

> Currently, there is no way to resize memzones, so freeing and reallocating is
> the only option. Since memzones are backed by regular malloc elements, we
> could add a memzone_resize API. That would help, because all of the
> references to the memzone itself will still be valid, even if memory ends up
> being reallocated.
> 

Agreed, but currently the following drivers use dma_zone_reserve API

drivers/net/iavf
drivers/net/e1000
drivers/net/bnx2x
drivers/net/nfp
drivers/net/atlantic
drivers/net/vmxnet3
drivers/net/thunderx
drivers/net/liquidio
drivers/net/sfc
drivers/net/ixgbe
drivers/net/axgbe
drivers/net/fm10k
drivers/net/i40e
drivers/net/ice

Most of them have a notion that dma_zone_reserve allocates the correct size.
(Although Most of the Intel NIC reserve max possible ring size).

Can we have the free and reserve for this release and move it to resize in the next?
AFAIK most of the existing drivers don’t have a different path for reconfigure and since it is same as configure they 
propagate the dma zone address properly.

> >
> >>
> >>> +
> >>>       return rte_memzone_reserve_aligned(z_name, size, socket_id,
> >>>               RTE_MEMZONE_IOVA_CONTIG, align);
> >>>   }
> >>
> >>
> >
> >
> 
> 
> --
> Thanks,
> Anatoly