On 2 Dec 2024, at 12:34, Ilya Maximets wrote:
> On 11/29/24 15:45, 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. >> >> Hi Ales, >> >> 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 > > Hmm, but if the flow is in TC, why it is not dumped back and revalidated? > It shouldn't matter if it has any traffic or not. Not sure I understand your comment. It’s revalidated and removed in the end (as no traffic is hitting this rule). But if traffic comes again, we install it in TC again, so the same problem repeats. >> >> 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. ;) >> >> 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. >> >> >> 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"]) >> + >> >> >> >> >>> ./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
