On Thu, Dec 5, 2024 at 11:17 AM Eelco Chaudron <echau...@redhat.com> wrote:
>
>
> On 2 Dec 2024, at 16:07, 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 <amu...@redhat.com>
> > ---
> > 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.
> > ---
>
> Thanks for adding the additional output! I have some small comments below,
> which I can apply during commit if you agree.
>
Hi Eelco,
thank you for the review.
>
> > 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..9466e4f65 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));
> > + 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 is not populated 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 15b3391f8..3167ae7ac 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -2534,6 +2534,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_DATA([flows2.txt], [dnl
> > +table=4,ip,action=drop
> > +])
> > +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])
> > +# Add unrelated OF change to populate cache
>
> Comments should end with a dot(.).
>
> > +AT_CHECK([ovs-ofctl add-flows br0 flows2.txt])
>
> No need to create a file, for just adding a single flow.
>
> Here is my diff, if you agree I can commit with this applied:
>
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -2548,9 +2548,6 @@ table=1,priority=200,ip,actions=group:1
> table=2,ip,actions=ovs-p2
> table=3,ip,actions=ovs-p1
> ])
> -AT_DATA([flows2.txt], [dnl
> -table=4,ip,action=drop
> -])
> AT_CHECK([ovs-ofctl add-groups br0 groups.txt])
> AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>
> @@ -2559,10 +2556,11 @@ NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -W 2
> 10.0.0.2 | FORMAT_PING], [0],
> ])
>
> AT_CHECK([ovs-appctl revalidator/wait])
> -# Add unrelated OF change to populate cache
> -AT_CHECK([ovs-ofctl add-flows br0 flows2.txt])
>
> +# 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])
>
The diff looks good to me.
Thanks,
Ales
>
> > +
> > +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/:never/:0.0s/' | \
> > + 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 | sed
> -E 's/n_bytes=8(54|68),/n_bytes=980,/'], [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 | sed
> -E 's/n_bytes=8(54|68),/n_bytes=980,/'], [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=8(54|68),/n_bytes=980,/'], [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 | 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 | 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
> > +AT_CLEANUP
> > +
> > AT_BANNER([MPLS])
> >
> > AT_SETUP([mpls - encap header dp-support])
> > --
> > 2.47.0
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
