net/i40e: fix mirror rule reset when port is stopped
PATCH 1/1 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( ).
--- drivers/net/i40e/i40e_ethdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/net/i40e/i40e_ethdev.c +1 -2
@@ -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 ;
Signature · 1 line
2.7.5
Quoted 37 lines
-----Original Message-----
From: Dai, Wei
Sent: Wednesday, September 6, 2017 10:52 PM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: [PATCH] net/i40e: fix mirror rule reset when port is stoppedWhen 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")Signed-off-by Wei Dai <[email protected]>--- drivers/net/i40e/i40e_ethdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/net/i40e/i40e_ethdev.c +1
-3@@ -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
Hi, Lijuan
How about your test result ?
Quoted 36 lines
-----Original Message-----
From: Dai, Wei
Sent: Wednesday, September 6, 2017 10:52 PM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: [PATCH] net/i40e: fix mirror rule reset when port is stoppedWhen 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")Signed-off-by Wei Dai <[email protected]>--- drivers/net/i40e/i40e_ethdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/net/i40e/i40e_ethdev.c +1
-3@@ -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
Tu, LijuanX A
Tested-byI have tested the patch ,it passed.
-----Original Message-----
From: Dai, Wei
Sent: Thursday, September 7, 2017 3:36 PM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>; Tu, LijuanX A <[email protected]>
Cc: [email protected]; [email protected]
Subject: RE: [PATCH] net/i40e: fix mirror rule reset when port is stopped
Hi, Lijuan
How about your test result ?
Quoted 8 lines
-----Original Message-----
From: Dai, Wei
Sent: Wednesday, September 6, 2017 10:52 PM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: [PATCH] net/i40e: fix mirror rule reset when port is stopped
Quoted 8 lines
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")Signed-off-by Wei Dai <[email protected]>
Quoted 20 lines
--- drivers/net/i40e/i40e_ethdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)(quoted diff) +0
-0
b/drivers/net/i40e/i40e_ethdev.cindex 5f26e24..93fb6cd 100644drivers/net/i40e/i40e_ethdev.c +1
-3@@ -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
Quoted 26 lines
-----Original Message-----
From: Dai, Wei
Sent: Wednesday, September 6, 2017 10:52 PM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: [PATCH] net/i40e: fix mirror rule reset when port is stoppedWhen 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")Signed-off-by Wei Dai <[email protected]>--- drivers/net/i40e/i40e_ethdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/net/i40e/i40e_ethdev.c +0
-0
drivers/net/i40e/i40e_ethdev.c i40e_dev_stop(struct rte_eth_dev *dev) · line 2100
Quoted 9 lines
drivers/net/i40e/i40e_ethdev.c +1
-2@@ -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
Quoted 46 lines
-----Original Message-----
From: Wu, Jingjing
Sent: Thursday, September 7, 2017 3:51 PM
To: Dai, Wei <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; [email protected]
Subject: RE: [PATCH] net/i40e: fix mirror rule reset when port is stoppedQuoted 25 lines
-----Original Message-----
From: Dai, Wei
Sent: Wednesday, September 6, 2017 10:52 PM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: [PATCH] net/i40e: fix mirror rule reset when port is stoppedWhen 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")Signed-off-by Wei Dai <[email protected]>--- drivers/net/i40e/i40e_ethdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)(quoted diff) +0
-0
b/drivers/net/i40e/i40e_ethdev.cindex 5f26e24..93fb6cd 100644drivers/net/i40e/i40e_ethdev.c i40e_dev_stop(struct rte_eth_dev *dev) · line 2100
Quoted 9 lines
drivers/net/i40e/i40e_ethdev.c +1
-2@@ -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
Quoted 8 lines
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 :-)
Quoted 4 lines
}Thanks
Jingjing
PATCH v2 1/1 net/i40e: fix mirror rule reset when port is stopped
In reply to Wei DaiWhen an i40e PF port is stopped, all mirror rules should be removed. All rule related software and hardware resources should also be removed.
--- drivers/net/i40e/i40e_ethdev.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
drivers/net/i40e/i40e_ethdev.c +15 -3
@@ -2094,10 +2094,22 @@ 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); +int ret; + ret =i40e_aq_del_mirror_rule (hw, pf->main_vsi->veb->seid, + p_mirror->rule_type, + p_mirror->entries, + p_mirror->num_entries, + p_mirror->id); +if (ret <0 ) { +PMD_DRV_LOG (ERR ,"failed to remove mirror rule: " +"status = %d, aq_err = %d." , ret, + hw->aq.asq_last_status); + }else { +TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules); +rte_free (p_mirror); + pf->nb_mirror_rule--; + } } - pf->nb_mirror_rule =0 ;if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */
Signature · 1 line
2.7.5
Quoted 37 lines
-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Wei Dai
Sent: Monday, September 11, 2017 10:12 AM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: [dpdk-dev] [PATCH v2] net/i40e: fix mirror rule reset when port is stoppedWhen an i40e PF port is stopped, all mirror rules should be removed. All rule related software and hardware resources should also be removed.
Fixes a4def5edf0fc ("i40e: enable port mirroring")Signed-off-by Wei Dai <[email protected]>Tested-by Lijuan Tu <[email protected]>--- drivers/net/i40e/i40e_ethdev.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)drivers/net/i40e/i40e_ethdev.c +5
-2@@ -2094,10 +2094,22 @@ 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); +int ret; + ret = i40e_aq_del_mirror_rule(hw, pf->main_vsi->veb->seid, + p_mirror->rule_type, + p_mirror->entries, + p_mirror->num_entries,
drivers/net/i40e/i40e_ethdev.c i40e_dev_stop(struct rte_eth_dev *dev) · line 2111
Quoted 10 lines
drivers/net/i40e/i40e_ethdev.c +10
-0+ p_mirror->id); +if (ret <0 ) { +PMD_DRV_LOG (ERR ,"failed to remove mirror rule: " +"status = %d, aq_err = %d." , ret, + hw->aq.asq_last_status); + }else { +TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules); +rte_free (p_mirror); + pf->nb_mirror_rule--; + }
little comment: else statement can be omitted here.
Quoted 7 lines
drivers/net/i40e/i40e_ethdev.c +0
-2} - pf->nb_mirror_rule =0 ;if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */ --2.7.5
Quoted 51 lines
-----Original Message-----
From: Xing, Beilei
Sent: Monday, September 11, 2017 10:46 AM
To: Dai, Wei <[email protected]>; Wu, Jingjing <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: RE: [dpdk-dev] [PATCH v2] net/i40e: fix mirror rule reset when port is stoppedQuoted 40 lines
-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Wei Dai
Sent: Monday, September 11, 2017 10:12 AM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: [dpdk-dev] [PATCH v2] net/i40e: fix mirror rule reset when port is stoppedWhen an i40e PF port is stopped, all mirror rules should be removed. All rule related software and hardware resources should also be removed.
Fixes a4def5edf0fc ("i40e: enable port mirroring")Signed-off-by Wei Dai <[email protected]>Tested-by Lijuan Tu <[email protected]>--- drivers/net/i40e/i40e_ethdev.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/net/i40e/i40e_ethdev.cindex 5f26e24..a278748 100644drivers/net/i40e/i40e_ethdev.c +9
-2@@ -2094,10 +2094,22 @@ 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); +int ret; + ret =i40e_aq_del_mirror_rule (hw, pf->main_vsi->veb->seid, + p_mirror->rule_type, + p_mirror->entries, + p_mirror->num_entries, + p_mirror->id); +if (ret <0 ) { + PMD_DRV_LOG(ERR, "failed to remove mirror rule: " + "status = %d, aq_err = %d.", ret,
Quoted 9 lines
drivers/net/i40e/i40e_ethdev.c i40e_dev_stop(struct rte_eth_dev *dev) · line 2111
Quoted 6 lines
drivers/net/i40e/i40e_ethdev.c +6
-0+ hw->aq.asq_last_status); + }else { +TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules); +rte_free (p_mirror); + pf->nb_mirror_rule--; + }little comment: else statement can be omitted here.
Another minor coding style comment:
Do not use braces (``{`` and ``}``) for control statements with zero or just a single statement, unless that statement is more than a single line in which case the braces are permitted.
Quoted 7 lines
Quoted 7 lines
drivers/net/i40e/i40e_ethdev.c +0
-2} - pf->nb_mirror_rule =0 ;if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */ --2.7.5
Quoted 38 lines
-----Original Message-----
From: Xing, Beilei
Sent: Monday, September 11, 2017 10:46 AM
To: Dai, Wei <[email protected]>; Wu, Jingjing <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: RE: [dpdk-dev] [PATCH v2] net/i40e: fix mirror rule reset when port is stoppedQuoted 27 lines
-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Wei Dai
Sent: Monday, September 11, 2017 10:12 AM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: [dpdk-dev] [PATCH v2] net/i40e: fix mirror rule reset when port is stoppedWhen an i40e PF port is stopped, all mirror rules should be removed. All rule related software and hardware resources should also be removed.
Fixes a4def5edf0fc ("i40e: enable port mirroring")Signed-off-by Wei Dai <[email protected]>Tested-by Lijuan Tu <[email protected]>--- drivers/net/i40e/i40e_ethdev.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/net/i40e/i40e_ethdev.cindex 5f26e24..a278748 100644drivers/net/i40e/i40e_ethdev.c +0
-0@@ -2094,10 +2094,22 @@ i40e_dev_stop(struct rte_eth_dev *dev)
Quoted 5 lines
drivers/net/i40e/i40e_ethdev.c i40e_dev_stop(struct rte_eth_dev *dev) · line 2097
Quoted 5 lines
drivers/net/i40e/i40e_ethdev.c +1
-2/* Remove all mirror rules */ while ((p_mirror =TAILQ_FIRST (&pf->mirror_list))) { -TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules); -rte_free (p_mirror); +int ret;
You can move the declaration of ret to the beginning of the func.
Quoted 8 lines
Quoted 8 lines
drivers/net/i40e/i40e_ethdev.c +8
-0+ ret =i40e_aq_del_mirror_rule (hw, pf->main_vsi->veb->seid, + p_mirror->rule_type, + p_mirror->entries, + p_mirror->num_entries, + p_mirror->id); +if (ret <0 ) { + PMD_DRV_LOG(ERR, "failed to remove mirror rule: " + "status = %d, aq_err = %d.", ret,
Quoted 8 lines
drivers/net/i40e/i40e_ethdev.c i40e_dev_stop(struct rte_eth_dev *dev) · line 2111
Quoted 6 lines
drivers/net/i40e/i40e_ethdev.c +6
-0+ hw->aq.asq_last_status); + }else { +TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules); +rte_free (p_mirror); + pf->nb_mirror_rule--; + }little comment: else statement can be omitted here.
Same comment, software entries need to be released.
Thanks
Jingjing
Many thanks to Jingjing and Beilei.
Sorry, this patch is a wrong v2 due to a building error. I have submitted another v2 .
Anyway, I will submit v3 patch per your feedback.
Quoted 76 lines
-----Original Message-----
From: Wu, Jingjing
Sent: Tuesday, September 19, 2017 10:21 AM
To: Xing, Beilei <[email protected]>; Dai, Wei <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: RE: [dpdk-dev] [PATCH v2] net/i40e: fix mirror rule reset when port is stoppedQuoted 20 lines
-----Original Message-----
From: Xing, Beilei
Sent: Monday, September 11, 2017 10:46 AM
To: Dai, Wei <[email protected]>; Wu, Jingjing <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: RE: [dpdk-dev] [PATCH v2] net/i40e: fix mirror rule reset when port is stoppedQuoted 11 lines
-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Wei Dai
Sent: Monday, September 11, 2017 10:12 AM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: [dpdk-dev] [PATCH v2] net/i40e: fix mirror rule reset when port is stoppedWhen an i40e PF port is stopped, all mirror rules should be removed. All rule related software and hardware resources should also be
removed.
Quoted 16 lines
Quoted 16 lines
Fixes a4def5edf0fc ("i40e: enable port mirroring")Signed-off-by Wei Dai <[email protected]>Tested-by Lijuan Tu <[email protected]>--- drivers/net/i40e/i40e_ethdev.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/net/i40e/i40e_ethdev.cindex 5f26e24..a278748 100644drivers/net/i40e/i40e_ethdev.c +0
-0@@ -2094,10 +2094,22 @@ i40e_dev_stop(struct rte_eth_dev *dev)Quoted 5 lines
drivers/net/i40e/i40e_ethdev.c i40e_dev_stop(struct rte_eth_dev *dev) · line 2097
Quoted 5 lines
drivers/net/i40e/i40e_ethdev.c +1
-2/* Remove all mirror rules */ while ((p_mirror =TAILQ_FIRST (&pf->mirror_list))) { -TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules); -rte_free (p_mirror); +int ret;You can move the declaration of ret to the beginning of the func.
Quoted 8 lines
Quoted 8 lines
drivers/net/i40e/i40e_ethdev.c +8
-0+ ret =i40e_aq_del_mirror_rule (hw, pf->main_vsi->veb->seid, + p_mirror->rule_type, + p_mirror->entries, + p_mirror->num_entries, + p_mirror->id); +if (ret <0 ) { + PMD_DRV_LOG(ERR, "failed to remove mirror rule: " + "status = %d, aq_err = %d.", ret,Quoted 8 lines
drivers/net/i40e/i40e_ethdev.c i40e_dev_stop(struct rte_eth_dev *dev) · line 2111
Quoted 6 lines
drivers/net/i40e/i40e_ethdev.c +6
-0+ hw->aq.asq_last_status); + }else { +TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules); +rte_free (p_mirror); + pf->nb_mirror_rule--; + }little comment: else statement can be omitted here.
Same comment, software entries need to be released.
Thanks
Jingjing
PATCH v2 1/1 net/i40e: fix mirror rule reset when port is stopped
In reply to Wei DaiWhen an i40e PF port is stopped, all mirror rules should be removed. All rule related software and hardware resources should also be removed.
--- drivers/net/i40e/i40e_ethdev.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
drivers/net/i40e/i40e_ethdev.c +22 -3
@@ -360,6 +360,12 @@ static int i40e_dev_sync_phy_type(struct i40e_hw *hw);static void i40e_configure_registers (struct i40e_hw *hw);static void i40e_hw_init (struct rte_eth_dev *dev);static int i40e_config_qinq (struct i40e_hw *hw,struct i40e_vsi *vsi); +static enum i40e_status_code i40e_aq_del_mirror_rule (struct i40e_hw *hw, +uint16_t seid, +uint16_t rule_type, +uint16_t *entries, +uint16_t count, +uint16_t rule_id);static int i40e_mirror_rule_set (struct rte_eth_dev *dev,struct rte_eth_mirror_conf *mirror_conf,uint8_t sw_id,uint8_t on);
@@ -2094,10 +2100,23 @@ 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); +int ret; + ret =i40e_aq_del_mirror_rule (hw, + pf->main_vsi->veb->seid, + p_mirror->rule_type, + p_mirror->entries, + p_mirror->num_entries, + p_mirror->id); +if (ret <0 ) { +PMD_DRV_LOG (ERR ,"failed to remove mirror rule: " +"status = %d, aq_err = %d." , ret, + hw->aq.asq_last_status); + }else { +TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules); +rte_free (p_mirror); + pf->nb_mirror_rule--; + } } - pf->nb_mirror_rule =0 ;if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */
Signature · 1 line
2.7.5
PATCH v3 1/1 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 software and hardware resources should also be removed.
--- drivers/net/i40e/i40e_ethdev.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
drivers/net/i40e/i40e_ethdev.c +22 -3
@@ -361,6 +361,12 @@ static int i40e_dev_sync_phy_type(struct i40e_hw *hw);static void i40e_configure_registers (struct i40e_hw *hw);static void i40e_hw_init (struct rte_eth_dev *dev);static int i40e_config_qinq (struct i40e_hw *hw,struct i40e_vsi *vsi); +static enum i40e_status_code i40e_aq_del_mirror_rule (struct i40e_hw *hw, +uint16_t seid, +uint16_t rule_type, +uint16_t *entries, +uint16_t count, +uint16_t rule_id);static int i40e_mirror_rule_set (struct rte_eth_dev *dev,struct rte_eth_mirror_conf *mirror_conf,uint8_t sw_id,uint8_t on);
@@ -2096,10 +2102,23 @@ 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); +int ret; + ret =i40e_aq_del_mirror_rule (hw, + pf->main_vsi->veb->seid, + p_mirror->rule_type, + p_mirror->entries, + p_mirror->num_entries, + p_mirror->id); +if (ret <0 ) { +PMD_DRV_LOG (ERR ,"failed to remove mirror rule: " +"status = %d, aq_err = %d." , ret, + hw->aq.asq_last_status); + }else { +TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules); +rte_free (p_mirror); + pf->nb_mirror_rule--; + } } - pf->nb_mirror_rule =0 ;if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */
Signature · 1 line
2.7.5
PATCH v4 1/1 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 software and hardware resources should also be removed.
--- drivers/net/i40e/i40e_ethdev.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
drivers/net/i40e/i40e_ethdev.c +22 -3
@@ -361,6 +361,12 @@ static int i40e_dev_sync_phy_type(struct i40e_hw *hw);static void i40e_configure_registers (struct i40e_hw *hw);static void i40e_hw_init (struct rte_eth_dev *dev);static int i40e_config_qinq (struct i40e_hw *hw,struct i40e_vsi *vsi); +static enum i40e_status_code i40e_aq_del_mirror_rule (struct i40e_hw *hw, +uint16_t seid, +uint16_t rule_type, +uint16_t *entries, +uint16_t count, +uint16_t rule_id);static int i40e_mirror_rule_set (struct rte_eth_dev *dev,struct rte_eth_mirror_conf *mirror_conf,uint8_t sw_id,uint8_t on);
@@ -2069,6 +2075,7 @@ i40e_dev_stop(struct rte_eth_dev *dev)struct rte_pci_device *pci_dev =RTE_ETH_DEV_TO_PCI (dev);struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;int i; +int ret;if (hw->adapter_stopped ==1 )return ;
@@ -2096,10 +2103,22 @@ 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); + ret =i40e_aq_del_mirror_rule (hw, + pf->main_vsi->veb->seid, + p_mirror->rule_type, + p_mirror->entries, + p_mirror->num_entries, + p_mirror->id); +if (ret <0 ) +PMD_DRV_LOG (ERR ,"failed to remove mirror rule: " +"status = %d, aq_err = %d." , ret, + hw->aq.asq_last_status); +else { +TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules); +rte_free (p_mirror); + pf->nb_mirror_rule--; + } } - pf->nb_mirror_rule =0 ;if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */
Signature · 1 line
2.7.5
PATCH v5 1/1 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 software and hardware resources should also be removed.
--- drivers/net/i40e/i40e_ethdev.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
drivers/net/i40e/i40e_ethdev.c +20 -1
@@ -361,6 +361,12 @@ static int i40e_dev_sync_phy_type(struct i40e_hw *hw);static void i40e_configure_registers (struct i40e_hw *hw);static void i40e_hw_init (struct rte_eth_dev *dev);static int i40e_config_qinq (struct i40e_hw *hw,struct i40e_vsi *vsi); +static enum i40e_status_code i40e_aq_del_mirror_rule (struct i40e_hw *hw, +uint16_t seid, +uint16_t rule_type, +uint16_t *entries, +uint16_t count, +uint16_t rule_id);static int i40e_mirror_rule_set (struct rte_eth_dev *dev,struct rte_eth_mirror_conf *mirror_conf,uint8_t sw_id,uint8_t on);
@@ -2069,6 +2075,7 @@ i40e_dev_stop(struct rte_eth_dev *dev)struct rte_pci_device *pci_dev =RTE_ETH_DEV_TO_PCI (dev);struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;int i; +int ret;if (hw->adapter_stopped ==1 )return ;
@@ -2096,10 +2103,22 @@ i40e_dev_stop(struct rte_eth_dev *dev)/* Remove all mirror rules */ while ((p_mirror =TAILQ_FIRST (&pf->mirror_list))) { + ret =i40e_aq_del_mirror_rule (hw, + pf->main_vsi->veb->seid, + p_mirror->rule_type, + p_mirror->entries, + p_mirror->num_entries, + p_mirror->id); +if (ret <0 ) +PMD_DRV_LOG (ERR ,"failed to remove mirror rule: " +"status = %d, aq_err = %d." , ret, + hw->aq.asq_last_status); + +/* remove mirror software resource any way */ TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules);rte_free (p_mirror); + pf->nb_mirror_rule--; } - pf->nb_mirror_rule =0 ;if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */
Signature · 1 line
2.7.5
Hi Wei,
Quoted 4 lines
When an i40e PF port is stopped, all mirror rules should be removed. All rule related software and hardware resources should also be removed.
Could you clarify why we have to remove all mirror rules when PF is stopped? As I remember mirror rule can direct to VF, which still can be running, no? Konstantin
Quoted 61 lines
Fixes a4def5edf0fc ("i40e: enable port mirroring")Signed-off-by Wei Dai <[email protected]>Tested-by Lijuan Tu <[email protected]>--- drivers/net/i40e/i40e_ethdev.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)drivers/net/i40e/i40e_ethdev.c +20
-2@@ -361,6 +361,12 @@ static int i40e_dev_sync_phy_type(struct i40e_hw *hw);static void i40e_configure_registers (struct i40e_hw *hw);static void i40e_hw_init (struct rte_eth_dev *dev);static int i40e_config_qinq (struct i40e_hw *hw,struct i40e_vsi *vsi); +static enum i40e_status_code i40e_aq_del_mirror_rule (struct i40e_hw *hw, +uint16_t seid, +uint16_t rule_type, +uint16_t *entries, +uint16_t count, +uint16_t rule_id);static int i40e_mirror_rule_set (struct rte_eth_dev *dev,struct rte_eth_mirror_conf *mirror_conf,uint8_t sw_id,uint8_t on);@@ -2069,6 +2075,7 @@ i40e_dev_stop(struct rte_eth_dev *dev)struct rte_pci_device *pci_dev =RTE_ETH_DEV_TO_PCI (dev);struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;int i; +int ret;if (hw->adapter_stopped ==1 )return ;@@ -2096,10 +2103,22 @@ i40e_dev_stop(struct rte_eth_dev *dev)/* Remove all mirror rules */ while ((p_mirror =TAILQ_FIRST (&pf->mirror_list))) { + ret =i40e_aq_del_mirror_rule (hw, + pf->main_vsi->veb->seid, + p_mirror->rule_type, + p_mirror->entries, + p_mirror->num_entries, + p_mirror->id); +if (ret <0 ) +PMD_DRV_LOG (ERR ,"failed to remove mirror rule: " +"status = %d, aq_err = %d." , ret, + hw->aq.asq_last_status); + +/* remove mirror software resource any way */ TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules);rte_free (p_mirror); + pf->nb_mirror_rule--; } - pf->nb_mirror_rule =0 ;if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */ --2.7.5
Hi, Konstantin
Without this patch, when a port is stopped, all mirror SW resource are cleared but HW settings are still there. And once the port is started again, creating mirror rule may fail due to remain HW settings.
There is a testpmd use case which can show why this patch is needed.
1. bind PF to igb_uio
#/root/dpdk-devbind.py -b igb_uio 0000:08:00.0
2. create 2 VFs
#echo 2 > /sys/bus/pci/devices/0000\:08\:00.0/max_vfs
3. launch testpmd with PF, and set port mirror configuration #./x86_64-native-linuxapp-gcc/app/testpmd -c 3 -n 4 -- -i Testpmd > set port 0 mirror-rule 0 pool-mirror-up 0x1 dst-pool 1 on Testpmd > quit
4. relaunch testpmd with PF, and set port mirror as the same configuration #./x86_64-native-linuxapp-gcc/app/testpmd -c 3 -n 4 -- -i Testpmd > set port 0 mirror-rule 0 pool-mirror-up 0x1 dst-pool 1 on I40e_mirror_rule_set( ): failed to add mirror rule: ret = -53, aq_err = 13. Mirror rule add error: (Function not implemented)
When testpmd is quitted, rte_eth_dev_stop( ) and then rte_eth_dev_close( ) are called. In these function, i40e_dev_stop( ) and i40e_dev_close( ) is called.
And i40e_dev_stop is also called by i40e_dev_close( ).
Without this patch, the error in second running of testpmd always happen. This patch can address this error.
Quoted 84 lines
-----Original Message-----
From: Ananyev, Konstantin
Sent: Wednesday, September 20, 2017 5:23 PM
To: Dai, Wei <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port is stoppedHi Wei,
Quoted 4 lines
When an i40e PF port is stopped, all mirror rules should be removed. All rule related software and hardware resources should also be removed.
Could you clarify why we have to remove all mirror rules when PF is stopped?
As I remember mirror rule can direct to VF, which still can be running, no? Konstantindrivers/net/i40e/i40e_ethdev.c static int i40e_dev_sync_phy_type(struct
Quoted 15 lines
Fixes a4def5edf0fc ("i40e: enable port mirroring")Signed-off-by Wei Dai <[email protected]>Tested-by Lijuan Tu <[email protected]>--- drivers/net/i40e/i40e_ethdev.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)(quoted diff) +0
-0
b/drivers/net/i40e/i40e_ethdev.cindex f12aefa..14cf6c0 100644drivers/net/i40e/i40e_ethdev.c +0
-0@@ -361,6 +361,12 @@ static int i40e_dev_sync_phy_type(struct
i40e_hwQuoted 4 lines
drivers/net/i40e/i40e_ethdev.c +0
-0*hw); static void i40e_configure_registers(struct i40e_hw *hw);static void
i40e_hw_init(struct rte_eth_dev *dev); static inti40e_config_qinq(struct i40e_hw *hw, struct i40e_vsi *vsi); +static enumi40e_status_codei40e_aq_del_mirror_rule(structi40e_hw*hw,
Quoted 42 lines
drivers/net/i40e/i40e_ethdev.c +19
-2+uint16_t seid, + uint16_t rule_type, + uint16_t *entries, + uint16_t count, + uint16_t rule_id);static int i40e_mirror_rule_set (struct rte_eth_dev *dev,struct rte_eth_mirror_conf *mirror_conf,uint8_t sw_id,uint8_t on);@@ -2069,6 +2075,7 @@ i40e_dev_stop(struct rte_eth_dev *dev)struct rte_pci_device *pci_dev =RTE_ETH_DEV_TO_PCI (dev);struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;int i; +int ret;if (hw->adapter_stopped ==1 )return ;@@ -2096,10 +2103,22 @@ i40e_dev_stop(struct rte_eth_dev *dev)/* Remove all mirror rules */ while ((p_mirror =TAILQ_FIRST (&pf->mirror_list))) { + ret =i40e_aq_del_mirror_rule (hw, + pf->main_vsi->veb->seid, + p_mirror->rule_type, + p_mirror->entries, + p_mirror->num_entries, + p_mirror->id); +if (ret <0 ) +PMD_DRV_LOG (ERR ,"failed to remove mirror rule: " +"status = %d, aq_err = %d." , ret, + hw->aq.asq_last_status); + +/* remove mirror software resource any way */ TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules);rte_free (p_mirror); + pf->nb_mirror_rule--; } - pf->nb_mirror_rule =0 ;if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */ --2.7.5
Hi Wei,
Quoted 21 lines
Hi, Konstantin
Without this patch, when a port is stopped, all mirror SW resource are cleared but HW settings are still there. And once the port is started again, creating mirror rule may fail due to remain HW settings.
There is a testpmd use case which can show why this patch is needed.
1. bind PF toigb_uio
#/root/dpdk-devbind.py -bigb_uio0000:08:00.0
2. create 2 VFs
#echo 2 > /sys/bus/pci/devices/0000\:08\:00.0/max_vfs
3. launch testpmd with PF, and set port mirror configuration #./x86_64-native-linuxapp-gcc/app/testpmd -c 3 -n 4 -- -i Testpmd > set port 0 mirror-rule 0 pool-mirror-up0x1dst-pool 1 on Testpmd > quit
4. relaunch testpmd with PF, and set port mirror as the same configuration #./x86_64-native-linuxapp-gcc/app/testpmd -c 3 -n 4 -- -i Testpmd > set port 0 mirror-rule 0 pool-mirror-up0x1dst-pool 1 onI40e_mirror_rule_set( ): failed to add mirror rule: ret = -53,aq_err= 13. Mirror rule add error: (Function not implemented)
Quoted 6 lines
When testpmd is quitted,
rte_eth_dev_stop( )and thenrte_eth_dev_close( )are called. In these function,i40e_dev_stop( )andi40e_dev_close( )is called.
Andi40e_dev_stopis also called byi40e_dev_close( ).Without this patch, the error in second running of testpmd always happen. This patch can address this error.
Thanks for explanation.
Though it seems that the patch while fixing the issue might introduce some inconsistences:
1. right now for i40e: dev_stop(port); dev_start(port) would keep existing HW mirror rule working. With your patch is not any more.
2. What about ixgbe? Do we also need to change its behavior?
As I can see right now ixgbe doesn't reset any mirror rules at dev_stop().
Would it be reset automatically, or do we need to update ixgbe PMD too?
About #1 - if we'll decide that this is a desired behavior that dev_stop() voids all mirror rules, then at least we probably need to update the documentation.
As alternative - at dev_stop() we can reset only actual HW rule, but keep SW configuration intact, and restore them at dev_start().
I personally think second option is a bit better - as it preserves existing behavior, and probably more convenient for the user.
About the patch itself, why just not:
while ((p_mirror = TAILQ_FIRST(&pf->mirror_list)))
i40e_mirror_rule_reset(dev, p_mirror->index);
?
Konstantin
Quoted 85 lines
Quoted 85 lines
-----Original Message-----
From: Ananyev, Konstantin
Sent: Wednesday, September 20, 2017 5:23 PM
To: Dai, Wei <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port is stoppedHi Wei,
Quoted 4 lines
When an i40e PF port is stopped, all mirror rules should be removed. All rule related software and hardware resources should also be removed.
Could you clarify why we have to remove all mirror rules when PF is stopped?
As I remember mirror rule can direct to VF, which still can be running, no? Konstantindrivers/net/i40e/i40e_ethdev.c static int i40e_dev_sync_phy_type(struct
Quoted 15 lines
Fixes a4def5edf0fc ("i40e: enable port mirroring")Signed-off-by Wei Dai <[email protected]>Tested-by Lijuan Tu <[email protected]>--- drivers/net/i40e/i40e_ethdev.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)(quoted diff) +0
-0
b/drivers/net/i40e/i40e_ethdev.cindex f12aefa..14cf6c0 100644drivers/net/i40e/i40e_ethdev.c +0
-0@@ -361,6 +361,12 @@ static int i40e_dev_sync_phy_type(struct
i40e_hwQuoted 4 lines
drivers/net/i40e/i40e_ethdev.c +0
-0*hw); static void i40e_configure_registers(struct i40e_hw *hw);static void
i40e_hw_init(struct rte_eth_dev *dev); static inti40e_config_qinq(struct i40e_hw *hw, struct i40e_vsi *vsi); +static enumi40e_status_codei40e_aq_del_mirror_rule(structi40e_hw*hw,
Quoted 42 lines
drivers/net/i40e/i40e_ethdev.c +19
-2+uint16_t seid, + uint16_t rule_type, + uint16_t *entries, + uint16_t count, + uint16_t rule_id);static int i40e_mirror_rule_set (struct rte_eth_dev *dev,struct rte_eth_mirror_conf *mirror_conf,uint8_t sw_id,uint8_t on);@@ -2069,6 +2075,7 @@ i40e_dev_stop(struct rte_eth_dev *dev)struct rte_pci_device *pci_dev =RTE_ETH_DEV_TO_PCI (dev);struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;int i; +int ret;if (hw->adapter_stopped ==1 )return ;@@ -2096,10 +2103,22 @@ i40e_dev_stop(struct rte_eth_dev *dev)/* Remove all mirror rules */ while ((p_mirror =TAILQ_FIRST (&pf->mirror_list))) { + ret =i40e_aq_del_mirror_rule (hw, + pf->main_vsi->veb->seid, + p_mirror->rule_type, + p_mirror->entries, + p_mirror->num_entries, + p_mirror->id); +if (ret <0 ) +PMD_DRV_LOG (ERR ,"failed to remove mirror rule: " +"status = %d, aq_err = %d." , ret, + hw->aq.asq_last_status); + +/* remove mirror software resource any way */ TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules);rte_free (p_mirror); + pf->nb_mirror_rule--; } - pf->nb_mirror_rule =0 ;if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */ --2.7.5
Quoted 52 lines
-----Original Message-----
From: Ananyev, Konstantin
Sent: Thursday, September 21, 2017 6:46 AM
To: Dai, Wei <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; [email protected]
Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port is stoppedHi Wei,
Quoted 4 lines
Hi, Konstantin
Without this patch, when a port is stopped, all mirror SW resource are cleared but HW
settings are still there.
Quoted 1 line
And once the port is started again, creating mirror rule may fail due to remain HW
settings.
Quoted 16 lines
There is a testpmd use case which can show why this patch is needed.
1. bind PF toigb_uio
#/root/dpdk-devbind.py -bigb_uio0000:08:00.0
2. create 2 VFs
#echo 2 > /sys/bus/pci/devices/0000\:08\:00.0/max_vfs
3. launch testpmd with PF, and set port mirror configuration #./x86_64-native-linuxapp-gcc/app/testpmd -c 3 -n 4 -- -i Testpmd > set port 0 mirror-rule 0 pool-mirror-up0x1dst-pool 1 on Testpmd > quit
4. relaunch testpmd with PF, and set port mirror as the same configuration #./x86_64-native-linuxapp-gcc/app/testpmd -c 3 -n 4 -- -i Testpmd > set port 0 mirror-rule 0 pool-mirror-up0x1dst-pool 1 onI40e_mirror_rule_set( ): failed to add mirror rule: ret = -53,aq_err= 13. Mirror rule add error: (Function not implemented)Quoted 6 lines
When testpmd is quitted,
rte_eth_dev_stop( )and thenrte_eth_dev_close( )are called. In these function,i40e_dev_stop( )andi40e_dev_close( )is called.
Andi40e_dev_stopis also called byi40e_dev_close( ).Without this patch, the error in second running of testpmd always happen. This patch can address this error.
Thanks for explanation.
Though it seems that the patch while fixing the issue might introduce some inconsistences:
1. right now for i40e:dev_stop(port);dev_start(port)would keep existing HW mirror rule working.
With your patch is not any more.
2. What about ixgbe? Do we also need to change its behavior?
As I can see right now ixgbe doesn't reset any mirror rules atdev_stop().
Would it be reset automatically, or do we need to update ixgbe PMD too?
Quoted 7 lines
About #1 - if we'll decide that this is a desired behavior that
dev_stop()voids all mirror rules, then at least we probably need to update the documentation. As alternative - atdev_stop()we can reset only actual HW rule, but keep SW configuration intact,
and restore them atdev_start().
I personally think second option is a bit better - as it preserves existing behavior, and probably more convenient for the user.
Yes, you reminded me the mirror is to VF. The mirror rule should be kept or at least be restored when device start again..
Because the dev_stop should only stop the pf interface, but not affect VF. It looks like we don't delete the VEB we dev_stop.
The reset behavior may need to be done in dev_close, but not in dev_stop.
Quoted 5 lines
About the patch itself, why just not:
while ((p_mirror=TAILQ_FIRST(&pf->mirror_list)))i40e_mirror_rule_reset(dev, p_mirror->index);
Wei's v1 patch is doing like that. But I commented it to change it. Because i40e_mirror_rule_reset is doing a search in the list which is not necessary.
Thanks
Jingjing
Quoted 69 lines
-----Original Message-----
From: Wu, Jingjing
Sent: Saturday, September 23, 2017 3:27 AM
To: Ananyev, Konstantin <[email protected]>; Dai, Wei <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; [email protected]
Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port is stoppedQuoted 50 lines
-----Original Message-----
From: Ananyev, Konstantin
Sent: Thursday, September 21, 2017 6:46 AM
To: Dai, Wei <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; [email protected]
Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port is stoppedHi Wei,
Quoted 4 lines
Hi, Konstantin
Without this patch, when a port is stopped, all mirror SW resource are cleared but HW
settings are still there.
Quoted 1 line
And once the port is started again, creating mirror rule may fail due to remain HW
settings.
Quoted 16 lines
There is a testpmd use case which can show why this patch is needed.
1. bind PF toigb_uio
#/root/dpdk-devbind.py -bigb_uio0000:08:00.0
2. create 2 VFs
#echo 2 > /sys/bus/pci/devices/0000\:08\:00.0/max_vfs
3. launch testpmd with PF, and set port mirror configuration #./x86_64-native-linuxapp-gcc/app/testpmd -c 3 -n 4 -- -i Testpmd > set port 0 mirror-rule 0 pool-mirror-up0x1dst-pool 1 on Testpmd > quit
4. relaunch testpmd with PF, and set port mirror as the same configuration #./x86_64-native-linuxapp-gcc/app/testpmd -c 3 -n 4 -- -i Testpmd > set port 0 mirror-rule 0 pool-mirror-up0x1dst-pool 1 onI40e_mirror_rule_set( ): failed to add mirror rule: ret = -53,aq_err= 13. Mirror rule add error: (Function not implemented)Quoted 6 lines
When testpmd is quitted,
rte_eth_dev_stop( )and thenrte_eth_dev_close( )are called. In these function,i40e_dev_stop( )andi40e_dev_close( )is called.
Andi40e_dev_stopis also called byi40e_dev_close( ).Without this patch, the error in second running of testpmd always happen. This patch can address this error.
Thanks for explanation.
Though it seems that the patch while fixing the issue might introduce some inconsistences:
1. right now for i40e:dev_stop(port);dev_start(port)would keep existing HW mirror rule working.
With your patch is not any more.
2. What about ixgbe? Do we also need to change its behavior?
As I can see right now ixgbe doesn't reset any mirror rules atdev_stop().
Would it be reset automatically, or do we need to update ixgbe PMD too?Quoted 7 lines
About #1 - if we'll decide that this is a desired behavior that
dev_stop()voids all mirror rules, then at least we probably need to update the documentation. As alternative - atdev_stop()we can reset only actual HW rule, but keep SW configuration intact,
and restore them atdev_start().
I personally think second option is a bit better - as it preserves existing behavior, and probably more convenient for the user.
Quoted 5 lines
Yes, you reminded me the mirror is to VF. The mirror rule should be kept or at least be restored when device start again..
Because thedev_stopshould only stop the pf interface, but not affect VF. It looks like we don't delete the VEB wedev_stop.
The reset behavior may need to be done indev_close, but not indev_stop.
if we can move that code into dev_close() , then indeed might be a better approach.
Quoted 8 lines
Quoted 6 lines
About the patch itself, why just not:
while ((p_mirror=TAILQ_FIRST(&pf->mirror_list)))i40e_mirror_rule_reset(dev, p_mirror->index);Wei's v1 patch is doing like that. But I commented it to change it. Because
i40e_mirror_rule_resetis doing a search in the list which is not necessary.
Yes, but it is a control path, so need to be extra quick here. As a plus - avoid code duplication and easier to control/maintain it. Konstantin
Quoted 103 lines
-----Original Message-----
From: Ananyev, Konstantin
Sent: Saturday, September 23, 2017 6:37 PM
To: Wu, Jingjing <[email protected]>; Dai, Wei <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; [email protected]
Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port is stoppedQuoted 47 lines
-----Original Message-----
From: Wu, Jingjing
Sent: Saturday, September 23, 2017 3:27 AM
To: Ananyev, Konstantin <[email protected]>; Dai, Wei <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; [email protected]
Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port is stoppedQuoted 36 lines
-----Original Message-----
From: Ananyev, Konstantin
Sent: Thursday, September 21, 2017 6:46 AM
To: Dai, Wei <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; [email protected]
Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port is stoppedHi Wei,
Quoted 5 lines
Hi, Konstantin
Without this patch, when a port is stopped, all mirror SW resource are cleared but HW
settings are still there.
Quoted 2 lines
And once the port is started again, creating mirror rule may fail due to remain HW
settings.
Quoted 16 lines
There is a testpmd use case which can show why this patch is needed.
1. bind PF toigb_uio
#/root/dpdk-devbind.py -bigb_uio0000:08:00.0 2. create 2 VFs #echo 2 > /sys/bus/pci/devices/0000\:08\:00.0/max_vfs
3. launch testpmd with PF, and set port mirror configuration #./x86_64-native-linuxapp-gcc/app/testpmd -c 3 -n 4 -- -i TestpmdQuoted 1 line
set port 0 mirror-rule 0 pool-mirror-up
0x1dst-pool 1 onTestpmd > quit 4. relaunch testpmd with PF, and set port mirror as the same configuration #./x86_64-native-linuxapp-gcc/app/testpmd -c 3 -n 4 -- -i Testpmd > set port 0 mirror-rule 0 pool-mirror-up
0x1dst-pool 1 onI40e_mirror_rule_set( ): failed to add mirror
rule: ret = -53,aq_err= 13.
Mirror rule add error: (Function not implemented)When testpmd is quitted,
rte_eth_dev_stop( )and then
rte_eth_dev_close( )are called.Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
In these function,
i40e_dev_stop( )andi40e_dev_close( )is called. Andi40e_dev_stopis also called byi40e_dev_close( ).Without this patch, the error in second running of testpmd always
happen.
Quoted 11 lines
Quoted 11 lines
Quoted 1 line
This patch can address this error.
Thanks for explanation.
Though it seems that the patch while fixing the issue might introduce some
inconsistences:
1. right now for i40e:dev_stop(port);dev_start(port)would keep existing HW mirror rule working.
With your patch is not any more.
2. What about ixgbe? Do we also need to change its behavior? As I can see right now ixgbe doesn't reset any mirror rules at
dev_stop().Quoted 1 line
Quoted 1 line
Would it be reset automatically, or do we need to update ixgbe PMD
too?
Quoted 3 lines
Quoted 3 lines
About #1 - if we'll decide that this is a desired behavior that
dev_stop()voids all mirror rules, then at least we probably need toupdate the documentation.
Quoted 10 lines
Quoted 4 lines
As alternative - at
dev_stop()we can reset only actual HW rule, but keep SW configuration intact, and restore them atdev_start(). I personally think second option is a bit better - as it preserves existing behavior, and probably more convenient for the user.Yes, you reminded me the mirror is to VF. The mirror rule should be kept or at least be restored when device start again.. Because the
dev_stopshould only stop the pf interface, but not affect VF. It looks like we don't delete the VEB wedev_stop.
The reset behavior may need to be done indev_close, but not indev_stop.if we can move that code into
dev_close(), then indeed might be a better approach.Quoted 6 lines
Quoted 6 lines
About the patch itself, why just not:
while ((p_mirror=TAILQ_FIRST(&pf->mirror_list)))i40e_mirror_rule_reset(dev, p_mirror->index);
Quoted 8 lines
Quoted 2 lines
Wei's v1 patch is doing like that. But I commented it to change it. Because
i40e_mirror_rule_resetis doing a search in the list which is notnecessary.
Yes, but it is a control path, so need to be extra quick here. As a plus - avoid code duplication and easier to control/maintain it. Konstantin
Thanks to Konstantin and Jingjing for all of your suggestion.
I will work out v6 patch to put all these mirror reset operations in dev_close( ). As to ixgbe, same scheme will be tried and tested.
PATCH v6 1/1 net/i40e: fix mirror rule reset when port is closed
In reply to Wei DaiWhen an i40e PF port is stopped, all mirror rules should be reserved. But when an i40e PF port is closed, all mirror rules should be removed. When a mirror rule is removed, its associated hardware and software resource should also be removed.
--- drivers/net/i40e/i40e_ethdev.c | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-)
drivers/net/i40e/i40e_ethdev.c +28 -8
@@ -361,6 +361,12 @@ static int i40e_dev_sync_phy_type(struct i40e_hw *hw);static void i40e_configure_registers (struct i40e_hw *hw);static void i40e_hw_init (struct rte_eth_dev *dev);static int i40e_config_qinq (struct i40e_hw *hw,struct i40e_vsi *vsi); +static enum i40e_status_code i40e_aq_del_mirror_rule (struct i40e_hw *hw, +uint16_t seid, +uint16_t rule_type, +uint16_t *entries, +uint16_t count, +uint16_t rule_id);static int i40e_mirror_rule_set (struct rte_eth_dev *dev,struct rte_eth_mirror_conf *mirror_conf,uint8_t sw_id,uint8_t on);
@@ -2065,7 +2071,6 @@ i40e_dev_stop(struct rte_eth_dev *dev)struct i40e_pf *pf =I40E_DEV_PRIVATE_TO_PF (dev->data->dev_private);struct i40e_hw *hw =I40E_DEV_PRIVATE_TO_HW (dev->data->dev_private);struct i40e_vsi *main_vsi = pf->main_vsi; -struct i40e_mirror_rule *p_mirror;struct rte_pci_device *pci_dev =RTE_ETH_DEV_TO_PCI (dev);struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;int i;
@@ -2094,13 +2099,6 @@ i40e_dev_stop(struct rte_eth_dev *dev)/* Set link down */ i40e_dev_set_link_down (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); - } - pf->nb_mirror_rule =0 ; -if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */ rte_intr_callback_register(intr_handle,
@@ -2127,12 +2125,34 @@ i40e_dev_close(struct rte_eth_dev *dev)struct i40e_hw *hw =I40E_DEV_PRIVATE_TO_HW (dev->data->dev_private);struct rte_pci_device *pci_dev =RTE_ETH_DEV_TO_PCI (dev);struct rte_intr_handle *intr_handle = &pci_dev->intr_handle; +struct i40e_mirror_rule *p_mirror;uint32_t reg;int i; +int ret;PMD_INIT_FUNC_TRACE ();i40e_dev_stop (dev); + +/* Remove all mirror rules */ +while ((p_mirror =TAILQ_FIRST (&pf->mirror_list))) { + ret =i40e_aq_del_mirror_rule (hw, + pf->main_vsi->veb->seid, + p_mirror->rule_type, + p_mirror->entries, + p_mirror->num_entries, + p_mirror->id); +if (ret <0 ) +PMD_DRV_LOG (ERR ,"failed to remove mirror rule: " +"status = %d, aq_err = %d." , ret, + hw->aq.asq_last_status); + +/* remove mirror software resource anyway */ +TAILQ_REMOVE (&pf->mirror_list, p_mirror, rules); +rte_free (p_mirror); + pf->nb_mirror_rule--; + } +i40e_dev_free_queues (dev);/* Disable interrupt */
Signature · 1 line
2.9.4
Re: [PATCH v6] net/i40e: fix mirror rule reset when port is closed
Quoted 15 lines
-----Original Message-----
From: Dai, Wei
Sent: Monday, September 25, 2017 2:37 PM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>; Ananyev, Konstantin <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: [PATCH v6] net/i40e: fix mirror rule reset when port is closedWhen an i40e PF port is stopped, all mirror rules should be reserved. But when an i40e PF port is closed, all mirror rules should be removed. When a mirror rule is removed, its associated hardware and software resource should also be removed.
Quoted 5 lines
Fixes a4def5edf0fc ("i40e: enable port mirroring")Signed-off-by Wei Dai <[email protected]>Tested-by Lijuan Tu <[email protected]>
Ferruh Yigit
AppliedRe: [dpdk-stable] [PATCH v6] net/i40e: fix mirror rule reset when port is closed
Quoted 15 lines · Jingjing
Quoted 15 lines
-----Original Message-----
From: Dai, Wei
Sent: Monday, September 25, 2017 2:37 PM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>; Ananyev, Konstantin <[email protected]>
Cc: [email protected]; Dai, Wei <[email protected]>; [email protected]
Subject: [PATCH v6] net/i40e: fix mirror rule reset when port is closedWhen an i40e PF port is stopped, all mirror rules should be reserved. But when an i40e PF port is closed, all mirror rules should be removed. When a mirror rule is removed, its associated hardware and software resource should also be removed.
Quoted 7 lines
Quoted 5 lines
Fixes a4def5edf0fc ("i40e: enable port mirroring")Signed-off-by Wei Dai <[email protected]>Tested-by Lijuan Tu <[email protected]>Acked-by Jingjing Wu <[email protected]>
Applied to dpdk-next-net/master, thanks.