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 <[email protected]>
>> Acked-by: Roi Dayan <[email protected]>
>> ---
>> 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
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
