On 11 Dec 2024, at 11:32, Eelco Chaudron wrote:
> On 5 Dec 2024, at 15:50, Ales Musil wrote:
>
>> The system and netdev datapath have different default pmd_id, which
>> resulted in empty output of ofproto/detrace with kernel datapath.
>> Also indicate that the UFID or cache wasn't available.
>>
>> 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 changes look good to me. One small comment from Ilya you missed, which we
> can apply on commit.
>
> Acked-by: Eelco Chaudron <[email protected]>
Thanks Ales,
With the nit fixed below applied to main.
//Eelco
>> ---
>> v5: Rebase on top of current main.
>> Adjust the empty cache message.
>> Adjust hte number of packets in the test.
>> Some other smaller test fixes.
>> v4: Add back the group test.
>> Use the OF update trick to force cache population.
>> v3: Rebase on top of latest main.
>> Make the output work with TC offload.
>> Add message for missing UFID or cache.
>> 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 | 15 +++++-
>> tests/ofproto-dpif.at | 56 ---------------------
>> tests/system-traffic.at | 92 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 105 insertions(+), 58 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index bb0c55a33..a5677389d 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -3339,8 +3339,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) {
>> @@ -3349,7 +3350,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. "
>> @@ -3362,8 +3363,15 @@ 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));
>
> cr/lf as requested by Ilya.
>
>> + pmd_id = !strcmp(type, "system") ? PMD_ID_NULL :
>> NON_PMD_CORE_ID;
>> + }
>> +
>> struct udpif_key *ukey = ukey_lookup(udpif, &ufid, pmd_id);
>> if (!ukey) {
>> + ds_put_format(&ds, "UFID was not found for %s\n",
>> + dpif_name(udpif->dpif));
>> continue;
>> }
>>
>> @@ -3373,6 +3381,9 @@ upcall_unixctl_ofproto_detrace(struct unixctl_conn
>> *conn, int argc,
>> if ((ukey->state == UKEY_VISIBLE || ukey->state == UKEY_OPERATIONAL)
>> && ukey->xcache) {
>> xlate_xcache_format(&ds, ukey->xcache);
>> + } else {
>> + ds_put_format(&ds, "Cache was not found for %s\n",
>> + dpif_name(udpif->dpif));
>> }
>> ovs_mutex_unlock(&ukey->mutex);
>> }
>> 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 a59994ba4..d681f2e87 100644
>> --- a/tests/system-traffic.at
>> +++ b/tests/system-traffic.at
>> @@ -2525,6 +2525,98 @@
>> 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])
>> +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 3 -i 0.1 -W 2 10.0.0.2 | FORMAT_PING],
>> [0], [dnl
>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
>> +])
>> +
>> +AT_CHECK([ovs-appctl revalidator/wait])
>> +# Add an unrelated OF change to populate the xcache during the next reval
>> run.
>> +AT_CHECK([ovs-ofctl add-flow br0 table=4,ip,action=drop])
>> +AT_CHECK([ovs-appctl revalidator/wait])
>> +
>> +AT_CHECK([ovs-appctl revalidator/pause])
>> +
>> +AT_CHECK([ovs-appctl dpctl/dump-flows], [0], [stdout])
>> +
>> +core=$(head -1 stdout | grep "flow-dump from pmd" | grep -o
>> "[[-]]\?[[0-9]]\+$")
>> +if test -z $core; then
>> + pmd_id=""
>> +else
>> + pmd_id="pmd=$core"
>> +fi
>> +
>> +AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep "ipv4" | strip_used |
>> strip_stats | \
>> + strip_duration | strip_dp_hash | strip_recirc | sed
>> 's/:never/:0.0s/' | \
>> + strip_ptype | strip_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 | \
>> + sed -E 's/n_bytes=2(66|80),/n_bytes=294,/'], [0], [dnl
>> +cookie=0x12345678, n_packets=3, n_bytes=294,
>> priority=100,ip,in_port=1,nw_dst=10.0.0.2,actions=resubmit(,1)
>> +table_id=1, n_packets=3, n_bytes=294, 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 | \
>> + sed -E 's/n_bytes=2(66|80),/n_bytes=294,/'], [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 | \
>> + sed -E 's/n_bytes=2(66|80),/n_bytes=294,/'], [0], [dnl
>> +table_id=2, n_packets=3, n_bytes=294, 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 | \
>> + sed -E 's/n_bytes=2(66|80),/n_bytes=294,/'], [0], [dnl
>> +cookie=0xabcedf, n_packets=3, n_bytes=294,
>> 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 | \
>> + sed -E 's/n_bytes=2(66|80),/n_bytes=294,/'], [0], [dnl
>> +table_id=3, n_packets=3, n_bytes=294, 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
