On 3/11/26 10:39 AM, Adrián Moreno wrote:
> On Fri, Mar 06, 2026 at 11:22:51AM +0100, Ilya Maximets wrote:
>> Current output of ofproto/detrace doesn't provide information on what
>> bridge the rules and groups belong to. This may be confusing in
>> setups with multiple bridges, especially when packets are resubmitted
>> multiple times through different patch ports.
>>
>> Translation cache does have the bridge information though as every
>> time we lookup the rule we add the XC_TABLE entry that has the bridge
>> information. We also know when the rule lookup was executed but
>> didn't result in a match. We can report that as well, so it's easier
>> to follow the resubmits.
>>
>> Note: On a miss, the packet hits the internal drop rule in table 254.
>> By reporting the fact that there was no match it's easier to
>> understand why that internal rule is getting hit.
>>
>> There is no extra overhead for providing the bridge information as the
>> data is already there, we just need to print it out. Retis, that is
>> collecting the detrace output, should also be able to take advantage
>> of this enhancement.
>
> Tested this on Retis and it "just works" :-)
>
>>
>> Signed-off-by: Ilya Maximets <[email protected]>
>> ---
>> ofproto/ofproto-dpif-xlate-cache.c | 15 +++++++++
>> tests/system-traffic.at | 51 ++++++++++++++++++++++++------
>> utilities/checkpatch_dict.txt | 1 +
>> 3 files changed, 57 insertions(+), 10 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-xlate-cache.c
>> b/ofproto/ofproto-dpif-xlate-cache.c
>> index c6d935cf0..a225a1b70 100644
>> --- a/ofproto/ofproto-dpif-xlate-cache.c
>> +++ b/ofproto/ofproto-dpif-xlate-cache.c
>> @@ -305,6 +305,7 @@ xlate_cache_steal_entries(struct xlate_cache *dst,
>> struct xlate_cache *src)
>> void
>> xlate_xcache_format(struct ds *s, const struct xlate_cache *xcache)
>> {
>> + struct ofproto_dpif *last_ofproto = NULL;
>> struct ofpbuf entries = xcache->entries;
>> struct xc_entry *entry;
>> struct ofgroup *ofg;
>> @@ -312,16 +313,30 @@ xlate_xcache_format(struct ds *s, const struct
>> xlate_cache *xcache)
>> XC_ENTRY_FOR_EACH (entry, &entries) {
>> switch (entry->type) {
>> case XC_RULE:
>> + ds_put_cstr(s, " ");
>> ofproto_rule_stats_ds(s, &entry->rule->up, true);
>> break;
>> +
>> case XC_GROUP:
>> + ds_put_cstr(s, " ");
>
> This results in a slightly weird output because groups with dp_hash
> will result in a recirculation with frozen_actions. In these cases,
> after thrawing there is no rule lookup and therefore, no XC_TABLE
> entry, but the action contains the OFP_GROUP which generates the
> XC_GROUP entry.
>
> The result is ofproto/detrace shows an indented" group entry with
> no "parent" bridge. The unit tests is a perfect example of this.
>
> Maybe something like this would help:
Makes sense. I can add something like this (or exactly this) in v2.
Thanks!
>
> --- i/ofproto/ofproto-dpif-xlate-cache.c
> +++ w/ofproto/ofproto-dpif-xlate-cache.c
> @@ -318,8 +318,14 @@ xlate_xcache_format(struct ds *s, const struct
> xlate_cache *xcache)
> break;
>
> case XC_GROUP:
> - ds_put_cstr(s, " ");
> ofg = &entry->group.group->up;
> + struct ofproto_dpif *group_ofproto =
> + CONTAINER_OF(ofg->ofproto, struct ofproto_dpif, up);
> + if (last_ofproto != group_ofproto) {
> + ds_put_format(s, "%s\n", group_ofproto->up.name);
> + last_ofproto = group_ofproto;
> + }
> + ds_put_cstr(s, " ");
> ofputil_group_format(s, ofg->group_id, ofg->type,
> entry->group.bucket, &ofg->buckets,
> &ofg->props, OFP15_VERSION,
>
>
>> ofg = &entry->group.group->up;
>> ofputil_group_format(s, ofg->group_id, ofg->type,
>> entry->group.bucket, &ofg->buckets,
>> &ofg->props, OFP15_VERSION,
>> false, NULL, NULL);
>> break;
>> +
>> case XC_TABLE:
>> + if (last_ofproto != entry->table.ofproto) {
>> + ds_put_format(s, "%s\n", entry->table.ofproto->up.name);
>> + last_ofproto = entry->table.ofproto;
>> + }
>> + if (!entry->table.match) {
>> + ds_put_format(s, " table_id=%"PRIu8", no match\n",
>> + entry->table.id);
>> + }
>> + break;
>> +
>> case XC_BOND:
>> case XC_NETDEV:
>> case XC_NETFLOW:
>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>> index 8f4fdf8b1..ea0dc5ec3 100644
>> --- a/tests/system-traffic.at
>> +++ b/tests/system-traffic.at
>> @@ -2733,7 +2733,12 @@ OVS_TRAFFIC_VSWITCHD_STOP
>> AT_CLEANUP
>>
>> AT_SETUP([datapath - Dump OF rules corresponding to UFID])
>> -OVS_TRAFFIC_VSWITCHD_START()
>> +OVS_TRAFFIC_VSWITCHD_START(
>> + [_ADD_BR([br1]) -- \
>> + add-port br0 patch1 -- set interface patch1 type=patch \
>> + options:peer=patch0 ofport_request=3 -- \
>> + add-port br1 patch0 -- set interface patch0 type=patch \
>> + options:peer=patch1 ofport_request=4 --])
>>
>> ADD_NAMESPACES(at_ns0, at_ns1)
>>
>> @@ -2749,14 +2754,24 @@
>> group_id=1,type=select,selection_method=dp_hash,bucket=bucket_id:0,weight:100,ac
>> ])
>> AT_DATA([flows.txt], [dnl
>> table=0,arp,actions=NORMAL
>> +table=0,priority=100,cookie=0x1234abcd,in_port=patch1,ip,nw_dst=10.0.0.2,actions=resubmit(,4)
>>
>> 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=2,ip,actions=ovs-p2,patch1
>> table=3,ip,actions=ovs-p1
>> +table=4,ip,actions=drop
>> +])
>> +AT_DATA([flows1.txt], [dnl
>> +table=0,arp,actions=drop
>> +table=0,actions=clone(resubmit(,1)),resubmit(,2)
>> +table=1,ip,actions=set_field:10.0.0.3->nw_dst,in_port
>> +table=2,ip,actions=in_port
>> ])
>> +
>> AT_CHECK([ovs-ofctl add-groups br0 groups.txt])
>> AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>> +AT_CHECK([ovs-ofctl add-flows br1 flows1.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
>> @@ -2782,8 +2797,8 @@ AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep
>> "ipv4" | strip_used | strip
>> 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(dst=10.0.0.2,proto=1,frag=no),
>> packets:0, bytes:0, used:0.0s, actions:ovs-p2
>> 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
>> ])
>> @@ -2791,32 +2806,48 @@
>> recirc_id(<recirc>),in_port(ovs-p2),eth_type(0x0800),ipv4(frag=no),
>> packets:0, b
>> 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
>> +br0
>> + 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)
>> +
>> group_id=1,type=select,selection_method=dp_hash,bucket=bucket_id:0,weight:100,actions=ct(commit,table=2)
>
> This is the weird output I was refering to in the above comment.
>
>> ])
>>
>> 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
>> + sed -E 's/n_bytes=2(66|80),/n_bytes=294,/' | \
>> + sed 's/table_id=254, n_packets=[[0-9]]*,
>> n_bytes=[[0-9]]*,/table_id=254,/'], [0], [dnl
>> +br0
>> + table_id=2, n_packets=3, n_bytes=294, ip,actions=output:2,output:3
>> +br1
>> + n_packets=3, n_bytes=294, ,actions=clone(resubmit(,1)),resubmit(,2)
>> + table_id=1, n_packets=3, n_bytes=294,
>> ip,actions=mod_nw_dst:10.0.0.3,IN_PORT
>> +br0
>> + table_id=0, no match
>> + table_id=254, priority=0,reg0=0x2,actions=drop
>> +br1
>> + table_id=2, n_packets=3, n_bytes=294, ip,actions=IN_PORT
>> +br0
>> + cookie=0x1234abcd, n_packets=3, n_bytes=294,
>> priority=100,ip,in_port=3,nw_dst=10.0.0.2,actions=resubmit(,4)
>> + table_id=4, ip,actions=drop
>> ])
>>
>> 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)
>> +br0
>> + 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
>> +br0
>> + table_id=3, n_packets=3, n_bytes=294, ip,actions=output:1
>> ])
>>
>> AT_CHECK([ovs-appctl revalidator/resume])
>> diff --git a/utilities/checkpatch_dict.txt b/utilities/checkpatch_dict.txt
>> index d40194a95..c1f43e5af 100644
>> --- a/utilities/checkpatch_dict.txt
>> +++ b/utilities/checkpatch_dict.txt
>> @@ -49,6 +49,7 @@ defragment
>> deref
>> dereference
>> dest
>> +detrace
>> dhcp
>> dhcpv4
>> dhcpv6
>> --
>> 2.53.0
>>
>
> Thanks.
> Adrián
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
