| Message ID | 20240712115410.978475-1-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v5] ofproto: Add ofproto/detrace command to map UFIDs to OpenFlow. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 7/12/24 13:54, Dumitru Ceara wrote: > It improves the debugging experience if we can easily get a list of > OpenFlow rules and groups that contribute to the creation of a datapath > flow. > > The suggested workflow is: > a. dump datapath flows (along with UUIDs), this also prints the core IDs > (PMD IDs) when applicable. > > $ ovs-appctl dpctl/dump-flows -m > flow-dump from pmd on cpu core: 7 > ufid:7460db8f..., recirc_id(0), .... > > b. dump related OpenFlow rules and groups: > $ ovs-appctl ofproto/detrace ufid:7460db8f... pmd=7 > cookie=0x12345678, table=0 priority=100,ip,in_port=2,nw_dst=10.0.0.2,actions=resubmit(,1) > cookie=0x0, table=1 priority=200,actions=group:1 > group_id=1,bucket=bucket_id:0,actions=ct(commit,table=2,nat(dst=20.0.0.2)) > cookie=0x0, table=2 actions=output:1 > > The new command only shows rules and groups attached to ukeys that are > in states UKEY_VISIBLE or UKEY_OPERATIONAL. That should be fine as all > other ukeys should not be relevant for the use case presented above. > > This commit tries to mimic the output format of the ovs-ofctl > dump-flows/dump-groups commands. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- A very quick review. Obviously, didn't dive deep, but see a couple comments below. > V5: > - Addressed Ilya's comments: > - I addressed all from here > https://mail.openvswitch.org/pipermail/ovs-dev/2024-June/415144.html > except the one about adding -m/-mm. It felt OK to always dump > statistics. > V4: > - Addressed Mike's comment: > - use predictable port IDs. > V3: > - Addressed Mike's comments: > - use ds_put_char() > - make the test less flaky > V2: > - Addressed Adrian's comments: > - check return value of populate_xcache() > - use flow_stats_ds() (renamed to ofproto_rule_stats_ds()) instead of > custom printing > - move ukey->state check to caller > - handle case when group bucket is not available > - update test case to cover the point above > --- > NEWS | 2 + > include/openvswitch/ofp-group.h | 14 +++ > lib/ofp-group.c | 131 +++++++++++++++++------------ > ofproto/ofproto-dpif-upcall.c | 51 +++++++++++ > ofproto/ofproto-dpif-xlate-cache.c | 29 +++++++ > ofproto/ofproto-dpif-xlate-cache.h | 2 + > ofproto/ofproto-dpif.c | 11 +++ > ofproto/ofproto-dpif.h | 2 + > ofproto/ofproto-provider.h | 2 + > ofproto/ofproto.c | 11 +-- > tests/ofproto-dpif.at | 62 ++++++++++++++ > tests/ofproto-macros.at | 14 ++- > 12 files changed, 271 insertions(+), 60 deletions(-) > > diff --git a/NEWS b/NEWS > index d18693315eaa..b7eb1baad8eb 100644 > --- a/NEWS > +++ b/NEWS > @@ -7,6 +7,8 @@ Post-v3.3.0 > or 'text' (by default). > * Added new option [--pretty] to print JSON output in a readable fashion. > * 'dpif/show' and 'list-commands' now support output in JSON format. > + * Added 'ofproto/detrace' to output the set of OpenFlow rules and > + groups that contributed to the creation of a specific datapath flow. > - Userspace datapath: > * Conntrack now supports 'random' flag for selecting ports in a range > while natting and 'persistent' flag for selection of the IP address > diff --git a/include/openvswitch/ofp-group.h b/include/openvswitch/ofp-group.h > index cd7af0ebff9c..7cbb2f70f316 100644 > --- a/include/openvswitch/ofp-group.h > +++ b/include/openvswitch/ofp-group.h > @@ -70,6 +70,10 @@ struct ofputil_bucket *ofputil_bucket_find(const struct ovs_list *, > bool ofputil_bucket_check_duplicate_id(const struct ovs_list *); > struct ofputil_bucket *ofputil_bucket_list_front(const struct ovs_list *); > struct ofputil_bucket *ofputil_bucket_list_back(const struct ovs_list *); > +void ofputil_bucket_format(struct ds *, const struct ofputil_bucket *, > + enum ofp11_group_type, enum ofp_version, > + const struct ofputil_port_map *, > + const struct ofputil_table_map *); > > static inline bool > ofputil_bucket_has_liveness(const struct ofputil_bucket *bucket) > @@ -88,6 +92,8 @@ struct ofputil_group_props { > void ofputil_group_properties_destroy(struct ofputil_group_props *); > void ofputil_group_properties_copy(struct ofputil_group_props *to, > const struct ofputil_group_props *from); > +void ofputil_group_properties_format(const struct ofputil_group_props *, > + struct ds *); > /* Protocol-independent group_mod. */ > struct ofputil_group_mod { > uint16_t command; /* One of OFPGC15_*. */ > @@ -199,6 +205,14 @@ enum ofperr ofputil_group_desc_format(struct ds *, const struct ofp_header *, > enum ofperr ofputil_group_features_format(struct ds *, > const struct ofp_header *); > > +/* Group formatting. */ > +void ofputil_group_format(struct ds *s, uint32_t group_id, uint8_t type, > + const struct ofputil_bucket *, > + const struct ovs_list *p_buckets, > + const struct ofputil_group_props *, > + enum ofp_version, bool suppress_type, > + const struct ofputil_port_map *, > + const struct ofputil_table_map *); > #ifdef __cplusplus > } > #endif > diff --git a/lib/ofp-group.c b/lib/ofp-group.c > index 737f48047b10..3edf1b01b372 100644 > --- a/lib/ofp-group.c > +++ b/lib/ofp-group.c > @@ -1526,6 +1526,31 @@ ofputil_group_properties_destroy(struct ofputil_group_props *gp) > free(gp->fields.values); > } > > +void > +ofputil_group_properties_format(const struct ofputil_group_props *gp, > + struct ds *ds) > +{ > + if (!gp->selection_method[0]) { > + return; > + } > + > + ds_put_format(ds, ",selection_method=%s", gp->selection_method); > + if (gp->selection_method_param) { > + ds_put_format(ds, ",selection_method_param=%"PRIu64, > + gp->selection_method_param); > + } > + > + size_t n = bitmap_count1(gp->fields.used.bm, MFF_N_IDS); > + if (n == 1) { > + ds_put_cstr(ds, ",fields="); > + oxm_format_field_array(ds, &gp->fields); > + } else if (n > 1) { > + ds_put_cstr(ds, ",fields("); > + oxm_format_field_array(ds, &gp->fields); > + ds_put_char(ds, ')'); > + } > +} > + > static enum ofperr > parse_group_prop_ntr_selection_method(struct ofpbuf *payload, > enum ofp11_group_type group_type, > @@ -1813,16 +1838,45 @@ ofp_print_bucket_id(struct ds *s, const char *label, uint32_t bucket_id, > ds_put_char(s, ','); > } > > -static void > -ofp_print_group(struct ds *s, uint32_t group_id, uint8_t type, > - const struct ovs_list *p_buckets, > - const struct ofputil_group_props *props, > - enum ofp_version ofp_version, bool suppress_type, > - const struct ofputil_port_map *port_map, > - const struct ofputil_table_map *table_map) > +void > +ofputil_bucket_format(struct ds * s, const struct ofputil_bucket *bucket, > + enum ofp11_group_type type, enum ofp_version ofp_version, > + const struct ofputil_port_map *port_map, > + const struct ofputil_table_map *table_map) > { > - struct ofputil_bucket *bucket; > + ds_put_cstr(s, "bucket="); > + > + ofp_print_bucket_id(s, "bucket_id:", bucket->bucket_id, ofp_version); > + if (bucket->weight != (type == OFPGT11_SELECT ? 1 : 0)) { > + ds_put_format(s, "weight:%"PRIu16",", bucket->weight); > + } > + if (bucket->watch_port != OFPP_NONE) { > + ds_put_cstr(s, "watch_port:"); > + ofputil_format_port(bucket->watch_port, port_map, s); > + ds_put_char(s, ','); > + } > + if (bucket->watch_group != OFPG_ANY) { > + ds_put_format(s, "watch_group:%"PRIu32",", bucket->watch_group); > + } > > + ds_put_cstr(s, "actions="); > + struct ofpact_format_params fp = { > + .port_map = port_map, > + .table_map = table_map, > + .s = s, > + }; > + ofpacts_format(bucket->ofpacts, bucket->ofpacts_len, &fp); > +} > + > +void > +ofputil_group_format(struct ds *s, uint32_t group_id, uint8_t type, > + const struct ofputil_bucket *bucket, > + const struct ovs_list *p_buckets, > + const struct ofputil_group_props *props, > + enum ofp_version ofp_version, bool suppress_type, > + const struct ofputil_port_map *port_map, > + const struct ofputil_table_map *table_map) > +{ > ds_put_format(s, "group_id=%"PRIu32, group_id); > > if (!suppress_type) { > @@ -1831,57 +1885,24 @@ ofp_print_group(struct ds *s, uint32_t group_id, uint8_t type, > ds_put_format(s, ",type=%s", type_str[type > 4 ? 4 : type]); > } > > - if (props->selection_method[0]) { > - ds_put_format(s, ",selection_method=%s", props->selection_method); > - if (props->selection_method_param) { > - ds_put_format(s, ",selection_method_param=%"PRIu64, > - props->selection_method_param); > - } > - > - size_t n = bitmap_count1(props->fields.used.bm, MFF_N_IDS); > - if (n == 1) { > - ds_put_cstr(s, ",fields="); > - oxm_format_field_array(s, &props->fields); > - } else if (n > 1) { > - ds_put_cstr(s, ",fields("); > - oxm_format_field_array(s, &props->fields); > - ds_put_char(s, ')'); > - } > - } > + ofputil_group_properties_format(props, s); > > - if (!p_buckets) { > + if (!bucket && !p_buckets) { > return; > } > > ds_put_char(s, ','); > > - LIST_FOR_EACH (bucket, list_node, p_buckets) { > - ds_put_cstr(s, "bucket="); > - > - ofp_print_bucket_id(s, "bucket_id:", bucket->bucket_id, ofp_version); > - if (bucket->weight != (type == OFPGT11_SELECT ? 1 : 0)) { > - ds_put_format(s, "weight:%"PRIu16",", bucket->weight); > - } > - if (bucket->watch_port != OFPP_NONE) { > - ds_put_cstr(s, "watch_port:"); > - ofputil_format_port(bucket->watch_port, port_map, s); > + if (bucket) { > + ofputil_bucket_format(s, bucket, type, ofp_version, NULL, NULL); > + } else { > + LIST_FOR_EACH (bucket, list_node, p_buckets) { > + ofputil_bucket_format(s, bucket, type, ofp_version, > + port_map, table_map); > ds_put_char(s, ','); > } > - if (bucket->watch_group != OFPG_ANY) { > - ds_put_format(s, "watch_group:%"PRIu32",", bucket->watch_group); > - } > - > - ds_put_cstr(s, "actions="); > - struct ofpact_format_params fp = { > - .port_map = port_map, > - .table_map = table_map, > - .s = s, > - }; > - ofpacts_format(bucket->ofpacts, bucket->ofpacts_len, &fp); > - ds_put_char(s, ','); > + ds_chomp(s, ','); > } > - > - ds_chomp(s, ','); > } > > enum ofperr > @@ -1901,8 +1922,9 @@ ofputil_group_desc_format(struct ds *s, const struct ofp_header *oh, > > ds_put_char(s, '\n'); > ds_put_char(s, ' '); > - ofp_print_group(s, gd.group_id, gd.type, &gd.buckets, &gd.props, > - oh->version, false, port_map, table_map); > + ofputil_group_format(s, gd.group_id, gd.type, NULL, &gd.buckets, > + &gd.props, oh->version, false, > + port_map, table_map); > ofputil_uninit_group_desc(&gd); > } > } > @@ -2368,8 +2390,9 @@ ofputil_group_mod_format__(struct ds *s, enum ofp_version ofp_version, > gm->command_bucket_id, ofp_version); > } > > - ofp_print_group(s, gm->group_id, gm->type, &gm->buckets, &gm->props, > - ofp_version, bucket_command, port_map, table_map); > + ofputil_group_format(s, gm->group_id, gm->type, NULL, &gm->buckets, > + &gm->props, ofp_version, bucket_command, > + port_map, table_map); > } > > enum ofperr > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 83609ec62b63..2054f1c056e5 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -383,6 +383,7 @@ static void upcall_unixctl_disable_ufid(struct unixctl_conn *, int argc, > const char *argv[], void *aux); > static void upcall_unixctl_enable_ufid(struct unixctl_conn *, int argc, > const char *argv[], void *aux); > + > static void upcall_unixctl_set_flow_limit(struct unixctl_conn *conn, int argc, > const char *argv[], void *aux); > static void upcall_unixctl_dump_wait(struct unixctl_conn *conn, int argc, > @@ -394,6 +395,9 @@ static void upcall_unixctl_pause(struct unixctl_conn *conn, int argc, > static void upcall_unixctl_resume(struct unixctl_conn *conn, int argc, > const char *argv[], void *aux); > > +static void upcall_unixctl_ofproto_detrace(struct unixctl_conn *, int argc, > + const char *argv[], void *aux); > + > static struct udpif_key *ukey_create_from_upcall(struct upcall *, > struct flow_wildcards *); > static int ukey_create_from_dpif_flow(const struct udpif *, > @@ -470,6 +474,8 @@ udpif_init(void) > upcall_unixctl_pause, NULL); > unixctl_command_register("revalidator/resume", NULL, 0, 0, > upcall_unixctl_resume, NULL); > + unixctl_command_register("ofproto/detrace", "UFID PMD-ID", 1, 2, > + upcall_unixctl_ofproto_detrace, NULL); > ovsthread_once_done(&once); > } > } > @@ -3310,6 +3316,51 @@ upcall_unixctl_resume(struct unixctl_conn *conn, int argc OVS_UNUSED, > unixctl_command_reply(conn, ""); > } > > +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]; > + ovs_u128 ufid; > + > + if (odp_ufid_from_string(key_s, &ufid) <= 0) { > + unixctl_command_reply_error(conn, "failed to parse ufid"); > + return; > + } > + > + if (argc == 3) { > + const char *pmd_str = argv[2]; > + if (!ovs_scan(pmd_str, "pmd=%d", &pmd_id)) { > + unixctl_command_reply_error(conn, > + "Invalid pmd argument format. " > + "Expecting 'pmd=PMD-ID'"); > + return; > + } > + } > + > + struct ds ds = DS_EMPTY_INITIALIZER; > + struct udpif *udpif; > + > + LIST_FOR_EACH (udpif, list_node, &all_udpifs) { > + struct udpif_key *ukey = ukey_lookup(udpif, &ufid, pmd_id); > + if (!ukey) { > + continue; > + } > + > + ovs_mutex_lock(&ukey->mutex); > + /* It only makes sense to format rules for ukeys that are (still) > + * in use. */ > + if ((ukey->state == UKEY_VISIBLE || ukey->state == UKEY_OPERATIONAL) > + && ukey->xcache) { > + xlate_xcache_format(&ds, ukey->xcache); > + } > + ovs_mutex_unlock(&ukey->mutex); > + } > + unixctl_command_reply(conn, ds_cstr(&ds)); > + ds_destroy(&ds); > +} > + > > /* Flows are sorted in the following order: > * netdev, flow state (offloaded/kernel path), flow_pps_rate. > diff --git a/ofproto/ofproto-dpif-xlate-cache.c b/ofproto/ofproto-dpif-xlate-cache.c > index 2e1fcb3a6f7a..e987ca8a4636 100644 > --- a/ofproto/ofproto-dpif-xlate-cache.c > +++ b/ofproto/ofproto-dpif-xlate-cache.c > @@ -301,3 +301,32 @@ xlate_cache_steal_entries(struct xlate_cache *dst, struct xlate_cache *src) > memcpy(p, src_entries->data, src_entries->size); > ofpbuf_clear(src_entries); > } > + > +void > +xlate_xcache_format(struct ds *s, const struct xlate_cache *xcache) > +{ > + struct ofpbuf entries = xcache->entries; > + struct xc_entry *entry; > + > + XC_ENTRY_FOR_EACH (entry, &entries) { > + switch (entry->type) { > + case XC_RULE: > + ofproto_rule_stats_ds(s, &entry->rule->up, true); > + break; > + case XC_GROUP: > + group_dpif_format(s, entry->group.group, entry->group.bucket); > + break; > + case XC_TABLE: > + case XC_BOND: > + case XC_NETDEV: > + case XC_NETFLOW: > + case XC_MIRROR: > + case XC_LEARN: > + case XC_NORMAL: > + case XC_FIN_TIMEOUT: > + case XC_TNL_NEIGH: > + case XC_TUNNEL_HEADER: > + break; > + } > + } > +} > diff --git a/ofproto/ofproto-dpif-xlate-cache.h b/ofproto/ofproto-dpif-xlate-cache.h > index 0fc6d2ea60ce..e701734d796a 100644 > --- a/ofproto/ofproto-dpif-xlate-cache.h > +++ b/ofproto/ofproto-dpif-xlate-cache.h > @@ -151,4 +151,6 @@ void xlate_cache_uninit(struct xlate_cache *); > void xlate_cache_delete(struct xlate_cache *); > void xlate_cache_steal_entries(struct xlate_cache *, struct xlate_cache *); > > +void xlate_xcache_format(struct ds *, const struct xlate_cache *); > + > #endif /* ofproto-dpif-xlate-cache.h */ > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 87dfb0043dd9..ff94532dbbbb 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -5408,6 +5408,17 @@ group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id, > version, take_ref); > return ofgroup ? group_dpif_cast(ofgroup) : NULL; > } > + > +void > +group_dpif_format(struct ds *ds, struct group_dpif *group, > + struct ofputil_bucket *bucket) > +{ > + struct ofgroup *ofg = &group->up; > + > + ofputil_group_format(ds, ofg->group_id, ofg->type, > + bucket, &ofg->buckets, &ofg->props, OFP15_VERSION, > + false, NULL, NULL); > +} Any reason for this function to exist? Can xlate_xcache_format() just call ofputil_group_format() directly? > > /* Sends 'packet' out 'ofport'. If 'port' is a tunnel and that tunnel type > * supports a notion of an OAM flag, sets it if 'oam' is true. > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h > index d33f73df8aed..acec2c08a3af 100644 > --- a/ofproto/ofproto-dpif.h > +++ b/ofproto/ofproto-dpif.h > @@ -151,6 +151,8 @@ void group_dpif_credit_stats(struct group_dpif *, > struct group_dpif *group_dpif_lookup(struct ofproto_dpif *, > uint32_t group_id, ovs_version_t version, > bool take_ref); > +void group_dpif_format(struct ds *, struct group_dpif *, > + struct ofputil_bucket *); > > > /* Backers. > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index 83c509fcf804..4f42cc02df64 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -450,6 +450,8 @@ void ofproto_rule_ref(struct rule *); > bool ofproto_rule_try_ref(struct rule *); > void ofproto_rule_unref(struct rule *); > > +void ofproto_rule_stats_ds(struct ds *, struct rule *, bool offload_stats); > + > static inline const struct rule_actions * rule_get_actions(const struct rule *); > static inline bool rule_is_table_miss(const struct rule *); > static inline bool rule_is_hidden(const struct rule *); > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 21c6a1d8257d..8ad5712f5fc6 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -4844,9 +4844,9 @@ handle_flow_stats_request(struct ofconn *ofconn, > return 0; > } > > -static void > -flow_stats_ds(struct ofproto *ofproto, struct rule *rule, struct ds *results, > - bool offload_stats) > +void > +ofproto_rule_stats_ds(struct ds *results, struct rule *rule, > + bool offload_stats) > { > struct pkt_stats stats; > const struct rule_actions *actions; > @@ -4875,7 +4875,8 @@ flow_stats_ds(struct ofproto *ofproto, struct rule *rule, struct ds *results, > ds_put_format(results, "n_offload_bytes=%"PRIu64", ", > stats.n_offload_bytes); > } > - cls_rule_format(&rule->cr, ofproto_get_tun_tab(ofproto), NULL, results); > + cls_rule_format(&rule->cr, ofproto_get_tun_tab(rule->ofproto), NULL, > + results); > ds_put_char(results, ','); > > ds_put_cstr(results, "actions="); > @@ -4897,7 +4898,7 @@ ofproto_get_all_flows(struct ofproto *p, struct ds *results, > struct rule *rule; > > CLS_FOR_EACH (rule, cr, &table->cls) { > - flow_stats_ds(p, rule, results, offload_stats); > + ofproto_rule_stats_ds(results, rule, offload_stats); > } > } > } > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 30ef0468c8d1..7e06c6e3503a 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -12176,3 +12176,65 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap | wc -l`]) > > OVS_VSWITCHD_STOP > AT_CLEANUP > + > +AT_SETUP([ofproto-dpif - Dump OF rules corresponding to UFID]) > +OVS_VSWITCHD_START( > +[add-port br0 p1 \ > + -- set bridge br0 datapath-type=dummy \ > + -- set interface p1 ofport_request=1 type=dummy \ > + -- add-port br0 p2 \ > + -- set interface p2 ofport_request=2 type=dummy\ > + -- add-port br0 p3 \ > + -- set interface p3 ofport_request=3 type=dummy > +], [], [], [DUMMY_NUMA]) You don't need dummy numa here. But also the whole thing can be replaced with: 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)']) > +ovs-appctl revalidator/wait > +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) > +]) > + > +ovs-appctl revalidator/resume > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at > index c22fb3c79c3f..c27d96177b64 100644 > --- a/tests/ofproto-macros.at > +++ b/tests/ofproto-macros.at > @@ -9,7 +9,9 @@ s/ duration=[0-9.]*s,// > s/ cookie=0x0,// > s/ table=0,// > s/ n_packets=0,// > +s/ n_offload_packets=0,// > s/ n_bytes=0,// > +s/ n_offload_bytes=0,// > s/ idle_age=[0-9]*,// > s/ hard_age=[0-9]*,// > s/dp_hash=0x[0-9a-f]*\//dp_hash=0x0\// > @@ -130,7 +132,7 @@ strip_used () { > > # Removes all 'duration=...' to make output easier to compare. > strip_duration () { > - sed 's/duration=[[0-9]]*\.[[0-9]]*s,//' > + sed 's/duration=[[0-9.]]*s,//' > } > > # Strips 'ufid:...' from output, to make it easier to compare. > @@ -140,6 +142,10 @@ strip_ufid () { > s/ufid:[[-0-9a-f]]* //' > } > > +parse_ufid () { > + grep -o 'ufid:[[-0-9a-f]]*' > +} > + > # Strips packets: and bytes: from output > strip_stats () { > sed 's/packets:[[0-9]]*/packets:0/ > @@ -169,6 +175,12 @@ strip_recirc() { > s/recirc_id=[[x0-9]]*/recirc_id=<recirc>/ > s/recirc([[x0-9]]*)/recirc(<recirc>)/' > } > + > +# Strips dp_hash from output. > +strip_dp_hash() { > + sed 's/dp_hash([[0-9a-fx/]]*),//' > +} > + > m4_divert_pop([PREPARE_TESTS]) > > m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m'])
On 7/12/24 14:11, Ilya Maximets wrote: > On 7/12/24 13:54, Dumitru Ceara wrote: >> It improves the debugging experience if we can easily get a list of >> OpenFlow rules and groups that contribute to the creation of a datapath >> flow. >> >> The suggested workflow is: >> a. dump datapath flows (along with UUIDs), this also prints the core IDs >> (PMD IDs) when applicable. >> >> $ ovs-appctl dpctl/dump-flows -m >> flow-dump from pmd on cpu core: 7 >> ufid:7460db8f..., recirc_id(0), .... >> >> b. dump related OpenFlow rules and groups: >> $ ovs-appctl ofproto/detrace ufid:7460db8f... pmd=7 >> cookie=0x12345678, table=0 priority=100,ip,in_port=2,nw_dst=10.0.0.2,actions=resubmit(,1) >> cookie=0x0, table=1 priority=200,actions=group:1 >> group_id=1,bucket=bucket_id:0,actions=ct(commit,table=2,nat(dst=20.0.0.2)) >> cookie=0x0, table=2 actions=output:1 >> >> The new command only shows rules and groups attached to ukeys that are >> in states UKEY_VISIBLE or UKEY_OPERATIONAL. That should be fine as all >> other ukeys should not be relevant for the use case presented above. >> >> This commit tries to mimic the output format of the ovs-ofctl >> dump-flows/dump-groups commands. >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- > > A very quick review. Obviously, didn't dive deep, but see a couple > comments below. > >> V5: >> - Addressed Ilya's comments: >> - I addressed all from here >> https://mail.openvswitch.org/pipermail/ovs-dev/2024-June/415144.html >> except the one about adding -m/-mm. It felt OK to always dump >> statistics. >> V4: >> - Addressed Mike's comment: >> - use predictable port IDs. >> V3: >> - Addressed Mike's comments: >> - use ds_put_char() >> - make the test less flaky >> V2: >> - Addressed Adrian's comments: >> - check return value of populate_xcache() >> - use flow_stats_ds() (renamed to ofproto_rule_stats_ds()) instead of >> custom printing >> - move ukey->state check to caller >> - handle case when group bucket is not available >> - update test case to cover the point above >> --- >> NEWS | 2 + >> include/openvswitch/ofp-group.h | 14 +++ >> lib/ofp-group.c | 131 +++++++++++++++++------------ >> ofproto/ofproto-dpif-upcall.c | 51 +++++++++++ >> ofproto/ofproto-dpif-xlate-cache.c | 29 +++++++ >> ofproto/ofproto-dpif-xlate-cache.h | 2 + >> ofproto/ofproto-dpif.c | 11 +++ >> ofproto/ofproto-dpif.h | 2 + >> ofproto/ofproto-provider.h | 2 + >> ofproto/ofproto.c | 11 +-- >> tests/ofproto-dpif.at | 62 ++++++++++++++ >> tests/ofproto-macros.at | 14 ++- >> 12 files changed, 271 insertions(+), 60 deletions(-) >> >> diff --git a/NEWS b/NEWS >> index d18693315eaa..b7eb1baad8eb 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -7,6 +7,8 @@ Post-v3.3.0 >> or 'text' (by default). >> * Added new option [--pretty] to print JSON output in a readable fashion. >> * 'dpif/show' and 'list-commands' now support output in JSON format. >> + * Added 'ofproto/detrace' to output the set of OpenFlow rules and >> + groups that contributed to the creation of a specific datapath flow. >> - Userspace datapath: >> * Conntrack now supports 'random' flag for selecting ports in a range >> while natting and 'persistent' flag for selection of the IP address >> diff --git a/include/openvswitch/ofp-group.h b/include/openvswitch/ofp-group.h >> index cd7af0ebff9c..7cbb2f70f316 100644 >> --- a/include/openvswitch/ofp-group.h >> +++ b/include/openvswitch/ofp-group.h >> @@ -70,6 +70,10 @@ struct ofputil_bucket *ofputil_bucket_find(const struct ovs_list *, >> bool ofputil_bucket_check_duplicate_id(const struct ovs_list *); >> struct ofputil_bucket *ofputil_bucket_list_front(const struct ovs_list *); >> struct ofputil_bucket *ofputil_bucket_list_back(const struct ovs_list *); >> +void ofputil_bucket_format(struct ds *, const struct ofputil_bucket *, >> + enum ofp11_group_type, enum ofp_version, >> + const struct ofputil_port_map *, >> + const struct ofputil_table_map *); >> >> static inline bool >> ofputil_bucket_has_liveness(const struct ofputil_bucket *bucket) >> @@ -88,6 +92,8 @@ struct ofputil_group_props { >> void ofputil_group_properties_destroy(struct ofputil_group_props *); >> void ofputil_group_properties_copy(struct ofputil_group_props *to, >> const struct ofputil_group_props *from); >> +void ofputil_group_properties_format(const struct ofputil_group_props *, >> + struct ds *); >> /* Protocol-independent group_mod. */ >> struct ofputil_group_mod { >> uint16_t command; /* One of OFPGC15_*. */ >> @@ -199,6 +205,14 @@ enum ofperr ofputil_group_desc_format(struct ds *, const struct ofp_header *, >> enum ofperr ofputil_group_features_format(struct ds *, >> const struct ofp_header *); >> >> +/* Group formatting. */ >> +void ofputil_group_format(struct ds *s, uint32_t group_id, uint8_t type, >> + const struct ofputil_bucket *, >> + const struct ovs_list *p_buckets, >> + const struct ofputil_group_props *, >> + enum ofp_version, bool suppress_type, >> + const struct ofputil_port_map *, >> + const struct ofputil_table_map *); >> #ifdef __cplusplus >> } >> #endif >> diff --git a/lib/ofp-group.c b/lib/ofp-group.c >> index 737f48047b10..3edf1b01b372 100644 >> --- a/lib/ofp-group.c >> +++ b/lib/ofp-group.c >> @@ -1526,6 +1526,31 @@ ofputil_group_properties_destroy(struct ofputil_group_props *gp) >> free(gp->fields.values); >> } >> >> +void >> +ofputil_group_properties_format(const struct ofputil_group_props *gp, >> + struct ds *ds) >> +{ >> + if (!gp->selection_method[0]) { >> + return; >> + } >> + >> + ds_put_format(ds, ",selection_method=%s", gp->selection_method); >> + if (gp->selection_method_param) { >> + ds_put_format(ds, ",selection_method_param=%"PRIu64, >> + gp->selection_method_param); >> + } >> + >> + size_t n = bitmap_count1(gp->fields.used.bm, MFF_N_IDS); >> + if (n == 1) { >> + ds_put_cstr(ds, ",fields="); >> + oxm_format_field_array(ds, &gp->fields); >> + } else if (n > 1) { >> + ds_put_cstr(ds, ",fields("); >> + oxm_format_field_array(ds, &gp->fields); >> + ds_put_char(ds, ')'); >> + } >> +} >> + >> static enum ofperr >> parse_group_prop_ntr_selection_method(struct ofpbuf *payload, >> enum ofp11_group_type group_type, >> @@ -1813,16 +1838,45 @@ ofp_print_bucket_id(struct ds *s, const char *label, uint32_t bucket_id, >> ds_put_char(s, ','); >> } >> >> -static void >> -ofp_print_group(struct ds *s, uint32_t group_id, uint8_t type, >> - const struct ovs_list *p_buckets, >> - const struct ofputil_group_props *props, >> - enum ofp_version ofp_version, bool suppress_type, >> - const struct ofputil_port_map *port_map, >> - const struct ofputil_table_map *table_map) >> +void >> +ofputil_bucket_format(struct ds * s, const struct ofputil_bucket *bucket, >> + enum ofp11_group_type type, enum ofp_version ofp_version, >> + const struct ofputil_port_map *port_map, >> + const struct ofputil_table_map *table_map) >> { >> - struct ofputil_bucket *bucket; >> + ds_put_cstr(s, "bucket="); >> + >> + ofp_print_bucket_id(s, "bucket_id:", bucket->bucket_id, ofp_version); >> + if (bucket->weight != (type == OFPGT11_SELECT ? 1 : 0)) { >> + ds_put_format(s, "weight:%"PRIu16",", bucket->weight); >> + } >> + if (bucket->watch_port != OFPP_NONE) { >> + ds_put_cstr(s, "watch_port:"); >> + ofputil_format_port(bucket->watch_port, port_map, s); >> + ds_put_char(s, ','); >> + } >> + if (bucket->watch_group != OFPG_ANY) { >> + ds_put_format(s, "watch_group:%"PRIu32",", bucket->watch_group); >> + } >> >> + ds_put_cstr(s, "actions="); >> + struct ofpact_format_params fp = { >> + .port_map = port_map, >> + .table_map = table_map, >> + .s = s, >> + }; >> + ofpacts_format(bucket->ofpacts, bucket->ofpacts_len, &fp); >> +} >> + >> +void >> +ofputil_group_format(struct ds *s, uint32_t group_id, uint8_t type, >> + const struct ofputil_bucket *bucket, >> + const struct ovs_list *p_buckets, >> + const struct ofputil_group_props *props, >> + enum ofp_version ofp_version, bool suppress_type, >> + const struct ofputil_port_map *port_map, >> + const struct ofputil_table_map *table_map) >> +{ >> ds_put_format(s, "group_id=%"PRIu32, group_id); >> >> if (!suppress_type) { >> @@ -1831,57 +1885,24 @@ ofp_print_group(struct ds *s, uint32_t group_id, uint8_t type, >> ds_put_format(s, ",type=%s", type_str[type > 4 ? 4 : type]); >> } >> >> - if (props->selection_method[0]) { >> - ds_put_format(s, ",selection_method=%s", props->selection_method); >> - if (props->selection_method_param) { >> - ds_put_format(s, ",selection_method_param=%"PRIu64, >> - props->selection_method_param); >> - } >> - >> - size_t n = bitmap_count1(props->fields.used.bm, MFF_N_IDS); >> - if (n == 1) { >> - ds_put_cstr(s, ",fields="); >> - oxm_format_field_array(s, &props->fields); >> - } else if (n > 1) { >> - ds_put_cstr(s, ",fields("); >> - oxm_format_field_array(s, &props->fields); >> - ds_put_char(s, ')'); >> - } >> - } >> + ofputil_group_properties_format(props, s); >> >> - if (!p_buckets) { >> + if (!bucket && !p_buckets) { >> return; >> } >> >> ds_put_char(s, ','); >> >> - LIST_FOR_EACH (bucket, list_node, p_buckets) { >> - ds_put_cstr(s, "bucket="); >> - >> - ofp_print_bucket_id(s, "bucket_id:", bucket->bucket_id, ofp_version); >> - if (bucket->weight != (type == OFPGT11_SELECT ? 1 : 0)) { >> - ds_put_format(s, "weight:%"PRIu16",", bucket->weight); >> - } >> - if (bucket->watch_port != OFPP_NONE) { >> - ds_put_cstr(s, "watch_port:"); >> - ofputil_format_port(bucket->watch_port, port_map, s); >> + if (bucket) { >> + ofputil_bucket_format(s, bucket, type, ofp_version, NULL, NULL); >> + } else { >> + LIST_FOR_EACH (bucket, list_node, p_buckets) { >> + ofputil_bucket_format(s, bucket, type, ofp_version, >> + port_map, table_map); >> ds_put_char(s, ','); >> } >> - if (bucket->watch_group != OFPG_ANY) { >> - ds_put_format(s, "watch_group:%"PRIu32",", bucket->watch_group); >> - } >> - >> - ds_put_cstr(s, "actions="); >> - struct ofpact_format_params fp = { >> - .port_map = port_map, >> - .table_map = table_map, >> - .s = s, >> - }; >> - ofpacts_format(bucket->ofpacts, bucket->ofpacts_len, &fp); >> - ds_put_char(s, ','); >> + ds_chomp(s, ','); >> } >> - >> - ds_chomp(s, ','); >> } >> >> enum ofperr >> @@ -1901,8 +1922,9 @@ ofputil_group_desc_format(struct ds *s, const struct ofp_header *oh, >> >> ds_put_char(s, '\n'); >> ds_put_char(s, ' '); >> - ofp_print_group(s, gd.group_id, gd.type, &gd.buckets, &gd.props, >> - oh->version, false, port_map, table_map); >> + ofputil_group_format(s, gd.group_id, gd.type, NULL, &gd.buckets, >> + &gd.props, oh->version, false, >> + port_map, table_map); >> ofputil_uninit_group_desc(&gd); >> } >> } >> @@ -2368,8 +2390,9 @@ ofputil_group_mod_format__(struct ds *s, enum ofp_version ofp_version, >> gm->command_bucket_id, ofp_version); >> } >> >> - ofp_print_group(s, gm->group_id, gm->type, &gm->buckets, &gm->props, >> - ofp_version, bucket_command, port_map, table_map); >> + ofputil_group_format(s, gm->group_id, gm->type, NULL, &gm->buckets, >> + &gm->props, ofp_version, bucket_command, >> + port_map, table_map); >> } >> >> enum ofperr >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >> index 83609ec62b63..2054f1c056e5 100644 >> --- a/ofproto/ofproto-dpif-upcall.c >> +++ b/ofproto/ofproto-dpif-upcall.c >> @@ -383,6 +383,7 @@ static void upcall_unixctl_disable_ufid(struct unixctl_conn *, int argc, >> const char *argv[], void *aux); >> static void upcall_unixctl_enable_ufid(struct unixctl_conn *, int argc, >> const char *argv[], void *aux); >> + >> static void upcall_unixctl_set_flow_limit(struct unixctl_conn *conn, int argc, >> const char *argv[], void *aux); >> static void upcall_unixctl_dump_wait(struct unixctl_conn *conn, int argc, >> @@ -394,6 +395,9 @@ static void upcall_unixctl_pause(struct unixctl_conn *conn, int argc, >> static void upcall_unixctl_resume(struct unixctl_conn *conn, int argc, >> const char *argv[], void *aux); >> >> +static void upcall_unixctl_ofproto_detrace(struct unixctl_conn *, int argc, >> + const char *argv[], void *aux); >> + >> static struct udpif_key *ukey_create_from_upcall(struct upcall *, >> struct flow_wildcards *); >> static int ukey_create_from_dpif_flow(const struct udpif *, >> @@ -470,6 +474,8 @@ udpif_init(void) >> upcall_unixctl_pause, NULL); >> unixctl_command_register("revalidator/resume", NULL, 0, 0, >> upcall_unixctl_resume, NULL); >> + unixctl_command_register("ofproto/detrace", "UFID PMD-ID", 1, 2, >> + upcall_unixctl_ofproto_detrace, NULL); >> ovsthread_once_done(&once); >> } >> } >> @@ -3310,6 +3316,51 @@ upcall_unixctl_resume(struct unixctl_conn *conn, int argc OVS_UNUSED, >> unixctl_command_reply(conn, ""); >> } >> >> +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]; >> + ovs_u128 ufid; >> + >> + if (odp_ufid_from_string(key_s, &ufid) <= 0) { >> + unixctl_command_reply_error(conn, "failed to parse ufid"); >> + return; >> + } >> + >> + if (argc == 3) { >> + const char *pmd_str = argv[2]; >> + if (!ovs_scan(pmd_str, "pmd=%d", &pmd_id)) { >> + unixctl_command_reply_error(conn, >> + "Invalid pmd argument format. " >> + "Expecting 'pmd=PMD-ID'"); >> + return; >> + } >> + } >> + >> + struct ds ds = DS_EMPTY_INITIALIZER; >> + struct udpif *udpif; >> + >> + LIST_FOR_EACH (udpif, list_node, &all_udpifs) { >> + struct udpif_key *ukey = ukey_lookup(udpif, &ufid, pmd_id); >> + if (!ukey) { >> + continue; >> + } >> + >> + ovs_mutex_lock(&ukey->mutex); >> + /* It only makes sense to format rules for ukeys that are (still) >> + * in use. */ >> + if ((ukey->state == UKEY_VISIBLE || ukey->state == UKEY_OPERATIONAL) >> + && ukey->xcache) { >> + xlate_xcache_format(&ds, ukey->xcache); >> + } >> + ovs_mutex_unlock(&ukey->mutex); >> + } >> + unixctl_command_reply(conn, ds_cstr(&ds)); >> + ds_destroy(&ds); >> +} >> + >> >> /* Flows are sorted in the following order: >> * netdev, flow state (offloaded/kernel path), flow_pps_rate. >> diff --git a/ofproto/ofproto-dpif-xlate-cache.c b/ofproto/ofproto-dpif-xlate-cache.c >> index 2e1fcb3a6f7a..e987ca8a4636 100644 >> --- a/ofproto/ofproto-dpif-xlate-cache.c >> +++ b/ofproto/ofproto-dpif-xlate-cache.c >> @@ -301,3 +301,32 @@ xlate_cache_steal_entries(struct xlate_cache *dst, struct xlate_cache *src) >> memcpy(p, src_entries->data, src_entries->size); >> ofpbuf_clear(src_entries); >> } >> + >> +void >> +xlate_xcache_format(struct ds *s, const struct xlate_cache *xcache) >> +{ >> + struct ofpbuf entries = xcache->entries; >> + struct xc_entry *entry; >> + >> + XC_ENTRY_FOR_EACH (entry, &entries) { >> + switch (entry->type) { >> + case XC_RULE: >> + ofproto_rule_stats_ds(s, &entry->rule->up, true); >> + break; >> + case XC_GROUP: >> + group_dpif_format(s, entry->group.group, entry->group.bucket); >> + break; >> + case XC_TABLE: >> + case XC_BOND: >> + case XC_NETDEV: >> + case XC_NETFLOW: >> + case XC_MIRROR: >> + case XC_LEARN: >> + case XC_NORMAL: >> + case XC_FIN_TIMEOUT: >> + case XC_TNL_NEIGH: >> + case XC_TUNNEL_HEADER: >> + break; >> + } >> + } >> +} >> diff --git a/ofproto/ofproto-dpif-xlate-cache.h b/ofproto/ofproto-dpif-xlate-cache.h >> index 0fc6d2ea60ce..e701734d796a 100644 >> --- a/ofproto/ofproto-dpif-xlate-cache.h >> +++ b/ofproto/ofproto-dpif-xlate-cache.h >> @@ -151,4 +151,6 @@ void xlate_cache_uninit(struct xlate_cache *); >> void xlate_cache_delete(struct xlate_cache *); >> void xlate_cache_steal_entries(struct xlate_cache *, struct xlate_cache *); >> >> +void xlate_xcache_format(struct ds *, const struct xlate_cache *); >> + >> #endif /* ofproto-dpif-xlate-cache.h */ >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index 87dfb0043dd9..ff94532dbbbb 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -5408,6 +5408,17 @@ group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id, >> version, take_ref); >> return ofgroup ? group_dpif_cast(ofgroup) : NULL; >> } >> + >> +void >> +group_dpif_format(struct ds *ds, struct group_dpif *group, >> + struct ofputil_bucket *bucket) >> +{ >> + struct ofgroup *ofg = &group->up; >> + >> + ofputil_group_format(ds, ofg->group_id, ofg->type, >> + bucket, &ofg->buckets, &ofg->props, OFP15_VERSION, >> + false, NULL, NULL); >> +} > > Any reason for this function to exist? > Can xlate_xcache_format() just call ofputil_group_format() directly? > It can, I wasn't sure about how much of the internals of ofgroup we should acces from xlate_xcache_format(). I'll remove it. >> >> /* Sends 'packet' out 'ofport'. If 'port' is a tunnel and that tunnel type >> * supports a notion of an OAM flag, sets it if 'oam' is true. >> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h >> index d33f73df8aed..acec2c08a3af 100644 >> --- a/ofproto/ofproto-dpif.h >> +++ b/ofproto/ofproto-dpif.h >> @@ -151,6 +151,8 @@ void group_dpif_credit_stats(struct group_dpif *, >> struct group_dpif *group_dpif_lookup(struct ofproto_dpif *, >> uint32_t group_id, ovs_version_t version, >> bool take_ref); >> +void group_dpif_format(struct ds *, struct group_dpif *, >> + struct ofputil_bucket *); >> >> >> /* Backers. >> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h >> index 83c509fcf804..4f42cc02df64 100644 >> --- a/ofproto/ofproto-provider.h >> +++ b/ofproto/ofproto-provider.h >> @@ -450,6 +450,8 @@ void ofproto_rule_ref(struct rule *); >> bool ofproto_rule_try_ref(struct rule *); >> void ofproto_rule_unref(struct rule *); >> >> +void ofproto_rule_stats_ds(struct ds *, struct rule *, bool offload_stats); >> + >> static inline const struct rule_actions * rule_get_actions(const struct rule *); >> static inline bool rule_is_table_miss(const struct rule *); >> static inline bool rule_is_hidden(const struct rule *); >> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c >> index 21c6a1d8257d..8ad5712f5fc6 100644 >> --- a/ofproto/ofproto.c >> +++ b/ofproto/ofproto.c >> @@ -4844,9 +4844,9 @@ handle_flow_stats_request(struct ofconn *ofconn, >> return 0; >> } >> >> -static void >> -flow_stats_ds(struct ofproto *ofproto, struct rule *rule, struct ds *results, >> - bool offload_stats) >> +void >> +ofproto_rule_stats_ds(struct ds *results, struct rule *rule, >> + bool offload_stats) >> { >> struct pkt_stats stats; >> const struct rule_actions *actions; >> @@ -4875,7 +4875,8 @@ flow_stats_ds(struct ofproto *ofproto, struct rule *rule, struct ds *results, >> ds_put_format(results, "n_offload_bytes=%"PRIu64", ", >> stats.n_offload_bytes); >> } >> - cls_rule_format(&rule->cr, ofproto_get_tun_tab(ofproto), NULL, results); >> + cls_rule_format(&rule->cr, ofproto_get_tun_tab(rule->ofproto), NULL, >> + results); >> ds_put_char(results, ','); >> >> ds_put_cstr(results, "actions="); >> @@ -4897,7 +4898,7 @@ ofproto_get_all_flows(struct ofproto *p, struct ds *results, >> struct rule *rule; >> >> CLS_FOR_EACH (rule, cr, &table->cls) { >> - flow_stats_ds(p, rule, results, offload_stats); >> + ofproto_rule_stats_ds(results, rule, offload_stats); >> } >> } >> } >> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >> index 30ef0468c8d1..7e06c6e3503a 100644 >> --- a/tests/ofproto-dpif.at >> +++ b/tests/ofproto-dpif.at >> @@ -12176,3 +12176,65 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap | wc -l`]) >> >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> + >> +AT_SETUP([ofproto-dpif - Dump OF rules corresponding to UFID]) >> +OVS_VSWITCHD_START( >> +[add-port br0 p1 \ >> + -- set bridge br0 datapath-type=dummy \ >> + -- set interface p1 ofport_request=1 type=dummy \ >> + -- add-port br0 p2 \ >> + -- set interface p2 ofport_request=2 type=dummy\ >> + -- add-port br0 p3 \ >> + -- set interface p3 ofport_request=3 type=dummy >> +], [], [], [DUMMY_NUMA]) > > You don't need dummy numa here. But also the whole thing can Copy+paste again, sorry. > be replaced with: > > OVS_VSWITCHD_START > > add_of_ports br0 1 2 3 > Nice, I didn't know that one. v6 posted: https://patchwork.ozlabs.org/project/openvswitch/patch/20240712134755.985581-1-dceara@redhat.com/ Regards, Dumitru
diff --git a/NEWS b/NEWS index d18693315eaa..b7eb1baad8eb 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,8 @@ Post-v3.3.0 or 'text' (by default). * Added new option [--pretty] to print JSON output in a readable fashion. * 'dpif/show' and 'list-commands' now support output in JSON format. + * Added 'ofproto/detrace' to output the set of OpenFlow rules and + groups that contributed to the creation of a specific datapath flow. - Userspace datapath: * Conntrack now supports 'random' flag for selecting ports in a range while natting and 'persistent' flag for selection of the IP address diff --git a/include/openvswitch/ofp-group.h b/include/openvswitch/ofp-group.h index cd7af0ebff9c..7cbb2f70f316 100644 --- a/include/openvswitch/ofp-group.h +++ b/include/openvswitch/ofp-group.h @@ -70,6 +70,10 @@ struct ofputil_bucket *ofputil_bucket_find(const struct ovs_list *, bool ofputil_bucket_check_duplicate_id(const struct ovs_list *); struct ofputil_bucket *ofputil_bucket_list_front(const struct ovs_list *); struct ofputil_bucket *ofputil_bucket_list_back(const struct ovs_list *); +void ofputil_bucket_format(struct ds *, const struct ofputil_bucket *, + enum ofp11_group_type, enum ofp_version, + const struct ofputil_port_map *, + const struct ofputil_table_map *); static inline bool ofputil_bucket_has_liveness(const struct ofputil_bucket *bucket) @@ -88,6 +92,8 @@ struct ofputil_group_props { void ofputil_group_properties_destroy(struct ofputil_group_props *); void ofputil_group_properties_copy(struct ofputil_group_props *to, const struct ofputil_group_props *from); +void ofputil_group_properties_format(const struct ofputil_group_props *, + struct ds *); /* Protocol-independent group_mod. */ struct ofputil_group_mod { uint16_t command; /* One of OFPGC15_*. */ @@ -199,6 +205,14 @@ enum ofperr ofputil_group_desc_format(struct ds *, const struct ofp_header *, enum ofperr ofputil_group_features_format(struct ds *, const struct ofp_header *); +/* Group formatting. */ +void ofputil_group_format(struct ds *s, uint32_t group_id, uint8_t type, + const struct ofputil_bucket *, + const struct ovs_list *p_buckets, + const struct ofputil_group_props *, + enum ofp_version, bool suppress_type, + const struct ofputil_port_map *, + const struct ofputil_table_map *); #ifdef __cplusplus } #endif diff --git a/lib/ofp-group.c b/lib/ofp-group.c index 737f48047b10..3edf1b01b372 100644 --- a/lib/ofp-group.c +++ b/lib/ofp-group.c @@ -1526,6 +1526,31 @@ ofputil_group_properties_destroy(struct ofputil_group_props *gp) free(gp->fields.values); } +void +ofputil_group_properties_format(const struct ofputil_group_props *gp, + struct ds *ds) +{ + if (!gp->selection_method[0]) { + return; + } + + ds_put_format(ds, ",selection_method=%s", gp->selection_method); + if (gp->selection_method_param) { + ds_put_format(ds, ",selection_method_param=%"PRIu64, + gp->selection_method_param); + } + + size_t n = bitmap_count1(gp->fields.used.bm, MFF_N_IDS); + if (n == 1) { + ds_put_cstr(ds, ",fields="); + oxm_format_field_array(ds, &gp->fields); + } else if (n > 1) { + ds_put_cstr(ds, ",fields("); + oxm_format_field_array(ds, &gp->fields); + ds_put_char(ds, ')'); + } +} + static enum ofperr parse_group_prop_ntr_selection_method(struct ofpbuf *payload, enum ofp11_group_type group_type, @@ -1813,16 +1838,45 @@ ofp_print_bucket_id(struct ds *s, const char *label, uint32_t bucket_id, ds_put_char(s, ','); } -static void -ofp_print_group(struct ds *s, uint32_t group_id, uint8_t type, - const struct ovs_list *p_buckets, - const struct ofputil_group_props *props, - enum ofp_version ofp_version, bool suppress_type, - const struct ofputil_port_map *port_map, - const struct ofputil_table_map *table_map) +void +ofputil_bucket_format(struct ds * s, const struct ofputil_bucket *bucket, + enum ofp11_group_type type, enum ofp_version ofp_version, + const struct ofputil_port_map *port_map, + const struct ofputil_table_map *table_map) { - struct ofputil_bucket *bucket; + ds_put_cstr(s, "bucket="); + + ofp_print_bucket_id(s, "bucket_id:", bucket->bucket_id, ofp_version); + if (bucket->weight != (type == OFPGT11_SELECT ? 1 : 0)) { + ds_put_format(s, "weight:%"PRIu16",", bucket->weight); + } + if (bucket->watch_port != OFPP_NONE) { + ds_put_cstr(s, "watch_port:"); + ofputil_format_port(bucket->watch_port, port_map, s); + ds_put_char(s, ','); + } + if (bucket->watch_group != OFPG_ANY) { + ds_put_format(s, "watch_group:%"PRIu32",", bucket->watch_group); + } + ds_put_cstr(s, "actions="); + struct ofpact_format_params fp = { + .port_map = port_map, + .table_map = table_map, + .s = s, + }; + ofpacts_format(bucket->ofpacts, bucket->ofpacts_len, &fp); +} + +void +ofputil_group_format(struct ds *s, uint32_t group_id, uint8_t type, + const struct ofputil_bucket *bucket, + const struct ovs_list *p_buckets, + const struct ofputil_group_props *props, + enum ofp_version ofp_version, bool suppress_type, + const struct ofputil_port_map *port_map, + const struct ofputil_table_map *table_map) +{ ds_put_format(s, "group_id=%"PRIu32, group_id); if (!suppress_type) { @@ -1831,57 +1885,24 @@ ofp_print_group(struct ds *s, uint32_t group_id, uint8_t type, ds_put_format(s, ",type=%s", type_str[type > 4 ? 4 : type]); } - if (props->selection_method[0]) { - ds_put_format(s, ",selection_method=%s", props->selection_method); - if (props->selection_method_param) { - ds_put_format(s, ",selection_method_param=%"PRIu64, - props->selection_method_param); - } - - size_t n = bitmap_count1(props->fields.used.bm, MFF_N_IDS); - if (n == 1) { - ds_put_cstr(s, ",fields="); - oxm_format_field_array(s, &props->fields); - } else if (n > 1) { - ds_put_cstr(s, ",fields("); - oxm_format_field_array(s, &props->fields); - ds_put_char(s, ')'); - } - } + ofputil_group_properties_format(props, s); - if (!p_buckets) { + if (!bucket && !p_buckets) { return; } ds_put_char(s, ','); - LIST_FOR_EACH (bucket, list_node, p_buckets) { - ds_put_cstr(s, "bucket="); - - ofp_print_bucket_id(s, "bucket_id:", bucket->bucket_id, ofp_version); - if (bucket->weight != (type == OFPGT11_SELECT ? 1 : 0)) { - ds_put_format(s, "weight:%"PRIu16",", bucket->weight); - } - if (bucket->watch_port != OFPP_NONE) { - ds_put_cstr(s, "watch_port:"); - ofputil_format_port(bucket->watch_port, port_map, s); + if (bucket) { + ofputil_bucket_format(s, bucket, type, ofp_version, NULL, NULL); + } else { + LIST_FOR_EACH (bucket, list_node, p_buckets) { + ofputil_bucket_format(s, bucket, type, ofp_version, + port_map, table_map); ds_put_char(s, ','); } - if (bucket->watch_group != OFPG_ANY) { - ds_put_format(s, "watch_group:%"PRIu32",", bucket->watch_group); - } - - ds_put_cstr(s, "actions="); - struct ofpact_format_params fp = { - .port_map = port_map, - .table_map = table_map, - .s = s, - }; - ofpacts_format(bucket->ofpacts, bucket->ofpacts_len, &fp); - ds_put_char(s, ','); + ds_chomp(s, ','); } - - ds_chomp(s, ','); } enum ofperr @@ -1901,8 +1922,9 @@ ofputil_group_desc_format(struct ds *s, const struct ofp_header *oh, ds_put_char(s, '\n'); ds_put_char(s, ' '); - ofp_print_group(s, gd.group_id, gd.type, &gd.buckets, &gd.props, - oh->version, false, port_map, table_map); + ofputil_group_format(s, gd.group_id, gd.type, NULL, &gd.buckets, + &gd.props, oh->version, false, + port_map, table_map); ofputil_uninit_group_desc(&gd); } } @@ -2368,8 +2390,9 @@ ofputil_group_mod_format__(struct ds *s, enum ofp_version ofp_version, gm->command_bucket_id, ofp_version); } - ofp_print_group(s, gm->group_id, gm->type, &gm->buckets, &gm->props, - ofp_version, bucket_command, port_map, table_map); + ofputil_group_format(s, gm->group_id, gm->type, NULL, &gm->buckets, + &gm->props, ofp_version, bucket_command, + port_map, table_map); } enum ofperr diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 83609ec62b63..2054f1c056e5 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -383,6 +383,7 @@ static void upcall_unixctl_disable_ufid(struct unixctl_conn *, int argc, const char *argv[], void *aux); static void upcall_unixctl_enable_ufid(struct unixctl_conn *, int argc, const char *argv[], void *aux); + static void upcall_unixctl_set_flow_limit(struct unixctl_conn *conn, int argc, const char *argv[], void *aux); static void upcall_unixctl_dump_wait(struct unixctl_conn *conn, int argc, @@ -394,6 +395,9 @@ static void upcall_unixctl_pause(struct unixctl_conn *conn, int argc, static void upcall_unixctl_resume(struct unixctl_conn *conn, int argc, const char *argv[], void *aux); +static void upcall_unixctl_ofproto_detrace(struct unixctl_conn *, int argc, + const char *argv[], void *aux); + static struct udpif_key *ukey_create_from_upcall(struct upcall *, struct flow_wildcards *); static int ukey_create_from_dpif_flow(const struct udpif *, @@ -470,6 +474,8 @@ udpif_init(void) upcall_unixctl_pause, NULL); unixctl_command_register("revalidator/resume", NULL, 0, 0, upcall_unixctl_resume, NULL); + unixctl_command_register("ofproto/detrace", "UFID PMD-ID", 1, 2, + upcall_unixctl_ofproto_detrace, NULL); ovsthread_once_done(&once); } } @@ -3310,6 +3316,51 @@ upcall_unixctl_resume(struct unixctl_conn *conn, int argc OVS_UNUSED, unixctl_command_reply(conn, ""); } +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]; + ovs_u128 ufid; + + if (odp_ufid_from_string(key_s, &ufid) <= 0) { + unixctl_command_reply_error(conn, "failed to parse ufid"); + return; + } + + if (argc == 3) { + const char *pmd_str = argv[2]; + if (!ovs_scan(pmd_str, "pmd=%d", &pmd_id)) { + unixctl_command_reply_error(conn, + "Invalid pmd argument format. " + "Expecting 'pmd=PMD-ID'"); + return; + } + } + + struct ds ds = DS_EMPTY_INITIALIZER; + struct udpif *udpif; + + LIST_FOR_EACH (udpif, list_node, &all_udpifs) { + struct udpif_key *ukey = ukey_lookup(udpif, &ufid, pmd_id); + if (!ukey) { + continue; + } + + ovs_mutex_lock(&ukey->mutex); + /* It only makes sense to format rules for ukeys that are (still) + * in use. */ + if ((ukey->state == UKEY_VISIBLE || ukey->state == UKEY_OPERATIONAL) + && ukey->xcache) { + xlate_xcache_format(&ds, ukey->xcache); + } + ovs_mutex_unlock(&ukey->mutex); + } + unixctl_command_reply(conn, ds_cstr(&ds)); + ds_destroy(&ds); +} + /* Flows are sorted in the following order: * netdev, flow state (offloaded/kernel path), flow_pps_rate. diff --git a/ofproto/ofproto-dpif-xlate-cache.c b/ofproto/ofproto-dpif-xlate-cache.c index 2e1fcb3a6f7a..e987ca8a4636 100644 --- a/ofproto/ofproto-dpif-xlate-cache.c +++ b/ofproto/ofproto-dpif-xlate-cache.c @@ -301,3 +301,32 @@ xlate_cache_steal_entries(struct xlate_cache *dst, struct xlate_cache *src) memcpy(p, src_entries->data, src_entries->size); ofpbuf_clear(src_entries); } + +void +xlate_xcache_format(struct ds *s, const struct xlate_cache *xcache) +{ + struct ofpbuf entries = xcache->entries; + struct xc_entry *entry; + + XC_ENTRY_FOR_EACH (entry, &entries) { + switch (entry->type) { + case XC_RULE: + ofproto_rule_stats_ds(s, &entry->rule->up, true); + break; + case XC_GROUP: + group_dpif_format(s, entry->group.group, entry->group.bucket); + break; + case XC_TABLE: + case XC_BOND: + case XC_NETDEV: + case XC_NETFLOW: + case XC_MIRROR: + case XC_LEARN: + case XC_NORMAL: + case XC_FIN_TIMEOUT: + case XC_TNL_NEIGH: + case XC_TUNNEL_HEADER: + break; + } + } +} diff --git a/ofproto/ofproto-dpif-xlate-cache.h b/ofproto/ofproto-dpif-xlate-cache.h index 0fc6d2ea60ce..e701734d796a 100644 --- a/ofproto/ofproto-dpif-xlate-cache.h +++ b/ofproto/ofproto-dpif-xlate-cache.h @@ -151,4 +151,6 @@ void xlate_cache_uninit(struct xlate_cache *); void xlate_cache_delete(struct xlate_cache *); void xlate_cache_steal_entries(struct xlate_cache *, struct xlate_cache *); +void xlate_xcache_format(struct ds *, const struct xlate_cache *); + #endif /* ofproto-dpif-xlate-cache.h */ diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 87dfb0043dd9..ff94532dbbbb 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -5408,6 +5408,17 @@ group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id, version, take_ref); return ofgroup ? group_dpif_cast(ofgroup) : NULL; } + +void +group_dpif_format(struct ds *ds, struct group_dpif *group, + struct ofputil_bucket *bucket) +{ + struct ofgroup *ofg = &group->up; + + ofputil_group_format(ds, ofg->group_id, ofg->type, + bucket, &ofg->buckets, &ofg->props, OFP15_VERSION, + false, NULL, NULL); +} /* Sends 'packet' out 'ofport'. If 'port' is a tunnel and that tunnel type * supports a notion of an OAM flag, sets it if 'oam' is true. diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index d33f73df8aed..acec2c08a3af 100644 --- a/ofproto/ofproto-dpif.h +++ b/ofproto/ofproto-dpif.h @@ -151,6 +151,8 @@ void group_dpif_credit_stats(struct group_dpif *, struct group_dpif *group_dpif_lookup(struct ofproto_dpif *, uint32_t group_id, ovs_version_t version, bool take_ref); +void group_dpif_format(struct ds *, struct group_dpif *, + struct ofputil_bucket *); /* Backers. diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 83c509fcf804..4f42cc02df64 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -450,6 +450,8 @@ void ofproto_rule_ref(struct rule *); bool ofproto_rule_try_ref(struct rule *); void ofproto_rule_unref(struct rule *); +void ofproto_rule_stats_ds(struct ds *, struct rule *, bool offload_stats); + static inline const struct rule_actions * rule_get_actions(const struct rule *); static inline bool rule_is_table_miss(const struct rule *); static inline bool rule_is_hidden(const struct rule *); diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 21c6a1d8257d..8ad5712f5fc6 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -4844,9 +4844,9 @@ handle_flow_stats_request(struct ofconn *ofconn, return 0; } -static void -flow_stats_ds(struct ofproto *ofproto, struct rule *rule, struct ds *results, - bool offload_stats) +void +ofproto_rule_stats_ds(struct ds *results, struct rule *rule, + bool offload_stats) { struct pkt_stats stats; const struct rule_actions *actions; @@ -4875,7 +4875,8 @@ flow_stats_ds(struct ofproto *ofproto, struct rule *rule, struct ds *results, ds_put_format(results, "n_offload_bytes=%"PRIu64", ", stats.n_offload_bytes); } - cls_rule_format(&rule->cr, ofproto_get_tun_tab(ofproto), NULL, results); + cls_rule_format(&rule->cr, ofproto_get_tun_tab(rule->ofproto), NULL, + results); ds_put_char(results, ','); ds_put_cstr(results, "actions="); @@ -4897,7 +4898,7 @@ ofproto_get_all_flows(struct ofproto *p, struct ds *results, struct rule *rule; CLS_FOR_EACH (rule, cr, &table->cls) { - flow_stats_ds(p, rule, results, offload_stats); + ofproto_rule_stats_ds(results, rule, offload_stats); } } } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 30ef0468c8d1..7e06c6e3503a 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -12176,3 +12176,65 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap | wc -l`]) OVS_VSWITCHD_STOP AT_CLEANUP + +AT_SETUP([ofproto-dpif - Dump OF rules corresponding to UFID]) +OVS_VSWITCHD_START( +[add-port br0 p1 \ + -- set bridge br0 datapath-type=dummy \ + -- set interface p1 ofport_request=1 type=dummy \ + -- add-port br0 p2 \ + -- set interface p2 ofport_request=2 type=dummy\ + -- add-port br0 p3 \ + -- set interface p3 ofport_request=3 type=dummy +], [], [], [DUMMY_NUMA]) + +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)']) +ovs-appctl revalidator/wait +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) +]) + +ovs-appctl revalidator/resume + +OVS_VSWITCHD_STOP +AT_CLEANUP diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index c22fb3c79c3f..c27d96177b64 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -9,7 +9,9 @@ s/ duration=[0-9.]*s,// s/ cookie=0x0,// s/ table=0,// s/ n_packets=0,// +s/ n_offload_packets=0,// s/ n_bytes=0,// +s/ n_offload_bytes=0,// s/ idle_age=[0-9]*,// s/ hard_age=[0-9]*,// s/dp_hash=0x[0-9a-f]*\//dp_hash=0x0\// @@ -130,7 +132,7 @@ strip_used () { # Removes all 'duration=...' to make output easier to compare. strip_duration () { - sed 's/duration=[[0-9]]*\.[[0-9]]*s,//' + sed 's/duration=[[0-9.]]*s,//' } # Strips 'ufid:...' from output, to make it easier to compare. @@ -140,6 +142,10 @@ strip_ufid () { s/ufid:[[-0-9a-f]]* //' } +parse_ufid () { + grep -o 'ufid:[[-0-9a-f]]*' +} + # Strips packets: and bytes: from output strip_stats () { sed 's/packets:[[0-9]]*/packets:0/ @@ -169,6 +175,12 @@ strip_recirc() { s/recirc_id=[[x0-9]]*/recirc_id=<recirc>/ s/recirc([[x0-9]]*)/recirc(<recirc>)/' } + +# Strips dp_hash from output. +strip_dp_hash() { + sed 's/dp_hash([[0-9a-fx/]]*),//' +} + m4_divert_pop([PREPARE_TESTS]) m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m'])
It improves the debugging experience if we can easily get a list of OpenFlow rules and groups that contribute to the creation of a datapath flow. The suggested workflow is: a. dump datapath flows (along with UUIDs), this also prints the core IDs (PMD IDs) when applicable. $ ovs-appctl dpctl/dump-flows -m flow-dump from pmd on cpu core: 7 ufid:7460db8f..., recirc_id(0), .... b. dump related OpenFlow rules and groups: $ ovs-appctl ofproto/detrace ufid:7460db8f... pmd=7 cookie=0x12345678, table=0 priority=100,ip,in_port=2,nw_dst=10.0.0.2,actions=resubmit(,1) cookie=0x0, table=1 priority=200,actions=group:1 group_id=1,bucket=bucket_id:0,actions=ct(commit,table=2,nat(dst=20.0.0.2)) cookie=0x0, table=2 actions=output:1 The new command only shows rules and groups attached to ukeys that are in states UKEY_VISIBLE or UKEY_OPERATIONAL. That should be fine as all other ukeys should not be relevant for the use case presented above. This commit tries to mimic the output format of the ovs-ofctl dump-flows/dump-groups commands. Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- V5: - Addressed Ilya's comments: - I addressed all from here https://mail.openvswitch.org/pipermail/ovs-dev/2024-June/415144.html except the one about adding -m/-mm. It felt OK to always dump statistics. V4: - Addressed Mike's comment: - use predictable port IDs. V3: - Addressed Mike's comments: - use ds_put_char() - make the test less flaky V2: - Addressed Adrian's comments: - check return value of populate_xcache() - use flow_stats_ds() (renamed to ofproto_rule_stats_ds()) instead of custom printing - move ukey->state check to caller - handle case when group bucket is not available - update test case to cover the point above --- NEWS | 2 + include/openvswitch/ofp-group.h | 14 +++ lib/ofp-group.c | 131 +++++++++++++++++------------ ofproto/ofproto-dpif-upcall.c | 51 +++++++++++ ofproto/ofproto-dpif-xlate-cache.c | 29 +++++++ ofproto/ofproto-dpif-xlate-cache.h | 2 + ofproto/ofproto-dpif.c | 11 +++ ofproto/ofproto-dpif.h | 2 + ofproto/ofproto-provider.h | 2 + ofproto/ofproto.c | 11 +-- tests/ofproto-dpif.at | 62 ++++++++++++++ tests/ofproto-macros.at | 14 ++- 12 files changed, 271 insertions(+), 60 deletions(-)