[PATCH 0/2] app/testpmd: fix quit testpmd with vfs and pf

[PATCH 0/2] app/testpmd: fix quit testpmd with vfs and pf

[wenxuanx.wu]

From: wenxuan wu <wenxuanx.wu@intel.com>

Apply our reverse iterator macro into testpmd to avoid testpmd startups with pf
and vfs quit without error heap-free-before-use 
 
wenxuan wu (2):
  app/testpmd: fix quit testpmd with vfs and pf
  lib/ethdev: add reverse macro to quit testpmd

 app/test-pmd/testpmd.c  |  4 ++--
 lib/ethdev/rte_ethdev.h | 14 +++++++++++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 1/2] app/testpmd: fix quit testpmd with vfs and pf

[wenxuanx.wu]

From: wenxuan wu <wenxuanx.wu@intel.com>

When testpmd startups with pf and vfs,this error occurs when quitting,
results in pf is released before vfs ,so the vf would access an
freed heap memory.

The solution is that release our allocated ports in reverse
order,add two macros RTE_ETH_FOREACH_DEV_REVERSE_OWNED_BY and
RTE_ETH_FOREACH_DEV_REVERSE,which would be used in quit procedure of
testpmd, error is fixed.

Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
Cc: stable@dpdk.org

Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
---
 app/test-pmd/testpmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e1da961311..698b6d8cc4 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3384,12 +3384,12 @@ pmd_test_exit(void)
 #endif
 	if (ports != NULL) {
 		no_link_check = 1;
-		RTE_ETH_FOREACH_DEV(pt_id) {
+		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
 			printf("\nStopping port %d...\n", pt_id);
 			fflush(stdout);
 			stop_port(pt_id);
 		}
-		RTE_ETH_FOREACH_DEV(pt_id) {
+		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
 			printf("\nShutting down port %d...\n", pt_id);
 			fflush(stdout);
 			close_port(pt_id);
-- 
2.25.1

RE: [PATCH 1/2] app/testpmd: fix quit testpmd with vfs and pf

[Zhang, Yuying]

Hi Wenxuan,

> -----Original Message-----
> From: Wu, WenxuanX <wenxuanx.wu@intel.com>
> Sent: Thursday, February 17, 2022 6:14 PM
> To: Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>; Singh, Aman Deep
> <aman.deep.singh@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: [PATCH 1/2] app/testpmd: fix quit testpmd with vfs and pf
> 
> From: wenxuan wu <wenxuanx.wu@intel.com>
> 
> When testpmd startups with pf and vfs,this error occurs when quitting, results in
> pf is released before vfs ,so the vf would access an freed heap memory.
> 
> The solution is that release our allocated ports in reverse order,add two macros
> RTE_ETH_FOREACH_DEV_REVERSE_OWNED_BY and
> RTE_ETH_FOREACH_DEV_REVERSE,which would be used in quit procedure of
> testpmd, error is fixed.
> 

Please correct grammatical errors and refine your commit log.

> Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
> Cc: stable@dpdk.org
> 
> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
> ---
>  app/test-pmd/testpmd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> e1da961311..698b6d8cc4 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3384,12 +3384,12 @@ pmd_test_exit(void)  #endif
>  	if (ports != NULL) {
>  		no_link_check = 1;
> -		RTE_ETH_FOREACH_DEV(pt_id) {
> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
>  			printf("\nStopping port %d...\n", pt_id);
>  			fflush(stdout);
>  			stop_port(pt_id);
>  		}
> -		RTE_ETH_FOREACH_DEV(pt_id) {
> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
>  			printf("\nShutting down port %d...\n", pt_id);
>  			fflush(stdout);
>  			close_port(pt_id);
> --
> 2.25.1

Re: [PATCH 1/2] app/testpmd: fix quit testpmd with vfs and pf

[Ferruh Yigit]

On 2/17/2022 10:14 AM, wenxuanx.wu@intel.com wrote:
> From: wenxuan wu <wenxuanx.wu@intel.com>
> 
> When testpmd startups with pf and vfs,this error occurs when quitting,
> results in pf is released before vfs ,so the vf would access an
> freed heap memory.
> 
> The solution is that release our allocated ports in reverse
> order,add two macros RTE_ETH_FOREACH_DEV_REVERSE_OWNED_BY and
> RTE_ETH_FOREACH_DEV_REVERSE,which would be used in quit procedure of
> testpmd, error is fixed.
> 
> Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
> Cc: stable@dpdk.org
> 
> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
> ---
>   app/test-pmd/testpmd.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index e1da961311..698b6d8cc4 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3384,12 +3384,12 @@ pmd_test_exit(void)
>   #endif
>   	if (ports != NULL) {
>   		no_link_check = 1;
> -		RTE_ETH_FOREACH_DEV(pt_id) {
> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {

This fix assumes PF is always probed before VFs (so PF has smaller
port id than VF).

I am not sure if this is guaranteed, ports ported based on scan
in the sysfs folder.

Instead, what do you think PF port stop/close fail when there
are still VFs probed? I think PF driver can know if there are
VFs ported.

>   			printf("\nStopping port %d...\n", pt_id);
>   			fflush(stdout);
>   			stop_port(pt_id);
>   		}
> -		RTE_ETH_FOREACH_DEV(pt_id) {
> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
>   			printf("\nShutting down port %d...\n", pt_id);
>   			fflush(stdout);
>   			close_port(pt_id);

[PATCH 2/2] lib/ethdev: add reverse macro to quit testpmd

[wenxuanx.wu]

From: wenxuan wu <wenxuanx.wu@intel.com>

There is a heap-free-after-use bug when quit testpmd
with pf and vfs, stop and close ports in reverse order
is a more reasonable approach.

Cc: stable@dpdk.org

Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
---
 lib/ethdev/rte_ethdev.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 2660e4f374..e080840b06 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2185,9 +2185,13 @@ struct rte_eth_dev_owner {
  * @return
  *   Next valid port ID owned by owner_id, RTE_MAX_ETHPORTS if there is none.
  */
-uint64_t rte_eth_find_next_owned_by(uint16_t port_id,
+uint64_t
+rte_eth_find_next_owned_by(uint16_t port_id,
 		const uint64_t owner_id);
 
+uint64_t
+rte_eth_find_prev_owned_by(uint16_t port_id, const uint64_t owner_id);
+
 /**
  * Macro to iterate over all enabled ethdev ports owned by a specific owner.
  */
@@ -2212,6 +2216,14 @@ uint16_t rte_eth_find_next(uint16_t port_id);
 #define RTE_ETH_FOREACH_DEV(p) \
 	RTE_ETH_FOREACH_DEV_OWNED_BY(p, RTE_ETH_DEV_NO_OWNER)
 
+/**
+ * Macro to iterate over all enabled and ownerless ethdev ports in reverse order, for quit purpose.
+ */
+#define RTE_ETH_FOREACH_DEV_REVERSE(p) \
+	for (p = (rte_eth_dev_count_total() - 1 >= 0) ? (rte_eth_dev_count_total() - 1) : 0; \
+	     p < rte_eth_dev_count_total(); \
+	     p--)
+
 /**
  * Iterates over ethdev ports of a specified device.
  *
-- 
2.25.1

RE: [PATCH 2/2] lib/ethdev: add reverse macro to quit testpmd

[Zhang, Yuying]

Hi Wenxuan,

> -----Original Message-----
> From: Wu, WenxuanX <wenxuanx.wu@intel.com>
> Sent: Thursday, February 17, 2022 6:14 PM
> To: Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>; Singh, Aman Deep
> <aman.deep.singh@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: [PATCH 2/2] lib/ethdev: add reverse macro to quit testpmd
> 
> From: wenxuan wu <wenxuanx.wu@intel.com>
> 
> There is a heap-free-after-use bug when quit testpmd with pf and vfs, stop and
> close ports in reverse order is a more reasonable approach.

Please explain the issue and your fix in detail. Since your patch is a fix, you need a fix line here.

> Cc: stable@dpdk.org
> 
> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
> ---
>  lib/ethdev/rte_ethdev.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
> 2660e4f374..e080840b06 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -2185,9 +2185,13 @@ struct rte_eth_dev_owner {
>   * @return
>   *   Next valid port ID owned by owner_id, RTE_MAX_ETHPORTS if there is none.
>   */
> -uint64_t rte_eth_find_next_owned_by(uint16_t port_id,
> +uint64_t
> +rte_eth_find_next_owned_by(uint16_t port_id,
>  		const uint64_t owner_id);
> 
> +uint64_t
> +rte_eth_find_prev_owned_by(uint16_t port_id, const uint64_t owner_id);
> +

Please keep consistency of code style of similar declaration.

>  /**
>   * Macro to iterate over all enabled ethdev ports owned by a specific owner.
>   */
> @@ -2212,6 +2216,14 @@ uint16_t rte_eth_find_next(uint16_t port_id);
> #define RTE_ETH_FOREACH_DEV(p) \
>  	RTE_ETH_FOREACH_DEV_OWNED_BY(p, RTE_ETH_DEV_NO_OWNER)
> 
> +/**
> + * Macro to iterate over all enabled and ownerless ethdev ports in reverse
> order, for quit purpose.
> + */
> +#define RTE_ETH_FOREACH_DEV_REVERSE(p) \
> +	for (p = (rte_eth_dev_count_total() - 1 >= 0) ?
> (rte_eth_dev_count_total() - 1) : 0; \
> +	     p < rte_eth_dev_count_total(); \
> +	     p--)

Use rte_eth_dev_count_total() function can only get the number of valid port instead of
port id separately. You should can refer to the logic of RTE_ETH_FOREACH_DEV(p).

> +
>  /**
>   * Iterates over ethdev ports of a specified device.
>   *
> --
> 2.25.1

Re: [PATCH 2/2] lib/ethdev: add reverse macro to quit testpmd

[Ferruh Yigit]

On 2/17/2022 10:14 AM, wenxuanx.wu@intel.com wrote:
> From: wenxuan wu <wenxuanx.wu@intel.com>
> 
> There is a heap-free-after-use bug when quit testpmd
> with pf and vfs, stop and close ports in reverse order
> is a more reasonable approach.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
> ---
>   lib/ethdev/rte_ethdev.h | 14 +++++++++++++-

This patch should come before testpmd patch, because testpmd
is using macro defined here.

>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 2660e4f374..e080840b06 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -2185,9 +2185,13 @@ struct rte_eth_dev_owner {
>    * @return
>    *   Next valid port ID owned by owner_id, RTE_MAX_ETHPORTS if there is none.
>    */
> -uint64_t rte_eth_find_next_owned_by(uint16_t port_id,
> +uint64_t
> +rte_eth_find_next_owned_by(uint16_t port_id,
>   		const uint64_t owner_id);
>   
> +uint64_t
> +rte_eth_find_prev_owned_by(uint16_t port_id, const uint64_t owner_id);
> +

This function declared but not implemented, can remove above.

>   /**
>    * Macro to iterate over all enabled ethdev ports owned by a specific owner.
>    */
> @@ -2212,6 +2216,14 @@ uint16_t rte_eth_find_next(uint16_t port_id);
>   #define RTE_ETH_FOREACH_DEV(p) \
>   	RTE_ETH_FOREACH_DEV_OWNED_BY(p, RTE_ETH_DEV_NO_OWNER)
>   
> +/**
> + * Macro to iterate over all enabled and ownerless ethdev ports in reverse order, for quit purpose.

No need to add comment related to "quit purpose", macro can be
used for different reasons.

> + */
> +#define RTE_ETH_FOREACH_DEV_REVERSE(p) \
> +	for (p = (rte_eth_dev_count_total() - 1 >= 0) ? (rte_eth_dev_count_total() - 1) : 0; \

Below check already relies on 'p' is unsigned,
also 'rte_eth_dev_count_total()' returns unsigned,
if so above check is unnecessary, it can just have:
"p = rte_eth_dev_count_total() - 1"

> +	     p < rte_eth_dev_count_total(); \
> +	     p--)
> +
>   /**
>    * Iterates over ethdev ports of a specified device.
>    *

[PATCH 0/2] app/testpmd: fix testpmd quit with pf and vfs

[wenxuanx.wu]

From: wenxuan wu <wenxuanx.wu@intel.com>

Apply our reverse iterator macro into testpmd to 
avoid testpmd startups with pf and vfs quit without 
error heap-free-before-use 
wenxuan wu (2):
  lib/ethdev: add reverse macro to quit testpmd
  app/testpmd:fix testpmd quit failure

 app/test-pmd/testpmd.c  |  4 ++--
 lib/ethdev/rte_ethdev.h | 11 ++++++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] lib/ethdev: add reverse macro to quit testpmd

[wenxuanx.wu]

From: wenxuan wu <wenxuanx.wu@intel.com>

There is a heap-free-after-use bug when quit testpmd
with pf and vfs, stop and close ports in reverse order
is a more reasonable approach.

Cc: stable@dpdk.org

Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
---
 lib/ethdev/rte_ethdev.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 2660e4f374..813f72e825 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2185,7 +2185,8 @@ struct rte_eth_dev_owner {
  * @return
  *   Next valid port ID owned by owner_id, RTE_MAX_ETHPORTS if there is none.
  */
-uint64_t rte_eth_find_next_owned_by(uint16_t port_id,
+uint64_t
+rte_eth_find_next_owned_by(uint16_t port_id,
 		const uint64_t owner_id);
 
 /**
@@ -2212,6 +2213,14 @@ uint16_t rte_eth_find_next(uint16_t port_id);
 #define RTE_ETH_FOREACH_DEV(p) \
 	RTE_ETH_FOREACH_DEV_OWNED_BY(p, RTE_ETH_DEV_NO_OWNER)
 
+/**
+ * Macro to iterate over all enabled and ownerless ethdev ports in reverse order.
+ */
+#define RTE_ETH_FOREACH_DEV_REVERSE(p) \
+	for (p = rte_eth_dev_count_total() - 1; \
+	     p < rte_eth_dev_count_total(); \
+	     p--)
+
 /**
  * Iterates over ethdev ports of a specified device.
  *
-- 
2.25.1

[PATCH v2 2/2] app/testpmd:fix testpmd quit failure

[wenxuanx.wu]

From: wenxuan wu <wenxuanx.wu@intel.com>

When testpmd start ed with 1 pf and 2 vfs, testpmd quited while vfs 
were still alive would result in failure. Root cause is that pf had
been released already but vfs were still accessing by func 
rte_eth_dev_info_get, which would result in heap-free-after-use error.

By quitting our ports in reverse order to avoid this.And the order is
guaranteed that vf are created after pfs.

Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
Cc: stable@dpdk.org

Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
---
 app/test-pmd/testpmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e1da961311..698b6d8cc4 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3384,12 +3384,12 @@ pmd_test_exit(void)
 #endif
 	if (ports != NULL) {
 		no_link_check = 1;
-		RTE_ETH_FOREACH_DEV(pt_id) {
+		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
 			printf("\nStopping port %d...\n", pt_id);
 			fflush(stdout);
 			stop_port(pt_id);
 		}
-		RTE_ETH_FOREACH_DEV(pt_id) {
+		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
 			printf("\nShutting down port %d...\n", pt_id);
 			fflush(stdout);
 			close_port(pt_id);
-- 
2.25.1

Re: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure

[Ferruh Yigit]

On 2/23/2022 11:32 AM, wenxuanx.wu@intel.com wrote:
> From: wenxuan wu <wenxuanx.wu@intel.com>
> 
> When testpmd start ed with 1 pf and 2 vfs, testpmd quited while vfs
> were still alive would result in failure. Root cause is that pf had
> been released already but vfs were still accessing by func
> rte_eth_dev_info_get, which would result in heap-free-after-use error.
> 
> By quitting our ports in reverse order to avoid this.And the order is
> guaranteed that vf are created after pfs.
> 
> Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
> Cc: stable@dpdk.org
> 
> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
> ---
>   app/test-pmd/testpmd.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index e1da961311..698b6d8cc4 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3384,12 +3384,12 @@ pmd_test_exit(void)
>   #endif
>   	if (ports != NULL) {
>   		no_link_check = 1;
> -		RTE_ETH_FOREACH_DEV(pt_id) {
> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {

The main problem with this patch was this logic,
can you please check comment on previous version?

>   			printf("\nStopping port %d...\n", pt_id);
>   			fflush(stdout);
>   			stop_port(pt_id);
>   		}
> -		RTE_ETH_FOREACH_DEV(pt_id) {
> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
>   			printf("\nShutting down port %d...\n", pt_id);
>   			fflush(stdout);
>   			close_port(pt_id);

RE: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure

[Wu, WenxuanX]

I found this meaning in DPDK testplan. 
Note that currently hot-plugging of representor ports is not supported so all the required representors must be specified on the creation of the PF or the trusted VF.
When testpmd is started with pf and vf representors, the order of representor is determined on creation. So it is guaranteed that ,pf is beneath the vf representors, we implemented in a reverse way is acceptable just at present,  depends on when the hot-plugging of representor is supported.  

> -----Original Message-----
> From: Wu, WenxuanX <wenxuanx.wu@intel.com>
> Sent: 2022年2月23日 19:33
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; dev@dpdk.org
> Cc: Wu, WenxuanX <wenxuanx.wu@intel.com>; stable@dpdk.org
> Subject: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
> 
> From: wenxuan wu <wenxuanx.wu@intel.com>
> 
> When testpmd start ed with 1 pf and 2 vfs, testpmd quited while vfs were
> still alive would result in failure. Root cause is that pf had been released
> already but vfs were still accessing by func rte_eth_dev_info_get, which
> would result in heap-free-after-use error.
> 
> By quitting our ports in reverse order to avoid this.And the order is
> guaranteed that vf are created after pfs.
> 
> Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
> Cc: stable@dpdk.org
> 
> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
> ---
>  app/test-pmd/testpmd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> e1da961311..698b6d8cc4 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3384,12 +3384,12 @@ pmd_test_exit(void)  #endif
>  	if (ports != NULL) {
>  		no_link_check = 1;
> -		RTE_ETH_FOREACH_DEV(pt_id) {
> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
>  			printf("\nStopping port %d...\n", pt_id);
>  			fflush(stdout);
>  			stop_port(pt_id);
>  		}
> -		RTE_ETH_FOREACH_DEV(pt_id) {
> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
>  			printf("\nShutting down port %d...\n", pt_id);
>  			fflush(stdout);
>  			close_port(pt_id);
> --
> 2.25.1

Re: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure

[Ferruh Yigit]

On 3/3/2022 1:22 PM, Wu, WenxuanX wrote:

moved down, please don't top post

>> -----Original Message-----
>> From: Wu, WenxuanX <wenxuanx.wu@intel.com>
>> Sent: 2022年2月23日 19:33
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; dev@dpdk.org
>> Cc: Wu, WenxuanX <wenxuanx.wu@intel.com>; stable@dpdk.org
>> Subject: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
>>
>> From: wenxuan wu <wenxuanx.wu@intel.com>
>>
>> When testpmd start ed with 1 pf and 2 vfs, testpmd quited while vfs were
>> still alive would result in failure. Root cause is that pf had been released
>> already but vfs were still accessing by func rte_eth_dev_info_get, which
>> would result in heap-free-after-use error.
>>
>> By quitting our ports in reverse order to avoid this.And the order is
>> guaranteed that vf are created after pfs.
>>
>> Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
>> ---
>>   app/test-pmd/testpmd.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> e1da961311..698b6d8cc4 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -3384,12 +3384,12 @@ pmd_test_exit(void)  #endif
>>   	if (ports != NULL) {
>>   		no_link_check = 1;
>> -		RTE_ETH_FOREACH_DEV(pt_id) {
>> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
>>   			printf("\nStopping port %d...\n", pt_id);
>>   			fflush(stdout);
>>   			stop_port(pt_id);
>>   		}
>> -		RTE_ETH_FOREACH_DEV(pt_id) {
>> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
>>   			printf("\nShutting down port %d...\n", pt_id);
>>   			fflush(stdout);
>>   			close_port(pt_id);
>> --
>> 2.25.1
> 
> 
> I found this meaning in DPDK testplan.
> Note that currently hot-plugging of representor ports is not supported so all the required representors must be specified on the creation of the PF or the trusted VF.
> When testpmd is started with pf and vf representors, the order of representor is determined on creation. So it is guaranteed that ,pf is beneath the vf representors, we implemented in a reverse way is acceptable just at present,  depends on when the hot-plugging of representor is supported.
> 

The patch mentions from PF and VFs, and now you are referring
to port representor.
Is the problem related to VF or port representor.

For both, VF and port reporesentor should be closed before
PF, that part is OK. My comment is if reversing port id
traverse will fix the issue or do we need more complex
solution.
Like have APIs to get VF and representor ports from a given
port id, and free them first.

RE: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure

[Wu, WenxuanX]

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: 2022年3月5日 0:16
> To: Wu, WenxuanX <wenxuanx.wu@intel.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: Re: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
> 
> On 3/3/2022 1:22 PM, Wu, WenxuanX wrote:
> 
> moved down, please don't top post
> 
> >> -----Original Message-----
> >> From: Wu, WenxuanX <wenxuanx.wu@intel.com>
> >> Sent: 2022年2月23日 19:33
> >> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>; dev@dpdk.org
> >> Cc: Wu, WenxuanX <wenxuanx.wu@intel.com>; stable@dpdk.org
> >> Subject: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
> >>
> >> From: wenxuan wu <wenxuanx.wu@intel.com>
> >>
> >> When testpmd start ed with 1 pf and 2 vfs, testpmd quited while vfs
> >> were still alive would result in failure. Root cause is that pf had
> >> been released already but vfs were still accessing by func
> >> rte_eth_dev_info_get, which would result in heap-free-after-use error.
> >>
> >> By quitting our ports in reverse order to avoid this.And the order is
> >> guaranteed that vf are created after pfs.
> >>
> >> Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
> >> ---
> >>   app/test-pmd/testpmd.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >> e1da961311..698b6d8cc4 100644
> >> --- a/app/test-pmd/testpmd.c
> >> +++ b/app/test-pmd/testpmd.c
> >> @@ -3384,12 +3384,12 @@ pmd_test_exit(void)  #endif
> >>   	if (ports != NULL) {
> >>   		no_link_check = 1;
> >> -		RTE_ETH_FOREACH_DEV(pt_id) {
> >> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
> >>   			printf("\nStopping port %d...\n", pt_id);
> >>   			fflush(stdout);
> >>   			stop_port(pt_id);
> >>   		}
> >> -		RTE_ETH_FOREACH_DEV(pt_id) {
> >> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
> >>   			printf("\nShutting down port %d...\n", pt_id);
> >>   			fflush(stdout);
> >>   			close_port(pt_id);
> >> --
> >> 2.25.1
> >
> >
> > I found this meaning in DPDK testplan.
> > Note that currently hot-plugging of representor ports is not supported so all
> the required representors must be specified on the creation of the PF or the
> trusted VF.
> > When testpmd is started with pf and vf representors, the order of
> representor is determined on creation. So it is guaranteed that ,pf is beneath
> the vf representors, we implemented in a reverse way is acceptable just at
> present,  depends on when the hot-plugging of representor is supported.
> >
> 
> The patch mentions from PF and VFs, and now you are referring to port
> representor.
> Is the problem related to VF or port representor.
> 
> For both, VF and port reporesentor should be closed before PF, that part is
> OK. My comment is if reversing port id traverse will fix the issue or do we
> need more complex solution.
> Like have APIs to get VF and representor ports from a given port id, and free
> them first.
This patch did fix the issue ,and was verified.But it was rejected.

RE: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure

[Wu, WenxuanX]

> -----Original Message-----
> From: Wu, WenxuanX
> Sent: 2022年3月9日 11:07
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
> 
> 
> 
> > -----Original Message-----
> > From: Yigit, Ferruh <ferruh.yigit@intel.com>
> > Sent: 2022年3月5日 0:16
> > To: Wu, WenxuanX <wenxuanx.wu@intel.com>; Li, Xiaoyun
> > <xiaoyun.li@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: Re: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
> >
> > On 3/3/2022 1:22 PM, Wu, WenxuanX wrote:
> >
> > moved down, please don't top post
> >
> > >> -----Original Message-----
> > >> From: Wu, WenxuanX <wenxuanx.wu@intel.com>
> > >> Sent: 2022年2月23日 19:33
> > >> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> > >> <ferruh.yigit@intel.com>; dev@dpdk.org
> > >> Cc: Wu, WenxuanX <wenxuanx.wu@intel.com>; stable@dpdk.org
> > >> Subject: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
> > >>
> > >> From: wenxuan wu <wenxuanx.wu@intel.com>
> > >>
> > >> When testpmd start ed with 1 pf and 2 vfs, testpmd quited while vfs
> > >> were still alive would result in failure. Root cause is that pf had
> > >> been released already but vfs were still accessing by func
> > >> rte_eth_dev_info_get, which would result in heap-free-after-use error.
> > >>
> > >> By quitting our ports in reverse order to avoid this.And the order
> > >> is guaranteed that vf are created after pfs.
> > >>
> > >> Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
> > >> Cc: stable@dpdk.org
> > >>
> > >> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
> > >> ---
> > >>   app/test-pmd/testpmd.c | 4 ++--
> > >>   1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > >> e1da961311..698b6d8cc4 100644
> > >> --- a/app/test-pmd/testpmd.c
> > >> +++ b/app/test-pmd/testpmd.c
> > >> @@ -3384,12 +3384,12 @@ pmd_test_exit(void)  #endif
> > >>   	if (ports != NULL) {
> > >>   		no_link_check = 1;
> > >> -		RTE_ETH_FOREACH_DEV(pt_id) {
> > >> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
> > >>   			printf("\nStopping port %d...\n", pt_id);
> > >>   			fflush(stdout);
> > >>   			stop_port(pt_id);
> > >>   		}
> > >> -		RTE_ETH_FOREACH_DEV(pt_id) {
> > >> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
> > >>   			printf("\nShutting down port %d...\n", pt_id);
> > >>   			fflush(stdout);
> > >>   			close_port(pt_id);
> > >> --
> > >> 2.25.1
> > >
> > >
> > > I found this meaning in DPDK testplan.
> > > Note that currently hot-plugging of representor ports is not
> > > supported so all
> > the required representors must be specified on the creation of the PF
> > or the trusted VF.
> > > When testpmd is started with pf and vf representors, the order of
> > representor is determined on creation. So it is guaranteed that ,pf is
> > beneath the vf representors, we implemented in a reverse way is
> > acceptable just at present,  depends on when the hot-plugging of
> representor is supported.
> > >
> >
> > The patch mentions from PF and VFs, and now you are referring to port
> > representor.
> > Is the problem related to VF or port representor.
> >
> > For both, VF and port reporesentor should be closed before PF, that
> > part is OK. My comment is if reversing port id traverse will fix the
> > issue or do we need more complex solution.
> > Like have APIs to get VF and representor ports from a given port id,
> > and free them first.
The problem is that when I attempted to use a func like get_representor_info(pid,info); I didn't found this func implemented by driver ,
so I can not get the type of port(VF or PF ) directly by get_representor_info(pid,info),especially when the connected driver is i40e, 
representor_info_get occurred below.

./drivers/net/mlx5/mlx5.c:      .representor_info_get = mlx5_representor_info_get,
./drivers/net/mlx5/mlx5.c:      .representor_info_get = mlx5_representor_info_get,
./drivers/net/mlx5/mlx5.c:      .representor_info_get = mlx5_representor_info_get,
./drivers/net/mlx5/mlx5.h:int mlx5_representor_info_get(struct rte_eth_dev *dev,
./drivers/net/mlx5/mlx5_ethdev.c:mlx5_representor_info_get(struct rte_eth_dev *dev,
./drivers/net/sfc/sfc_ethdev.c:sfc_representor_info_get(struct rte_eth_dev *dev,
./drivers/net/sfc/sfc_ethdev.c: .representor_info_get           = sfc_representor_info_get,
./app/test-pmd/cmdline.c:       ret = rte_eth_representor_info_get(res->cmd_pid, NULL);
./app/test-pmd/cmdline.c:       ret = rte_eth_representor_info_get(res->cmd_pid, info);
./app/test-pmd/testpmd.c:       ret = rte_eth_representor_info_get(pi,&info);
./lib/ethdev/version.map:       rte_eth_representor_info_get;
./lib/ethdev/rte_ethdev.h:int rte_eth_representor_info_get(uint16_t port_id,
./lib/ethdev/ethdev_driver.h:typedef int (*eth_representor_info_get_t)(struct rte_eth_dev *dev,
./lib/ethdev/ethdev_driver.h:   eth_representor_info_get_t representor_info_get;
./lib/ethdev/ethdev_driver.h: * The mapping is retrieved from rte_eth_representor_info_get().
./lib/ethdev/rte_ethdev.c:      ret = rte_eth_representor_info_get(port_id, NULL);
./lib/ethdev/rte_ethdev.c:      ret = rte_eth_representor_info_get(port_id, info);
./lib/ethdev/rte_ethdev.c:rte_eth_representor_info_get(uint16_t port_id,
./lib/ethdev/rte_ethdev.c:      RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->representor_info_get, -ENOTSUP);
./lib/ethdev/rte_ethdev.c:      return eth_err(port_id, (*dev->dev_ops->representor_info_get)(dev, info));
We should focus on changing the logic of is_bonding_slave to avoid this bug ,right ? The procedure of port_close is like this :
FOR_EACH_DEV(pi):
	If Is_bonding_slave(pi)
		Continue;
	.... 
	Etc.
In is_bonding_slave(pi): 

This func get_dev_info(pid,dev_info) would be called to get dev.dev_flag &SLAVE ,this would 
be result in error ,when port sequence is like port 0  PF ,port 1 VF_REPR, port 2 VF_REPR, there is no 
obstacles in closing port 0 ,then pid iterate to port 1.

But port 1 is a VF_REPR which based on port 0 ,when in func is_bonding_slave(port 1), it would
 call get_dev_info(1,dev_info) ,error occurred. Because we use get_dev_info(especially when PF is released )
 not ports[1] which had been pre allocated . would result in this error.
 Two solutions :
                        1. Reverse order to close like I mentioned before (PF and VF_repr  order is determined at creation time with no representor hot_plug).
                        2.change func get_dev_info(pid,info) to  ports[pid)  to get dev_flag
Both can resolve this bug ,which one do you prefer?
             

RE: [PATCH 2/2] lib/ethdev: add reverse macro to quit testpmd

[Wu, WenxuanX]

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: 2022年2月19日 1:05
> To: Wu, WenxuanX <wenxuanx.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>; Singh, Aman Deep <aman.deep.singh@intel.com>;
> Zhang, Yuying <yuying.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 2/2] lib/ethdev: add reverse macro to quit testpmd
> 
> On 2/17/2022 10:14 AM, wenxuanx.wu@intel.com wrote:
> > From: wenxuan wu <wenxuanx.wu@intel.com>
> >
> > There is a heap-free-after-use bug when quit testpmd with pf and vfs,
> > stop and close ports in reverse order is a more reasonable approach.
> >
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
> > ---
> >   lib/ethdev/rte_ethdev.h | 14 +++++++++++++-
> 
> This patch should come before testpmd patch, because testpmd is using
> macro defined here.
> 
> >   1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
> > 2660e4f374..e080840b06 100644
> > --- a/lib/ethdev/rte_ethdev.h
> > +++ b/lib/ethdev/rte_ethdev.h
> > @@ -2185,9 +2185,13 @@ struct rte_eth_dev_owner {
> >    * @return
> >    *   Next valid port ID owned by owner_id, RTE_MAX_ETHPORTS if there is
> none.
> >    */
> > -uint64_t rte_eth_find_next_owned_by(uint16_t port_id,
> > +uint64_t
> > +rte_eth_find_next_owned_by(uint16_t port_id,
> >   		const uint64_t owner_id);
> >
> > +uint64_t
> > +rte_eth_find_prev_owned_by(uint16_t port_id, const uint64_t
> > +owner_id);
> > +
> 
> This function declared but not implemented, can remove above.


> 
> >   /**
> >    * Macro to iterate over all enabled ethdev ports owned by a specific
> owner.
> >    */
> > @@ -2212,6 +2216,14 @@ uint16_t rte_eth_find_next(uint16_t port_id);
> >   #define RTE_ETH_FOREACH_DEV(p) \
> >   	RTE_ETH_FOREACH_DEV_OWNED_BY(p, RTE_ETH_DEV_NO_OWNER)
> >
> > +/**
> > + * Macro to iterate over all enabled and ownerless ethdev ports in reverse
> order, for quit purpose.

Hi,
My next plan is like this:
Rough one:
---
For each dev:
	If( dev is a pf)
		Continue
	Close(dev)
For each dev :
	Close(dev)

Future version:
----
This will be a quick fix before 22.07,but if time is accepted,  I will try a more appropriate way ,like u mentioned . 
For each dev:
	If (dev has pf)://new api
		for each vf of this dev: //new api 
			close(vf)
	close(dev)
> 
> No need to add comment related to "quit purpose", macro can be used for
> different reasons.
> 
> > + */
> > +#define RTE_ETH_FOREACH_DEV_REVERSE(p) \
> > +	for (p = (rte_eth_dev_count_total() - 1 >= 0) ?
> > +(rte_eth_dev_count_total() - 1) : 0; \
> 
> Below check already relies on 'p' is unsigned, also
> 'rte_eth_dev_count_total()' returns unsigned, if so above check is
> unnecessary, it can just have:
> "p = rte_eth_dev_count_total() - 1"
> 
> > +	     p < rte_eth_dev_count_total(); \
> > +	     p--)
> > +
> >   /**
> >    * Iterates over ethdev ports of a specified device.
> >    *