On 7/11/24 13:29, Dumitru Ceara wrote:
> 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 <dce...@redhat.com>
>>> ---
>>> 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.

It's technically against the coding style, i.e. the output params
should go last, but we do that in most cases, but we also do the
opposite in some libraries, so it's a weird thing.  Not sure how
to document.

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

It's not very important, so let's keep the OFP15_VERSION.

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

I suppose, since the xcache thing is moving to a proper
ofproto-dpif-xlate-cache module, this fucntion should be
exported via ofproto-dpif.h and accept struct dpif_rule.

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

Not the 'code is unnecessary' the 'code move' is unnecessary.
i.e. keep the function where it was, don't move it.

> 
>>> +
>>>  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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to