On 8/25/23 08:12, Mike Pattrick wrote: > On Thu, Aug 24, 2023 at 6:01 PM Ilya Maximets <[email protected]> wrote: >> >> On 8/22/23 22:45, Mike Pattrick wrote: >>> When the a revalidator thread is updating statistics for an XC_LEARN >>> xcache entry in xlate_push_stats_entry it uses ofproto_flow_mod_learn. >>> The revalidator will update stats for rules even if they are in a >>> removed state or marked as invisible. However, ofproto_flow_mod_learn >>> will detect if a flow has been removed and re-add it in that case. This >>> can result in an old learn action replacing the new learn action that >>> had replaced it in the first place. >>> >>> This change adds a new stats_used parameter to ofproto_flow_mod_learn >>> allowing the caller to provide a timestamp that will be fed into the >>> learned rule's modified time. The provided timestamp should be the time >>> of the last packet activity. If stats_used is not set then the current >>> time is used, as is the current behaviour. >>> >>> This change also adds a check when replacing a learned rule to favour the >>> newest rule. >>> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2213892 >>> Signed-off-by: Mike Pattrick <[email protected]> >>> --- >>> v2: Added additional checks if rule is removed >>> v3: v2 patch was corrupted in transit >>> v4: Added check against dpif flow stats >>> v5: Fixed typos, updated commit message >>> Changed timestamps to use datapath timestamp more consistently >>> --- >> >> I'll quote my previous review comment: >> >> It should be possible to add a test for this issue using time >> warping. Please, add one. > > I tried to make a test with netdev-dummy/receive and time/warp, > however, it didn't reproduce the behaviour that's observed with a full > test. Do you have any suggestions?
It's hard to guess what went wrong with your test without looking at it. But the following script reproduces the behavior consistently for me in a sandbox: ovs-vsctl show ovs-vsctl add-br br0 ovs-vsctl add-port br0 p0 ovs-vsctl add-port br0 p1 ovs-vsctl add-port br0 p2 ovs-ofctl del-flows br0 ovs-ofctl add-flow br0 'in_port=p1,actions=learn(cookie=0xabc,in_port=p0,output:in_port),p0' ovs-ofctl add-flow br0 'in_port=p2,actions=learn(cookie=0xabc,in_port=p0,output:in_port),p0' ovs-ofctl dump-flows br0 packet='recirc_id(0),eth(src=f8:bc:12:44:34:b6,dst=ff:ff:ff:ff:ff:ff)' # Stop the time, so flows do not expire. ovs-appctl time/stop # Send 2 packets to create learning datapath flows. ovs-appctl netdev-dummy/receive p1 $packet ovs-appctl netdev-dummy/receive p2 $packet ovs-appctl revalidator/wait # Running twice in a loop to be sure we're not just lucky # with revalidation order. for i in 1 2; do # Pause revalidators, so they will revalidate both datapath flows # in the same revalidation cycle next time. ovs-appctl revalidator/pause ovs-appctl time/warp 10 # Send a packet on p1, then on p2 after 10 ms. ovs-appctl netdev-dummy/receive p1 $packet ovs-appctl time/warp 10 ovs-appctl netdev-dummy/receive p2 $packet ovs-appctl time/warp 10 # Un-pause and wait for revalidation. ovs-appctl revalidator/resume ovs-appctl revalidator/wait echo "Should be p2 ?" ovs-ofctl dump-flows br0 'cookie=0xabc/0xfff' # Do the same, but send packets in the opposite order. ovs-appctl revalidator/pause ovs-appctl time/warp 10 ovs-appctl netdev-dummy/receive p2 $packet ovs-appctl time/warp 10 ovs-appctl netdev-dummy/receive p1 $packet ovs-appctl time/warp 10 ovs-appctl revalidator/resume ovs-appctl revalidator/wait echo "Should be p1 ?" ovs-ofctl dump-flows br0 'cookie=0xabc/0xfff' done Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
