On 15 Nov 2024, at 11:48, Eelco Chaudron 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: >> >> ./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 > > I guess it would be good to figure out why. I’ll take another look after the > OVS conference. I tried to investigate why it was failing in this case. For me, it fails every 10–20 runs with this error, as the ukey is not (or is no longer) present in the system. I know that tc sometimes takes a bit longer to be revalidated, but that doesn’t seem to be the issue here, as adding a delay before the pause hasn’t resolved it. Interestingly, attempting to debug further using AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg ofproto_dpif_upcall:dbg]) causes the test to stop failing (at least not for 200+ runs 😉). I’ll see if I can add specific debug logs to determine why the ukey is being removed. I know we have a USDT script for this; let me give that a try tomorrow. 😊 Cheers, Eelco >>>> +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
