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
