On 05-May-20 1:49 PM, Renata Saiakhova wrote:
Hi Anatoly,
thanks! The fact that memzone is found and matched only based on the
name, could it create potential problem like the one I described besides
HW rings?
Technically, yes, it could. However, at this point, we can't really do
anything. Memzones having names is both an advantage (less code to find
memory areas you're looking for) and a disadvantage (lots of ways to
shoot yourself in the foot *if* you have a habit of naming different
memzones with identical names... which our drivers apparently do!).
IMO, trying to fix it will lead us down the rabbit hole of, 1) check if
memzone exists, 2) check if it matches the requirements, and 3) handle
all possible conditions related to 1) and 2) (do the requirements match?
do they have to match *exactly*, or "this or bigger" will do? do we
throw an error if we detect mismatch? do we deallocate old memzone?). So
from our perspective, it is better to leave it up to the user, and just
ensure that our drivers do mostly sane things.
Kind regards,
Renata
------------------------------------------------------------------------
*From:* Burakov, Anatoly <anatoly.bura...@intel.com>
*Sent:* Tuesday, May 5, 2020 12:45 PM
*To:* Renata Saiakhova <renata.saiakh...@ekinops.com>; dev@dpdk.org
<dev@dpdk.org>
*Subject:* Re: [dpdk-dev] [PATCH 1/2] librte_ethdev: Introduce a
function to release HW rings
On 05-May-20 11:25 AM, Burakov, Anatoly wrote:
On 03-May-20 5:26 PM, Renata Saiakhova wrote:
Free previously allocated memzone for HW rings
Signed-off-by: Renata Saiakhova <renata.saiakh...@ekinops.com>
---
lib/librte_ethdev/rte_ethdev.c | 23 +++++++++++++++++++++++
lib/librte_ethdev/rte_ethdev_driver.h | 14 ++++++++++++++
lib/librte_ethdev/rte_ethdev_version.map | 1 +
3 files changed, 38 insertions(+)
diff --git a/lib/librte_ethdev/rte_ethdev.c
b/lib/librte_ethdev/rte_ethdev.c
index 72aed59a5..c6d27e1aa 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4206,6 +4206,29 @@ rte_eth_dma_zone_reserve(const struct
rte_eth_dev *dev, const char *ring_name,
RTE_MEMZONE_IOVA_CONTIG, align);
}
+int
+rte_eth_dma_zone_free(const struct rte_eth_dev *dev, const char
*ring_name,
+ uint16_t queue_id)
+{
+ char z_name[RTE_MEMZONE_NAMESIZE];
+ const struct rte_memzone *mz;
+ int rc = 0;
+
+ snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s",
+ dev->data->port_id, queue_id, ring_name);
+ if (rc >= RTE_MEMZONE_NAMESIZE) {
+ RTE_ETHDEV_LOG(ERR, "ring name too long\n");
+ rte_errno = ENAMETOOLONG;
+ return NULL;
+ }
+
+ mz = rte_memzone_lookup(z_name);
+ if (mz)
+ rc = rte_memzone_free(mz);
This is racy. Please just use rte_memzone_free() unconditionally. It'll
return 0 if memzone existed, or will set rte_errno to EINVAL if it
didn't. (this is suboptimal, it should be ENOENT, but changing this
would be an API break... I'll submit a patch for future release to fix
this)
My apologies, just using rte_memzone_free will not solve the problem
because you don't have memzone pointer. Now that i think of it, the
rte_eth_dma_zone_reserve() suffers from this issue too, and the problem
is lack of atomic "find or create" memzone API.
This patch is OK for now, as it follows similar code in
rte_eth_dma_zone_reserve(), but ideally, this should be fixed at the
memzone API level. I'll see if i can cobble together a quick patchset
adding atomic "find or reserve" and "find and free" operations.
--
Thanks,
Anatoly
--
Thanks,
Anatoly
