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 <g...@mellanox.com> > Acked-by: Roi Dayan <r...@mellanox.com> > --- > 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. > > +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. > 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 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev