Hi Eelco,

Thanks again replies Ilnie.

> -----Original Message-----
> From: Eelco Chaudron <echau...@redhat.com>
> Sent: Tuesday, May 24, 2022 3:15 PM
> To: Amber, Kumar <kumar.am...@intel.com>
> Cc: ovs-dev@openvswitch.org; Ferriter, Cian <cian.ferri...@intel.com>;
> i.maxim...@ovn.org; Stokes, Ian <ian.sto...@intel.com>; Van Haaren, Harry
> <harry.van.haa...@intel.com>
> Subject: Re: [PATCH v3] tests/mfex: Improve pcap script for mfex tests.
> 
> 
> 
> On 24 May 2022, at 7:40, Amber, Kumar wrote:
> 
> > Hi Eelco,
> >
> > Thanks again for reviews . Please find my replies inline.
> >
> > <Snip>
> >
> >>> +        tcp = TCP(dport=RandShort(), sport=RandShort(), flags='S',
> >>> +                  dataofs=random.randint(0, 15))
> >>
> >> Maybe a new line before the command, as it was before.
> >>
> >
> > Fixed in next version.
> >
> >>> +        # IPv4 packets with fuzzing
> >>> +        pkt.append(fuzz(eth / ipv4 / udp))
> >>> +        pkt.append(fuzz(eth / ipv4 / tcp))
> >>> +        pkt.append(fuzz(eth / vlan / ipv4 / udp))
> >>> +        pkt.append(fuzz(eth / vlan / ipv4 / tcp))
> >>> +
> >>> +        # IPv6 packets with fuzzing
> >>> +        pkt.append(fuzz(eth / ipv6 / udp))
> >>> +        pkt.append(fuzz(eth / ipv6 / tcp))
> >>> +        pkt.append(fuzz(eth / vlan / ipv6 / udp))
> >>> +        pkt.append(fuzz(eth / vlan / ipv6 / tcp))
> >>> +
> >>> +    else:
> >>> +        mac_addr = "52:54:00:FF:FF:%02x" % (random.randint(0, 255),)
> >>> +        src_port = random.randrange(600, 800)
> >>> +        dst_port = random.randrange(800, 1000)
> >>
> >> MAC and ports are still random, they should be fixed also for
> >> replication purposes.
> >>
> >
> > Have a fixed mac and l4 ports in next version.
> >
> >>> +        eth = Ether(src=mac_addr, dst=mac_addr)
> >>> +        vlan = Dot1Q(vlan=random.randrange(1, 20))
> >>
> >> Should be a fixed range, not random.
> >>
> >
> > Done something like that if this is what you expect:
> >     vlan = Dot1Q(vlan=(i % 10))
> 
> Yes, something that assures, each time you run the script you get the same
> pcap.
> 

Will send the V4.

> >>> +        udp = UDP(dport=src_port, sport=dst_port)
> >>> +        ipv4 = IP(src=RandIP()._fix(), dst=RandIP()._fix())
> >>> +        ipv6 = IPv6(src=RandIP6()._fix(), dst=RandIP6()._fix())
> >>
> >> Why use _fix()? We should use fixed IP ranges, _fix to me sound like
> >> we are using a private function?
> >> You should build the src/dst using the i variable.
> >
> > Same as vlan:
> >
> >         ipv4_addr = "192.168.150." + str((i % 255))
> >         ipv6_addr = "2001:0db8:85a3:0000:0000:8a2e:0370:" + str(i)
> 
> Yes, this looks good, with the note that this will limit the IPv4 addresses to
> 255 unique ones.
> 
> For IPv6 you also need to make sure you print hex with max 4 leading zero’s
> and do an (i % 0xffff).
>

Thanks, included in V4.
 
> >>
> >>> +        tcp = TCP(dport=src_port, sport=dst_port, flags='S')
> >>
> >> Maybe a new line before the command, as it was before.
> >>
> >>> +        # IPv4 packets
> >>> +        pkt.append(eth / ipv4 / udp)
> >>> +        pkt.append(eth / ipv4 / tcp)
> >>> +        pkt.append(eth / vlan / ipv4 / udp)
> >>> +        pkt.append(eth / vlan / ipv4 / tcp)
> >>> +
> >>> +        # IPv6 packets
> >>> +        pkt.append(eth / ipv6 / udp)
> >>> +        pkt.append(eth / ipv6 / tcp)
> >>> +        pkt.append(eth / vlan / ipv6 / udp)
> >>> +        pkt.append(eth / vlan / ipv6 / tcp)
> >>> +
> >>> +pktdump.write(pkt)
> >>> diff --git a/tests/pcap/mfex_test.pcap b/tests/pcap/mfex_test.pcap
> >>> deleted file mode 100644 index
> >>>
> >>
> 1aac67b8d643ecb016c758cba4cc32212a80f52a..000000000000000000000000
> >> 0000
> >>> 000000000000
> >>> GIT binary patch
> >>> literal 0
> >>> HcmV?d00001
> >>>
> >>> literal 416
> >>>
> >>
> zcmca|c+)~A1{MYw`2U}Qff2}Q<eHVR>K`M68ITRa|G@yFii5$Gfk6YL%z>@uY&
> >> }o|
> >>>
> >>
> z2s4N<1VH2&7y^V87$)XGOtD~MV$cFgfG~zBGGJ2#YtF$<F=a4i;9x8Q*<ZrSM6
> >> Ufz
> >>> xK>KST_NTIwYriok6N4Vm)gX-
> >> Q@<yO<!C`>c^{cp<7_5LgK^UuU{2>VS0RZ!RQ+EIW
> >>>
> >>> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index
> >>> 7d2715c4a..6b7d73a68 100644
> >>> --- a/tests/system-dpdk.at
> >>> +++ b/tests/system-dpdk.at
> >>> @@ -226,17 +226,19 @@ dnl
> >>> --------------------------------------------------------------------
> >>> --
> >>> ----
> >>>  dnl Add standard DPDK PHY port
> >>>  AT_SETUP([OVS-DPDK - MFEX Autovalidator])
> >>>  AT_KEYWORDS([dpdk])
> >>> -
> >>> +OVS_DPDK_PRE_CHECK()
> >>>  OVS_DPDK_START()
> >>> -
> >>> -dnl Add userspace bridge and attach it to OVS  AT_CHECK([ovs-vsctl
> >>> add-br br0 -- set bridge br0 datapath_type=netdev])
> >>> -AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dpdk
> >>> options:dpdk-
> >> devargs=net_pcap1,rx_pcap=$srcdir/pcap/mfex_test.pcap,inf
> >>> inite_rx=1], [], [stdout], [stderr]) -AT_CHECK([ovs-vsctl show], [],
> >>> [stdout])
> >>> -
> >>>  AT_SKIP_IF([! ovs-appctl dpif-netdev/miniflow-parser-get | sed 1,4d
> >>> | grep "True"], [], [dnl
> >>>  ])
> >>>
> >>> +AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], [])
> >>> +AT_CHECK([$PYTHON3 $srcdir/mfex_fuzzy.py $srcdir 2000], [],
> >>> +[stdout])
> >>> +
> >>> +dnl Add userspace bridge and attach it to OVS AT_CHECK([ovs-vsctl
> >>> +add-port br0 p1 -- set Interface p1 type=dpdk
> >>> +options:dpdk-devargs=net_pcap1,rx_pcap=$srcdir/pcap/fuzzy.pcap,infi
> >>> +ni te_rx=1], [], [stdout], [stderr]) AT_CHECK([ovs-vsctl show], [],
> >>> +[stdout])
> >>> +
> >>>  AT_CHECK([ovs-appctl dpif-netdev/dpif-impl-set dpif_avx512], [0],
> >>> [dnl  DPIF implementation set to dpif_avx512.
> >>>  ])
> >>> @@ -245,11 +247,15 @@ AT_CHECK([ovs-appctl
> >>> dpif-netdev/miniflow-parser-set autovalidator], [0], [dnl  Miniflow
> >>> extract
> >> implementation set to autovalidator.
> >>>  ])
> >>>
> >>> -OVS_WAIT_UNTIL([test `ovs-vsctl get interface p1 statistics | grep
> >>> -oP 'rx_packets=\s*\K\d+'` -ge 1000])
> >>> +OVS_WAIT_UNTIL([test `ovs-vsctl get interface p1 statistics | grep
> >>> +-oP 'rx_packets=\s*\K\d+'` -ge 16000])
> >>>
> >>>  dnl Clean up
> >>>  AT_CHECK([ovs-vsctl del-port br0 p1], [], [stdout], [stderr])
> >>> -OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
> >>> +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> >>> +\@upcall: datapath reached the dynamic limit of .* flows.@d
> >>> +\@received packet on unknown port .* on bridge bra while
> >>> +processing@d \@upcall_cb failure: ukey installation fails@d
> >>
> >> Still not convinced we need these additional messages. I remember
> >> there was an earlier discussion around these (I think with Ilya), can
> >> you point me to the conclusion on this?
> >>
> > The discussion was for the "MFEX-fuzzy " which experiences context
> > changes on CPU Mentioned below.
> >
> > Sure the discussion are here :
> > https://mail.openvswitch.org/pipermail/ovs-dev/2022-March/392374.html
> >
> > But I will remove these changes in fuzzy test for now as this patch
> > holds importance for us for testing IPv6 Patches and fuzzy topic can be
> delayed for now.
> 
> I’m not talking about the fuzzy topic, but about the error messages:
> 
> \@upcall: datapath reached the dynamic limit of .* flows.@d \@received
> packet on unknown port .* on bridge bra while processing@d \@upcall_cb
> failure: ukey installation fails@d
> 
> And the ones below:
> 
> \@received packet on unknown port .* on bridge br0 while processing@d
> \@upcall_cb failure: ukey installation fails@d \@Unreasonably long .* poll
> interval@d \@context switches: .* voluntary, .* involuntary@d
> \@faults: .* minor, .* major@d
> \@upcall_cb failure: ukey installation fails@d
> 
> Do we fully understand why they are generated? I can see some are due to
> limited CPU availability, and the number of entries:
> 
> \@upcall: datapath reached the dynamic limit of .* flows.@d \@context
> switches: .* voluntary, .* involuntary@d \@Unreasonably long .* poll
> interval@d
> \@faults: .* minor, .* major@d
> 
> But I do not understand the following (and I do not have a system to
> replicate this):
> 
> \@received packet on unknown port .* on bridge bra while processing@d
> \@upcall_cb failure: ukey installation fails@d
> 

Yes I guess I should have explained about each warning in a detailed commit 
message.
So to clarify all the warnings I will publish a separate patch and will put up 
explanation for
Each of them in the commit message.

Until then I will remove the following from this patch.

Thanks again
Amber
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to