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.
>
> 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
