From: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
To: "stephen@networkplumber.org" <stephen@networkplumber.org>,
"thomas@monjalon.net" <thomas@monjalon.net>,
"arybchenko@solarflare.com" <arybchenko@solarflare.com>,
Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>,
"ferruh.yigit@intel.com" <ferruh.yigit@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [EXT] Re: [PATCH] ethdev: fix DMA zone reserve not honoring size
Date: Tue, 2 Apr 2019 08:25:09 +0000 [thread overview]
Message-ID: <12cbd37b9f47b234459892f8374ac95616070638.camel@marvell.com> (raw)
Message-ID: <20190402082509.qopd14PQU0MYnom78xMDh-TZbq1UT7pjIezw0944WBo@z> (raw)
In-Reply-To: <31ff511d-db25-7a64-63cb-97272f363dc1@solarflare.com>
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:
> > > External Email
> > > 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.
> >
> > 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.
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?
> 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.
Handling in driver means return error?
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.
>
> > 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.
next prev parent reply other threads:[~2019-04-02 8:25 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 [this message]
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
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=12cbd37b9f47b234459892f8374ac95616070638.camel@marvell.com \
--to=jerinj@marvell.com \
--cc=arybchenko@solarflare.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=pbhagavatula@marvell.com \
--cc=stable@dpdk.org \
--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).