Re: [dpdk-dev] [PATCH 1/2] librte_ethdev: Introduce a function to release HW rings

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

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.burakov@intel.com>
> *Sent:* Tuesday, May 5, 2020 12:45 PM
> *To:* Renata Saiakhova <renata.saiakhova@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.Saiakhova@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