* [dpdk-dev] [PATCH v1 0/2] support RTE_ETH_DEV_CLOSE_REMOVE flag for AF_XDP
@ 2019-04-26 5:09 Xiaolong Ye
2019-04-26 5:09 ` Xiaolong Ye
` (3 more replies)
0 siblings, 4 replies; 34+ messages in thread
From: Xiaolong Ye @ 2019-04-26 5:09 UTC (permalink / raw)
To: dev, Ferruh Yigit, Thomas Monjalon; +Cc: Xiaolong Ye
Hi,
This series is to support the RTE_ETH_DEV_CLOSE_REMOVE in AF_XDP pmd.
Xiaolong Ye (2):
net/af_xdp: remove resources when port is closed
net/af_xdp: set MAC addrs field to NULL
drivers/net/af_xdp/rte_eth_af_xdp.c | 47 +++++++++++++++--------------
1 file changed, 25 insertions(+), 22 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [dpdk-dev] [PATCH v1 0/2] support RTE_ETH_DEV_CLOSE_REMOVE flag for AF_XDP
2019-04-26 5:09 [dpdk-dev] [PATCH v1 0/2] support RTE_ETH_DEV_CLOSE_REMOVE flag for AF_XDP Xiaolong Ye
@ 2019-04-26 5:09 ` Xiaolong Ye
2019-04-26 5:09 ` [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed Xiaolong Ye
` (2 subsequent siblings)
3 siblings, 0 replies; 34+ messages in thread
From: Xiaolong Ye @ 2019-04-26 5:09 UTC (permalink / raw)
To: dev, Ferruh Yigit, Thomas Monjalon; +Cc: Xiaolong Ye
Hi,
This series is to support the RTE_ETH_DEV_CLOSE_REMOVE in AF_XDP pmd.
Xiaolong Ye (2):
net/af_xdp: remove resources when port is closed
net/af_xdp: set MAC addrs field to NULL
drivers/net/af_xdp/rte_eth_af_xdp.c | 47 +++++++++++++++--------------
1 file changed, 25 insertions(+), 22 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-26 5:09 [dpdk-dev] [PATCH v1 0/2] support RTE_ETH_DEV_CLOSE_REMOVE flag for AF_XDP Xiaolong Ye
2019-04-26 5:09 ` Xiaolong Ye
@ 2019-04-26 5:09 ` Xiaolong Ye
2019-04-26 5:09 ` Xiaolong Ye
2019-04-29 17:00 ` Ferruh Yigit
2019-04-26 5:09 ` [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL Xiaolong Ye
2019-04-30 8:39 ` [dpdk-dev] [PATCH v3] net/af_xdp: remove resources when port is closed Xiaolong Ye
3 siblings, 2 replies; 34+ messages in thread
From: Xiaolong Ye @ 2019-04-26 5:09 UTC (permalink / raw)
To: dev, Ferruh Yigit, Thomas Monjalon; +Cc: Xiaolong Ye
Since 18.11, it is suggested that driver should release all its private
resources at the dev_close routine. So all resources previously released
in remove routine are now released at the dev_close routine, and the
dev_close routine will be called in driver remove routine in order to
support removing a device without closing its ports.
Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
flag during probe stage.
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
drivers/net/af_xdp/rte_eth_af_xdp.c | 41 +++++++++++++----------------
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index acf9ad605..a12551cbf 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -426,6 +426,19 @@ remove_xdp_program(struct pmd_internals *internals)
XDP_FLAGS_UPDATE_IF_NOEXIST);
}
+static void
+xdp_umem_destroy(struct xsk_umem_info *umem)
+{
+ rte_memzone_free(umem->mz);
+ umem->mz = NULL;
+
+ rte_ring_free(umem->buf_ring);
+ umem->buf_ring = NULL;
+
+ rte_free(umem);
+ umem = NULL;
+}
+
static void
eth_dev_close(struct rte_eth_dev *dev)
{
@@ -444,6 +457,9 @@ eth_dev_close(struct rte_eth_dev *dev)
}
(void)xsk_umem__delete(internals->umem->umem);
+
+ xdp_umem_destroy(internals->umem);
+
remove_xdp_program(internals);
}
@@ -459,19 +475,6 @@ eth_link_update(struct rte_eth_dev *dev __rte_unused,
return 0;
}
-static void
-xdp_umem_destroy(struct xsk_umem_info *umem)
-{
- rte_memzone_free(umem->mz);
- umem->mz = NULL;
-
- rte_ring_free(umem->buf_ring);
- umem->buf_ring = NULL;
-
- rte_free(umem);
- umem = NULL;
-}
-
static struct
xsk_umem_info *xdp_umem_configure(struct pmd_internals *internals)
{
@@ -856,6 +859,8 @@ init_internals(struct rte_vdev_device *dev,
eth_dev->dev_ops = &ops;
eth_dev->rx_pkt_burst = eth_af_xdp_rx;
eth_dev->tx_pkt_burst = eth_af_xdp_tx;
+ /* Let rte_eth_dev_close() release the port resources. */
+ eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
return eth_dev;
@@ -923,7 +928,6 @@ static int
rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
{
struct rte_eth_dev *eth_dev = NULL;
- struct pmd_internals *internals;
AF_XDP_LOG(INFO, "Removing AF_XDP ethdev on numa socket %u\n",
rte_socket_id());
@@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
if (eth_dev == NULL)
return -1;
- internals = eth_dev->data->dev_private;
-
- rte_ring_free(internals->umem->buf_ring);
- rte_memzone_free(internals->umem->mz);
- rte_free(internals->umem);
-
- rte_eth_dev_release_port(eth_dev);
-
+ eth_dev_close(eth_dev);
return 0;
}
--
2.17.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-26 5:09 ` [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed Xiaolong Ye
@ 2019-04-26 5:09 ` Xiaolong Ye
2019-04-29 17:00 ` Ferruh Yigit
1 sibling, 0 replies; 34+ messages in thread
From: Xiaolong Ye @ 2019-04-26 5:09 UTC (permalink / raw)
To: dev, Ferruh Yigit, Thomas Monjalon; +Cc: Xiaolong Ye
Since 18.11, it is suggested that driver should release all its private
resources at the dev_close routine. So all resources previously released
in remove routine are now released at the dev_close routine, and the
dev_close routine will be called in driver remove routine in order to
support removing a device without closing its ports.
Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
flag during probe stage.
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
drivers/net/af_xdp/rte_eth_af_xdp.c | 41 +++++++++++++----------------
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index acf9ad605..a12551cbf 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -426,6 +426,19 @@ remove_xdp_program(struct pmd_internals *internals)
XDP_FLAGS_UPDATE_IF_NOEXIST);
}
+static void
+xdp_umem_destroy(struct xsk_umem_info *umem)
+{
+ rte_memzone_free(umem->mz);
+ umem->mz = NULL;
+
+ rte_ring_free(umem->buf_ring);
+ umem->buf_ring = NULL;
+
+ rte_free(umem);
+ umem = NULL;
+}
+
static void
eth_dev_close(struct rte_eth_dev *dev)
{
@@ -444,6 +457,9 @@ eth_dev_close(struct rte_eth_dev *dev)
}
(void)xsk_umem__delete(internals->umem->umem);
+
+ xdp_umem_destroy(internals->umem);
+
remove_xdp_program(internals);
}
@@ -459,19 +475,6 @@ eth_link_update(struct rte_eth_dev *dev __rte_unused,
return 0;
}
-static void
-xdp_umem_destroy(struct xsk_umem_info *umem)
-{
- rte_memzone_free(umem->mz);
- umem->mz = NULL;
-
- rte_ring_free(umem->buf_ring);
- umem->buf_ring = NULL;
-
- rte_free(umem);
- umem = NULL;
-}
-
static struct
xsk_umem_info *xdp_umem_configure(struct pmd_internals *internals)
{
@@ -856,6 +859,8 @@ init_internals(struct rte_vdev_device *dev,
eth_dev->dev_ops = &ops;
eth_dev->rx_pkt_burst = eth_af_xdp_rx;
eth_dev->tx_pkt_burst = eth_af_xdp_tx;
+ /* Let rte_eth_dev_close() release the port resources. */
+ eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
return eth_dev;
@@ -923,7 +928,6 @@ static int
rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
{
struct rte_eth_dev *eth_dev = NULL;
- struct pmd_internals *internals;
AF_XDP_LOG(INFO, "Removing AF_XDP ethdev on numa socket %u\n",
rte_socket_id());
@@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
if (eth_dev == NULL)
return -1;
- internals = eth_dev->data->dev_private;
-
- rte_ring_free(internals->umem->buf_ring);
- rte_memzone_free(internals->umem->mz);
- rte_free(internals->umem);
-
- rte_eth_dev_release_port(eth_dev);
-
+ eth_dev_close(eth_dev);
return 0;
}
--
2.17.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL
2019-04-26 5:09 [dpdk-dev] [PATCH v1 0/2] support RTE_ETH_DEV_CLOSE_REMOVE flag for AF_XDP Xiaolong Ye
2019-04-26 5:09 ` Xiaolong Ye
2019-04-26 5:09 ` [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed Xiaolong Ye
@ 2019-04-26 5:09 ` Xiaolong Ye
2019-04-26 5:09 ` Xiaolong Ye
2019-04-29 17:02 ` Ferruh Yigit
2019-04-30 8:39 ` [dpdk-dev] [PATCH v3] net/af_xdp: remove resources when port is closed Xiaolong Ye
3 siblings, 2 replies; 34+ messages in thread
From: Xiaolong Ye @ 2019-04-26 5:09 UTC (permalink / raw)
To: dev, Ferruh Yigit, Thomas Monjalon; +Cc: Xiaolong Ye
As af_xdp pmd doesn't allocate MAC addresses dynamically, it needs to be
set as NULL, so it won't be released by rte_eth_dev_release_port(),
otherwise, there would be "EAL: Error: Invalid memory" error.
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
drivers/net/af_xdp/rte_eth_af_xdp.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index a12551cbf..f659c0496 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -458,6 +458,12 @@ eth_dev_close(struct rte_eth_dev *dev)
(void)xsk_umem__delete(internals->umem->umem);
+ /*
+ * MAC is not allocated dynamically, setting it to NULL would prevent
+ * from releasing it in rte_eth_dev_release_port.
+ */
+ dev->data->mac_addrs = NULL;
+
xdp_umem_destroy(internals->umem);
remove_xdp_program(internals);
--
2.17.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL
2019-04-26 5:09 ` [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL Xiaolong Ye
@ 2019-04-26 5:09 ` Xiaolong Ye
2019-04-29 17:02 ` Ferruh Yigit
1 sibling, 0 replies; 34+ messages in thread
From: Xiaolong Ye @ 2019-04-26 5:09 UTC (permalink / raw)
To: dev, Ferruh Yigit, Thomas Monjalon; +Cc: Xiaolong Ye
As af_xdp pmd doesn't allocate MAC addresses dynamically, it needs to be
set as NULL, so it won't be released by rte_eth_dev_release_port(),
otherwise, there would be "EAL: Error: Invalid memory" error.
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
drivers/net/af_xdp/rte_eth_af_xdp.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index a12551cbf..f659c0496 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -458,6 +458,12 @@ eth_dev_close(struct rte_eth_dev *dev)
(void)xsk_umem__delete(internals->umem->umem);
+ /*
+ * MAC is not allocated dynamically, setting it to NULL would prevent
+ * from releasing it in rte_eth_dev_release_port.
+ */
+ dev->data->mac_addrs = NULL;
+
xdp_umem_destroy(internals->umem);
remove_xdp_program(internals);
--
2.17.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-26 5:09 ` [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed Xiaolong Ye
2019-04-26 5:09 ` Xiaolong Ye
@ 2019-04-29 17:00 ` Ferruh Yigit
2019-04-29 17:00 ` Ferruh Yigit
` (2 more replies)
1 sibling, 3 replies; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-29 17:00 UTC (permalink / raw)
To: Xiaolong Ye, dev, Thomas Monjalon
On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> Since 18.11, it is suggested that driver should release all its private
> resources at the dev_close routine. So all resources previously released
> in remove routine are now released at the dev_close routine, and the
> dev_close routine will be called in driver remove routine in order to
> support removing a device without closing its ports.
>
> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> flag during probe stage.
>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
<...>
> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
> if (eth_dev == NULL)
> return -1;
>
> - internals = eth_dev->data->dev_private;
> -
> - rte_ring_free(internals->umem->buf_ring);
> - rte_memzone_free(internals->umem->mz);
> - rte_free(internals->umem);
> -
> - rte_eth_dev_release_port(eth_dev);
I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
'rte_eth_dev_close()' but still needed in '.remove()' path.
> -
> + eth_dev_close(eth_dev);
>
> return 0;
> }
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-29 17:00 ` Ferruh Yigit
@ 2019-04-29 17:00 ` Ferruh Yigit
2019-04-29 20:14 ` Thomas Monjalon
2019-04-30 2:06 ` Ye Xiaolong
2 siblings, 0 replies; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-29 17:00 UTC (permalink / raw)
To: Xiaolong Ye, dev, Thomas Monjalon
On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> Since 18.11, it is suggested that driver should release all its private
> resources at the dev_close routine. So all resources previously released
> in remove routine are now released at the dev_close routine, and the
> dev_close routine will be called in driver remove routine in order to
> support removing a device without closing its ports.
>
> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> flag during probe stage.
>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
<...>
> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
> if (eth_dev == NULL)
> return -1;
>
> - internals = eth_dev->data->dev_private;
> -
> - rte_ring_free(internals->umem->buf_ring);
> - rte_memzone_free(internals->umem->mz);
> - rte_free(internals->umem);
> -
> - rte_eth_dev_release_port(eth_dev);
I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
'rte_eth_dev_close()' but still needed in '.remove()' path.
> -
> + eth_dev_close(eth_dev);
>
> return 0;
> }
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL
2019-04-26 5:09 ` [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL Xiaolong Ye
2019-04-26 5:09 ` Xiaolong Ye
@ 2019-04-29 17:02 ` Ferruh Yigit
2019-04-29 17:02 ` Ferruh Yigit
2019-04-30 2:04 ` Ye Xiaolong
1 sibling, 2 replies; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-29 17:02 UTC (permalink / raw)
To: Xiaolong Ye, dev, Thomas Monjalon
On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> As af_xdp pmd doesn't allocate MAC addresses dynamically, it needs to be
> set as NULL, so it won't be released by rte_eth_dev_release_port(),
> otherwise, there would be "EAL: Error: Invalid memory" error.
>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> ---
> drivers/net/af_xdp/rte_eth_af_xdp.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index a12551cbf..f659c0496 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -458,6 +458,12 @@ eth_dev_close(struct rte_eth_dev *dev)
>
> (void)xsk_umem__delete(internals->umem->umem);
>
> + /*
> + * MAC is not allocated dynamically, setting it to NULL would prevent
> + * from releasing it in rte_eth_dev_release_port.
> + */
> + dev->data->mac_addrs = NULL;
> +
> xdp_umem_destroy(internals->umem);
>
> remove_xdp_program(internals);
>
Keeping this patch in separate patch will cause the above error after first
patch, what do you think merging this with prev patch?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL
2019-04-29 17:02 ` Ferruh Yigit
@ 2019-04-29 17:02 ` Ferruh Yigit
2019-04-30 2:04 ` Ye Xiaolong
1 sibling, 0 replies; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-29 17:02 UTC (permalink / raw)
To: Xiaolong Ye, dev, Thomas Monjalon
On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> As af_xdp pmd doesn't allocate MAC addresses dynamically, it needs to be
> set as NULL, so it won't be released by rte_eth_dev_release_port(),
> otherwise, there would be "EAL: Error: Invalid memory" error.
>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> ---
> drivers/net/af_xdp/rte_eth_af_xdp.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index a12551cbf..f659c0496 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -458,6 +458,12 @@ eth_dev_close(struct rte_eth_dev *dev)
>
> (void)xsk_umem__delete(internals->umem->umem);
>
> + /*
> + * MAC is not allocated dynamically, setting it to NULL would prevent
> + * from releasing it in rte_eth_dev_release_port.
> + */
> + dev->data->mac_addrs = NULL;
> +
> xdp_umem_destroy(internals->umem);
>
> remove_xdp_program(internals);
>
Keeping this patch in separate patch will cause the above error after first
patch, what do you think merging this with prev patch?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-29 17:00 ` Ferruh Yigit
2019-04-29 17:00 ` Ferruh Yigit
@ 2019-04-29 20:14 ` Thomas Monjalon
2019-04-29 20:14 ` Thomas Monjalon
2019-04-29 22:28 ` Ferruh Yigit
2019-04-30 2:06 ` Ye Xiaolong
2 siblings, 2 replies; 34+ messages in thread
From: Thomas Monjalon @ 2019-04-29 20:14 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Xiaolong Ye, dev
29/04/2019 19:00, Ferruh Yigit:
> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> > Since 18.11, it is suggested that driver should release all its private
> > resources at the dev_close routine. So all resources previously released
> > in remove routine are now released at the dev_close routine, and the
> > dev_close routine will be called in driver remove routine in order to
> > support removing a device without closing its ports.
> >
> > Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> > flag during probe stage.
> >
> > Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>
> <...>
>
> > @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
> > if (eth_dev == NULL)
> > return -1;
> >
> > - internals = eth_dev->data->dev_private;
> > -
> > - rte_ring_free(internals->umem->buf_ring);
> > - rte_memzone_free(internals->umem->mz);
> > - rte_free(internals->umem);
> > -
> > - rte_eth_dev_release_port(eth_dev);
>
> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
> 'rte_eth_dev_close()' but still needed in '.remove()' path.
I don't understand your comment.
Calling the close function looks the right thing to do in "remove".
> > -
> > + eth_dev_close(eth_dev);
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-29 20:14 ` Thomas Monjalon
@ 2019-04-29 20:14 ` Thomas Monjalon
2019-04-29 22:28 ` Ferruh Yigit
1 sibling, 0 replies; 34+ messages in thread
From: Thomas Monjalon @ 2019-04-29 20:14 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Xiaolong Ye, dev
29/04/2019 19:00, Ferruh Yigit:
> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> > Since 18.11, it is suggested that driver should release all its private
> > resources at the dev_close routine. So all resources previously released
> > in remove routine are now released at the dev_close routine, and the
> > dev_close routine will be called in driver remove routine in order to
> > support removing a device without closing its ports.
> >
> > Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> > flag during probe stage.
> >
> > Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>
> <...>
>
> > @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
> > if (eth_dev == NULL)
> > return -1;
> >
> > - internals = eth_dev->data->dev_private;
> > -
> > - rte_ring_free(internals->umem->buf_ring);
> > - rte_memzone_free(internals->umem->mz);
> > - rte_free(internals->umem);
> > -
> > - rte_eth_dev_release_port(eth_dev);
>
> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
> 'rte_eth_dev_close()' but still needed in '.remove()' path.
I don't understand your comment.
Calling the close function looks the right thing to do in "remove".
> > -
> > + eth_dev_close(eth_dev);
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-29 20:14 ` Thomas Monjalon
2019-04-29 20:14 ` Thomas Monjalon
@ 2019-04-29 22:28 ` Ferruh Yigit
2019-04-29 22:28 ` Ferruh Yigit
2019-04-29 22:34 ` Thomas Monjalon
1 sibling, 2 replies; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-29 22:28 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Xiaolong Ye, dev
On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
> 29/04/2019 19:00, Ferruh Yigit:
>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>> Since 18.11, it is suggested that driver should release all its private
>>> resources at the dev_close routine. So all resources previously released
>>> in remove routine are now released at the dev_close routine, and the
>>> dev_close routine will be called in driver remove routine in order to
>>> support removing a device without closing its ports.
>>>
>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>> flag during probe stage.
>>>
>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>
>> <...>
>>
>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>> if (eth_dev == NULL)
>>> return -1;
>>>
>>> - internals = eth_dev->data->dev_private;
>>> -
>>> - rte_ring_free(internals->umem->buf_ring);
>>> - rte_memzone_free(internals->umem->mz);
>>> - rte_free(internals->umem);
>>> -
>>> - rte_eth_dev_release_port(eth_dev);
>>
>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>
> I don't understand your comment.
> Calling the close function looks the right thing to do in "remove".
No concern on calling the 'close'.
My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
>
>>> -
>>> + eth_dev_close(eth_dev);
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-29 22:28 ` Ferruh Yigit
@ 2019-04-29 22:28 ` Ferruh Yigit
2019-04-29 22:34 ` Thomas Monjalon
1 sibling, 0 replies; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-29 22:28 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Xiaolong Ye, dev
On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
> 29/04/2019 19:00, Ferruh Yigit:
>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>> Since 18.11, it is suggested that driver should release all its private
>>> resources at the dev_close routine. So all resources previously released
>>> in remove routine are now released at the dev_close routine, and the
>>> dev_close routine will be called in driver remove routine in order to
>>> support removing a device without closing its ports.
>>>
>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>> flag during probe stage.
>>>
>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>
>> <...>
>>
>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>> if (eth_dev == NULL)
>>> return -1;
>>>
>>> - internals = eth_dev->data->dev_private;
>>> -
>>> - rte_ring_free(internals->umem->buf_ring);
>>> - rte_memzone_free(internals->umem->mz);
>>> - rte_free(internals->umem);
>>> -
>>> - rte_eth_dev_release_port(eth_dev);
>>
>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>
> I don't understand your comment.
> Calling the close function looks the right thing to do in "remove".
No concern on calling the 'close'.
My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
>
>>> -
>>> + eth_dev_close(eth_dev);
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-29 22:28 ` Ferruh Yigit
2019-04-29 22:28 ` Ferruh Yigit
@ 2019-04-29 22:34 ` Thomas Monjalon
2019-04-29 22:34 ` Thomas Monjalon
2019-04-30 7:33 ` Ferruh Yigit
1 sibling, 2 replies; 34+ messages in thread
From: Thomas Monjalon @ 2019-04-29 22:34 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Xiaolong Ye, dev
30/04/2019 00:28, Ferruh Yigit:
> On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
> > 29/04/2019 19:00, Ferruh Yigit:
> >> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> >>> Since 18.11, it is suggested that driver should release all its private
> >>> resources at the dev_close routine. So all resources previously released
> >>> in remove routine are now released at the dev_close routine, and the
> >>> dev_close routine will be called in driver remove routine in order to
> >>> support removing a device without closing its ports.
> >>>
> >>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> >>> flag during probe stage.
> >>>
> >>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> >>
> >> <...>
> >>
> >>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
> >>> if (eth_dev == NULL)
> >>> return -1;
> >>>
> >>> - internals = eth_dev->data->dev_private;
> >>> -
> >>> - rte_ring_free(internals->umem->buf_ring);
> >>> - rte_memzone_free(internals->umem->mz);
> >>> - rte_free(internals->umem);
> >>> -
> >>> - rte_eth_dev_release_port(eth_dev);
> >>
> >> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
> >> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
> >> 'rte_eth_dev_close()' but still needed in '.remove()' path.
> >
> > I don't understand your comment.
> > Calling the close function looks the right thing to do in "remove".
>
> No concern on calling the 'close'.
> My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
rte_eth_dev_release_port() is called in eth_dev_close(), isn't it?
> >>> -
> >>> + eth_dev_close(eth_dev);
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-29 22:34 ` Thomas Monjalon
@ 2019-04-29 22:34 ` Thomas Monjalon
2019-04-30 7:33 ` Ferruh Yigit
1 sibling, 0 replies; 34+ messages in thread
From: Thomas Monjalon @ 2019-04-29 22:34 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Xiaolong Ye, dev
30/04/2019 00:28, Ferruh Yigit:
> On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
> > 29/04/2019 19:00, Ferruh Yigit:
> >> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> >>> Since 18.11, it is suggested that driver should release all its private
> >>> resources at the dev_close routine. So all resources previously released
> >>> in remove routine are now released at the dev_close routine, and the
> >>> dev_close routine will be called in driver remove routine in order to
> >>> support removing a device without closing its ports.
> >>>
> >>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> >>> flag during probe stage.
> >>>
> >>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> >>
> >> <...>
> >>
> >>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
> >>> if (eth_dev == NULL)
> >>> return -1;
> >>>
> >>> - internals = eth_dev->data->dev_private;
> >>> -
> >>> - rte_ring_free(internals->umem->buf_ring);
> >>> - rte_memzone_free(internals->umem->mz);
> >>> - rte_free(internals->umem);
> >>> -
> >>> - rte_eth_dev_release_port(eth_dev);
> >>
> >> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
> >> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
> >> 'rte_eth_dev_close()' but still needed in '.remove()' path.
> >
> > I don't understand your comment.
> > Calling the close function looks the right thing to do in "remove".
>
> No concern on calling the 'close'.
> My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
rte_eth_dev_release_port() is called in eth_dev_close(), isn't it?
> >>> -
> >>> + eth_dev_close(eth_dev);
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL
2019-04-29 17:02 ` Ferruh Yigit
2019-04-29 17:02 ` Ferruh Yigit
@ 2019-04-30 2:04 ` Ye Xiaolong
2019-04-30 2:04 ` Ye Xiaolong
1 sibling, 1 reply; 34+ messages in thread
From: Ye Xiaolong @ 2019-04-30 2:04 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev, Thomas Monjalon
On 04/29, Ferruh Yigit wrote:
>On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>> As af_xdp pmd doesn't allocate MAC addresses dynamically, it needs to be
>> set as NULL, so it won't be released by rte_eth_dev_release_port(),
>> otherwise, there would be "EAL: Error: Invalid memory" error.
>>
>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>> ---
>> drivers/net/af_xdp/rte_eth_af_xdp.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
>> index a12551cbf..f659c0496 100644
>> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
>> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
>> @@ -458,6 +458,12 @@ eth_dev_close(struct rte_eth_dev *dev)
>>
>> (void)xsk_umem__delete(internals->umem->umem);
>>
>> + /*
>> + * MAC is not allocated dynamically, setting it to NULL would prevent
>> + * from releasing it in rte_eth_dev_release_port.
>> + */
>> + dev->data->mac_addrs = NULL;
>> +
>> xdp_umem_destroy(internals->umem);
>>
>> remove_xdp_program(internals);
>>
>
>Keeping this patch in separate patch will cause the above error after first
>patch, what do you think merging this with prev patch?
Make sence, will squash it into first patch.
Thanks,
Xiaolong
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL
2019-04-30 2:04 ` Ye Xiaolong
@ 2019-04-30 2:04 ` Ye Xiaolong
0 siblings, 0 replies; 34+ messages in thread
From: Ye Xiaolong @ 2019-04-30 2:04 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev, Thomas Monjalon
On 04/29, Ferruh Yigit wrote:
>On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>> As af_xdp pmd doesn't allocate MAC addresses dynamically, it needs to be
>> set as NULL, so it won't be released by rte_eth_dev_release_port(),
>> otherwise, there would be "EAL: Error: Invalid memory" error.
>>
>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>> ---
>> drivers/net/af_xdp/rte_eth_af_xdp.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
>> index a12551cbf..f659c0496 100644
>> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
>> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
>> @@ -458,6 +458,12 @@ eth_dev_close(struct rte_eth_dev *dev)
>>
>> (void)xsk_umem__delete(internals->umem->umem);
>>
>> + /*
>> + * MAC is not allocated dynamically, setting it to NULL would prevent
>> + * from releasing it in rte_eth_dev_release_port.
>> + */
>> + dev->data->mac_addrs = NULL;
>> +
>> xdp_umem_destroy(internals->umem);
>>
>> remove_xdp_program(internals);
>>
>
>Keeping this patch in separate patch will cause the above error after first
>patch, what do you think merging this with prev patch?
Make sence, will squash it into first patch.
Thanks,
Xiaolong
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-29 17:00 ` Ferruh Yigit
2019-04-29 17:00 ` Ferruh Yigit
2019-04-29 20:14 ` Thomas Monjalon
@ 2019-04-30 2:06 ` Ye Xiaolong
2019-04-30 2:06 ` Ye Xiaolong
2019-04-30 7:35 ` Ferruh Yigit
2 siblings, 2 replies; 34+ messages in thread
From: Ye Xiaolong @ 2019-04-30 2:06 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev, Thomas Monjalon
Hi, Ferruh
On 04/29, Ferruh Yigit wrote:
>On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>> Since 18.11, it is suggested that driver should release all its private
>> resources at the dev_close routine. So all resources previously released
>> in remove routine are now released at the dev_close routine, and the
>> dev_close routine will be called in driver remove routine in order to
>> support removing a device without closing its ports.
>>
>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>> flag during probe stage.
>>
>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>
><...>
>
>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>> if (eth_dev == NULL)
>> return -1;
>>
>> - internals = eth_dev->data->dev_private;
>> -
>> - rte_ring_free(internals->umem->buf_ring);
>> - rte_memzone_free(internals->umem->mz);
>> - rte_free(internals->umem);
>> -
>> - rte_eth_dev_release_port(eth_dev);
>
>I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>'rte_eth_dev_close()' but still needed in '.remove()' path.
>
remove() would call eth_dev_close which includes the rte_eth_dev_release_port().
Thanks,
Xiaolong
>> -
>> + eth_dev_close(eth_dev);
>>
>> return 0;
>> }
>>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-30 2:06 ` Ye Xiaolong
@ 2019-04-30 2:06 ` Ye Xiaolong
2019-04-30 7:35 ` Ferruh Yigit
1 sibling, 0 replies; 34+ messages in thread
From: Ye Xiaolong @ 2019-04-30 2:06 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev, Thomas Monjalon
Hi, Ferruh
On 04/29, Ferruh Yigit wrote:
>On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>> Since 18.11, it is suggested that driver should release all its private
>> resources at the dev_close routine. So all resources previously released
>> in remove routine are now released at the dev_close routine, and the
>> dev_close routine will be called in driver remove routine in order to
>> support removing a device without closing its ports.
>>
>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>> flag during probe stage.
>>
>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>
><...>
>
>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>> if (eth_dev == NULL)
>> return -1;
>>
>> - internals = eth_dev->data->dev_private;
>> -
>> - rte_ring_free(internals->umem->buf_ring);
>> - rte_memzone_free(internals->umem->mz);
>> - rte_free(internals->umem);
>> -
>> - rte_eth_dev_release_port(eth_dev);
>
>I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>'rte_eth_dev_close()' but still needed in '.remove()' path.
>
remove() would call eth_dev_close which includes the rte_eth_dev_release_port().
Thanks,
Xiaolong
>> -
>> + eth_dev_close(eth_dev);
>>
>> return 0;
>> }
>>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-29 22:34 ` Thomas Monjalon
2019-04-29 22:34 ` Thomas Monjalon
@ 2019-04-30 7:33 ` Ferruh Yigit
2019-04-30 7:33 ` Ferruh Yigit
2019-04-30 7:55 ` Thomas Monjalon
1 sibling, 2 replies; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-30 7:33 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Xiaolong Ye, dev
On 4/29/2019 11:34 PM, Thomas Monjalon wrote:
> 30/04/2019 00:28, Ferruh Yigit:
>> On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
>>> 29/04/2019 19:00, Ferruh Yigit:
>>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>>>> Since 18.11, it is suggested that driver should release all its private
>>>>> resources at the dev_close routine. So all resources previously released
>>>>> in remove routine are now released at the dev_close routine, and the
>>>>> dev_close routine will be called in driver remove routine in order to
>>>>> support removing a device without closing its ports.
>>>>>
>>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>>>> flag during probe stage.
>>>>>
>>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>>>
>>>> <...>
>>>>
>>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>>>> if (eth_dev == NULL)
>>>>> return -1;
>>>>>
>>>>> - internals = eth_dev->data->dev_private;
>>>>> -
>>>>> - rte_ring_free(internals->umem->buf_ring);
>>>>> - rte_memzone_free(internals->umem->mz);
>>>>> - rte_free(internals->umem);
>>>>> -
>>>>> - rte_eth_dev_release_port(eth_dev);
>>>>
>>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>>>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>>>
>>> I don't understand your comment.
>>> Calling the close function looks the right thing to do in "remove".
>>
>> No concern on calling the 'close'.
>> My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
>
> rte_eth_dev_release_port() is called in eth_dev_close(), isn't it?
No, 'eth_dev_close()' is local 'dev_close()' ops, the one to clear driver
private resources.
I assume it is confused with 'rte_eth_dev_close()' ...
>
>>>>> -
>>>>> + eth_dev_close(eth_dev);
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-30 7:33 ` Ferruh Yigit
@ 2019-04-30 7:33 ` Ferruh Yigit
2019-04-30 7:55 ` Thomas Monjalon
1 sibling, 0 replies; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-30 7:33 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Xiaolong Ye, dev
On 4/29/2019 11:34 PM, Thomas Monjalon wrote:
> 30/04/2019 00:28, Ferruh Yigit:
>> On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
>>> 29/04/2019 19:00, Ferruh Yigit:
>>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>>>> Since 18.11, it is suggested that driver should release all its private
>>>>> resources at the dev_close routine. So all resources previously released
>>>>> in remove routine are now released at the dev_close routine, and the
>>>>> dev_close routine will be called in driver remove routine in order to
>>>>> support removing a device without closing its ports.
>>>>>
>>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>>>> flag during probe stage.
>>>>>
>>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>>>
>>>> <...>
>>>>
>>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>>>> if (eth_dev == NULL)
>>>>> return -1;
>>>>>
>>>>> - internals = eth_dev->data->dev_private;
>>>>> -
>>>>> - rte_ring_free(internals->umem->buf_ring);
>>>>> - rte_memzone_free(internals->umem->mz);
>>>>> - rte_free(internals->umem);
>>>>> -
>>>>> - rte_eth_dev_release_port(eth_dev);
>>>>
>>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>>>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>>>
>>> I don't understand your comment.
>>> Calling the close function looks the right thing to do in "remove".
>>
>> No concern on calling the 'close'.
>> My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
>
> rte_eth_dev_release_port() is called in eth_dev_close(), isn't it?
No, 'eth_dev_close()' is local 'dev_close()' ops, the one to clear driver
private resources.
I assume it is confused with 'rte_eth_dev_close()' ...
>
>>>>> -
>>>>> + eth_dev_close(eth_dev);
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-30 2:06 ` Ye Xiaolong
2019-04-30 2:06 ` Ye Xiaolong
@ 2019-04-30 7:35 ` Ferruh Yigit
2019-04-30 7:35 ` Ferruh Yigit
2019-04-30 8:37 ` Ye Xiaolong
1 sibling, 2 replies; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-30 7:35 UTC (permalink / raw)
To: Ye Xiaolong; +Cc: dev, Thomas Monjalon
On 4/30/2019 3:06 AM, Ye Xiaolong wrote:
> Hi, Ferruh
>
> On 04/29, Ferruh Yigit wrote:
>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>> Since 18.11, it is suggested that driver should release all its private
>>> resources at the dev_close routine. So all resources previously released
>>> in remove routine are now released at the dev_close routine, and the
>>> dev_close routine will be called in driver remove routine in order to
>>> support removing a device without closing its ports.
>>>
>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>> flag during probe stage.
>>>
>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>
>> <...>
>>
>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>> if (eth_dev == NULL)
>>> return -1;
>>>
>>> - internals = eth_dev->data->dev_private;
>>> -
>>> - rte_ring_free(internals->umem->buf_ring);
>>> - rte_memzone_free(internals->umem->mz);
>>> - rte_free(internals->umem);
>>> -
>>> - rte_eth_dev_release_port(eth_dev);
>>
>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>>
>
> remove() would call eth_dev_close which includes the rte_eth_dev_release_port().
'eth_dev_close()' doesn't call the 'rte_eth_dev_release_port()', and it
shouldn't really, am I missing something?
>
> Thanks,
> Xiaolong
>
>>> -
>>> + eth_dev_close(eth_dev);
>>>
>>> return 0;
>>> }
>>>
>>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-30 7:35 ` Ferruh Yigit
@ 2019-04-30 7:35 ` Ferruh Yigit
2019-04-30 8:37 ` Ye Xiaolong
1 sibling, 0 replies; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-30 7:35 UTC (permalink / raw)
To: Ye Xiaolong; +Cc: dev, Thomas Monjalon
On 4/30/2019 3:06 AM, Ye Xiaolong wrote:
> Hi, Ferruh
>
> On 04/29, Ferruh Yigit wrote:
>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>> Since 18.11, it is suggested that driver should release all its private
>>> resources at the dev_close routine. So all resources previously released
>>> in remove routine are now released at the dev_close routine, and the
>>> dev_close routine will be called in driver remove routine in order to
>>> support removing a device without closing its ports.
>>>
>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>> flag during probe stage.
>>>
>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>
>> <...>
>>
>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>> if (eth_dev == NULL)
>>> return -1;
>>>
>>> - internals = eth_dev->data->dev_private;
>>> -
>>> - rte_ring_free(internals->umem->buf_ring);
>>> - rte_memzone_free(internals->umem->mz);
>>> - rte_free(internals->umem);
>>> -
>>> - rte_eth_dev_release_port(eth_dev);
>>
>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>>
>
> remove() would call eth_dev_close which includes the rte_eth_dev_release_port().
'eth_dev_close()' doesn't call the 'rte_eth_dev_release_port()', and it
shouldn't really, am I missing something?
>
> Thanks,
> Xiaolong
>
>>> -
>>> + eth_dev_close(eth_dev);
>>>
>>> return 0;
>>> }
>>>
>>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-30 7:33 ` Ferruh Yigit
2019-04-30 7:33 ` Ferruh Yigit
@ 2019-04-30 7:55 ` Thomas Monjalon
2019-04-30 7:55 ` Thomas Monjalon
2019-04-30 8:10 ` Ferruh Yigit
1 sibling, 2 replies; 34+ messages in thread
From: Thomas Monjalon @ 2019-04-30 7:55 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Xiaolong Ye, dev
30/04/2019 09:33, Ferruh Yigit:
> On 4/29/2019 11:34 PM, Thomas Monjalon wrote:
> > 30/04/2019 00:28, Ferruh Yigit:
> >> On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
> >>> 29/04/2019 19:00, Ferruh Yigit:
> >>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> >>>>> Since 18.11, it is suggested that driver should release all its private
> >>>>> resources at the dev_close routine. So all resources previously released
> >>>>> in remove routine are now released at the dev_close routine, and the
> >>>>> dev_close routine will be called in driver remove routine in order to
> >>>>> support removing a device without closing its ports.
> >>>>>
> >>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> >>>>> flag during probe stage.
> >>>>>
> >>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> >>>>
> >>>> <...>
> >>>>
> >>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
> >>>>> if (eth_dev == NULL)
> >>>>> return -1;
> >>>>>
> >>>>> - internals = eth_dev->data->dev_private;
> >>>>> -
> >>>>> - rte_ring_free(internals->umem->buf_ring);
> >>>>> - rte_memzone_free(internals->umem->mz);
> >>>>> - rte_free(internals->umem);
> >>>>> -
> >>>>> - rte_eth_dev_release_port(eth_dev);
> >>>>
> >>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
> >>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
> >>>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
> >>>
> >>> I don't understand your comment.
> >>> Calling the close function looks the right thing to do in "remove".
> >>
> >> No concern on calling the 'close'.
> >> My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
> >
> > rte_eth_dev_release_port() is called in eth_dev_close(), isn't it?
>
> No, 'eth_dev_close()' is local 'dev_close()' ops, the one to clear driver
> private resources.
> I assume it is confused with 'rte_eth_dev_close()' ...
>
> >>>>> -
> >>>>> + eth_dev_close(eth_dev);
Ah yes, I overlooked it.
Why not calling rte_eth_dev_close()?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-30 7:55 ` Thomas Monjalon
@ 2019-04-30 7:55 ` Thomas Monjalon
2019-04-30 8:10 ` Ferruh Yigit
1 sibling, 0 replies; 34+ messages in thread
From: Thomas Monjalon @ 2019-04-30 7:55 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Xiaolong Ye, dev
30/04/2019 09:33, Ferruh Yigit:
> On 4/29/2019 11:34 PM, Thomas Monjalon wrote:
> > 30/04/2019 00:28, Ferruh Yigit:
> >> On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
> >>> 29/04/2019 19:00, Ferruh Yigit:
> >>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> >>>>> Since 18.11, it is suggested that driver should release all its private
> >>>>> resources at the dev_close routine. So all resources previously released
> >>>>> in remove routine are now released at the dev_close routine, and the
> >>>>> dev_close routine will be called in driver remove routine in order to
> >>>>> support removing a device without closing its ports.
> >>>>>
> >>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> >>>>> flag during probe stage.
> >>>>>
> >>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> >>>>
> >>>> <...>
> >>>>
> >>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
> >>>>> if (eth_dev == NULL)
> >>>>> return -1;
> >>>>>
> >>>>> - internals = eth_dev->data->dev_private;
> >>>>> -
> >>>>> - rte_ring_free(internals->umem->buf_ring);
> >>>>> - rte_memzone_free(internals->umem->mz);
> >>>>> - rte_free(internals->umem);
> >>>>> -
> >>>>> - rte_eth_dev_release_port(eth_dev);
> >>>>
> >>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
> >>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
> >>>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
> >>>
> >>> I don't understand your comment.
> >>> Calling the close function looks the right thing to do in "remove".
> >>
> >> No concern on calling the 'close'.
> >> My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
> >
> > rte_eth_dev_release_port() is called in eth_dev_close(), isn't it?
>
> No, 'eth_dev_close()' is local 'dev_close()' ops, the one to clear driver
> private resources.
> I assume it is confused with 'rte_eth_dev_close()' ...
>
> >>>>> -
> >>>>> + eth_dev_close(eth_dev);
Ah yes, I overlooked it.
Why not calling rte_eth_dev_close()?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-30 7:55 ` Thomas Monjalon
2019-04-30 7:55 ` Thomas Monjalon
@ 2019-04-30 8:10 ` Ferruh Yigit
2019-04-30 8:10 ` Ferruh Yigit
1 sibling, 1 reply; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-30 8:10 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Xiaolong Ye, dev
On 4/30/2019 8:55 AM, Thomas Monjalon wrote:
> 30/04/2019 09:33, Ferruh Yigit:
>> On 4/29/2019 11:34 PM, Thomas Monjalon wrote:
>>> 30/04/2019 00:28, Ferruh Yigit:
>>>> On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
>>>>> 29/04/2019 19:00, Ferruh Yigit:
>>>>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>>>>>> Since 18.11, it is suggested that driver should release all its private
>>>>>>> resources at the dev_close routine. So all resources previously released
>>>>>>> in remove routine are now released at the dev_close routine, and the
>>>>>>> dev_close routine will be called in driver remove routine in order to
>>>>>>> support removing a device without closing its ports.
>>>>>>>
>>>>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>>>>>> flag during probe stage.
>>>>>>>
>>>>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>>>>>
>>>>>> <...>
>>>>>>
>>>>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>>>>>> if (eth_dev == NULL)
>>>>>>> return -1;
>>>>>>>
>>>>>>> - internals = eth_dev->data->dev_private;
>>>>>>> -
>>>>>>> - rte_ring_free(internals->umem->buf_ring);
>>>>>>> - rte_memzone_free(internals->umem->mz);
>>>>>>> - rte_free(internals->umem);
>>>>>>> -
>>>>>>> - rte_eth_dev_release_port(eth_dev);
>>>>>>
>>>>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>>>>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>>>>>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>>>>>
>>>>> I don't understand your comment.
>>>>> Calling the close function looks the right thing to do in "remove".
>>>>
>>>> No concern on calling the 'close'.
>>>> My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
>>>
>>> rte_eth_dev_release_port() is called in eth_dev_close(), isn't it?
>>
>> No, 'eth_dev_close()' is local 'dev_close()' ops, the one to clear driver
>> private resources.
>> I assume it is confused with 'rte_eth_dev_close()' ...
>>
>>>>>>> -
>>>>>>> + eth_dev_close(eth_dev);
>
> Ah yes, I overlooked it.
> Why not calling rte_eth_dev_close()?
>
It may work, but I am for keeping 'dev_close()' and 'rte_eth_dev_release_port()'
as two steps in '.remove()', I think there is no need to include
'rte_eth_dev_close()' API here and its possible/future side affects etc...
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-30 8:10 ` Ferruh Yigit
@ 2019-04-30 8:10 ` Ferruh Yigit
0 siblings, 0 replies; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-30 8:10 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Xiaolong Ye, dev
On 4/30/2019 8:55 AM, Thomas Monjalon wrote:
> 30/04/2019 09:33, Ferruh Yigit:
>> On 4/29/2019 11:34 PM, Thomas Monjalon wrote:
>>> 30/04/2019 00:28, Ferruh Yigit:
>>>> On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
>>>>> 29/04/2019 19:00, Ferruh Yigit:
>>>>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>>>>>> Since 18.11, it is suggested that driver should release all its private
>>>>>>> resources at the dev_close routine. So all resources previously released
>>>>>>> in remove routine are now released at the dev_close routine, and the
>>>>>>> dev_close routine will be called in driver remove routine in order to
>>>>>>> support removing a device without closing its ports.
>>>>>>>
>>>>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>>>>>> flag during probe stage.
>>>>>>>
>>>>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>>>>>
>>>>>> <...>
>>>>>>
>>>>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>>>>>> if (eth_dev == NULL)
>>>>>>> return -1;
>>>>>>>
>>>>>>> - internals = eth_dev->data->dev_private;
>>>>>>> -
>>>>>>> - rte_ring_free(internals->umem->buf_ring);
>>>>>>> - rte_memzone_free(internals->umem->mz);
>>>>>>> - rte_free(internals->umem);
>>>>>>> -
>>>>>>> - rte_eth_dev_release_port(eth_dev);
>>>>>>
>>>>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>>>>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>>>>>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>>>>>
>>>>> I don't understand your comment.
>>>>> Calling the close function looks the right thing to do in "remove".
>>>>
>>>> No concern on calling the 'close'.
>>>> My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
>>>
>>> rte_eth_dev_release_port() is called in eth_dev_close(), isn't it?
>>
>> No, 'eth_dev_close()' is local 'dev_close()' ops, the one to clear driver
>> private resources.
>> I assume it is confused with 'rte_eth_dev_close()' ...
>>
>>>>>>> -
>>>>>>> + eth_dev_close(eth_dev);
>
> Ah yes, I overlooked it.
> Why not calling rte_eth_dev_close()?
>
It may work, but I am for keeping 'dev_close()' and 'rte_eth_dev_release_port()'
as two steps in '.remove()', I think there is no need to include
'rte_eth_dev_close()' API here and its possible/future side affects etc...
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-30 7:35 ` Ferruh Yigit
2019-04-30 7:35 ` Ferruh Yigit
@ 2019-04-30 8:37 ` Ye Xiaolong
2019-04-30 8:37 ` Ye Xiaolong
1 sibling, 1 reply; 34+ messages in thread
From: Ye Xiaolong @ 2019-04-30 8:37 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev, Thomas Monjalon
On 04/30, Ferruh Yigit wrote:
>On 4/30/2019 3:06 AM, Ye Xiaolong wrote:
>> Hi, Ferruh
>>
>> On 04/29, Ferruh Yigit wrote:
>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>>> Since 18.11, it is suggested that driver should release all its private
>>>> resources at the dev_close routine. So all resources previously released
>>>> in remove routine are now released at the dev_close routine, and the
>>>> dev_close routine will be called in driver remove routine in order to
>>>> support removing a device without closing its ports.
>>>>
>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>>> flag during probe stage.
>>>>
>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>>
>>> <...>
>>>
>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>>> if (eth_dev == NULL)
>>>> return -1;
>>>>
>>>> - internals = eth_dev->data->dev_private;
>>>> -
>>>> - rte_ring_free(internals->umem->buf_ring);
>>>> - rte_memzone_free(internals->umem->mz);
>>>> - rte_free(internals->umem);
>>>> -
>>>> - rte_eth_dev_release_port(eth_dev);
>>>
>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>>>
>>
>> remove() would call eth_dev_close which includes the rte_eth_dev_release_port().
>
>'eth_dev_close()' doesn't call the 'rte_eth_dev_release_port()', and it
>shouldn't really, am I missing something?
Sorry, it's the rte_eth_dev_close that calls rte_eth_dev_release_port, here in
.remove we do need to call rte_eth_dev_release_port explicitly.
will send a new version.
Thanks,
Xiaolong
>
>>
>> Thanks,
>> Xiaolong
>>
>>>> -
>>>> + eth_dev_close(eth_dev);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed
2019-04-30 8:37 ` Ye Xiaolong
@ 2019-04-30 8:37 ` Ye Xiaolong
0 siblings, 0 replies; 34+ messages in thread
From: Ye Xiaolong @ 2019-04-30 8:37 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev, Thomas Monjalon
On 04/30, Ferruh Yigit wrote:
>On 4/30/2019 3:06 AM, Ye Xiaolong wrote:
>> Hi, Ferruh
>>
>> On 04/29, Ferruh Yigit wrote:
>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>>> Since 18.11, it is suggested that driver should release all its private
>>>> resources at the dev_close routine. So all resources previously released
>>>> in remove routine are now released at the dev_close routine, and the
>>>> dev_close routine will be called in driver remove routine in order to
>>>> support removing a device without closing its ports.
>>>>
>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>>> flag during probe stage.
>>>>
>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>>
>>> <...>
>>>
>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>>> if (eth_dev == NULL)
>>>> return -1;
>>>>
>>>> - internals = eth_dev->data->dev_private;
>>>> -
>>>> - rte_ring_free(internals->umem->buf_ring);
>>>> - rte_memzone_free(internals->umem->mz);
>>>> - rte_free(internals->umem);
>>>> -
>>>> - rte_eth_dev_release_port(eth_dev);
>>>
>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>>>
>>
>> remove() would call eth_dev_close which includes the rte_eth_dev_release_port().
>
>'eth_dev_close()' doesn't call the 'rte_eth_dev_release_port()', and it
>shouldn't really, am I missing something?
Sorry, it's the rte_eth_dev_close that calls rte_eth_dev_release_port, here in
.remove we do need to call rte_eth_dev_release_port explicitly.
will send a new version.
Thanks,
Xiaolong
>
>>
>> Thanks,
>> Xiaolong
>>
>>>> -
>>>> + eth_dev_close(eth_dev);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [dpdk-dev] [PATCH v3] net/af_xdp: remove resources when port is closed
2019-04-26 5:09 [dpdk-dev] [PATCH v1 0/2] support RTE_ETH_DEV_CLOSE_REMOVE flag for AF_XDP Xiaolong Ye
` (2 preceding siblings ...)
2019-04-26 5:09 ` [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL Xiaolong Ye
@ 2019-04-30 8:39 ` Xiaolong Ye
2019-04-30 8:39 ` Xiaolong Ye
2019-04-30 11:02 ` Ferruh Yigit
3 siblings, 2 replies; 34+ messages in thread
From: Xiaolong Ye @ 2019-04-30 8:39 UTC (permalink / raw)
To: dev, Ferruh Yigit, Thomas Monjalon; +Cc: Xiaolong Ye
Since 18.11, it is suggested that driver should release all its private
resources at the dev_close routine. So all resources previously released
in remove routine are now released at the dev_close routine, and the
dev_close routine will be called in driver remove routine in order to
support removing a device without closing its ports.
Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
flag during probe stage.
Also as af_xdp pmd doesn't allocate MAC addresses dynamically, it needs
to be set as NULL, so it won't be released by rte_eth_dev_release_port(),
otherwise, there would be "EAL: Error: Invalid memory" error.
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
V3: keep rte_eth_dev_release_port in .remove ops
V2: merge previous 2 patches into 1 patch since the first one would
introduce one error (second patch is the fix), it makes more sense
to squash the second into the first one.
drivers/net/af_xdp/rte_eth_af_xdp.c | 45 ++++++++++++++++-------------
1 file changed, 25 insertions(+), 20 deletions(-)
diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index acf9ad605..35c72272c 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -426,6 +426,19 @@ remove_xdp_program(struct pmd_internals *internals)
XDP_FLAGS_UPDATE_IF_NOEXIST);
}
+static void
+xdp_umem_destroy(struct xsk_umem_info *umem)
+{
+ rte_memzone_free(umem->mz);
+ umem->mz = NULL;
+
+ rte_ring_free(umem->buf_ring);
+ umem->buf_ring = NULL;
+
+ rte_free(umem);
+ umem = NULL;
+}
+
static void
eth_dev_close(struct rte_eth_dev *dev)
{
@@ -444,6 +457,15 @@ eth_dev_close(struct rte_eth_dev *dev)
}
(void)xsk_umem__delete(internals->umem->umem);
+
+ /*
+ * MAC is not allocated dynamically, setting it to NULL would prevent
+ * from releasing it in rte_eth_dev_release_port.
+ */
+ dev->data->mac_addrs = NULL;
+
+ xdp_umem_destroy(internals->umem);
+
remove_xdp_program(internals);
}
@@ -459,19 +481,6 @@ eth_link_update(struct rte_eth_dev *dev __rte_unused,
return 0;
}
-static void
-xdp_umem_destroy(struct xsk_umem_info *umem)
-{
- rte_memzone_free(umem->mz);
- umem->mz = NULL;
-
- rte_ring_free(umem->buf_ring);
- umem->buf_ring = NULL;
-
- rte_free(umem);
- umem = NULL;
-}
-
static struct
xsk_umem_info *xdp_umem_configure(struct pmd_internals *internals)
{
@@ -856,6 +865,8 @@ init_internals(struct rte_vdev_device *dev,
eth_dev->dev_ops = &ops;
eth_dev->rx_pkt_burst = eth_af_xdp_rx;
eth_dev->tx_pkt_burst = eth_af_xdp_tx;
+ /* Let rte_eth_dev_close() release the port resources. */
+ eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
return eth_dev;
@@ -923,7 +934,6 @@ static int
rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
{
struct rte_eth_dev *eth_dev = NULL;
- struct pmd_internals *internals;
AF_XDP_LOG(INFO, "Removing AF_XDP ethdev on numa socket %u\n",
rte_socket_id());
@@ -936,12 +946,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
if (eth_dev == NULL)
return -1;
- internals = eth_dev->data->dev_private;
-
- rte_ring_free(internals->umem->buf_ring);
- rte_memzone_free(internals->umem->mz);
- rte_free(internals->umem);
-
+ eth_dev_close(eth_dev);
rte_eth_dev_release_port(eth_dev);
--
2.17.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [dpdk-dev] [PATCH v3] net/af_xdp: remove resources when port is closed
2019-04-30 8:39 ` [dpdk-dev] [PATCH v3] net/af_xdp: remove resources when port is closed Xiaolong Ye
@ 2019-04-30 8:39 ` Xiaolong Ye
2019-04-30 11:02 ` Ferruh Yigit
1 sibling, 0 replies; 34+ messages in thread
From: Xiaolong Ye @ 2019-04-30 8:39 UTC (permalink / raw)
To: dev, Ferruh Yigit, Thomas Monjalon; +Cc: Xiaolong Ye
Since 18.11, it is suggested that driver should release all its private
resources at the dev_close routine. So all resources previously released
in remove routine are now released at the dev_close routine, and the
dev_close routine will be called in driver remove routine in order to
support removing a device without closing its ports.
Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
flag during probe stage.
Also as af_xdp pmd doesn't allocate MAC addresses dynamically, it needs
to be set as NULL, so it won't be released by rte_eth_dev_release_port(),
otherwise, there would be "EAL: Error: Invalid memory" error.
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
V3: keep rte_eth_dev_release_port in .remove ops
V2: merge previous 2 patches into 1 patch since the first one would
introduce one error (second patch is the fix), it makes more sense
to squash the second into the first one.
drivers/net/af_xdp/rte_eth_af_xdp.c | 45 ++++++++++++++++-------------
1 file changed, 25 insertions(+), 20 deletions(-)
diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index acf9ad605..35c72272c 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -426,6 +426,19 @@ remove_xdp_program(struct pmd_internals *internals)
XDP_FLAGS_UPDATE_IF_NOEXIST);
}
+static void
+xdp_umem_destroy(struct xsk_umem_info *umem)
+{
+ rte_memzone_free(umem->mz);
+ umem->mz = NULL;
+
+ rte_ring_free(umem->buf_ring);
+ umem->buf_ring = NULL;
+
+ rte_free(umem);
+ umem = NULL;
+}
+
static void
eth_dev_close(struct rte_eth_dev *dev)
{
@@ -444,6 +457,15 @@ eth_dev_close(struct rte_eth_dev *dev)
}
(void)xsk_umem__delete(internals->umem->umem);
+
+ /*
+ * MAC is not allocated dynamically, setting it to NULL would prevent
+ * from releasing it in rte_eth_dev_release_port.
+ */
+ dev->data->mac_addrs = NULL;
+
+ xdp_umem_destroy(internals->umem);
+
remove_xdp_program(internals);
}
@@ -459,19 +481,6 @@ eth_link_update(struct rte_eth_dev *dev __rte_unused,
return 0;
}
-static void
-xdp_umem_destroy(struct xsk_umem_info *umem)
-{
- rte_memzone_free(umem->mz);
- umem->mz = NULL;
-
- rte_ring_free(umem->buf_ring);
- umem->buf_ring = NULL;
-
- rte_free(umem);
- umem = NULL;
-}
-
static struct
xsk_umem_info *xdp_umem_configure(struct pmd_internals *internals)
{
@@ -856,6 +865,8 @@ init_internals(struct rte_vdev_device *dev,
eth_dev->dev_ops = &ops;
eth_dev->rx_pkt_burst = eth_af_xdp_rx;
eth_dev->tx_pkt_burst = eth_af_xdp_tx;
+ /* Let rte_eth_dev_close() release the port resources. */
+ eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
return eth_dev;
@@ -923,7 +934,6 @@ static int
rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
{
struct rte_eth_dev *eth_dev = NULL;
- struct pmd_internals *internals;
AF_XDP_LOG(INFO, "Removing AF_XDP ethdev on numa socket %u\n",
rte_socket_id());
@@ -936,12 +946,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
if (eth_dev == NULL)
return -1;
- internals = eth_dev->data->dev_private;
-
- rte_ring_free(internals->umem->buf_ring);
- rte_memzone_free(internals->umem->mz);
- rte_free(internals->umem);
-
+ eth_dev_close(eth_dev);
rte_eth_dev_release_port(eth_dev);
--
2.17.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v3] net/af_xdp: remove resources when port is closed
2019-04-30 8:39 ` [dpdk-dev] [PATCH v3] net/af_xdp: remove resources when port is closed Xiaolong Ye
2019-04-30 8:39 ` Xiaolong Ye
@ 2019-04-30 11:02 ` Ferruh Yigit
2019-04-30 11:02 ` Ferruh Yigit
1 sibling, 1 reply; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-30 11:02 UTC (permalink / raw)
To: Xiaolong Ye, dev, Thomas Monjalon
On 4/30/2019 9:39 AM, Xiaolong Ye wrote:
> Since 18.11, it is suggested that driver should release all its private
> resources at the dev_close routine. So all resources previously released
> in remove routine are now released at the dev_close routine, and the
> dev_close routine will be called in driver remove routine in order to
> support removing a device without closing its ports.
>
> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> flag during probe stage.
>
> Also as af_xdp pmd doesn't allocate MAC addresses dynamically, it needs
> to be set as NULL, so it won't be released by rte_eth_dev_release_port(),
> otherwise, there would be "EAL: Error: Invalid memory" error.
>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Applied to dpdk-next-net/master, thanks.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [dpdk-dev] [PATCH v3] net/af_xdp: remove resources when port is closed
2019-04-30 11:02 ` Ferruh Yigit
@ 2019-04-30 11:02 ` Ferruh Yigit
0 siblings, 0 replies; 34+ messages in thread
From: Ferruh Yigit @ 2019-04-30 11:02 UTC (permalink / raw)
To: Xiaolong Ye, dev, Thomas Monjalon
On 4/30/2019 9:39 AM, Xiaolong Ye wrote:
> Since 18.11, it is suggested that driver should release all its private
> resources at the dev_close routine. So all resources previously released
> in remove routine are now released at the dev_close routine, and the
> dev_close routine will be called in driver remove routine in order to
> support removing a device without closing its ports.
>
> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> flag during probe stage.
>
> Also as af_xdp pmd doesn't allocate MAC addresses dynamically, it needs
> to be set as NULL, so it won't be released by rte_eth_dev_release_port(),
> otherwise, there would be "EAL: Error: Invalid memory" error.
>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Applied to dpdk-next-net/master, thanks.
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2019-04-30 11:02 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-26 5:09 [dpdk-dev] [PATCH v1 0/2] support RTE_ETH_DEV_CLOSE_REMOVE flag for AF_XDP Xiaolong Ye
2019-04-26 5:09 ` Xiaolong Ye
2019-04-26 5:09 ` [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed Xiaolong Ye
2019-04-26 5:09 ` Xiaolong Ye
2019-04-29 17:00 ` Ferruh Yigit
2019-04-29 17:00 ` Ferruh Yigit
2019-04-29 20:14 ` Thomas Monjalon
2019-04-29 20:14 ` Thomas Monjalon
2019-04-29 22:28 ` Ferruh Yigit
2019-04-29 22:28 ` Ferruh Yigit
2019-04-29 22:34 ` Thomas Monjalon
2019-04-29 22:34 ` Thomas Monjalon
2019-04-30 7:33 ` Ferruh Yigit
2019-04-30 7:33 ` Ferruh Yigit
2019-04-30 7:55 ` Thomas Monjalon
2019-04-30 7:55 ` Thomas Monjalon
2019-04-30 8:10 ` Ferruh Yigit
2019-04-30 8:10 ` Ferruh Yigit
2019-04-30 2:06 ` Ye Xiaolong
2019-04-30 2:06 ` Ye Xiaolong
2019-04-30 7:35 ` Ferruh Yigit
2019-04-30 7:35 ` Ferruh Yigit
2019-04-30 8:37 ` Ye Xiaolong
2019-04-30 8:37 ` Ye Xiaolong
2019-04-26 5:09 ` [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL Xiaolong Ye
2019-04-26 5:09 ` Xiaolong Ye
2019-04-29 17:02 ` Ferruh Yigit
2019-04-29 17:02 ` Ferruh Yigit
2019-04-30 2:04 ` Ye Xiaolong
2019-04-30 2:04 ` Ye Xiaolong
2019-04-30 8:39 ` [dpdk-dev] [PATCH v3] net/af_xdp: remove resources when port is closed Xiaolong Ye
2019-04-30 8:39 ` Xiaolong Ye
2019-04-30 11:02 ` Ferruh Yigit
2019-04-30 11:02 ` Ferruh Yigit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).