On Thu, Jun 27, 2024 at 4:13 PM Ilya Maximets <[email protected]> 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.
>
>
> As an idea, should we call it ofproto/detrace ?
>
> >
> > 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.
>
> >
> > 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.
>
> >
> >
> > 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.
>
> >
> > 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.
>
> > +}
> > +
> > 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.
>
> > +{
> > + 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.
>
> > + 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() ?
>
> >
> > /* 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.
>
> >
> > 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.
>
> > +
> > 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'
>
> > +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.
>
> > +], [], [], [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?
> Or hide them behind a -m argument? Offload stats can be
> hidded behind -mm.
>
> 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?
>
>
> > +])
> > +
> > +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.
>
> > 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' ?
>
> > + grep -oE 'ufid:[[-0-9a-f]]+' | sort -u
>
> Plus sign is not portable, it will fail tests on FreeSBD.
> And is -E actually needed?
Fwiw, grep on a freebsd 14 vm supports + with -E, and the freebsd
egrep man page dated 1997 mentions support for +.
Cheers,
M
>
> > +}
> > +
> > # Strips packets: and bytes: from output
> > strip_stats () {
> > sed 's/packets:[[0-9]]*/packets:0/
>
> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
