On Fri, Nov 29, 2024 at 3:45 PM Eelco Chaudron <[email protected]> wrote:
> On 14 Nov 2024, at 17:47, Ales Musil wrote: > > > On Thu, Nov 14, 2024 at 3:58 PM Eelco Chaudron <[email protected]> > wrote: > > > >> > >> > >> On 12 Nov 2024, at 12:56, Ales Musil wrote: > >> > >>> The system and netdev datapath have different default pmd_id, which > >>> resulted in empty output of ofproto/detrace with kernel datapath. > >>> > >>> Make sure we use the correct default pmd_id when it's not specified > >>> as an argument. At the same time move slightly adjusted test into > >>> system tests so it is tested with both datapaths. > >>> > >>> Fixes: 600125b2c380 ("ofproto: Add ofproto/detrace command to map UFIDs > >> to OpenFlow.") > >>> Signed-off-by: Ales Musil <[email protected]> > >> > >> The change looks good only one comment. > >> > >> //Eelco > >> > >> > >>> --- > >>> v2: Rebase on top of latest main. > >>> Address comments from Dumitru: > >>> - Fix typo in the commit title. > >>> - Simplify the mathod for pmd_id. > >>> - Use proper function to get dpif type. > >>> --- > >>> ofproto/ofproto-dpif-upcall.c | 10 +++- > >>> tests/ofproto-dpif.at | 56 ----------------------- > >>> tests/system-traffic.at | 86 > +++++++++++++++++++++++++++++++++++ > >>> 3 files changed, 94 insertions(+), 58 deletions(-) > >>> > >>> diff --git a/ofproto/ofproto-dpif-upcall.c > >> b/ofproto/ofproto-dpif-upcall.c > >>> index e7d4c2b2c..1d8d514b1 100644 > >>> --- a/ofproto/ofproto-dpif-upcall.c > >>> +++ b/ofproto/ofproto-dpif-upcall.c > >>> @@ -3338,8 +3338,9 @@ static void > >>> upcall_unixctl_ofproto_detrace(struct unixctl_conn *conn, int argc, > >>> const char *argv[], void *aux > OVS_UNUSED) > >>> { > >>> - unsigned int pmd_id = NON_PMD_CORE_ID; > >>> const char *key_s = argv[1]; > >>> + const char *pmd_str = NULL; > >>> + unsigned int pmd_id; > >>> ovs_u128 ufid; > >>> > >>> if (odp_ufid_from_string(key_s, &ufid) <= 0) { > >>> @@ -3348,7 +3349,7 @@ upcall_unixctl_ofproto_detrace(struct > unixctl_conn > >> *conn, int argc, > >>> } > >>> > >>> if (argc == 3) { > >>> - const char *pmd_str = argv[2]; > >>> + pmd_str = argv[2]; > >>> if (!ovs_scan(pmd_str, "pmd=%d", &pmd_id)) { > >>> unixctl_command_reply_error(conn, > >>> "Invalid pmd argument format. > " > >>> @@ -3361,6 +3362,11 @@ upcall_unixctl_ofproto_detrace(struct > >> unixctl_conn *conn, int argc, > >>> struct udpif *udpif; > >>> > >>> LIST_FOR_EACH (udpif, list_node, &all_udpifs) { > >>> + if (!pmd_str) { > >>> + const char *type = > >> dpif_normalize_type(dpif_type(udpif->dpif)); > >>> + pmd_id = !strcmp(type, "system") ? PMD_ID_NULL : > >> NON_PMD_CORE_ID; > >>> + } > >>> + > >>> struct udpif_key *ukey = ukey_lookup(udpif, &ufid, pmd_id); > >>> if (!ukey) { > >>> continue; > >>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > >>> index 18bd359bf..8f9eab0d2 100644 > >>> --- a/tests/ofproto-dpif.at > >>> +++ b/tests/ofproto-dpif.at > >>> @@ -12797,62 +12797,6 @@ Datapath actions: > >> psample(group=42,cookie=0x64000000c8),drop > >>> OVS_VSWITCHD_STOP("/Enabling an unsupported feature is very > >> dangerous/d") > >>> AT_CLEANUP > >>> > >>> -AT_SETUP([ofproto-dpif - Dump OF rules corresponding to UFID]) > >>> -OVS_VSWITCHD_START > >>> - > >>> -add_of_ports br0 1 2 3 > >>> - > >>> -dnl Add some OpenFlow rules and groups. > >>> -AT_DATA([groups.txt], [dnl > >>> > >> > -group_id=1,type=select,selection_method=dp_hash,bucket=bucket_id:0,weight:100,actions=ct(commit,table=2,nat(dst=20.0.0.2)) > >>> -group_id=2,type=all,bucket=resubmit(,3),bucket=resubmit(,4) > >>> -]) > >>> -AT_DATA([flows.txt], [dnl > >>> > >> > -table=0,priority=100,cookie=0x12345678,in_port=p1,ip,nw_dst=10.0.0.2,actions=resubmit(,1) > >>> -table=1,priority=200,ip,actions=group:1 > >>> -table=2,ip,actions=group:2 > >>> -table=3,ip,actions=p2 > >>> -table=4,ip,actions=p3 > >>> -]) > >>> -AT_CHECK([ovs-ofctl add-groups br0 groups.txt]) > >>> -AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > >>> - > >>> -AT_CHECK([ovs-appctl netdev-dummy/receive p1 > >> 'ipv4(src=10.0.0.1,dst=10.0.0.2,proto=6),tcp(src=1,dst=2)']) > >>> -AT_CHECK([ovs-appctl netdev-dummy/receive p1 > >> 'ipv4(src=10.0.0.1,dst=10.0.0.2,proto=6),tcp(src=1,dst=2)']) > >>> -AT_CHECK([ovs-appctl revalidator/wait]) > >>> -AT_CHECK([ovs-appctl revalidator/pause]) > >>> - > >>> -AT_CHECK([ovs-appctl dpctl/dump-flows | strip_used | strip_stats | > >> strip_duration | strip_dp_hash | sort], [0], [dnl > >>> -flow-dump from the main thread: > >>> > >> > -recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=10.0.0.2,frag=no), > >> packets:0, bytes:0, used:0.0s, actions:hash(l4(0)),recirc(0x1) > >>> > >> > -recirc_id(0x1),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), > >> packets:0, bytes:0, used:0.0s, > >> actions:ct(commit,nat(dst=20.0.0.2)),recirc(0x2) > >>> > >> > -recirc_id(0x2),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), > >> packets:0, bytes:0, used:0.0s, actions:2,3 > >>> -]) > >>> - > >>> -ufid=$(ovs-appctl dpctl/dump-flows -m filter='recirc_id(0)' | > >> parse_ufid) > >>> -AT_CHECK([ovs-appctl ofproto/detrace $ufid | ofctl_strip], [0], [dnl > >>> -cookie=0x12345678, n_packets=2, n_bytes=236, > >> priority=100,ip,in_port=1,nw_dst=10.0.0.2,actions=resubmit(,1) > >>> -table_id=1, n_packets=2, n_bytes=236, priority=200,ip,actions=group:1 > >>> -]) > >>> - > >>> -ufid=$(ovs-appctl dpctl/dump-flows -m filter='recirc_id(0x1)' | > >> parse_ufid) > >>> -AT_CHECK([ovs-appctl ofproto/detrace $ufid | ofctl_strip], [0], [dnl > >>> > >> > -group_id=1,type=select,selection_method=dp_hash,bucket=bucket_id:0,weight:100,actions=ct(commit,table=2,nat(dst=20.0.0.2)) > >>> -]) > >>> - > >>> -ufid=$(ovs-appctl dpctl/dump-flows -m filter='recirc_id(0x2)' | > >> parse_ufid) > >>> -AT_CHECK([ovs-appctl ofproto/detrace $ufid | ofctl_strip], [0], [dnl > >>> -table_id=2, n_packets=2, n_bytes=236, ip,actions=group:2 > >>> -table_id=3, n_packets=2, n_bytes=236, ip,actions=output:2 > >>> -table_id=4, n_packets=2, n_bytes=236, ip,actions=output:3 > >>> > >> > -group_id=2,type=all,bucket=bucket_id:0,actions=resubmit(,3),bucket=bucket_id:1,actions=resubmit(,4) > >>> -]) > >>> - > >>> -AT_CHECK([ovs-appctl revalidator/resume]) > >>> - > >>> -OVS_VSWITCHD_STOP > >>> -AT_CLEANUP > >>> - > >>> AT_SETUP([ofproto-dpif - Cleanup missing datapath flows]) > >>> > >>> OVS_VSWITCHD_START > >>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at > >>> index 2b1686e99..010b044d5 100644 > >>> --- a/tests/system-traffic.at > >>> +++ b/tests/system-traffic.at > >>> @@ -2530,6 +2530,92 @@ > >> > recirc_id(<recirc>),in_port(2),eth_type(0x86dd),ipv6(proto=58,frag=no),icmpv6(ty > >>> OVS_TRAFFIC_VSWITCHD_STOP > >>> AT_CLEANUP > >>> > >>> +AT_SETUP([datapath - Dump OF rules corresponding to UFID]) > >>> +CHECK_NO_TC_OFFLOAD() > >> > >> Can we make this work for TC offload? > >> > >> There is an example DUMP_CLEAN_SORTED() which removed the “never” items > if > >> this is the only problem. > >> > > > > > > I have tried it once again with the "never" sorted out, but the test > still > > fails. It's missing OF in one of the detrace calls, I'm not sure why. > > Hi Ales, > Hi Eelco, thank you for the update. > It took me a bit longer to figure out what was going on, but I found the > issue. Your test case creates three DP flows. The first two flows uses the > dp_hash()/hash() match/action, which is not offloadable, while the third > flow is simple and offloadable. > > What happens next is that the first two flows are processed in the ovs > kernel module. However, as the third recirculated flow is applied in TC, it > still does not exist in the kernel DP. As a result, the packet is sent for > upcall handling. This process repeats for every packet as the flow keeps > being installed in TC. > > - [ovs] > recirc_id(0),in_port(2),eth(),eth_type(0x0800),ipv4(dst=10.0.0.2,frag=no), > packets:9, bytes:882, used:0.125s, actions:hash(l4(0)),recirc(0x3) > - [ovs] > recirc_id(0x3),dp_hash(0xa/0xf),in_port(2),eth(),eth_type(0x0800),ipv4(frag=no), > packets:9, bytes:882, used:0.125s, actions:ct(commit),recirc(0x4) > - [tc ] recirc_id(0x4),in_port(2),eth(),eth_type(0x0800),ipv4(frag=no), > packets:0, bytes:0, used:never, actions:3 > > I tried to find a quick and clean workaround, but OVS does not store > enough information to make this happen. I will continue working on this > next week. For this patch, my question is: do you need this complex rule to > run the detrace test? Maybe you could simplify it so that it works for all > datapaths. ;) > After offline discussion I've made changes to the flows so that they seem to work with all datapaths. > > In addition, it might be a good idea to enhance detrace to report whether > the UFID was not found or if it was found but the xcache was not present. > I have added some basic messages if the cache is not populated or the ufid_lookup wasn't successful. > > > Cheers, > > Eelco > > > FYI, here are the quick-and-dirty diffs I used to make it work with TC. > The byte count is off for TC, as it does not include the 12-byte Ethernet > header. I’m not sure why I got the (R)STP message; I just ignored them for > now. > > -- > > AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep "ipv4" | strip_used > | strip_stats | \ > - strip_duration | strip_dp_hash | strip_recirc | \ > + strip_duration | strip_dp_hash | strip_recirc | sed > 's/:never/:0.0s/' | \ > sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;' > | sort], [0], [dnl > > -- > > ufid=$(ovs-appctl dpctl/dump-flows -m > filter='recirc_id(0),in_port(ovs-p2),ipv4' | grep "ct," | parse_ufid) > -AT_CHECK([ovs-appctl ofproto/detrace $ufid $pmd_id | ofctl_strip], [0], > [dnl > +AT_CHECK([ovs-appctl ofproto/detrace $ufid $pmd_id | ofctl_strip | sed -E > 's/n_bytes=8(54|68),/n_bytes=980,/'], [0], [dnl > cookie=0xabcedf, n_packets=10, n_bytes=980, > priority=100,ip,in_port=2,nw_dst=10.0.0.1,actions=ct(table=3) > ]) > > ufid=$(ovs-appctl dpctl/dump-flows -m filter='in_port(ovs-p2),ipv4' | > grep "actions:ovs-p1" | parse_ufid) > -AT_CHECK([ovs-appctl ofproto/detrace $ufid $pmd_id | ofctl_strip], [0], > [dnl > +AT_CHECK([ovs-appctl ofproto/detrace $ufid $pmd_id | ofctl_strip | sed -E > 's/n_bytes=8(54|68),/n_bytes=980,/'], [0], [dnl > table_id=3, n_packets=10, n_bytes=980, ip,actions=output:1 > ]) > > AT_CHECK([ovs-appctl revalidator/resume]) > > -OVS_TRAFFIC_VSWITCHD_STOP > +OVS_TRAFFIC_VSWITCHD_STOP(["dnl > +/cannot get RSTP status on nonexistent port/d > +/cannot get STP status on nonexistent port/d"]) > + > > > I've also added the n_bytes workarounds and the STP one. Thanks, Ales > > > ./system-traffic.at:2599: ovs-appctl ofproto/detrace $ufid $pmd_id | > > ofctl_strip > > --- - 2024-11-14 16:41:55.302704236 +0000 > > +++ > > > /home/runner/work/ovs/ovs/tests/system-offloads-testsuite.dir/at-groups/69/stdout > > 2024-11-14 16:41:55.300807120 +0000 > > @@ -1,2 +1 @@ > > -table_id=2, n_packets=10, n_bytes=980, ip,actions=output:2 > > > > > > > >>> +OVS_TRAFFIC_VSWITCHD_START() > >>> + > >>> +ADD_NAMESPACES(at_ns0, at_ns1) > >>> + > >>> +ADD_VETH(p1, at_ns0, br0, "10.0.0.1/24") > >>> +ADD_VETH(p2, at_ns1, br0, "10.0.0.2/24") > >>> + > >>> +AT_CHECK([ovs-vsctl -- set interface ovs-p1 ofport_request=1 \ > >>> + -- set interface ovs-p2 ofport_request=2]) > >>> + > >>> +dnl Add some OpenFlow rules and groups. > >>> +AT_DATA([groups.txt], [dnl > >>> > >> > +group_id=1,type=select,selection_method=dp_hash,bucket=bucket_id:0,weight:100,actions=ct(commit,table=2) > >>> +]) > >>> +AT_DATA([flows.txt], [dnl > >>> +table=0,arp,actions=NORMAL > >>> > >> > +table=0,priority=100,cookie=0x12345678,in_port=ovs-p1,ip,nw_dst=10.0.0.2,actions=resubmit(,1) > >>> > >> > +table=0,priority=100,cookie=0xabcedf,in_port=ovs-p2,ip,nw_dst=10.0.0.1,actions=ct(table=3) > >>> +table=1,priority=200,ip,actions=group:1 > >>> +table=2,ip,actions=ovs-p2 > >>> +table=3,ip,actions=ovs-p1 > >>> +]) > >>> +AT_CHECK([ovs-ofctl add-groups br0 groups.txt]) > >>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > >>> + > >>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -W 2 10.0.0.2 | > >> FORMAT_PING], [0], [dnl > >>> +10 packets transmitted, 10 received, 0% packet loss, time 0ms > >>> +]) > >>> + > >>> +AT_CHECK([ovs-appctl revalidator/wait]) > >>> +AT_CHECK([ovs-appctl revalidator/pause]) > >>> + > >>> +get_pmd_arg() { > >>> + local core=$(ovs-appctl dpctl/dump-flows | head -n1 | grep > >> "flow-dump from pmd" | grep -o "[[-]]\?[[0-9]]\+$") > >>> + if test -z $core; then > >>> + echo "" > >>> + else > >>> + echo "pmd=$core" > >>> + fi > >>> +} > >>> + > >>> +pmd_id=$(get_pmd_arg) > >>> + > >>> +AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep "ipv4" | > >> strip_used | strip_stats | \ > >>> + strip_duration | strip_dp_hash | strip_recirc | \ > >>> + sed > >> 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;' | sort], > [0], > >> [dnl > >>> > >> > +recirc_id(<recirc>),in_port(ovs-p1),eth_type(0x0800),ipv4(dst=10.0.0.2,frag=no), > >> packets:0, bytes:0, used:0.0s, actions:hash(l4(0)),recirc(<recirc>) > >>> +recirc_id(<recirc>),in_port(ovs-p1),eth_type(0x0800),ipv4(frag=no), > >> packets:0, bytes:0, used:0.0s, actions:ct(commit),recirc(<recirc>) > >>> +recirc_id(<recirc>),in_port(ovs-p1),eth_type(0x0800),ipv4(frag=no), > >> packets:0, bytes:0, used:0.0s, actions:ovs-p2 > >>> > >> > +recirc_id(<recirc>),in_port(ovs-p2),eth_type(0x0800),ipv4(dst=10.0.0.1,frag=no), > >> packets:0, bytes:0, used:0.0s, actions:ct,recirc(<recirc>) > >>> +recirc_id(<recirc>),in_port(ovs-p2),eth_type(0x0800),ipv4(frag=no), > >> packets:0, bytes:0, used:0.0s, actions:ovs-p1 > >>> +]) > >>> + > >>> +ufid=$(ovs-appctl dpctl/dump-flows -m > >> filter='recirc_id(0),in_port(ovs-p1),ipv4' | parse_ufid) > >>> +AT_CHECK([ovs-appctl ofproto/detrace $ufid $pmd_id | ofctl_strip], > [0], > >> [dnl > >>> +cookie=0x12345678, n_packets=10, n_bytes=980, > >> priority=100,ip,in_port=1,nw_dst=10.0.0.2,actions=resubmit(,1) > >>> +table_id=1, n_packets=10, n_bytes=980, priority=200,ip,actions=group:1 > >>> +]) > >>> + > >>> +ufid=$(ovs-appctl dpctl/dump-flows -m filter='in_port(ovs-p1),ipv4' | > >> grep "ct(commit)" | parse_ufid) > >>> +AT_CHECK([ovs-appctl ofproto/detrace $ufid $pmd_id | ofctl_strip], > [0], > >> [dnl > >>> > >> > +group_id=1,type=select,selection_method=dp_hash,bucket=bucket_id:0,weight:100,actions=ct(commit,table=2) > >>> +]) > >>> + > >>> +ufid=$(ovs-appctl dpctl/dump-flows -m filter='in_port(ovs-p1),ipv4' | > >> grep "actions:ovs-p2" | parse_ufid) > >>> +AT_CHECK([ovs-appctl ofproto/detrace $ufid $pmd_id | ofctl_strip], > [0], > >> [dnl > >>> +table_id=2, n_packets=10, n_bytes=980, ip,actions=output:2 > >>> +]) > >>> + > >>> +ufid=$(ovs-appctl dpctl/dump-flows -m > >> filter='recirc_id(0),in_port(ovs-p2),ipv4' | grep "ct," | parse_ufid) > >>> +AT_CHECK([ovs-appctl ofproto/detrace $ufid $pmd_id | ofctl_strip], > [0], > >> [dnl > >>> +cookie=0xabcedf, n_packets=10, n_bytes=980, > >> priority=100,ip,in_port=2,nw_dst=10.0.0.1,actions=ct(table=3) > >>> +]) > >>> + > >>> +ufid=$(ovs-appctl dpctl/dump-flows -m filter='in_port(ovs-p2),ipv4' | > >> grep "actions:ovs-p1" | parse_ufid) > >>> +AT_CHECK([ovs-appctl ofproto/detrace $ufid $pmd_id | ofctl_strip], > [0], > >> [dnl > >>> +table_id=3, n_packets=10, n_bytes=980, ip,actions=output:1 > >>> +]) > >>> + > >>> +AT_CHECK([ovs-appctl revalidator/resume]) > >>> + > >>> +OVS_TRAFFIC_VSWITCHD_STOP > >>> +AT_CLEANUP > >>> + > >>> AT_BANNER([MPLS]) > >>> > >>> AT_SETUP([mpls - encap header dp-support]) > >>> -- > >>> 2.47.0 > >>> > >>> _______________________________________________ > >>> dev mailing list > >>> [email protected] > >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > >> > > Thanks, > > Ales > > > > -- > > > > Ales Musil > > > > Senior Software Engineer - OVN Core > > > > Red Hat EMEA <https://www.redhat.com> > > > > [email protected] > > <https://red.ht/sig> > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
