[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