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

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

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
  

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

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