| Message ID | 20241105172338.457419-1-amusil@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ofproto: Fix default pmd_id for ofproto/trace. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Hi Ales, Thanks for the fix! The commit message should mention 'ofproto/detrace' instead of 'ofproto/trace'. On 11/5/24 18:23, 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 <amusil@redhat.com> > --- > ofproto/ofproto-dpif-upcall.c | 11 ++++- > tests/ofproto-dpif.at | 56 ----------------------- > tests/system-traffic.at | 86 +++++++++++++++++++++++++++++++++++ > 3 files changed, 95 insertions(+), 58 deletions(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index e7d4c2b2c..9d0f4590d 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,12 @@ upcall_unixctl_ofproto_detrace(struct unixctl_conn *conn, int argc, > struct udpif *udpif; > > LIST_FOR_EACH (udpif, list_node, &all_udpifs) { > + if (!pmd_str) { > + pmd_id = !strcmp(udpif->dpif->dpif_class->type, "system") Maybe it's cleaner to use dpif_normalize_type(dpif_type(udpif->dpif)) here? > + ? 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 a04d96110..afcc0d508 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() > +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" | rev | cut -d" " -f1) Can we simplify this a bit? It's also a bit fragile as $core will be wrong if the pmd ID is more than one digit long. > + 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 |\ Nit: missing space before \ > + 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]) Thanks, Dumitru
On Tue, Nov 12, 2024 at 9:42 AM Dumitru Ceara <dceara@redhat.com> wrote: > Hi Ales, > > Thanks for the fix! > > > Hi Dumitru, thank you for the review. > > The commit message should mention 'ofproto/detrace' instead of > 'ofproto/trace'. > Oops, yeah definitely! > On 11/5/24 18:23, 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 <amusil@redhat.com> > > --- > > ofproto/ofproto-dpif-upcall.c | 11 ++++- > > tests/ofproto-dpif.at | 56 ----------------------- > > tests/system-traffic.at | 86 +++++++++++++++++++++++++++++++++++ > > 3 files changed, 95 insertions(+), 58 deletions(-) > > > > diff --git a/ofproto/ofproto-dpif-upcall.c > b/ofproto/ofproto-dpif-upcall.c > > index e7d4c2b2c..9d0f4590d 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,12 @@ upcall_unixctl_ofproto_detrace(struct > unixctl_conn *conn, int argc, > > struct udpif *udpif; > > > > LIST_FOR_EACH (udpif, list_node, &all_udpifs) { > > + if (!pmd_str) { > > + pmd_id = !strcmp(udpif->dpif->dpif_class->type, "system") > > Maybe it's cleaner to use dpif_normalize_type(dpif_type(udpif->dpif)) here? > Yeah we should probably avoid direct comparison. > > + ? 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 a04d96110..afcc0d508 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() > > +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" | rev | cut -d" " -f1) > > Can we simplify this a bit? It's also a bit fragile as $core will be > wrong if the pmd ID is more than one digit long. > > You are right it would be reversed, I have replaced it with grep. > > > + 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 |\ > > Nit: missing space before \ > Fixed in v2. > > + 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]) > > Thanks, > Dumitru > > Thanks, Ales
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index e7d4c2b2c..9d0f4590d 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,12 @@ upcall_unixctl_ofproto_detrace(struct unixctl_conn *conn, int argc, struct udpif *udpif; LIST_FOR_EACH (udpif, list_node, &all_udpifs) { + if (!pmd_str) { + pmd_id = !strcmp(udpif->dpif->dpif_class->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 a04d96110..afcc0d508 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() +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" | rev | cut -d" " -f1) + 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])
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 <amusil@redhat.com> --- ofproto/ofproto-dpif-upcall.c | 11 ++++- tests/ofproto-dpif.at | 56 ----------------------- tests/system-traffic.at | 86 +++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 58 deletions(-)