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

[Renata Saiakhova <renata.saiakhova@ekinops.com>]

Hi Lukasz,

thanks for your comments! I understand Anatoly is going to to make a fix rather in memzone API level, to introduce atomic "find and allocate" and "find and free" operations, so this patch code won't stay long and in this case maybe better not to disturb the original code of rte_eth_dma_zone_reserve() ? Just a question.

Kind regards,
Renata
________________________________
From: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Sent: Tuesday, May 5, 2020 5:47 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

Hi Renata,

few minor hints inline

W dniu 03.05.2020 o 18:26, Renata Saiakhova pisze:
> 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);

probably rc=snprintf(...)

but maybe create a macro for that fancy memzone name as the same code
appears already in rte_eth_dma_zone_reserve and keeping it in one place
seems to be better idea to me.

> +     if (rc >= RTE_MEMZONE_NAMESIZE) {
> +             RTE_ETHDEV_LOG(ERR, "ring name too long\n");
> +             rte_errno = ENAMETOOLONG;
> +             return NULL;
It's an int returning function so instead of setting rte_errno, just:
     return -ENAMETOOLONG;
> +     }
> +
> +     mz = rte_memzone_lookup(z_name);
> +     if (mz)
> +             rc = rte_memzone_free(mz);
> +
> +     return rc;
> +}
> +
>   int
>   rte_eth_dev_create(struct rte_device *device, const char *name,
>        size_t priv_data_size,
> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index 99d4cd6cd..2769a185b 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -180,6 +180,20 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *eth_dev, const char *name,
>                         uint16_t queue_id, size_t size,
>                         unsigned align, int socket_id);
>
> +/**
> + * Free previously allocated memzone for HW rings.
> + *
> + * @param eth_dev
> + *   The *eth_dev* pointer is the address of the *rte_eth_dev* structure
param name is dev
> + * @param name
> + *   The name of the memory zone
param name is ring_name
> + * @param queue_id
> + *   The index of the queue to add to name
> + * @param size
There is no size param, but some info about return would be probably nice:
  * @return
*   Negative errno value on error, 0 on success.


And as it's a new API function maybe it should be experimental. Should it?

> + */
> +int rte_eth_dma_zone_free(const struct rte_eth_dev *dev, const char *ring_name,
> +              uint16_t queue_id);
> +
>   /**
>    * @internal
>    * Atomically set the link status for the specific device.
> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
> index 715505604..5d3c209bc 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -82,6 +82,7 @@ DPDK_20.0 {
>        rte_eth_dev_vlan_filter;
>        rte_eth_devices;
>        rte_eth_dma_zone_reserve;
> +     rte_eth_dma_zone_free;
>        rte_eth_find_next;
>        rte_eth_find_next_owned_by;
>        rte_eth_iterator_cleanup;

Best regards

Lukasz

--

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com