[ovs-dev] [PATCH 2/2] dpctl: Expand the flow dump type filter

Simon Horman simon.horman at netronome.com
Thu Sep 13 14:58:13 UTC 2018


On Wed, Sep 12, 2018 at 09:27:37AM +0000, Gavi Teitz wrote:
> On Thursday, August 30, 2018 at 14:06, Simon Horman wrote: 
> >On Fri, Aug 10, 2018 at 11:30:08AM +0300, Gavi Teitz wrote:
> >> Added new types to the flow dump filter, and allowed multiple filter 
> >> types to be passed at once, as a comma separated list. The new types 
> >> added are:
> >>  * tc - specifies flows handled by the tc dp
> >>  * non-offloaded - specifies flows not offloaded to the HW
> >>  * all - specifies flows of all types
> >> 
> >> The type list is now fully parsed by the dpctl, and a new struct was 
> >> added to dpif which enables dpctl to define which types of dumps to 
> >> provide, rather than passing the type string and having dpif parse it.
> >> 
> >> Signed-off-by: Gavi Teitz <gavi at mellanox.com>
> >> Acked-by: Roi Dayan <roid at mellanox.com>

Thanks for the clarification below, I have applied this series to master.

> >> ---
> >>  lib/dpctl.c         |  112 +++++++++++++++++++++++++++++++++++++++-----------
> >>  lib/dpctl.man       |   14 +++++--
> >>  lib/dpif-netdev.c   |    2 +-
> >>  lib/dpif-netlink.c  |   45 +++++++-------------
> >>  lib/dpif-provider.h |    8 ++-
> >>  lib/dpif.c          |    5 +-
> >>  lib/dpif.h          |    7 +++-
> >>  7 files changed, 128 insertions(+), 65 deletions(-)
> >> 
> >> diff --git a/lib/dpctl.c b/lib/dpctl.c index c600eeb..995db29 100644
> >> --- a/lib/dpctl.c
> >> +++ b/lib/dpctl.c
> >> @@ -792,18 +792,85 @@ format_dpif_flow(struct ds *ds, const struct dpif_flow *f, struct hmap *ports,
> >>      format_odp_actions(ds, f->actions, f->actions_len, ports);  }
> >>  
> >> -static char *supported_dump_types[] = {
> >> -    "offloaded",
> >> -    "ovs",
> >> +struct dump_types {
> >> +    bool ovs;
> >> +    bool tc;
> >> +    bool offloaded;
> >> +    bool non_offloaded;
> >>  };
> >
> >I am curious to know why a structure with bool fields is used as opposed to an integer bitmask.
> >
> 
> I find that explicitly naming boolean fields makes code clearer at all
> stages, and less prone to mistakes.
> 
> >>  
> >> +static void
> >> +enable_all_dump_types(struct dump_types *dump_types) {
> >> +    dump_types->ovs = true;
> >> +    dump_types->tc = true;
> >> +    dump_types->offloaded = true;
> >> +    dump_types->non_offloaded = true; }
> >> +
> >> +static int
> >> +populate_dump_types(char *types_list, struct dump_types *dump_types,
> >> +                    struct dpctl_params *dpctl_p) {
> >> +    if (!types_list) {
> >> +        enable_all_dump_types(dump_types);
> >> +        return 0;
> >> +    }
> >> +
> >> +    char *current_type;
> >> +
> >> +    while (types_list && types_list[0] != '\0') {
> >> +        current_type = types_list;
> >> +        size_t type_len = strcspn(current_type, ",");
> >> +
> >> +        types_list += type_len + (types_list[type_len] != '\0');
> >> +        current_type[type_len] = '\0';
> >> +
> >> +        if (!strcmp(current_type, "ovs")) {
> >> +            dump_types->ovs = true;
> >> +        } else if (!strcmp(current_type, "tc")) {
> >> +            dump_types->tc = true;
> >> +        } else if (!strcmp(current_type, "offloaded")) {
> >> +            dump_types->offloaded = true;
> >> +        } else if (!strcmp(current_type, "non-offloaded")) {
> >> +            dump_types->non_offloaded = true;
> >> +        } else if (!strcmp(current_type, "all")) {
> >> +            enable_all_dump_types(dump_types);
> >> +        } else {
> >> +            dpctl_error(dpctl_p, EINVAL, "Failed to parse type (%s)",
> >> +                        current_type);
> >> +            return EINVAL;
> >> +        }
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >> +static void
> >> +determine_dpif_flow_dump_types(struct dump_types *dump_types,
> >> +                               struct dpif_flow_dump_types 
> >> +*dpif_dump_types) {
> >> +    dpif_dump_types->ovs_flows = dump_types->ovs || dump_types->non_offloaded;
> >> +    dpif_dump_types->netdev_flows = dump_types->tc || dump_types->offloaded
> >> +                                    || dump_types->non_offloaded; }
> >> +
> >>  static bool
> >> -flow_passes_type_filter(const struct dpif_flow *f, char *type)
> >> +flow_passes_type_filter(const struct dpif_flow *f,
> >> +                        struct dump_types *dump_types)
> >>  {
> >> -    if (!strcmp(type, "offloaded")) {
> >> -        return f->attrs.offloaded;
> >> +    if (dump_types->ovs && !strcmp(f->attrs.dp_layer, "ovs")) {
> >> +        return true;
> >> +    }
> >> +    if (dump_types->tc && !strcmp(f->attrs.dp_layer, "tc")) {
> >> +        return true;
> >> +    }
> >> +    if (dump_types->offloaded && f->attrs.offloaded) {
> >> +        return true;
> >> +    }
> >> +    if (dump_types->non_offloaded && !(f->attrs.offloaded)) {
> >> +        return true;
> >>      }
> >> -    return true;
> >> +    return false;
> >>  }
> >>  
> >>  static struct hmap *
> >> @@ -843,9 +910,11 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
> >>      struct ds ds;
> >>  
> >>      char *filter = NULL;
> >> -    char *type = NULL;
> >>      struct flow flow_filter;
> >>      struct flow_wildcards wc_filter;
> >> +    char *types_list = NULL;
> >> +    struct dump_types dump_types;
> >> +    struct dpif_flow_dump_types dpif_dump_types;
> >>  
> >>      struct dpif_flow_dump_thread *flow_dump_thread;
> >>      struct dpif_flow_dump *flow_dump; @@ -858,8 +927,8 @@ 
> >> dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
> >>          lastargc = argc;
> >>          if (!strncmp(argv[argc - 1], "filter=", 7) && !filter) {
> >>              filter = xstrdup(argv[--argc] + 7);
> >> -        } else if (!strncmp(argv[argc - 1], "type=", 5) && !type) {
> >> -            type = xstrdup(argv[--argc] + 5);
> >> +        } else if (!strncmp(argv[argc - 1], "type=", 5) && !types_list) {
> >> +            types_list = xstrdup(argv[--argc] + 5);
> >>          }
> >>      }
> >>  
> >> @@ -892,19 +961,12 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
> >>          }
> >>      }
> >>  
> >> -    if (type) {
> >> -        error = EINVAL;
> >> -        for (int i = 0; i < ARRAY_SIZE(supported_dump_types); i++) {
> >> -            if (!strcmp(supported_dump_types[i], type)) {
> >> -                error = 0;
> >> -                break;
> >> -            }
> >> -        }
> >> -        if (error) {
> >> -            dpctl_error(dpctl_p, error, "Failed to parse type (%s)", type);
> >> -            goto out_free;
> >> -        }
> >> +    memset(&dump_types, 0, sizeof dump_types);
> >> +    error = populate_dump_types(types_list, &dump_types, dpctl_p);
> >> +    if (error) {
> >> +        goto out_free;
> >>      }
> >> +    determine_dpif_flow_dump_types(&dump_types, &dpif_dump_types);
> >>  
> >>      /* Make sure that these values are different. PMD_ID_NULL means that the
> >>       * pmd is unspecified (e.g. because the datapath doesn't have 
> >> different @@ -914,7 +976,7 @@ dpctl_dump_flows(int argc, const char 
> >> *argv[], struct dpctl_params *dpctl_p)
> >>  
> >>      ds_init(&ds);
> >>      memset(&f, 0, sizeof f);
> >> -    flow_dump = dpif_flow_dump_create(dpif, false, (type ? type : "dpctl"));
> >> +    flow_dump = dpif_flow_dump_create(dpif, false, &dpif_dump_types);
> >>      flow_dump_thread = dpif_flow_dump_thread_create(flow_dump);
> >>      while (dpif_flow_dump_next(flow_dump_thread, &f, 1)) {
> >>          if (filter) {
> >> @@ -950,7 +1012,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
> >>              }
> >>              pmd_id = f.pmd_id;
> >>          }
> >> -        if (!type || flow_passes_type_filter(&f, type)) {
> >> +        if (flow_passes_type_filter(&f, &dump_types)) {
> >>              format_dpif_flow(&ds, &f, portno_names, dpctl_p);
> >>              dpctl_print(dpctl_p, "%s\n", ds_cstr(&ds));
> >>          }
> >> @@ -968,7 +1030,7 @@ out_dpifclose:
> >>      dpif_close(dpif);
> >>  out_free:
> >>      free(filter);
> >> -    free(type);
> >> +    free(types_list);
> >>      return error;
> >>  }
> >>  
> >> diff --git a/lib/dpctl.man b/lib/dpctl.man index 5d987e6..e83546b 
> >> 100644
> >> --- a/lib/dpctl.man
> >> +++ b/lib/dpctl.man
> >> @@ -118,10 +118,16 @@ The \fIfilter\fR is also useful to match 
> >> wildcarded fields in the datapath  flow. As an example, 
> >> \fBfilter='tcp,tp_src=100'\fR will match the  datapath flow containing '\fBtcp(src=80/0xff00,dst=8080/0xff)\fR'.
> >>  .IP
> >> -If \fBtype=\fItype\fR is specified, only displays flows of a specific type.
> >> -\fItype\fR can be \fBoffloaded\fR to display only rules offloaded to 
> >> the HW -or \fBovs\fR to display only rules from the OVS tables.
> >> -By default all rules are displayed.
> >> +If \fBtype=\fItype\fR is specified, only displays flows of the specified types.
> >> +\fItype\fR is a comma separated list, which can contain any of the following:
> >> +.
> >> +   \fBovs\fR - displays flows handled in the ovs dp
> >> +   \fBtc\fR - displays flows handled in the tc dp
> >> +   \fBoffloaded\fR - displays flows offloaded to the HW
> >> +   \fBnon-offloaded\fR - displays flows not offloaded to the HW
> >> +   \fBall\fR - displays all the types of flows .IP By default all the 
> >> +types of flows are displayed.
> >>  .
> >>  .IP "\*(DX\fBadd\-flow\fR [\fIdp\fR] \fIflow actions\fR"
> >>  .TP
> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 
> >> 4b7e231..6d52db4 100644
> >> --- a/lib/dpif-netdev.c
> >> +++ b/lib/dpif-netdev.c
> >> @@ -3484,7 +3484,7 @@ dpif_netdev_flow_dump_cast(struct dpif_flow_dump 
> >> *dump)
> >>  
> >>  static struct dpif_flow_dump *
> >>  dpif_netdev_flow_dump_create(const struct dpif *dpif_, bool terse,
> >> -                             char *type OVS_UNUSED)
> >> +                             struct dpif_flow_dump_types *types 
> >> + OVS_UNUSED)
> >>  {
> >>      struct dpif_netdev_flow_dump *dump;
> >>  
> >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 
> >> f669b11..0114f7b 100644
> >> --- a/lib/dpif-netlink.c
> >> +++ b/lib/dpif-netlink.c
> >> @@ -1463,16 +1463,6 @@ dpif_netlink_init_flow_del(struct dpif_netlink *dpif,
> >>                                   del->ufid, del->terse, request);  }
> >>  
> >> -enum {
> >> -    DUMP_OVS_FLOWS_BIT    = 0,
> >> -    DUMP_NETDEV_FLOWS_BIT = 1,
> >> -};
> >> -
> >> -enum {
> >> -    DUMP_OVS_FLOWS    = (1 << DUMP_OVS_FLOWS_BIT),
> >> -    DUMP_NETDEV_FLOWS = (1 << DUMP_NETDEV_FLOWS_BIT),
> >> -};
> >> -
> >>  struct dpif_netlink_flow_dump {
> >>      struct dpif_flow_dump up;
> >>      struct nl_dump nl_dump;
> >> @@ -1481,7 +1471,7 @@ struct dpif_netlink_flow_dump {
> >>      int netdev_dumps_num;                    /* Number of netdev_flow_dumps */
> >>      struct ovs_mutex netdev_lock;            /* Guards the following. */
> >>      int netdev_current_dump OVS_GUARDED;     /* Shared current dump */
> >> -    int type;                                /* Type of dump */
> >> +    struct dpif_flow_dump_types types;       /* Type of dump */
> >>  };
> >>  
> >>  static struct dpif_netlink_flow_dump * @@ -1496,7 +1486,7 @@ 
> >> start_netdev_dump(const struct dpif *dpif_,  {
> >>      ovs_mutex_init(&dump->netdev_lock);
> >>  
> >> -    if (!(dump->type & DUMP_NETDEV_FLOWS)) {
> >> +    if (!(dump->types.netdev_flows)) {
> >>          dump->netdev_dumps_num = 0;
> >>          dump->netdev_dumps = NULL;
> >>          return;
> >> @@ -1510,24 +1500,21 @@ start_netdev_dump(const struct dpif *dpif_,
> >>      ovs_mutex_unlock(&dump->netdev_lock);
> >>  }
> >>  
> >> -static int
> >> -dpif_netlink_get_dump_type(char *str) {
> >> -    int type = 0;
> >> -
> >> -    if (!str || !strcmp(str, "ovs") || !strcmp(str, "dpctl")) {
> >> -        type |= DUMP_OVS_FLOWS;
> >> -    }
> >> -    if ((netdev_is_flow_api_enabled() && !str)
> >> -        || (str && (!strcmp(str, "offloaded") || !strcmp(str, "dpctl")))) {
> >> -        type |= DUMP_NETDEV_FLOWS;
> >> +static void
> >> +dpif_netlink_populate_flow_dump_types(struct dpif_netlink_flow_dump *dump,
> >> +                                      struct dpif_flow_dump_types 
> >> +*types) {
> >> +    if (!types) {
> >> +        dump->types.ovs_flows = true;
> >> +        dump->types.netdev_flows = true;
> >> +    } else {
> >> +        memcpy(&dump->types, types, sizeof *types);
> >>      }
> >> -
> >> -    return type;
> >>  }
> >>  
> >>  static struct dpif_flow_dump *
> >>  dpif_netlink_flow_dump_create(const struct dpif *dpif_, bool terse,
> >> -                              char *type)
> >> +                              struct dpif_flow_dump_types *types)
> >>  {
> >>      const struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
> >>      struct dpif_netlink_flow_dump *dump; @@ -1537,9 +1524,9 @@ 
> >> dpif_netlink_flow_dump_create(const struct dpif *dpif_, bool terse,
> >>      dump = xmalloc(sizeof *dump);
> >>      dpif_flow_dump_init(&dump->up, dpif_);
> >>  
> >> -    dump->type = dpif_netlink_get_dump_type(type);
> >> +    dpif_netlink_populate_flow_dump_types(dump, types);
> >>  
> >> -    if (dump->type & DUMP_OVS_FLOWS) {
> >> +    if (dump->types.ovs_flows) {
> >>          dpif_netlink_flow_init(&request);
> >>          request.cmd = OVS_FLOW_CMD_GET;
> >>          request.dp_ifindex = dpif->dp_ifindex; @@ -1566,7 +1553,7 @@ 
> >> dpif_netlink_flow_dump_destroy(struct dpif_flow_dump *dump_)
> >>      unsigned int nl_status = 0;
> >>      int dump_status;
> >>  
> >> -    if (dump->type & DUMP_OVS_FLOWS) {
> >> +    if (dump->types.ovs_flows) {
> >>          nl_status = nl_dump_done(&dump->nl_dump);
> >>      }
> >>  
> >> @@ -1802,7 +1789,7 @@ dpif_netlink_flow_dump_next(struct dpif_flow_dump_thread *thread_,
> >>          }
> >>      }
> >>  
> >> -    if (!(dump->type & DUMP_OVS_FLOWS)) {
> >> +    if (!(dump->types.ovs_flows)) {
> >>          return n_flows;
> >>      }
> >>  
> >> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h index 
> >> 62b3598..5c8b64c 100644
> >> --- a/lib/dpif-provider.h
> >> +++ b/lib/dpif-provider.h
> >> @@ -284,9 +284,11 @@ struct dpif_class {
> >>       * If 'terse' is true, then only UID and statistics will
> >>       * be returned in the dump. Otherwise, all fields will be returned.
> >>       *
> >> -     * If 'type' isn't null, dumps only the flows of the given type. */
> >> -    struct dpif_flow_dump *(*flow_dump_create)(const struct dpif *dpif,
> >> -                                               bool terse, char *type);
> >> +     * If 'types' isn't null, dumps only the flows of the passed types. */
> >> +    struct dpif_flow_dump *(*flow_dump_create)(
> >> +        const struct dpif *dpif,
> >> +        bool terse,
> >> +        struct dpif_flow_dump_types *types);
> >
> >I don't think the coding style change included in the above is desired.
> >
> 
> This was done due to the line length constraint, and based on the style
> used three lines down for flow_dump_thread_create().
> 
> >>      int (*flow_dump_destroy)(struct dpif_flow_dump *dump);
> >>  
> >>      struct dpif_flow_dump_thread *(*flow_dump_thread_create)( diff 
> >> --git a/lib/dpif.c b/lib/dpif.c index c267bcf..a047e3f 100644
> >> --- a/lib/dpif.c
> >> +++ b/lib/dpif.c
> >> @@ -1080,9 +1080,10 @@ dpif_flow_del(struct dpif *dpif,
> >>   * This function always successfully returns a dpif_flow_dump.  Error
> >>   * reporting is deferred to dpif_flow_dump_destroy(). */  struct 
> >> dpif_flow_dump * -dpif_flow_dump_create(const struct dpif *dpif, bool 
> >> terse, char *type)
> >> +dpif_flow_dump_create(const struct dpif *dpif, bool terse,
> >> +                      struct dpif_flow_dump_types *types)
> >>  {
> >> -    return dpif->dpif_class->flow_dump_create(dpif, terse, type);
> >> +    return dpif->dpif_class->flow_dump_create(dpif, terse, types);
> >>  }
> >>  
> >>  /* Destroys 'dump', which must have been created with dpif_flow_dump_create().
> >> diff --git a/lib/dpif.h b/lib/dpif.h
> >> index 33d2d0b..457cddc 100644
> >> --- a/lib/dpif.h
> >> +++ b/lib/dpif.h
> >> @@ -512,6 +512,11 @@ struct dpif_flow_attrs {
> >>      const char *dp_layer;   /* DP layer the flow is handled in. */
> >>  };
> >>  
> >> +struct dpif_flow_dump_types {
> >> +    bool ovs_flows;
> >> +    bool netdev_flows;
> >> +};
> >> +
> >>  void dpif_flow_stats_extract(const struct flow *, const struct dp_packet *packet,
> >>                               long long int used, struct 
> >> dpif_flow_stats *);  void dpif_flow_stats_format(const struct 
> >> dpif_flow_stats *, struct ds *); @@ -573,7 +578,7 @@ int dpif_flow_get(struct dpif *,
> >>   * All error reporting is deferred to the call to dpif_flow_dump_destroy().
> >>   */
> >>  struct dpif_flow_dump *dpif_flow_dump_create(const struct dpif *, bool terse,
> >> -                                             char *type);
> >> +                                             struct 
> >> + dpif_flow_dump_types *);
> >>  int dpif_flow_dump_destroy(struct dpif_flow_dump *);
> >>  
> >>  struct dpif_flow_dump_thread *dpif_flow_dump_thread_create(
> >> --
> >> 1.7.1
> >> 
> > 


More information about the dev mailing list