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

Wu, Jingjing jingjing.wu at intel.com
Sat Sep 23 04:26:37 CEST 2017



> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, September 21, 2017 6:46 AM
> To: Dai, Wei <wei.dai at intel.com>; Wu, Jingjing <jingjing.wu at intel.com>; Xing, Beilei
> <beilei.xing at intel.com>
> Cc: dev at dpdk.org; stable at 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


More information about the dev mailing list