diff mbox

[dpdk-dev] net/i40e: fix mirror rule reset when port is stopped

Message ID 1504709549-50804-1-git-send-email-wei.dai@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Wei Dai Sept. 6, 2017, 2:52 p.m. UTC 
  When an i40e PF port is stopped, all mirror rules should be removed.
All rule related SW and HW resources should also be removed. All of
them are should be removed by calling i40e_mirror_rule_reset( ).

Fixes: a4def5edf0fc ("i40e: enable port mirroring")
Cc: stable@dpdk.org

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Wei Dai Sept. 7, 2017, 7:34 a.m. UTC | #1 
> -----Original Message-----
> From: Dai, Wei
> Sent: Wednesday, September 6, 2017 10:52 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Dai, Wei <wei.dai@intel.com>; stable@dpdk.org
> Subject: [PATCH] net/i40e: fix mirror rule reset when port is stopped
> 
> When an i40e PF port is stopped, all mirror rules should be removed.
> All rule related SW and HW resources should also be removed. All of them
> are should be removed by calling i40e_mirror_rule_reset( ).
> 
> Fixes: a4def5edf0fc ("i40e: enable port mirroring")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 5f26e24..93fb6cd 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -2094,8 +2094,7 @@ i40e_dev_stop(struct rte_eth_dev *dev)
> 
>  	/* Remove all mirror rules */
>  	while ((p_mirror = TAILQ_FIRST(&pf->mirror_list))) {
> -		TAILQ_REMOVE(&pf->mirror_list, p_mirror, rules);
> -		rte_free(p_mirror);
> +		i40e_mirror_rule_reset(dev, p_mirror->index);
>  	}
>  	pf->nb_mirror_rule = 0;
> 
> --
> 2.7.5
Wei Dai Sept. 7, 2017, 7:35 a.m. UTC | #2 
Hi, Lijuan
How about your test result ?

> -----Original Message-----
> From: Dai, Wei
> Sent: Wednesday, September 6, 2017 10:52 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Dai, Wei <wei.dai@intel.com>; stable@dpdk.org
> Subject: [PATCH] net/i40e: fix mirror rule reset when port is stopped
> 
> When an i40e PF port is stopped, all mirror rules should be removed.
> All rule related SW and HW resources should also be removed. All of them
> are should be removed by calling i40e_mirror_rule_reset( ).
> 
> Fixes: a4def5edf0fc ("i40e: enable port mirroring")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 5f26e24..93fb6cd 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -2094,8 +2094,7 @@ i40e_dev_stop(struct rte_eth_dev *dev)
> 
>  	/* Remove all mirror rules */
>  	while ((p_mirror = TAILQ_FIRST(&pf->mirror_list))) {
> -		TAILQ_REMOVE(&pf->mirror_list, p_mirror, rules);
> -		rte_free(p_mirror);
> +		i40e_mirror_rule_reset(dev, p_mirror->index);
>  	}
>  	pf->nb_mirror_rule = 0;
> 
> --
> 2.7.5
Lijuan Tu Sept. 7, 2017, 7:37 a.m. UTC | #3 
I have tested the patch ,it passed.

-----Original Message-----
From: Dai, Wei 
Sent: Thursday, September 7, 2017 3:36 PM
To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Tu, LijuanX A <lijuanx.a.tu@intel.com>
Cc: dev@dpdk.org; stable@dpdk.org
Subject: RE: [PATCH] net/i40e: fix mirror rule reset when port is stopped

Hi, Lijuan
How about your test result ?

> -----Original Message-----
> From: Dai, Wei
> Sent: Wednesday, September 6, 2017 10:52 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei 
> <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Dai, Wei <wei.dai@intel.com>; stable@dpdk.org
> Subject: [PATCH] net/i40e: fix mirror rule reset when port is stopped
> 
> When an i40e PF port is stopped, all mirror rules should be removed.
> All rule related SW and HW resources should also be removed. All of 
> them are should be removed by calling i40e_mirror_rule_reset( ).
> 
> Fixes: a4def5edf0fc ("i40e: enable port mirroring")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>
Tested-by: Lijuan Tu<lijuanx.a.tu@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c 
> b/drivers/net/i40e/i40e_ethdev.c index 5f26e24..93fb6cd 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -2094,8 +2094,7 @@ i40e_dev_stop(struct rte_eth_dev *dev)
> 
>  	/* Remove all mirror rules */
>  	while ((p_mirror = TAILQ_FIRST(&pf->mirror_list))) {
> -		TAILQ_REMOVE(&pf->mirror_list, p_mirror, rules);
> -		rte_free(p_mirror);
> +		i40e_mirror_rule_reset(dev, p_mirror->index);
>  	}
>  	pf->nb_mirror_rule = 0;
> 
> --
> 2.7.5
Jingjing Wu Sept. 7, 2017, 7:50 a.m. UTC | #4 
> -----Original Message-----
> From: Dai, Wei
> Sent: Wednesday, September 6, 2017 10:52 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Dai, Wei <wei.dai@intel.com>; stable@dpdk.org
> Subject: [PATCH] net/i40e: fix mirror rule reset when port is stopped
> 
> When an i40e PF port is stopped, all mirror rules should be removed.
> All rule related SW and HW resources should also be removed. All of
> them are should be removed by calling i40e_mirror_rule_reset( ).
> 
> Fixes: a4def5edf0fc ("i40e: enable port mirroring")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 5f26e24..93fb6cd 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -2094,8 +2094,7 @@ i40e_dev_stop(struct rte_eth_dev *dev)
> 
>  	/* Remove all mirror rules */
>  	while ((p_mirror = TAILQ_FIRST(&pf->mirror_list))) {
> -		TAILQ_REMOVE(&pf->mirror_list, p_mirror, rules);
> -		rte_free(p_mirror);
> +		i40e_mirror_rule_reset(dev, p_mirror->index);
>  	}
>  	pf->nb_mirror_rule = 0;
> 
It is correct to remove mirror rule in HW. But looking into the function i40e_mirror_rul_reset, it's waste to call the function here.
It is much economic to do like
  	while ((p_mirror = TAILQ_FIRST(&pf->mirror_list))) {
		i40e_aq_del_mirror_rule(hw, seid,
				p_mirror->rule_type,
				p_mirror->entries,
				p_mirror->num_entries, p_mirror->id);
		TAILQ_REMOVE(&pf->mirror_list, p_mirror, rules);
		rte_free(p_mirror);
		i40e_mirror_rule_reset(dev, p_mirror->index);
  	}

Thanks
Jingjing
Wei Dai Sept. 7, 2017, 9:22 a.m. UTC | #5 
> -----Original Message-----
> From: Wu, Jingjing
> Sent: Thursday, September 7, 2017 3:51 PM
> To: Dai, Wei <wei.dai@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [PATCH] net/i40e: fix mirror rule reset when port is stopped
> 
> 
> 
> > -----Original Message-----
> > From: Dai, Wei
> > Sent: Wednesday, September 6, 2017 10:52 PM
> > To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>
> > Cc: dev@dpdk.org; Dai, Wei <wei.dai@intel.com>; stable@dpdk.org
> > Subject: [PATCH] net/i40e: fix mirror rule reset when port is stopped
> >
> > When an i40e PF port is stopped, all mirror rules should be removed.
> > All rule related SW and HW resources should also be removed. All of
> > them are should be removed by calling i40e_mirror_rule_reset( ).
> >
> > Fixes: a4def5edf0fc ("i40e: enable port mirroring")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Wei Dai <wei.dai@intel.com>
> > ---
> >  drivers/net/i40e/i40e_ethdev.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 5f26e24..93fb6cd 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -2094,8 +2094,7 @@ i40e_dev_stop(struct rte_eth_dev *dev)
> >
> >  	/* Remove all mirror rules */
> >  	while ((p_mirror = TAILQ_FIRST(&pf->mirror_list))) {
> > -		TAILQ_REMOVE(&pf->mirror_list, p_mirror, rules);
> > -		rte_free(p_mirror);
> > +		i40e_mirror_rule_reset(dev, p_mirror->index);
> >  	}
> >  	pf->nb_mirror_rule = 0;
> >
> It is correct to remove mirror rule in HW. But looking into the function
> i40e_mirror_rul_reset, it's waste to call the function here.
> It is much economic to do like
>   	while ((p_mirror = TAILQ_FIRST(&pf->mirror_list))) {
> 		i40e_aq_del_mirror_rule(hw, seid,
> 				p_mirror->rule_type,
> 				p_mirror->entries,
> 				p_mirror->num_entries, p_mirror->id);
> 		TAILQ_REMOVE(&pf->mirror_list, p_mirror, rules);
> 		rte_free(p_mirror);
> 		i40e_mirror_rule_reset(dev, p_mirror->index);

Indeed, the function i40e_mirror_rule_reset( ) includes above 3 code lines:
I40e_aq_del_mirror_rule( ),  TAILQ_REMOVE( ) and rte_free( ).
So did you suggest that use these 3 lines instead of calling i40e_mirror_rule_reset( ) ?
I mean i40e_mirror_rule_reset( ) is not necessary in your suggestion.

What's more, all these are in i40e_dev_stop( ), so it doesn't matter which method
to get same result :-)


>   	}
> 
> Thanks
> Jingjing
diff mbox

Patch

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 5f26e24..93fb6cd 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -2094,8 +2094,7 @@  i40e_dev_stop(struct rte_eth_dev *dev)
 
 	/* Remove all mirror rules */
 	while ((p_mirror = TAILQ_FIRST(&pf->mirror_list))) {
-		TAILQ_REMOVE(&pf->mirror_list, p_mirror, rules);
-		rte_free(p_mirror);
+		i40e_mirror_rule_reset(dev, p_mirror->index);
 	}
 	pf->nb_mirror_rule = 0;