Re: [dpdk-dev] [PATCH] app/testpmd: fix flow rules list after port stop

[Thomas Monjalon <thomas@monjalon.net>]

13/09/2020 14:12, Ori Kam:
> Hi Ferruh,
> Can we proceed with this patch?

Below, you said "first thing to do it update the rte_flow doc".
So I am expecting a patch on the doc to start this discussion.
This testpmd patch is on hold in my understanding.


> From: Ori Kam
> > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > On 8/20/2020 9:40 AM, Gregory Etelson wrote:
> > > > Hello,
> > > >
> > > > Is this patch scheduled for merge with dpdk.org ?
> > > > Please update me.
> > > >
> > > >> From: Gregory Etelson <getelson@mellanox.com>
> > > >>
> > > >> According to current RTE API, port flow rules must not be kept after port
> > > >> stop.
> > >
> > > Hi Gregory, Ori,
> > >
> > > Can you please point where this is documented?
> > >
> > From: rte_ethdev.h
> > "Please note that some configuration is not stored between calls to
> >  rte_eth_dev_stop()/rte_eth_dev_start(). The following configuration will
> >  be retained:
> > 
> >      - MTU
> >      - flow control settings
> >      - receive mode configuration (promiscuous mode, all-multicast mode,
> >        hardware checksum mode, RSS/VMDQ settings etc.)
> >      - VLAN filtering configuration
> >      - default MAC address
> >      - MAC addresses supplied to MAC address array
> >      - flow director filtering mode (but not filtering rules)
> >      - NIC queue statistics mappings"
> > 
> > From my understanding this means that flows should not be stored on device
> > stop.
> > 
> > 
> > > >>
> > > >> Testpmd did not flush port flow rules after `port stop' command was
> > called.
> > > >> As the result, after the port was restarted, it showed bogus flow rules.
> > >
> > > There are two issues,
> > >
> > > 1) According what I see in the rte_flow documentation, not sure if the "port
> > > stop" should clear the rules:
> > > "
> > > PMDs, not applications, are responsible for maintaining flow rules
> > > configuration
> > > when stopping and restarting a port or performing other actions which may
> > > affect
> > > them. They can only be destroyed explicitly by applications.
> > > "
> > >
> > Good catch I think this part should be removed, since it has many issues. The
> > application is the only
> > one that can be responsible for the rules.
> > 
> > Thinks about the following scenario: application configures 2 queues 0 and 1.
> > It insert flow with queue action 1.
> > It stops the port and remove queue 1. What should the PMD do?
> > What happens if he changed some thing else in configuration that make
> > the actions invalid?
> > 
> > For those reason (the description in rte_ethdev.h and the above issues with
> > keeping the rules)
> > we (Mellanox) modified our code to remove the flows in stop function from the
> > device.
> > This code was inserted to DPDK in 20.05 release.
> > One more reason is that saving the flows also waste a lot of memory
> > which is very costly to many applications.
> > 
> > 
> > > As I tested with i40e, it keeps the rules after stop/start, cc'ing @Jeff,
> > > @Beilei & @Qi if this is done intentionally.
> > >
> > >
> > > 2) From the perspective of the testers, users of the testpmd. If they are
> > > testing a complex set of filter rules, stopping and starting the port flushing
> > > all rules may be troublesome.
> > > Since there is explicit command to remove a rte_flow rule or to remove them
> > > all,
> > > user may prefer to call it when required to delete the rules, instead of this is
> > > done implicitly in port stop.
> > >
> > > Btw, this is based on PMD should handle the rules on stop/start, we need to
> > > agree on it first, but even that is not the case, we are in the application
> > > domain now and we can apply the rules back again in the 'start' if it serves
> > > better to the user.
> > >
> > First like I said above I think we should agree that it is the application
> > responsibility to manage the rules and not the PMD, and first thing to do it
> > update the rte_flow doc.
> > 
> > Second I agree that we should discuss if test-pmd should keep the rules and
> > reapply them,
> > but just like for the PMD the user may create invalid configuration, so re-
> > applying the rules
> > maybe incorrect.
> > Currently test-pmd is not build to support large number of rules, unless using a
> > script, and if the user uses a script
> > he can reuse this script.