igb and ixgbe and some other 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. v3->v4: Improve return code in rte_eth_dma_zone_free() in case mz is not found; Release explicitly i40e Flow Director rx/tx rings by calling rte_eth_dma_zone_free(). v2->v3: Remove #undef ETH_DMA_MZONE_NAME and minor changes in code standard. v1->v2: Minor changes on code standard and additional fixes in i40e em and ice drivers. Renata Saiakhova (4): librte_ethdev: Introduce a function to release HW rings drivers/net: Fix in igb and ixgbe HW rings memory drivers/net: Fix in i40e HW rings memory overlap drivers/net: Fix in em and ice HW rings memory overlap drivers/net/e1000/em_rxtx.c | 2 ++ drivers/net/e1000/igb_rxtx.c | 2 ++ drivers/net/i40e/i40e_fdir.c | 3 +++ drivers/net/i40e/i40e_rxtx.c | 2 ++ drivers/net/ice/ice_rxtx.c | 2 ++ drivers/net/ixgbe/ixgbe_rxtx.c | 2 ++ lib/librte_ethdev/rte_ethdev.c | 30 ++++++++++++++++++++++-- lib/librte_ethdev/rte_ethdev_driver.h | 20 ++++++++++++++++ lib/librte_ethdev/rte_ethdev_version.map | 1 + 9 files changed, 62 insertions(+), 2 deletions(-) -- 2.17.2
Free previously allocated memzone for HW rings Signed-off-by: Renata Saiakhova <Renata.Saiakhova@ekinops.com> --- lib/librte_ethdev/rte_ethdev.c | 30 ++++++++++++++++++++++-- lib/librte_ethdev/rte_ethdev_driver.h | 20 ++++++++++++++++ lib/librte_ethdev/rte_ethdev_version.map | 1 + 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 72aed59a5..ec1da2006 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -4181,6 +4181,10 @@ rte_eth_dev_rx_intr_ctl_q_get_fd(uint16_t port_id, uint16_t queue_id) return fd; } +#define ETH_DMA_MZONE_NAME(_name, _port_id, _queue_id, _ring_name) \ + snprintf(_name, sizeof(_name), "eth_p%d_q%d_%s", \ + _port_id, _queue_id, _ring_name) + const struct rte_memzone * rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name, uint16_t queue_id, size_t size, unsigned align, @@ -4190,8 +4194,7 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name, const struct rte_memzone *mz; int rc; - rc = snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s", - dev->data->port_id, queue_id, ring_name); + rc = ETH_DMA_MZONE_NAME(z_name, 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; @@ -4206,6 +4209,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; + + rc = ETH_DMA_MZONE_NAME(z_name, dev->data->port_id, queue_id, ring_name); + if (rc >= RTE_MEMZONE_NAMESIZE) { + RTE_ETHDEV_LOG(ERR, "ring name too long\n"); + return -ENAMETOOLONG; + } + + mz = rte_memzone_lookup(z_name); + if (mz) + rc = rte_memzone_free(mz); + else + rc = -EINVAL; + + 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..462e765d1 100644 --- a/lib/librte_ethdev/rte_ethdev_driver.h +++ b/lib/librte_ethdev/rte_ethdev_driver.h @@ -180,6 +180,26 @@ 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); +/** + * @warning + * @b EXPERIMENTAL: this API may change without prior notice. + * + * 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 + * @return + * Negative errno value on error, 0 on success. + */ +__rte_experimental +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..139a81302 100644 --- a/lib/librte_ethdev/rte_ethdev_version.map +++ b/lib/librte_ethdev/rte_ethdev_version.map @@ -241,4 +241,5 @@ EXPERIMENTAL { __rte_ethdev_trace_rx_burst; __rte_ethdev_trace_tx_burst; rte_flow_get_aged_flows; + rte_eth_dma_zone_free; }; -- 2.17.2
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.2
Delete memzones for HW rings in i40e while freeing queues Signed-off-by: Renata Saiakhova <Renata.Saiakhova@ekinops.com> --- drivers/net/i40e/i40e_fdir.c | 3 +++ drivers/net/i40e/i40e_rxtx.c | 2 ++ 2 files changed, 5 insertions(+) diff --git a/drivers/net/i40e/i40e_fdir.c b/drivers/net/i40e/i40e_fdir.c index d59399afe..3a82cafac 100644 --- a/drivers/net/i40e/i40e_fdir.c +++ b/drivers/net/i40e/i40e_fdir.c @@ -269,6 +269,7 @@ i40e_fdir_teardown(struct i40e_pf *pf) { struct i40e_hw *hw = I40E_PF_TO_HW(pf); struct i40e_vsi *vsi; + struct rte_eth_dev *dev = pf->adapter->eth_dev; vsi = pf->fdir.fdir_vsi; if (!vsi) @@ -280,8 +281,10 @@ i40e_fdir_teardown(struct i40e_pf *pf) if (err) PMD_DRV_LOG(DEBUG, "Failed to do FDIR RX switch off"); i40e_dev_rx_queue_release(pf->fdir.rxq); + rte_eth_dma_zone_free(dev, "fdir_rx_ring", pf->fdir.rxq->queue_id); pf->fdir.rxq = NULL; i40e_dev_tx_queue_release(pf->fdir.txq); + rte_eth_dma_zone_free(dev, "fdir_tx_ring", pf->fdir.txq->queue_id); pf->fdir.txq = NULL; i40e_vsi_release(vsi); pf->fdir.fdir_vsi = NULL; diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c index 5e7c86ed8..99cec9b99 100644 --- a/drivers/net/i40e/i40e_rxtx.c +++ b/drivers/net/i40e/i40e_rxtx.c @@ -2900,6 +2900,7 @@ i40e_dev_free_queues(struct rte_eth_dev *dev) continue; i40e_dev_rx_queue_release(dev->data->rx_queues[i]); dev->data->rx_queues[i] = NULL; + rte_eth_dma_zone_free(dev, "rx_ring", i); } for (i = 0; i < dev->data->nb_tx_queues; i++) { @@ -2907,6 +2908,7 @@ i40e_dev_free_queues(struct rte_eth_dev *dev) continue; i40e_dev_tx_queue_release(dev->data->tx_queues[i]); dev->data->tx_queues[i] = NULL; + rte_eth_dma_zone_free(dev, "tx_ring", i); } } -- 2.17.2
Delete memzones for HW rings in em and ice while freeing queues Signed-off-by: Renata Saiakhova <Renata.Saiakhova@ekinops.com> --- drivers/net/e1000/em_rxtx.c | 2 ++ drivers/net/ice/ice_rxtx.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/drivers/net/e1000/em_rxtx.c b/drivers/net/e1000/em_rxtx.c index 49c53712a..67a271e8c 100644 --- a/drivers/net/e1000/em_rxtx.c +++ b/drivers/net/e1000/em_rxtx.c @@ -1611,12 +1611,14 @@ em_dev_free_queues(struct rte_eth_dev *dev) for (i = 0; i < dev->data->nb_rx_queues; i++) { eth_em_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_em_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/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c index 1c9f31efd..7c2dad9d6 100644 --- a/drivers/net/ice/ice_rxtx.c +++ b/drivers/net/ice/ice_rxtx.c @@ -1905,6 +1905,7 @@ ice_free_queues(struct rte_eth_dev *dev) continue; ice_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; @@ -1913,6 +1914,7 @@ ice_free_queues(struct rte_eth_dev *dev) continue; ice_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.2
On 6/22/2020 10:54 AM, 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 | 30 ++++++++++++++++++++++--
> lib/librte_ethdev/rte_ethdev_driver.h | 20 ++++++++++++++++
> lib/librte_ethdev/rte_ethdev_version.map | 1 +
> 3 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 72aed59a5..ec1da2006 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -4181,6 +4181,10 @@ rte_eth_dev_rx_intr_ctl_q_get_fd(uint16_t port_id, uint16_t queue_id)
> return fd;
> }
>
> +#define ETH_DMA_MZONE_NAME(_name, _port_id, _queue_id, _ring_name) \
> + snprintf(_name, sizeof(_name), "eth_p%d_q%d_%s", \
> + _port_id, _queue_id, _ring_name)
> +
> const struct rte_memzone *
> rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
> uint16_t queue_id, size_t size, unsigned align,
> @@ -4190,8 +4194,7 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
> const struct rte_memzone *mz;
> int rc;
>
> - rc = snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s",
> - dev->data->port_id, queue_id, ring_name);
> + rc = ETH_DMA_MZONE_NAME(z_name, 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;
> @@ -4206,6 +4209,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;
> +
> + rc = ETH_DMA_MZONE_NAME(z_name, dev->data->port_id, queue_id, ring_name);
> + if (rc >= RTE_MEMZONE_NAMESIZE) {
> + RTE_ETHDEV_LOG(ERR, "ring name too long\n");
> + return -ENAMETOOLONG;
> + }
> +
> + mz = rte_memzone_lookup(z_name);
> + if (mz)
> + rc = rte_memzone_free(mz);
> + else
> + rc = -EINVAL;
> +
> + 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..462e765d1 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -180,6 +180,26 @@ 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);
>
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * 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
> + * @return
> + * Negative errno value on error, 0 on success.
> + */
> +__rte_experimental
> +int
> +rte_eth_dma_zone_free(const struct rte_eth_dev *dev, const char *ring_name,
> + uint16_t queue_id);
> +
Hi Renata,
This API is not for applications, only for drivers, right?
We recently (last release ?) introduced 'internal' (__rte_internal) attribute,
we can use '__rte_internal' instead of '__rte_experimental' if the API is not
for applications. This requires adding 'INTERNAL' block in .map file (sample
'lib/librte_eal/rte_eal_version.map')
Also there are a few contribution conventions related issues which can be fixed
while merging but please check them if you will be doing new version:
subsystem is only libname without prefix: 'ethdev'
Commit title description starts with lowercase: 'introduce'.
sample => "ethdev: add function to release HW rings"
Thanks,
ferruh
On 6/22/2020 10:54 AM, Renata Saiakhova wrote: > Free previously allocated memzone for HW rings > > Signed-off-by: Renata Saiakhova <Renata.Saiakhova@ekinops.com> <...> > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * 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 The param names for doxygen and actual param names are not same, causing doxygen warning. "@param eth_dev" -> 'dev' "@param name" -> 'ring_name' > + * @return > + * Negative errno value on error, 0 on success. > + */ > +__rte_experimental > +int > +rte_eth_dma_zone_free(const struct rte_eth_dev *dev, const char *ring_name, > + uint16_t queue_id); > +
Hi Ferruh,
I added an INTERNAL block in .map file, that gives me an error:
Build targets in project: 806
Found ninja-1.8.2 at /usr/bin/ninja
[7/627] Linking target lib/librte_ethdev.so.20.0.3.
FAILED: lib/librte_ethdev.so.20.0.3
ccache cc -o lib/librte_ethdev.so.20.0.3 'lib/lib@@rte_ethdev@sta/librte_ethdev_ethdev_private.c.o' 'lib/lib@@rte_ethdev@sta/librte_ethdev_ethdev_profile.c.o' 'lib/lib@@rte_ethdev@sta/librte_ethdev_ethdev_trace_points.c.o' 'lib/lib@@rte_ethdev@sta/librte_ethdev_rte_class_eth.c.o' 'lib/lib@@rte_ethdev@sta/librte_ethdev_rte_ethdev.c.o' 'lib/lib@@rte_ethdev@sta/librte_ethdev_rte_flow.c.o' 'lib/lib@@rte_ethdev@sta/librte_ethdev_rte_mtr.c.o' 'lib/lib@@rte_ethdev@sta/librte_ethdev_rte_tm.c.o' -Wl,--no-undefined -Wl,--as-needed -Wl,-O1 -shared -fPIC -Wl,--start-group -Wl,-soname,librte_ethdev.so.20.0 -Wl,--no-as-needed -pthread -lm -ldl -lnuma lib/librte_eal.so.20.0.3 lib/librte_kvargs.so.20.0.3 lib/librte_telemetry.so.0.200.3 lib/librte_net.so.20.0.3 lib/librte_mbuf.so.20.0.3 lib/librte_mempool.so.20.0.3 lib/librte_ring.so.20.0.3 lib/librte_meter.so.20.0.3 -Wl,--end-group -Wl,--version-script=/home/renata/work/dpdk/lib/librte_ethdev/rte_ethdev_version.map '-Wl,-rpath,$ORIGIN/' -Wl,-rpath-link,/home/renata/work/dpdk/build/lib
/usr/bin/ld:/home/renata/work/dpdk/lib/librte_ethdev/rte_ethdev_version.map:0: syntax error in VERSION script
collect2: error: ld returned 1 exit status
What could be an issue, is that I need to correct the version somewhere?
KR,
Renata
________________________________
From: Ferruh Yigit <ferruh.yigit@intel.com>
Sent: Monday, June 22, 2020 7:09 PM
To: Renata Saiakhova <renata.saiakhova@ekinops.com>; dev@dpdk.org <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v4 1/4] librte_ethdev: Introduce a function to release HW rings
On 6/22/2020 10:54 AM, 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 | 30 ++++++++++++++++++++++--
> lib/librte_ethdev/rte_ethdev_driver.h | 20 ++++++++++++++++
> lib/librte_ethdev/rte_ethdev_version.map | 1 +
> 3 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 72aed59a5..ec1da2006 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -4181,6 +4181,10 @@ rte_eth_dev_rx_intr_ctl_q_get_fd(uint16_t port_id, uint16_t queue_id)
> return fd;
> }
>
> +#define ETH_DMA_MZONE_NAME(_name, _port_id, _queue_id, _ring_name) \
> + snprintf(_name, sizeof(_name), "eth_p%d_q%d_%s", \
> + _port_id, _queue_id, _ring_name)
> +
> const struct rte_memzone *
> rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
> uint16_t queue_id, size_t size, unsigned align,
> @@ -4190,8 +4194,7 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
> const struct rte_memzone *mz;
> int rc;
>
> - rc = snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s",
> - dev->data->port_id, queue_id, ring_name);
> + rc = ETH_DMA_MZONE_NAME(z_name, 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;
> @@ -4206,6 +4209,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;
> +
> + rc = ETH_DMA_MZONE_NAME(z_name, dev->data->port_id, queue_id, ring_name);
> + if (rc >= RTE_MEMZONE_NAMESIZE) {
> + RTE_ETHDEV_LOG(ERR, "ring name too long\n");
> + return -ENAMETOOLONG;
> + }
> +
> + mz = rte_memzone_lookup(z_name);
> + if (mz)
> + rc = rte_memzone_free(mz);
> + else
> + rc = -EINVAL;
> +
> + 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..462e765d1 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -180,6 +180,26 @@ 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);
>
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * 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
> + * @return
> + * Negative errno value on error, 0 on success.
> + */
> +__rte_experimental
> +int
> +rte_eth_dma_zone_free(const struct rte_eth_dev *dev, const char *ring_name,
> + uint16_t queue_id);
> +
Hi Renata,
This API is not for applications, only for drivers, right?
We recently (last release ?) introduced 'internal' (__rte_internal) attribute,
we can use '__rte_internal' instead of '__rte_experimental' if the API is not
for applications. This requires adding 'INTERNAL' block in .map file (sample
'lib/librte_eal/rte_eal_version.map')
Also there are a few contribution conventions related issues which can be fixed
while merging but please check them if you will be doing new version:
subsystem is only libname without prefix: 'ethdev'
Commit title description starts with lowercase: 'introduce'.
sample => "ethdev: add function to release HW rings"
Thanks,
ferruh
On 6/23/2020 10:11 AM, Renata Saiakhova wrote: > Hi Ferruh, > > I added an INTERNAL block in .map file, that gives me an error: > > Build targets in project: 806 > Found ninja-1.8.2 at /usr/bin/ninja > [7/627] Linking target lib/librte_ethdev.so.20.0.3. > FAILED: lib/librte_ethdev.so.20.0.3 > ccache cc -o lib/librte_ethdev.so.20.0.3 > 'lib/lib@@rte_ethdev@sta/librte_ethdev_ethdev_private.c.o' > 'lib/lib@@rte_ethdev@sta/librte_ethdev_ethdev_profile.c.o' > 'lib/lib@@rte_ethdev@sta/librte_ethdev_ethdev_trace_points.c.o' > 'lib/lib@@rte_ethdev@sta/librte_ethdev_rte_class_eth.c.o' > 'lib/lib@@rte_ethdev@sta/librte_ethdev_rte_ethdev.c.o' > 'lib/lib@@rte_ethdev@sta/librte_ethdev_rte_flow.c.o' > 'lib/lib@@rte_ethdev@sta/librte_ethdev_rte_mtr.c.o' > 'lib/lib@@rte_ethdev@sta/librte_ethdev_rte_tm.c.o' -Wl,--no-undefined > -Wl,--as-needed -Wl,-O1 -shared -fPIC -Wl,--start-group > -Wl,-soname,librte_ethdev.so.20.0 -Wl,--no-as-needed -pthread -lm -ldl -lnuma > lib/librte_eal.so.20.0.3 lib/librte_kvargs.so.20.0.3 > lib/librte_telemetry.so.0.200.3 lib/librte_net.so.20.0.3 > lib/librte_mbuf.so.20.0.3 lib/librte_mempool.so.20.0.3 lib/librte_ring.so.20.0.3 > lib/librte_meter.so.20.0.3 -Wl,--end-group > -Wl,--version-script=/home/renata/work/dpdk/lib/librte_ethdev/rte_ethdev_version.map > '-Wl,-rpath,$ORIGIN/' -Wl,-rpath-link,/home/renata/work/dpdk/build/lib > /usr/bin/ld:/home/renata/work/dpdk/lib/librte_ethdev/rte_ethdev_version.map:0: > syntax error in VERSION script > collect2: error: ld returned 1 exit status > > What could be an issue, is that I need to correct the version somewhere? Hi Renata, The log says "syntax error in VERSION script", can be related to it. I did following update on top of your patches and it seems worked for me: diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev /rte_ethdev_driver.h index 462e765d1..ca16f1bcc 100644 --- a/lib/librte_ethdev/rte_ethdev_driver.h +++ b/lib/librte_ethdev/rte_ethdev_driver.h @@ -181,9 +181,6 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *eth_dev, const char *name, unsigned align, int socket_id); /** - * @warning - * @b EXPERIMENTAL: this API may change without prior notice. - * * Free previously allocated memzone for HW rings. * * @param eth_dev @@ -195,7 +192,7 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *eth_dev, const char *name, * @return * Negative errno value on error, 0 on success. */ -__rte_experimental +__rte_internal int rte_eth_dma_zone_free(const struct rte_eth_dev *dev, const char *ring_name, uint16_t queue_id); diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev /rte_ethdev_version.map index 139a81302..1212a17d3 100644 --- a/lib/librte_ethdev/rte_ethdev_version.map +++ b/lib/librte_ethdev/rte_ethdev_version.map @@ -241,5 +241,10 @@ EXPERIMENTAL { __rte_ethdev_trace_rx_burst; __rte_ethdev_trace_tx_burst; rte_flow_get_aged_flows; +}; + +INTERNAL { + global: + rte_eth_dma_zone_free; }; > > -------------------------------------------------------------------------------- > *From:* Ferruh Yigit <ferruh.yigit@intel.com> > *Sent:* Monday, June 22, 2020 7:09 PM > *To:* Renata Saiakhova <renata.saiakhova@ekinops.com>; dev@dpdk.org <dev@dpdk.org> > *Subject:* Re: [dpdk-dev] [PATCH v4 1/4] librte_ethdev: Introduce a function to > release HW rings > > On 6/22/2020 10:54 AM, 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 | 30 ++++++++++++++++++++++-- >> lib/librte_ethdev/rte_ethdev_driver.h | 20 ++++++++++++++++ >> lib/librte_ethdev/rte_ethdev_version.map | 1 + >> 3 files changed, 49 insertions(+), 2 deletions(-) >> >> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c >> index 72aed59a5..ec1da2006 100644 >> --- a/lib/librte_ethdev/rte_ethdev.c >> +++ b/lib/librte_ethdev/rte_ethdev.c >> @@ -4181,6 +4181,10 @@ rte_eth_dev_rx_intr_ctl_q_get_fd(uint16_t port_id, uint16_t queue_id) >> return fd; >> } >> >> +#define ETH_DMA_MZONE_NAME(_name, _port_id, _queue_id, _ring_name) \ >> + snprintf(_name, sizeof(_name), "eth_p%d_q%d_%s", \ >> + _port_id, _queue_id, _ring_name) >> + >> const struct rte_memzone * >> rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name, >> uint16_t queue_id, size_t size, unsigned align, >> @@ -4190,8 +4194,7 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name, >> const struct rte_memzone *mz; >> int rc; >> >> - rc = snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s", >> - dev->data->port_id, queue_id, ring_name); >> + rc = ETH_DMA_MZONE_NAME(z_name, 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; >> @@ -4206,6 +4209,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; >> + >> + rc = ETH_DMA_MZONE_NAME(z_name, dev->data->port_id, queue_id, ring_name); >> + if (rc >= RTE_MEMZONE_NAMESIZE) { >> + RTE_ETHDEV_LOG(ERR, "ring name too long\n"); >> + return -ENAMETOOLONG; >> + } >> + >> + mz = rte_memzone_lookup(z_name); >> + if (mz) >> + rc = rte_memzone_free(mz); >> + else >> + rc = -EINVAL; >> + >> + 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..462e765d1 100644 >> --- a/lib/librte_ethdev/rte_ethdev_driver.h >> +++ b/lib/librte_ethdev/rte_ethdev_driver.h >> @@ -180,6 +180,26 @@ 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); >> >> +/** >> + * @warning >> + * @b EXPERIMENTAL: this API may change without prior notice. >> + * >> + * 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 >> + * @return >> + * Negative errno value on error, 0 on success. >> + */ >> +__rte_experimental >> +int >> +rte_eth_dma_zone_free(const struct rte_eth_dev *dev, const char *ring_name, >> + uint16_t queue_id); >> + > > Hi Renata, > > This API is not for applications, only for drivers, right? > We recently (last release ?) introduced 'internal' (__rte_internal) attribute, > we can use '__rte_internal' instead of '__rte_experimental' if the API is not > for applications. This requires adding 'INTERNAL' block in .map file (sample > 'lib/librte_eal/rte_eal_version.map') > > > Also there are a few contribution conventions related issues which can be fixed > while merging but please check them if you will be doing new version: > subsystem is only libname without prefix: 'ethdev' > Commit title description starts with lowercase: 'introduce'. > sample => "ethdev: add function to release HW rings" > > > Thanks, > ferruh
From: Renata Saiakhova <renata.saiakhova@ekinops.com> Free previously allocated memzone for HW rings Signed-off-by: Renata Saiakhova <renata.saiakhova@ekinops.com> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com> --- v5: * Converted macro to static inline function * Updated API return error value --- lib/librte_ethdev/rte_ethdev.c | 36 ++++++++++++++++++++++-- lib/librte_ethdev/rte_ethdev_driver.h | 17 +++++++++++ lib/librte_ethdev/rte_ethdev_version.map | 7 +++++ 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 5ff52046dc..049d0a684c 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -4200,6 +4200,14 @@ rte_eth_dev_rx_intr_ctl_q_get_fd(uint16_t port_id, uint16_t queue_id) return fd; } +static inline int +eth_dma_mzone_name(char *name, size_t len, uint16_t port_id, uint16_t queue_id, + const char *ring_name) +{ + return snprintf(name, len, "eth_p%d_q%d_%s", + port_id, queue_id, ring_name); +} + const struct rte_memzone * rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name, uint16_t queue_id, size_t size, unsigned align, @@ -4209,8 +4217,8 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name, const struct rte_memzone *mz; int rc; - rc = snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s", - dev->data->port_id, queue_id, ring_name); + rc = eth_dma_mzone_name(z_name, sizeof(z_name), 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; @@ -4235,6 +4243,30 @@ 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; + + rc = eth_dma_mzone_name(z_name, sizeof(z_name), dev->data->port_id, + queue_id, ring_name); + if (rc >= RTE_MEMZONE_NAMESIZE) { + RTE_ETHDEV_LOG(ERR, "ring name too long\n"); + return -ENAMETOOLONG; + } + + mz = rte_memzone_lookup(z_name); + if (mz) + rc = rte_memzone_free(mz); + else + rc = -ENOENT; + + 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 99d4cd6cd0..13fd049c0d 100644 --- a/lib/librte_ethdev/rte_ethdev_driver.h +++ b/lib/librte_ethdev/rte_ethdev_driver.h @@ -180,6 +180,23 @@ 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 + * @return + * Negative errno value on error, 0 on success. + */ +__rte_internal +int +rte_eth_dma_zone_free(const struct rte_eth_dev *eth_dev, const char *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 7155056045..664d6b86a2 100644 --- a/lib/librte_ethdev/rte_ethdev_version.map +++ b/lib/librte_ethdev/rte_ethdev_version.map @@ -242,3 +242,10 @@ EXPERIMENTAL { __rte_ethdev_trace_tx_burst; rte_flow_get_aged_flows; }; + +INTERNAL { + global: + + rte_eth_dma_zone_free; +}; + -- 2.25.4
From: Renata Saiakhova <renata.saiakhova@ekinops.com> Delete memzones for HW rings in igb and ixgbe while freeing queues Updated igb, ixgbe, i40e, ice & em drivers. Signed-off-by: Renata Saiakhova <renata.saiakhova@ekinops.com> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com> --- drivers/net/e1000/em_rxtx.c | 2 ++ drivers/net/e1000/igb_rxtx.c | 2 ++ drivers/net/i40e/i40e_fdir.c | 3 +++ drivers/net/i40e/i40e_rxtx.c | 2 ++ drivers/net/ice/ice_rxtx.c | 2 ++ drivers/net/ixgbe/ixgbe_rxtx.c | 2 ++ 6 files changed, 13 insertions(+) diff --git a/drivers/net/e1000/em_rxtx.c b/drivers/net/e1000/em_rxtx.c index 49c53712a2..67a271e8ce 100644 --- a/drivers/net/e1000/em_rxtx.c +++ b/drivers/net/e1000/em_rxtx.c @@ -1611,12 +1611,14 @@ em_dev_free_queues(struct rte_eth_dev *dev) for (i = 0; i < dev->data->nb_rx_queues; i++) { eth_em_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_em_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/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c index 6883202842..5717cdb706 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/i40e/i40e_fdir.c b/drivers/net/i40e/i40e_fdir.c index 4a778db4c6..71eb31fb8b 100644 --- a/drivers/net/i40e/i40e_fdir.c +++ b/drivers/net/i40e/i40e_fdir.c @@ -269,6 +269,7 @@ i40e_fdir_teardown(struct i40e_pf *pf) { struct i40e_hw *hw = I40E_PF_TO_HW(pf); struct i40e_vsi *vsi; + struct rte_eth_dev *dev = pf->adapter->eth_dev; vsi = pf->fdir.fdir_vsi; if (!vsi) @@ -280,8 +281,10 @@ i40e_fdir_teardown(struct i40e_pf *pf) if (err) PMD_DRV_LOG(DEBUG, "Failed to do FDIR RX switch off"); i40e_dev_rx_queue_release(pf->fdir.rxq); + rte_eth_dma_zone_free(dev, "fdir_rx_ring", pf->fdir.rxq->queue_id); pf->fdir.rxq = NULL; i40e_dev_tx_queue_release(pf->fdir.txq); + rte_eth_dma_zone_free(dev, "fdir_tx_ring", pf->fdir.txq->queue_id); pf->fdir.txq = NULL; i40e_vsi_release(vsi); pf->fdir.fdir_vsi = NULL; diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c index 840b6f387f..2d2efb71a5 100644 --- a/drivers/net/i40e/i40e_rxtx.c +++ b/drivers/net/i40e/i40e_rxtx.c @@ -2928,6 +2928,7 @@ i40e_dev_free_queues(struct rte_eth_dev *dev) continue; i40e_dev_rx_queue_release(dev->data->rx_queues[i]); dev->data->rx_queues[i] = NULL; + rte_eth_dma_zone_free(dev, "rx_ring", i); } for (i = 0; i < dev->data->nb_tx_queues; i++) { @@ -2935,6 +2936,7 @@ i40e_dev_free_queues(struct rte_eth_dev *dev) continue; i40e_dev_tx_queue_release(dev->data->tx_queues[i]); dev->data->tx_queues[i] = NULL; + rte_eth_dma_zone_free(dev, "tx_ring", i); } } diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c index 5d6f693c52..cc3139042e 100644 --- a/drivers/net/ice/ice_rxtx.c +++ b/drivers/net/ice/ice_rxtx.c @@ -1905,6 +1905,7 @@ ice_free_queues(struct rte_eth_dev *dev) continue; ice_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; @@ -1913,6 +1914,7 @@ ice_free_queues(struct rte_eth_dev *dev) continue; ice_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 2e20e18c7a..977ecf5137 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.25.4