[dpdk-dev] [PATCH v1 0/2] support RTE_ETH_DEV_CLOSE_REMOVE flag for AF_XDP

[dpdk-dev] [PATCH v1 0/2] support RTE_ETH_DEV_CLOSE_REMOVE flag for AF_XDP

[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

[dpdk-dev] [PATCH v1 0/2] support RTE_ETH_DEV_CLOSE_REMOVE flag for AF_XDP

[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

[dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[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

[dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[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

[dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL

[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

[dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL

[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

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

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

> -
> +	eth_dev_close(eth_dev);
>  
>  	return 0;
>  }
> 

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

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

> -
> +	eth_dev_close(eth_dev);
>  
>  	return 0;
>  }
> 

Re: [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL

[Ferruh Yigit]

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?

Re: [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL

[Ferruh Yigit]

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?

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Thomas Monjalon]

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

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Thomas Monjalon]

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

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

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

> 
>>> -
>>> +	eth_dev_close(eth_dev);
> 
> 
> 

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

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

> 
>>> -
>>> +	eth_dev_close(eth_dev);
> 
> 
> 

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Thomas Monjalon]

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

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Thomas Monjalon]

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

Re: [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL

[Ye Xiaolong]

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

Re: [dpdk-dev] [PATCH v1 2/2] net/af_xdp: set MAC addrs field to NULL

[Ye Xiaolong]

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

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Ye Xiaolong]

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

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Ye Xiaolong]

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

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[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);
> 
> 
> 

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[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);
> 
> 
> 

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Ferruh Yigit]

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

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Ferruh Yigit]

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

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Thomas Monjalon]

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()?

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Thomas Monjalon]

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()?

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Ferruh Yigit]

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

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Ferruh Yigit]

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

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Ye Xiaolong]

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

Re: [dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Ye Xiaolong]

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

[dpdk-dev] [PATCH v3] net/af_xdp: remove resources when port is closed

[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

[dpdk-dev] [PATCH v3] net/af_xdp: remove resources when port is closed

[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

Re: [dpdk-dev] [PATCH v3] net/af_xdp: remove resources when port is closed

[Ferruh Yigit]

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.

Re: [dpdk-dev] [PATCH v3] net/af_xdp: remove resources when port is closed

[Ferruh Yigit]

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.