On Tue, Nov 26, 2024 at 5:00 PM Eelco Chaudron <[email protected]> wrote:
>
>
>
> 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. 😊
Thank you for looking into this.
>
>
> 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>
>
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA
[email protected]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
