On 6/27/24 22:13, Ilya Maximets wrote: > On 6/18/24 21:41, 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 upcall/dump-ufid-rules 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 > > Hi, Dumitru. Thanks for the patch! > > This is definitely an interesting debug interface. See some comments inline. >
Hi Ilya, Thanks for the review! I was trying to see if I can respin this before hard freeze but I'm a bit stuck with a couple of the comments below. Replied inline. > > As an idea, should we call it ofproto/detrace ? > Sure, I can rename it. >> >> 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. >> >> For ukeys that don't have an xcache populated yet, the command goes >> ahead and populates one. In theory this is creates a slight overhead as >> those ukeys might not need an xcache until they see traffic (and get >> revalidated) but in practice the overhead should be minimal. > > The flow dumped for the first time should always be revalidated, > even if it didn't see any more traffic. See the should_revalidate() > function. This means that there is only very narrow window between > the flow installation and the first revalidation, so the cache should > be availabel in all practical cases. > > Kepeing that in mind, I don't think that populating xcache from the > debug appctl is a good idea. We're holding the mutex for a long time > while effectively revalidating the ukey from the appctl and more > importantly we're revalidating it with side effects allowed. This > means we can learn some rules or update some other caches or even > emit some packets. We should not do that from the debug appctl. > > I suggest to avoid this and just return early if the cache is not > yet available. It should be available in most practical situations. > Makes sense, it's probably fine. >> >> This commit tries to mimic the output format of the ovs-ofctl >> dump-flows/dump-groups commands. For groups it actually uses >> ofputil_group/_bucket functions for formatting. For rules it uses >> flow_stats_ds() (the original function was exported and renamed to >> ofproto_rule_stats_ds()). >> >> Signed-off-by: Dumitru Ceara <[email protected]> >> --- >> 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 | 3 + >> include/openvswitch/ofp-group.h | 7 ++ >> lib/ofp-group.c | 110 +++++++++++++++++++------------- >> ofproto/ofproto-dpif-upcall.c | 85 ++++++++++++++++++++++++ >> ofproto/ofproto-dpif.c | 24 +++++++ >> ofproto/ofproto-dpif.h | 2 + >> ofproto/ofproto-provider.h | 1 + >> ofproto/ofproto.c | 85 ++++++++++++------------ >> tests/ofproto-dpif.at | 47 ++++++++++++++ >> tests/ofproto-macros.at | 9 ++- >> 10 files changed, 285 insertions(+), 88 deletions(-) >> >> diff --git a/NEWS b/NEWS >> index 5ae0108d552b..1bc085f97045 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -9,6 +9,9 @@ Post-v3.3.0 >> https://github.com/openvswitch/ovs.git >> - DPDK: >> * OVS validated with DPDK 23.11.1. >> + - ovs-appctl: >> + * Added 'upcall/dump-ufid-rules' to output the set of OpenFlow rules >> and >> + groups that contributed to the creation of a specific datapath flow. > > I'd move this to the top, alpabetically. I know that some entries > here are not in order, but it's better to keep them at least in > approximate order. > Ack. >> >> >> v3.3.0 - 16 Feb 2024 >> diff --git a/include/openvswitch/ofp-group.h >> b/include/openvswitch/ofp-group.h >> index cd7af0ebff9c..79fcb3a4c0d1 100644 >> --- a/include/openvswitch/ofp-group.h >> +++ b/include/openvswitch/ofp-group.h >> @@ -70,6 +70,11 @@ 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(const struct ofputil_bucket *, >> + enum ofp11_group_type, enum ofp_version, >> + const struct ofputil_port_map *, >> + const struct ofputil_table_map *, >> + struct ds *); > > Most of 'format' functions we have in OVS accept dynamic string > as a first argument. It's not an output parameter per se, it's > the argument that we're working on. > I didn't realize that the majority is the other way around. Should this be mentioned in the coding guidelines? In any case, I'll change it. >> >> static inline bool >> ofputil_bucket_has_liveness(const struct ofputil_bucket *bucket) >> @@ -88,6 +93,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_*. */ >> diff --git a/lib/ofp-group.c b/lib/ofp-group.c >> index 737f48047b10..0be453d15203 100644 >> --- a/lib/ofp-group.c >> +++ b/lib/ofp-group.c >> @@ -58,14 +58,16 @@ ofputil_group_from_string(const char *s, uint32_t >> *group_idp) >> return true; >> } >> >> -/* Appends to 's' a string representation of the OpenFlow group ID >> 'group_id'. >> - * Most groups' string representation is just the number, but for special >> - * groups, e.g. OFPG_ALL, it is the name, e.g. "ALL". */ >> +/* Appends to 's' a string representation of the OpenFlow group 'group_id'. >> + * Most groups' string representation is just 'group_id=' followed by the >> ID, > > s/ID/number/ > >> + * but for special groups, e.g. OFPG_ALL, the ID is replaced by the name, > > s/ID/number/ > > But, do we actually need this change at all? Seems unnecessary. > >> + * e.g. "ALL". */ >> void >> ofputil_format_group(uint32_t group_id, struct ds *s) >> { >> char name[MAX_GROUP_NAME_LEN + 1]; >> >> + ds_put_cstr(s, "group_id="); >> ofputil_group_to_string(group_id, name, sizeof name); >> ds_put_cstr(s, name); >> } >> @@ -297,7 +299,7 @@ ofputil_group_desc_request_format(struct ds *string, >> const struct ofp_header *oh) >> { >> uint32_t group_id = ofputil_decode_group_desc_request(oh); >> - ds_put_cstr(string, " group_id="); >> + ds_put_char(string, ' '); >> ofputil_format_group(group_id, string); >> >> return 0; >> @@ -585,7 +587,7 @@ ofputil_group_stats_request_format(struct ds *string, >> return error; >> } >> >> - ds_put_cstr(string, " group_id="); >> + ds_put_char(string, ' '); >> ofputil_format_group(group_id, string); >> return 0; >> } >> @@ -1526,6 +1528,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,6 +1840,37 @@ ofp_print_bucket_id(struct ds *s, const char *label, >> uint32_t bucket_id, >> ds_put_char(s, ','); >> } >> >> +void >> +ofputil_bucket_format(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 ds *s) >> +{ >> + 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); >> +} >> + >> static void >> ofp_print_group(struct ds *s, uint32_t group_id, uint8_t type, >> const struct ovs_list *p_buckets, >> @@ -1831,23 +1889,7 @@ 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) { >> return; >> @@ -1856,28 +1898,8 @@ ofp_print_group(struct ds *s, uint32_t group_id, >> uint8_t type, >> 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); >> - 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); >> + ofputil_bucket_format(bucket, type, ofp_version, >> + port_map, table_map, s); >> ds_put_char(s, ','); >> } >> >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >> index 83609ec62b63..e2fec413f461 100644 >> --- a/ofproto/ofproto-dpif-upcall.c >> +++ b/ofproto/ofproto-dpif-upcall.c >> @@ -383,6 +383,9 @@ 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_dump_ufid_rules(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, >> @@ -460,6 +463,8 @@ udpif_init(void) >> upcall_unixctl_disable_ufid, NULL); >> unixctl_command_register("upcall/enable-ufid", "", 0, 0, >> upcall_unixctl_enable_ufid, NULL); >> + unixctl_command_register("upcall/dump-ufid-rules", "UFID PMD-ID", >> 1, 2, >> + upcall_unixctl_dump_ufid_rules, NULL); >> unixctl_command_register("upcall/set-flow-limit", >> "flow-limit-number", >> 1, 1, upcall_unixctl_set_flow_limit, NULL); >> unixctl_command_register("revalidator/wait", "", 0, 0, >> @@ -2479,6 +2484,42 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key >> *ukey, >> return result; >> } >> >> +static void >> +ukey_xcache_format(struct udpif *udpif, struct udpif_key *ukey, struct ds >> *ds) >> + OVS_REQUIRES(ukey->mutex) >> +{ >> + if (!ukey->xcache) { >> + if (populate_xcache(udpif, ukey, ukey->stats.tcp_flags)) { >> + return; >> + } >> + } >> + >> + struct ofpbuf entries = ukey->xcache->entries; >> + struct xc_entry *entry; >> + >> + XC_ENTRY_FOR_EACH (entry, &entries) { >> + switch (entry->type) { >> + case XC_RULE: >> + ofproto_rule_stats_ds(&entry->rule->up, ds, true); >> + break; >> + case XC_GROUP: >> + group_dpif_format(entry->group.group, entry->group.bucket, ds); >> + 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; >> + } >> + } > > We should not access internals of xcache in this module. There should be > xlate_xcache_format() function in ofproto-dpif-xlate-cache.c instead. > Ack. >> +} >> + >> static void >> delete_op_init__(struct udpif *udpif, struct ukey_op *op, >> const struct dpif_flow *flow) >> @@ -3220,6 +3261,50 @@ upcall_unixctl_enable_ufid(struct unixctl_conn *conn, >> int argc OVS_UNUSED, >> "for supported datapaths"); >> } >> >> +static void >> +upcall_unixctl_dump_ufid_rules(struct unixctl_conn *conn, int argc, >> + const char *argv[], void *aux OVS_UNUSED) >> +{ >> + unsigned int pmd_id = PMD_ID_NULL; >> + 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_format(udpif, ukey, &ds); >> + } >> + ovs_mutex_unlock(&ukey->mutex); >> + } >> + unixctl_command_reply(conn, ds_cstr(&ds)); >> + ds_destroy(&ds); >> +} >> + >> /* Set the flow limit. >> * >> * This command is only needed for advanced debugging, so it's not >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index fcd7cd753ca4..4049c439e62d 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -5407,6 +5407,30 @@ 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 group_dpif *group, struct ofputil_bucket *bucket, >> + struct ds *ds) > > Move ds to the beginning. > Ack. >> +{ >> + struct ofgroup *ofg = &group->up; >> + >> + ofputil_format_group(ofg->group_id, ds); >> + ofputil_group_properties_format(&ofg->props, ds); >> + ds_put_char(ds, ','); >> + >> + if (bucket) { >> + ofputil_bucket_format(bucket, ofg->type, OFP15_VERSION, > > Hardcoding a version doesn't seem right. > The problem I'm facing is that I can't find a good way of retrieving the openflow version the group was created with. Do you have suggestions on how to do that? Or maybe I should just print the bucket id as integer and that's it. Wdyt? >> + NULL, NULL, ds); >> + } else { >> + LIST_FOR_EACH (bucket, list_node, &ofg->buckets) { >> + ofputil_bucket_format(bucket, ofg->type, OFP15_VERSION, >> + NULL, NULL, ds); >> + ds_put_char(ds, ','); >> + } >> + } >> + ds_chomp(ds, ','); >> + ds_put_char(ds, '\n'); >> +} > > The whole function is very similar to ofp_print_group(). Can we add > a 'bucket' argument to it and export as ofputil_group_format() ? > I could, but I'm still facing the same issue as above, at the call site I need to provide the OpenFlow version to format to. >> >> /* 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..7be1515c4636 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 group_dpif *, struct ofputil_bucket *, >> + struct ds *); >> >> >> /* Backers. >> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h >> index 83c509fcf804..e7309c71289b 100644 >> --- a/ofproto/ofproto-provider.h >> +++ b/ofproto/ofproto-provider.h >> @@ -449,6 +449,7 @@ struct rule { >> 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 rule *, struct ds *, bool offload_stats); > > ofproto-provider.h header is for ofproto.c to use functions exported > by different providers. This function should be exported via ofproto.h > instead. > OK, but 'struct rule' is defined in ofproto-provider.h so I'm not sure how to do this. >> >> static inline const struct rule_actions * rule_get_actions(const struct >> rule *); >> static inline bool rule_is_table_miss(const struct rule *); >> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c >> index 21c6a1d8257d..aeace33d0654 100644 >> --- a/ofproto/ofproto.c >> +++ b/ofproto/ofproto.c >> @@ -3086,6 +3086,48 @@ ofproto_rule_unref(struct rule *rule) >> } >> } >> >> +void >> +ofproto_rule_stats_ds(struct rule *rule, struct ds *results, >> + bool offload_stats) >> +{ >> + struct pkt_stats stats; >> + const struct rule_actions *actions; >> + long long int created, used; >> + >> + rule->ofproto->ofproto_class->rule_get_stats(rule, &stats, &used); >> + >> + ovs_mutex_lock(&rule->mutex); >> + actions = rule_get_actions(rule); >> + created = rule->created; >> + ovs_mutex_unlock(&rule->mutex); >> + >> + if (rule->flow_cookie != 0) { >> + ds_put_format(results, "cookie=0x%"PRIx64", ", >> + ntohll(rule->flow_cookie)); >> + } >> + if (rule->table_id != 0) { >> + ds_put_format(results, "table_id=%"PRIu8", ", rule->table_id); >> + } >> + ds_put_format(results, "duration=%llds, ", (time_msec() - created) / >> 1000); >> + ds_put_format(results, "n_packets=%"PRIu64", ", stats.n_packets); >> + ds_put_format(results, "n_bytes=%"PRIu64", ", stats.n_bytes); >> + if (offload_stats) { >> + ds_put_format(results, "n_offload_packets=%"PRIu64", ", >> + stats.n_offload_packets); >> + ds_put_format(results, "n_offload_bytes=%"PRIu64", ", >> + stats.n_offload_bytes); >> + } >> + cls_rule_format(&rule->cr, ofproto_get_tun_tab(rule->ofproto), NULL, >> + results); >> + ds_put_char(results, ','); >> + >> + ds_put_cstr(results, "actions="); >> + struct ofpact_format_params fp = { .s = results }; >> + ofpacts_format(actions->ofpacts, actions->ofpacts_len, &fp); >> + >> + ds_put_cstr(results, "\n"); >> +} > > This code move seems unnecessary. Especially since we don't > actually need to group this with ofproto_rule_unref and friends. > I'm not sure I follow, what code is unnecessary? I need to call something that formats a rule from ofproto-dpif-xlate-cache.c. That's why I turned the initially static flow_stats_ds() into a public ofproto_rule_stats_ds(). But maybe I'm misreading your comment.. >> + >> static void >> remove_rule_rcu__(struct rule *rule) >> OVS_REQUIRES(ofproto_mutex) >> @@ -4844,47 +4886,6 @@ 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) >> -{ >> - struct pkt_stats stats; >> - const struct rule_actions *actions; >> - long long int created, used; >> - >> - rule->ofproto->ofproto_class->rule_get_stats(rule, &stats, &used); >> - >> - ovs_mutex_lock(&rule->mutex); >> - actions = rule_get_actions(rule); >> - created = rule->created; >> - ovs_mutex_unlock(&rule->mutex); >> - >> - if (rule->flow_cookie != 0) { >> - ds_put_format(results, "cookie=0x%"PRIx64", ", >> - ntohll(rule->flow_cookie)); >> - } >> - if (rule->table_id != 0) { >> - ds_put_format(results, "table_id=%"PRIu8", ", rule->table_id); >> - } >> - ds_put_format(results, "duration=%llds, ", (time_msec() - created) / >> 1000); >> - ds_put_format(results, "n_packets=%"PRIu64", ", stats.n_packets); >> - ds_put_format(results, "n_bytes=%"PRIu64", ", stats.n_bytes); >> - if (offload_stats) { >> - ds_put_format(results, "n_offload_packets=%"PRIu64", ", >> - stats.n_offload_packets); >> - ds_put_format(results, "n_offload_bytes=%"PRIu64", ", >> - stats.n_offload_bytes); >> - } >> - cls_rule_format(&rule->cr, ofproto_get_tun_tab(ofproto), NULL, results); >> - ds_put_char(results, ','); >> - >> - ds_put_cstr(results, "actions="); >> - struct ofpact_format_params fp = { .s = results }; >> - ofpacts_format(actions->ofpacts, actions->ofpacts_len, &fp); >> - >> - ds_put_cstr(results, "\n"); >> -} >> - >> /* Adds a pretty-printed description of all flows to 'results', including >> * hidden flows (e.g., set up by in-band control). */ >> void >> @@ -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(rule, results, offload_stats); >> } >> } >> } >> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >> index 0b23fd6c5ea6..6cc2872b13fe 100644 >> --- a/tests/ofproto-dpif.at >> +++ b/tests/ofproto-dpif.at >> @@ -12136,3 +12136,50 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap >> | wc -l`]) >> >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> + >> +AT_SETUP([ofproto-dpif - dump-ufid-rules]) > > Give it a more human-readble name, e.g. 'Dump OF rules corresponding to UFID' > OK. >> +OVS_VSWITCHD_START( >> +[add-port br0 p1 \ >> + -- set bridge br0 datapath-type=dummy \ >> + -- set interface p1 type=dummy-pmd ofport_request=1 \ >> + -- add-port br0 p2 \ >> + -- set interface p2 type=dummy-pmd ofport_request=2 \ >> + -- add-port br0 p3 \ >> + -- set interface p3 type=dummy-pmd ofport_request=3 > > Do we need these to be PMD ports? You're not actually testing > the filtering in any meaningful way. > We don't, copy+paste, will fix it up. >> +], [], [], [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,actions=group:1 >> +table=2,actions=group:2 >> +table=3,actions=p2 >> +table=4,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)']) >> + >> +OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/dump-flows | sed 's/.*core: >> [[0-9]]*//' | ofctl_strip | sort], [ >> +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:never, actions:hash(l4(0)),recirc(0x1) >> +recirc_id(0x1),dp_hash(0x0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), >> packets:0, bytes:0, used:never, >> 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:never, actions:2,3]) >> + >> +ufids=$(ovs-appctl dpctl/dump-flows -m | match_ufid) >> +AT_CHECK([for ufid in $ufids; do ovs-appctl upcall/dump-ufid-rules $ufid >> pmd=0; done | ofctl_strip | sort], [0], [dnl >> +cookie=0x12345678, n_packets=1, n_bytes=118, n_offload_packets=0, >> n_offload_bytes=0, >> priority=100,ip,in_port=1,nw_dst=10.0.0.2,actions=resubmit(,1) >> +group_id=1,selection_method=dp_hash,bucket=bucket_id:0,weight:100,actions=ct(commit,table=2,nat(dst=20.0.0.2)) >> +group_id=2,bucket=bucket_id:0,actions=resubmit(,3),bucket=bucket_id:1,actions=resubmit(,4) >> +table_id=1, n_packets=1, n_bytes=118, n_offload_packets=0, >> n_offload_bytes=0, priority=200,actions=group:1 >> +table_id=2, n_packets=1, n_bytes=118, n_offload_packets=0, >> n_offload_bytes=0, ,actions=group:2 >> +table_id=3, n_packets=1, n_bytes=118, n_offload_packets=0, >> n_offload_bytes=0, ,actions=output:2 >> +table_id=4, n_packets=1, n_bytes=118, n_offload_packets=0, >> n_offload_bytes=0, ,actions=output:3 > > There is something strange with the ', ,' going on here. > Also, maybe we can just strip out all the counters from the > output to make it shorter? Maybe don't dump stats at all? I think stats are good for debugging. > Or hide them behind a -m argument? Offload stats can be > hidded behind -mm. > OK, I'll see what I can do for that. > And do we need to sort these? Cache entries are stored in > ofp buffer and the cache is populated in the order in which > we hit OF rules. So, it should be stable, right? > > The flow dump can be in a random order, but you may sort that > instead, or even just get these flows one by one based on > their recirculation ID, i.e. dump with a recirc id filter, > you should get exacly one flow, trace it. Then dump the > ID 0x1, trace it and same for 0x2. What do you think? > Sounds good. > >> +]) >> + >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at >> index c22fb3c79c3f..bf442025bcd5 100644 >> --- a/tests/ofproto-macros.at >> +++ b/tests/ofproto-macros.at >> @@ -1,7 +1,7 @@ >> m4_divert_push([PREPARE_TESTS]) >> [ >> -# Strips out uninteresting parts of ovs-ofctl output, as well as parts >> -# that vary from one run to another. >> +# Strips out uninteresting parts of flow dump outputs (ofctl/dpctl), as well >> +# as parts that vary from one run to another. >> ofctl_strip () { >> sed ' >> s/ (xid=0x[0-9a-fA-F]*)// >> @@ -13,6 +13,7 @@ s/ n_bytes=0,// >> s/ idle_age=[0-9]*,// >> s/ hard_age=[0-9]*,// >> s/dp_hash=0x[0-9a-f]*\//dp_hash=0x0\// >> +s/dp_hash([0-9a-fx/]*)/dp_hash(0x0)/ > > It's an ofctl_strip, not dpctl_strip. > I'll add a new dpctl_strip. >> s/recirc_id=0x[0-9a-f]*,/recirc_id=0x0,/ >> >> s/[0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]T[0-9][0-9]:[0-9][0-9]:[0-9][0-9]Z|// >> s/dir\/[0-9]*\/br0.mgmt/dir\/XXXX\/br0.mgmt/ >> @@ -140,6 +141,10 @@ strip_ufid () { >> s/ufid:[[-0-9a-f]]* //' >> } >> >> +match_ufid () { > > 'match' is a strange word. Maybe 'get' or 'parse' ? > Ack. >> + grep -oE 'ufid:[[-0-9a-f]]+' | sort -u > > Plus sign is not portable, it will fail tests on FreeSBD. > And is -E actually needed? > Maybe not, I'll try to fix this. >> +} >> + >> # Strips packets: and bytes: from output >> strip_stats () { >> sed 's/packets:[[0-9]]*/packets:0/ > > Best regards, Ilya Maximets. > Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
