[1/2] librte_ethdev: Introduce a function to release HW rings
Checks
Commit Message
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(+)
Comments
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)
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.
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?
Kind regards,
Renata
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
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
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
W dniu 05.05.2020 o 19:25, Renata Saiakhova pisze:
> 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.
I don't know. It's up to Anatoly, I guess. But your patch probably will
be required also. You will just need to use some atomic rte_memzone_...
function to avoid race, the function that Anatoly will propose.
>
> 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
>
On 05-May-20 6:25 PM, Renata Saiakhova wrote:
> 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.
>
I wouldn't really count on that API appearing in stable any time soon,
so i suggest proceeding with your approach.
@@ -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);
+
+ return rc;
+}
+
int
rte_eth_dev_create(struct rte_device *device, const char *name,
size_t priv_data_size,
@@ -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
+ * The name of the memory zone
+ * @param queue_id
+ * The index of the queue to add to name
+ * @param size
+ */
+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.
@@ -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;