[ovs-dev] [PATCH v2] Fix redundant datapath set ethernet action with NSH Decap

Martin Varghese martinvarghesenokia at gmail.com
Mon Jun 7 14:47:21 UTC 2021


On Wed, May 19, 2021 at 12:26:40PM +0200, Ilya Maximets wrote:
> On 5/19/21 5:26 AM, Martin Varghese wrote:
> > On Tue, May 18, 2021 at 10:03:39PM +0200, Ilya Maximets wrote:
> >> On 5/17/21 3:45 PM, Martin Varghese wrote:
> >>> From: Martin Varghese <martin.varghese at nokia.com>
> >>>
> >>> When a decap action is applied on NSH header encapsulatiing a
> >>> ethernet packet a redundant set mac address action is programmed
> >>> to the datapath.
> >>>
> >>> Fixes: f839892a206a ("OF support and translation of generic encap and decap")
> >>> Signed-off-by: Martin Varghese <martin.varghese at nokia.com>
> >>> Acked-by: Jan Scheurich <jan.scheurich at ericsson.com>
> >>> Acked-by: Eelco Chaudron <echaudro at redhat.com>
> >>> ---
> >>> Changes in v2:
> >>>   - Fixed code styling
> >>>   - Added Ack from jan.scheurich at ericsson.com
> >>>   - Added Ack from echaudro at redhat.com
> >>>
> >>
> >> Hi, Martin.
> >> For some reason this patch triggers frequent failures of the following
> >> unit test:
> >>   
> >> 2314. packet-type-aware.at:619: testing ptap - L3 over patch port
> >> ...

The test is failing as, during revalidation, NORMAL action is dropping packets.
With these changes, the mac address in flow structures get cleared with decap
action. Hence the NORMAL action drops the packet assuming a loop (SRC and DST mac address are zero). I assume NORMAL action handling in xlate_push_stats_entry is not adapted for l3 packet. The timing at which revalidator gets triggered explains the sporadicity of the issue. The issue is never seen as the MAC addresses in flow structure were not cleared with decap before.

So can we use NORMAL action with a L3 packet ?  Does OVS handle all the L3
use cases with Normal action ? If not, shouldn't we not use NORMAL action in this test case
 
Comments? 


> >> stdout:
> >> warped
> >> ./packet-type-aware.at:726:
> >>     ovs-appctl dpctl/dump-flows --names dummy at ovs-dummy | strip_used | grep -v ipv6 | sort
> >>
> >> --- -   2021-05-18 21:57:56.810513366 +0200
> >> +++ /home/i.maximets/work/git/ovs/tests/testsuite.dir/at-groups/2314/stdout     2021-05-18 21:57:56.806609814 +0200
> >> @@ -1,3 +1,3 @@
> >>  flow-dump from the main thread:
> >> -recirc_id(0),in_port(n0),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),eth_type(0x0800),ipv4(tos=0/0x3,frag=no), packets:1, bytes:98, used:0.0s, actions:pop_eth,clone(tnl_push(tnl_port(gre_sys),header(size=38,type=3,eth(dst=de:af:be:ef:ba:be,src=aa:55:00:00:00:02,dl_type=0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,proto=47,tos=0,ttl=64,frag=0x4000),gre((flags=0x0,proto=0x800))),out_port(br2)),n2)
> >> +recirc_id(0),in_port(n0),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),eth_type(0x0800),ipv4(tos=0/0x3,frag=no), packets:1, bytes:98, used:0.0s, actions:drop
> >>
> >>
> >> It fails very frequently in GitHub Actions, but it's harder to make it fail
> >> on my local machine.  Following change to the test allows to reproduce the
> >> failure almost always on my local machine:
> >>
> >> diff --git a/tests/packet-type-aware.at b/tests/packet-type-aware.at
> >> index 540cf98f3..01dbc8030 100644
> >> --- a/tests/packet-type-aware.at
> >> +++ b/tests/packet-type-aware.at
> >> @@ -721,7 +721,7 @@ AT_CHECK([
> >>      ovs-appctl netdev-dummy/receive n0 1e2ce92a669e3a6dd2099cab0800450000548a83400040011aadc0a80a0ac0a80a1e0800b7170a4d0002fd509a5800000000de1c020000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637
> >>  ], [0], [ignore])
> >>  
> >> -ovs-appctl time/warp 1000
> >> +ovs-appctl time/warp 1000 100
> >>  
> >>  AT_CHECK([
> >>      ovs-appctl dpctl/dump-flows --names dummy at ovs-dummy | strip_used | grep -v ipv6 | sort
> >> --
> >>
> >> Without your patch I can not make it fail locally even with above wrapping
> >> change applied.
> >>
> >> Could you, please, take a look?
> >>
> > 
> > Hi Ilya
> > 
> > Travis CI was good. i will rebase & try again
> > https://travis-ci.org/github/martinpattara/ovs/builds/770919003
> 
> Travis has only one job with tests enabled and it tests on arm.
> GitHub Actions (which is our main CI now) wasn't good:
>   https://github.com/martinpattara/ovs/runs/2567454405
> 
> Best regards, Ilya Maximets.


More information about the dev mailing list