[dpdk-dev,v5] net/i40e: fix mirror rule reset when port is stopped
Checks
Commit Message
When 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")
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 | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
Comments
Hi Wei,
>
> 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
>
> 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 | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index f12aefa..14cf6c0 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -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.
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Wednesday, September 20, 2017 5:23 PM
> To: Dai, Wei <wei.dai@intel.com>; 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: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port
> is stopped
>
> Hi Wei,
>
> >
> > 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
>
> >
> > 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 | 21 ++++++++++++++++++++-
> > 1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index f12aefa..14cf6c0 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -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 Wei,
>
> 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.
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
>
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Wednesday, September 20, 2017 5:23 PM
> > To: Dai, Wei <wei.dai@intel.com>; 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: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port
> > is stopped
> >
> > Hi Wei,
> >
> > >
> > > 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
> >
> > >
> > > 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 | 21 ++++++++++++++++++++-
> > > 1 file changed, 20 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > b/drivers/net/i40e/i40e_ethdev.c index f12aefa..14cf6c0 100644
> > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > @@ -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
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, September 21, 2017 6:46 AM
> To: Dai, Wei <wei.dai@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port is stopped
>
> Hi Wei,
>
> >
> > 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.
>
> 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.
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.
>
> 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
> -----Original Message-----
> From: Wu, Jingjing
> Sent: Saturday, September 23, 2017 3:27 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Dai, Wei <wei.dai@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port is stopped
>
>
>
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Thursday, September 21, 2017 6:46 AM
> > To: Dai, Wei <wei.dai@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>
> > Cc: dev@dpdk.org; stable@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port is stopped
> >
> > Hi Wei,
> >
> > >
> > > 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.
> >
> > 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.
>
> 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.
if we can move that code into dev_close() , then indeed might be a better approach.
>
> >
> > 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.
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
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Saturday, September 23, 2017 6:37 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Dai, Wei <wei.dai@intel.com>;
> Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset when port
> is stopped
>
>
>
> > -----Original Message-----
> > From: Wu, Jingjing
> > Sent: Saturday, September 23, 2017 3:27 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Dai, Wei
> > <wei.dai@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> > Cc: dev@dpdk.org; stable@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset
> > when port is stopped
> >
> >
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Thursday, September 21, 2017 6:46 AM
> > > To: Dai, Wei <wei.dai@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> > > Cc: dev@dpdk.org; stable@dpdk.org
> > > Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix mirror rule reset
> > > when port is stopped
> > >
> > > Hi Wei,
> > >
> > > >
> > > > 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.
> > >
> > > 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.
> >
> > 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.
>
> if we can move that code into dev_close() , then indeed might be a better
> approach.
>
> >
> > >
> > > 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.
>
> 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.
@@ -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 */