[dpdk-dev] [PATCH 0/2] Memory corruption due to HW rings allocation

[dpdk-dev] [PATCH 0/2] Memory corruption due to HW rings allocation

[Renata Saiakhova]

igb and ixgbe drivers allocate HW rings using rte_eth_dma_zone_reserve(),
which checks first if the memzone exists for a given name, consisting of port
id, queue_id, rx/tx direction, but not for the size, alignment, and socket_id.
If the memzone with a given name exists it is returned, otherwise it is
allocated.
Disconnecting dpdk port from one type of interface (igb) and connecting it
to another type of interface (ixgbe) for the same port id, potentially creates
memory overlap and corruption, because it may require memzone of bigger size.
That's what is happening from switching from igb to ixgbe having the same port
id.

Renata Saiakhova (2):
  librte_ethdev: Introduce a function to release HW rings
  drivers/net: Fix in e1000 and ixgbe HW rings memory overlap

 drivers/net/e1000/igb_rxtx.c             |  2 ++
 drivers/net/ixgbe/ixgbe_rxtx.c           |  2 ++
 lib/librte_ethdev/rte_ethdev.c           | 23 +++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_driver.h    | 14 ++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  1 +
 5 files changed, 42 insertions(+)

-- 
2.17.1

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

[Renata Saiakhova]

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);
+
+	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
+ *   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.
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;
-- 
2.17.1

[dpdk-dev] [PATCH 2/2] drivers/net: Fix in e1000 and ixgbe HW rings memory overlap

[Renata Saiakhova]

Delete memzones for HW rings in igb and ixgbe while freeing queues

Signed-off-by: Renata Saiakhova <Renata.Saiakhova@ekinops.com>
---
 drivers/net/e1000/igb_rxtx.c   | 2 ++
 drivers/net/ixgbe/ixgbe_rxtx.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c
index 684fa4ad8..fe80c0f0d 100644
--- a/drivers/net/e1000/igb_rxtx.c
+++ b/drivers/net/e1000/igb_rxtx.c
@@ -1884,12 +1884,14 @@ igb_dev_free_queues(struct rte_eth_dev *dev)
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		eth_igb_rx_queue_release(dev->data->rx_queues[i]);
 		dev->data->rx_queues[i] = NULL;
+		rte_eth_dma_zone_free(dev, "rx_ring", i);
 	}
 	dev->data->nb_rx_queues = 0;
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		eth_igb_tx_queue_release(dev->data->tx_queues[i]);
 		dev->data->tx_queues[i] = NULL;
+		rte_eth_dma_zone_free(dev, "tx_ring", i);
 	}
 	dev->data->nb_tx_queues = 0;
 }
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 2e20e18c7..977ecf513 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -3368,12 +3368,14 @@ ixgbe_dev_free_queues(struct rte_eth_dev *dev)
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		ixgbe_dev_rx_queue_release(dev->data->rx_queues[i]);
 		dev->data->rx_queues[i] = NULL;
+		rte_eth_dma_zone_free(dev, "rx_ring", i);
 	}
 	dev->data->nb_rx_queues = 0;
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		ixgbe_dev_tx_queue_release(dev->data->tx_queues[i]);
 		dev->data->tx_queues[i] = NULL;
+		rte_eth_dma_zone_free(dev, "tx_ring", i);
 	}
 	dev->data->nb_tx_queues = 0;
 }
-- 
2.17.1

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

[Burakov, Anatoly]

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)


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH 2/2] drivers/net: Fix in e1000 and ixgbe HW rings memory overlap

[Burakov, Anatoly]

On 03-May-20 5:26 PM, Renata Saiakhova wrote:
> Delete memzones for HW rings in igb and ixgbe while freeing queues
> 
> Signed-off-by: Renata Saiakhova <Renata.Saiakhova@ekinops.com>
> ---

+Thomas

Should this perhaps be fixed in all drivers, not just ixgbe/igb? Is this 
safe to do in multiprocess? I'm not too well versed in ethdev mechanics 
when it comes to multiprocess, presumably the application itself is 
responsible for synchronizing access to ports, so freeing the resources 
should be OK?

-- 
Thanks,
Anatoly

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

[Burakov, Anatoly]

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

Re: [dpdk-dev] [PATCH 2/2] drivers/net: Fix in e1000 and ixgbe HW rings memory overlap

[Thomas Monjalon]

05/05/2020 12:28, Burakov, Anatoly:
> On 03-May-20 5:26 PM, Renata Saiakhova wrote:
> > Delete memzones for HW rings in igb and ixgbe while freeing queues
> > 
> > Signed-off-by: Renata Saiakhova <Renata.Saiakhova@ekinops.com>
> > ---
> 
> +Thomas
> 
> Should this perhaps be fixed in all drivers, not just ixgbe/igb? Is this 
> safe to do in multiprocess? I'm not too well versed in ethdev mechanics 
> when it comes to multiprocess, presumably the application itself is 
> responsible for synchronizing access to ports, so freeing the resources 
> should be OK?

The application is responsible of port policy.
If the application decides to close a port,
it must be safe in all threads and processes,
meaning it is application responsibility to not refer to port resources.

About fixing in all drivers, is it something missing in other drivers?

Re: [dpdk-dev] [PATCH 0/2] Memory corruption due to HW rings allocation

[Thomas Monjalon]

03/05/2020 18:26, Renata Saiakhova:
> igb and ixgbe drivers allocate HW rings using rte_eth_dma_zone_reserve(),
> which checks first if the memzone exists for a given name, consisting of port
> id, queue_id, rx/tx direction, but not for the size, alignment, and socket_id.
> If the memzone with a given name exists it is returned, otherwise it is
> allocated.
> Disconnecting dpdk port from one type of interface (igb) and connecting it
> to another type of interface (ixgbe) for the same port id, potentially creates
> memory overlap and corruption, because it may require memzone of bigger size.
> That's what is happening from switching from igb to ixgbe having the same port
> id.

I don't understand the use case.
Do you mean you close one port and open another one,
so the port id is recycled by ethdev?

Please could you elaborate with some code snippet?

Re: [dpdk-dev] [PATCH 0/2] Memory corruption due to HW rings allocation

[Renata Saiakhova]

Hi Thomas,

In our application dpdk port can be connected and disconnected:

Connect:
new_port_id = netdev_dpdk_get_port_by_devargs(dpdk_port->pci_addr_str);

   if (!rte_eth_dev_is_valid_port(new_port_id)) {
      /* Device not found in DPDK, attempt to attach it */
      if (rte_dev_probe(dpdk_port->pci_addr_str)) {
         new_port_id = DPDK_ETH_PORT_ID_INVALID;
      } else {
         new_port_id = netdev_dpdk_get_port_by_devargs(dpdk_port->pci_addr_str);
         if (rte_eth_dev_is_valid_port(new_port_id)) {
            LO_TRACE("Device '%s' attached to DPDK", dpdk_port->pci_addr_str);
         } else {
            /* Attach unsuccessful */
            new_port_id = DPDK_ETH_PORT_ID_INVALID;
         }
      }
   }

Disconnect:

   dp_ConfigureRxQueues(dpdk_port, FALSE);
   rte_eth_dev_set_link_down(port_id);
   rte_eth_dev_stop(port_id);
   rte_eth_dev_close(port_id);

and yes, exactly, port id is recycled by eth_dev. Next time, switching from igb port to ixgbe with the same port and using HW rings of bigger size for ixgbe creates memory corruption.

Kind regards,
Renata



________________________________
From: Thomas Monjalon <thomas@monjalon.net>
Sent: Tuesday, May 5, 2020 1:01 PM
To: Renata Saiakhova <renata.saiakhova@ekinops.com>
Cc: dev@dpdk.org <dev@dpdk.org>; ferruh.yigit@intel.com <ferruh.yigit@intel.com>; arybchenko@solarflare.com <arybchenko@solarflare.com>
Subject: Re: [dpdk-dev] [PATCH 0/2] Memory corruption due to HW rings allocation

03/05/2020 18:26, Renata Saiakhova:
> igb and ixgbe drivers allocate HW rings using rte_eth_dma_zone_reserve(),
> which checks first if the memzone exists for a given name, consisting of port
> id, queue_id, rx/tx direction, but not for the size, alignment, and socket_id.
> If the memzone with a given name exists it is returned, otherwise it is
> allocated.
> Disconnecting dpdk port from one type of interface (igb) and connecting it
> to another type of interface (ixgbe) for the same port id, potentially creates
> memory overlap and corruption, because it may require memzone of bigger size.
> That's what is happening from switching from igb to ixgbe having the same port
> id.

I don't understand the use case.
Do you mean you close one port and open another one,
so the port id is recycled by ethdev?

Please could you elaborate with some code snippet?

Re: [dpdk-dev] [PATCH 2/2] drivers/net: Fix in e1000 and ixgbe HW rings memory overlap

[Burakov, Anatoly]

On 05-May-20 11:59 AM, Thomas Monjalon wrote:
> 05/05/2020 12:28, Burakov, Anatoly:
>> On 03-May-20 5:26 PM, Renata Saiakhova wrote:
>>> Delete memzones for HW rings in igb and ixgbe while freeing queues
>>>
>>> Signed-off-by: Renata Saiakhova <Renata.Saiakhova@ekinops.com>
>>> ---
>>
>> +Thomas
>>
>> Should this perhaps be fixed in all drivers, not just ixgbe/igb? Is this
>> safe to do in multiprocess? I'm not too well versed in ethdev mechanics
>> when it comes to multiprocess, presumably the application itself is
>> responsible for synchronizing access to ports, so freeing the resources
>> should be OK?
> 
> The application is responsible of port policy.
> If the application decides to close a port,
> it must be safe in all threads and processes,
> meaning it is application responsibility to not refer to port resources.
> 
> About fixing in all drivers, is it something missing in other drivers?
> 

I can see several other drivers using the dma_reserve API, so presumably 
they would suffer from the same issue, unless they use this API for a 
different purpose.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH 0/2] Memory corruption due to HW rings allocation

[Thomas Monjalon]

05/05/2020 13:19, Renata Saiakhova:
> Hi Thomas,
> 
> In our application dpdk port can be connected and disconnected:
> 
> Connect:
> new_port_id = netdev_dpdk_get_port_by_devargs(dpdk_port->pci_addr_str);
> 
>    if (!rte_eth_dev_is_valid_port(new_port_id)) {
>       /* Device not found in DPDK, attempt to attach it */
>       if (rte_dev_probe(dpdk_port->pci_addr_str)) {
>          new_port_id = DPDK_ETH_PORT_ID_INVALID;
>       } else {
>          new_port_id = netdev_dpdk_get_port_by_devargs(dpdk_port->pci_addr_str);
>          if (rte_eth_dev_is_valid_port(new_port_id)) {
>             LO_TRACE("Device '%s' attached to DPDK", dpdk_port->pci_addr_str);
>          } else {
>             /* Attach unsuccessful */
>             new_port_id = DPDK_ETH_PORT_ID_INVALID;
>          }
>       }
>    }
> 
> Disconnect:
> 
>    dp_ConfigureRxQueues(dpdk_port, FALSE);
>    rte_eth_dev_set_link_down(port_id);
>    rte_eth_dev_stop(port_id);
>    rte_eth_dev_close(port_id);
> 
> and yes, exactly, port id is recycled by eth_dev. Next time, switching from igb port to ixgbe with the same port and using HW rings of bigger size for ixgbe creates memory corruption.

I see.

Reusing the same rings for a new port seems really wrong indeed.
Please check if same issue must be fixed in other PMDs.

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

[Renata Saiakhova]

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
________________________________
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

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

[Burakov, Anatoly]

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

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

[Lukasz Wojciechowski]

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

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

[Renata Saiakhova]

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

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

[Lukasz Wojciechowski]

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
>
-- 

Lukasz Wojciechowski
Principal Software Engineer

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

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

[Burakov, Anatoly]

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.

-- 
Thanks,
Anatoly