Re: [patch iproute2/net-next repost] devlink: Add support for pipeline debug (dpipe)
From: Jiri Pirko Date: Sat, 15 Apr 2017 10:59:03 +0200 > Sat, Apr 15, 2017 at 01:01:38AM CEST, step...@networkplumber.org wrote: >>On Thu, 13 Apr 2017 11:30:27 +0200 >>Jiri Pirko wrote: >> >>> We actually took this code from teamdctl (at least it was an influence). >>> Devlink style is so much different in every aspect from the rest of the >>> iproute2 suite. And I did it on purpose, because it is much nicer and >>> easier to read. I would like to continue on this and don't do things in >>> the was the existing tools do. I don't see any problem with that. >> >>No. That is road to ruin. Every package is free to use what ever style >>it wants. But don't crossover please. > > So we are stuck with some ugly way just because some other util does it? > Well, in devlink I wrote lot of things differently, nicer. For example > cmdline parsing. Why is it a "road to ruin"? I agree with Jiri, if he wants to stylize devlink a certain way that makes sense for devlink information, that is perfectly fine and does not impact the way the rest of iproute2's output appears to the user at all.
Re: [patch iproute2/net-next repost] devlink: Add support for pipeline debug (dpipe)
Sat, Apr 15, 2017 at 01:01:38AM CEST, step...@networkplumber.org wrote: >On Thu, 13 Apr 2017 11:30:27 +0200 >Jiri Pirko wrote: > >> We actually took this code from teamdctl (at least it was an influence). >> Devlink style is so much different in every aspect from the rest of the >> iproute2 suite. And I did it on purpose, because it is much nicer and >> easier to read. I would like to continue on this and don't do things in >> the was the existing tools do. I don't see any problem with that. > >No. That is road to ruin. Every package is free to use what ever style >it wants. But don't crossover please. So we are stuck with some ugly way just because some other util does it? Well, in devlink I wrote lot of things differently, nicer. For example cmdline parsing. Why is it a "road to ruin"?
Re: [patch iproute2/net-next repost] devlink: Add support for pipeline debug (dpipe)
On Thu, 13 Apr 2017 11:30:27 +0200 Jiri Pirko wrote: > We actually took this code from teamdctl (at least it was an influence). > Devlink style is so much different in every aspect from the rest of the > iproute2 suite. And I did it on purpose, because it is much nicer and > easier to read. I would like to continue on this and don't do things in > the was the existing tools do. I don't see any problem with that. No. That is road to ruin. Every package is free to use what ever style it wants. But don't crossover please.
Re: [patch iproute2/net-next repost] devlink: Add support for pipeline debug (dpipe)
Thu, Apr 13, 2017 at 01:45:25AM CEST, step...@networkplumber.org wrote:
>On Tue, 28 Mar 2017 17:26:54 +0200
>Jiri Pirko wrote:
>
>>
>> #define pr_err(args...) fprintf(stderr, ##args)
>> -#define pr_out(args...) fprintf(stdout, ##args)
>> +#define pr_out(args...) \
>> +do {\
>> +if (g_indent_newline) { \
>> +fprintf(stdout, "%s", g_indent_str);\
>> +g_indent_newline = false; \
>> +} \
>> +fprintf(stdout, ##args);\
>> +} while (0)
>> +
>> #define pr_out_sp(num, args...) \
>> do {\
>> int ret = fprintf(stdout, ##args); \
>> @@ -42,6 +50,35 @@
>> fprintf(stdout, "%*s", num - ret, ""); \
>> } while (0)
>>
>> +static int g_indent_level;
>> +static bool g_indent_newline;
>> +#define INDENT_STR_STEP 2
>> +#define INDENT_STR_MAXLEN 32
>> +static char g_indent_str[INDENT_STR_MAXLEN + 1] = "";
>> +
>> +static void __pr_out_indent_inc(void)
>> +{
>> +if (g_indent_level + INDENT_STR_STEP > INDENT_STR_MAXLEN)
>> +return;
>> +g_indent_level += INDENT_STR_STEP;
>> +memset(g_indent_str, ' ', sizeof(g_indent_str));
>> +g_indent_str[g_indent_level] = '\0';
>> +}
>> +
>> +static void __pr_out_indent_dec(void)
>> +{
>> +if (g_indent_level - INDENT_STR_STEP < 0)
>> +return;
>> +g_indent_level -= INDENT_STR_STEP;
>> +g_indent_str[g_indent_level] = '\0';
>> +}
>> +
>> +static void __pr_out_newline(void)
>> +{
>> +pr_out("\n");
>> +g_indent_newline = true;
>> +}
>> +
>
>Thanks for adding the support. Like many reviews, I am fine with the
>functionality but it is the details of the implementation that are of
>concern.
>
>Why this new set of output formatting routines, this doesn't resemble other
>code
>in ip utilities. Looks more like you copied and pasted it from somewhere else.
>The indentation in existing ip command output isn't pretty or fancy but it
>works.
>
>Please try and make this code look like all the other code. Yes, I know it
>may not be to your taste (it isn't mine either), but consistency is more
>important
>than individual style.
We actually took this code from teamdctl (at least it was an influence).
Devlink style is so much different in every aspect from the rest of the
iproute2 suite. And I did it on purpose, because it is much nicer and
easier to read. I would like to continue on this and don't do things in
the was the existing tools do. I don't see any problem with that.
>
>> @ -314,6 +356,102 @@ static int attr_cb(const struct nlattr *attr, void
>> *data)
>> if (type == DEVLINK_ATTR_ESWITCH_INLINE_MODE &&
>> mnl_attr_validate(attr, MNL_TYPE_U8) < 0)
>> return MNL_CB_ERROR;
>> +if (type == DEVLINK_ATTR_DPIPE_TABLES &&
>> +mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +return MNL_CB_ERROR;
>> +if (type == DEVLINK_ATTR_DPIPE_TABLE &&
>> +mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +return MNL_CB_ERROR;
>> +if (type == DEVLINK_ATTR_DPIPE_TABLE_NAME &&
>> +mnl_attr_validate(attr, MNL_TYPE_STRING) < 0)
>> +return MNL_CB_ERROR;
>> +if (type == DEVLINK_ATTR_DPIPE_TABLE_SIZE &&
>> +mnl_attr_validate(attr, MNL_TYPE_U64) < 0)
>> +return MNL_CB_ERROR;
>> +if (type == DEVLINK_ATTR_DPIPE_TABLE_MATCHES &&
>> +mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +return MNL_CB_ERROR;
>> +if (type == DEVLINK_ATTR_DPIPE_TABLE_ACTIONS &&
>> +mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +return MNL_CB_ERROR;
>> +if (type == DEVLINK_ATTR_DPIPE_TABLE_COUNTERS_ENABLED &&
>> +mnl_attr_validate(attr, MNL_TYPE_U8) < 0)
>> +return MNL_CB_ERROR;
>> +if (type == DEVLINK_ATTR_DPIPE_ENTRIES &&
>> +mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +return MNL_CB_ERROR;
>> +if (type == DEVLINK_ATTR_DPIPE_ENTRY &&
>> +mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +return MNL_CB_ERROR;
>> +if (type == DEVLINK_ATTR_DPIPE_ENTRY_INDEX &&
>> +mnl_attr_validate(attr, MNL_TYPE_U64) < 0)
>> +return MNL_CB_ERROR;
>> +if (type == DEVLINK_ATTR_DPIPE_ENTRY_MATCH_VALUES &&
>> +mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +return MNL_CB_ERROR;
>> +if (type == DEVLINK_ATTR_DPIPE_ENTRY_ACTION_VALUES &&
>> +mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +return MNL_CB_ERROR;
>> +if (type == DEVLINK_ATTR_DPIPE_ENTRY_COUNTER &&
>> +mnl_attr_validate(attr, MNL_TYPE_U64) < 0)
>> +return MNL_CB_ERROR;
>> +
Re: [patch iproute2/net-next repost] devlink: Add support for pipeline debug (dpipe)
On Tue, 28 Mar 2017 17:26:54 +0200
Jiri Pirko wrote:
>
> #define pr_err(args...) fprintf(stderr, ##args)
> -#define pr_out(args...) fprintf(stdout, ##args)
> +#define pr_out(args...) \
> + do {\
> + if (g_indent_newline) { \
> + fprintf(stdout, "%s", g_indent_str);\
> + g_indent_newline = false; \
> + } \
> + fprintf(stdout, ##args);\
> + } while (0)
> +
> #define pr_out_sp(num, args...) \
> do {\
> int ret = fprintf(stdout, ##args); \
> @@ -42,6 +50,35 @@
> fprintf(stdout, "%*s", num - ret, ""); \
> } while (0)
>
> +static int g_indent_level;
> +static bool g_indent_newline;
> +#define INDENT_STR_STEP 2
> +#define INDENT_STR_MAXLEN 32
> +static char g_indent_str[INDENT_STR_MAXLEN + 1] = "";
> +
> +static void __pr_out_indent_inc(void)
> +{
> + if (g_indent_level + INDENT_STR_STEP > INDENT_STR_MAXLEN)
> + return;
> + g_indent_level += INDENT_STR_STEP;
> + memset(g_indent_str, ' ', sizeof(g_indent_str));
> + g_indent_str[g_indent_level] = '\0';
> +}
> +
> +static void __pr_out_indent_dec(void)
> +{
> + if (g_indent_level - INDENT_STR_STEP < 0)
> + return;
> + g_indent_level -= INDENT_STR_STEP;
> + g_indent_str[g_indent_level] = '\0';
> +}
> +
> +static void __pr_out_newline(void)
> +{
> + pr_out("\n");
> + g_indent_newline = true;
> +}
> +
Thanks for adding the support. Like many reviews, I am fine with the
functionality but it is the details of the implementation that are of
concern.
Why this new set of output formatting routines, this doesn't resemble other code
in ip utilities. Looks more like you copied and pasted it from somewhere else.
The indentation in existing ip command output isn't pretty or fancy but it
works.
Please try and make this code look like all the other code. Yes, I know it
may not be to your taste (it isn't mine either), but consistency is more
important
than individual style.
> @ -314,6 +356,102 @@ static int attr_cb(const struct nlattr *attr, void *data)
> if (type == DEVLINK_ATTR_ESWITCH_INLINE_MODE &&
> mnl_attr_validate(attr, MNL_TYPE_U8) < 0)
> return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_TABLES &&
> + mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_TABLE &&
> + mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_TABLE_NAME &&
> + mnl_attr_validate(attr, MNL_TYPE_STRING) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_TABLE_SIZE &&
> + mnl_attr_validate(attr, MNL_TYPE_U64) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_TABLE_MATCHES &&
> + mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_TABLE_ACTIONS &&
> + mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_TABLE_COUNTERS_ENABLED &&
> + mnl_attr_validate(attr, MNL_TYPE_U8) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_ENTRIES &&
> + mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_ENTRY &&
> + mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_ENTRY_INDEX &&
> + mnl_attr_validate(attr, MNL_TYPE_U64) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_ENTRY_MATCH_VALUES &&
> + mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_ENTRY_ACTION_VALUES &&
> + mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_ENTRY_COUNTER &&
> + mnl_attr_validate(attr, MNL_TYPE_U64) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_MATCH &&
> + mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_MATCH_VALUE &&
> + mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_MATCH_TYPE &&
> + mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
> + return MNL_CB_ERROR;
> + if (type == DEVLINK_ATTR_DPIPE_ACTION &&
> +
[patch iproute2/net-next repost] devlink: Add support for pipeline debug (dpipe)
From: Arkadi Sharshevsky
Add support for pipeline debug (dpipe). The headers are used both the
gain visibillity into the headers supported by the hardware, and to
build the headers/field database which is used by other commands.
Examples:
First we can see the headers supported by the hardware:
$devlink dpipe header show pci/:03:00.0
pci/:03:00.0:
name mlxsw_meta
field:
name erif_port bitwidth 32 mapping_type ifindex
name l3_forward bitwidth 1
name l2_drop bitwidth 1
Note that mapping_type is presented only if relevant. Also the header/
field id's are reported by the kernel they are not shown by default.
They can be observed by using the -v option. Also the headers scope
(global/local) is specified.
$devlink -v dpipe header show pci/:03:00.0
pci/:03:00.0:
name mlxsw_meta id 0 global false
field:
name erif_port id 0 bitwidth 32 mapping_type ifindex
name l3_forward id 1 bitwidth 1
name l2_drop id 2 bitwidth 1
Second we can examine the tables supported by the hardware. In order
to dump all the tables no table name should be provided:
$devlink dpipe table show pci/:03:00.0
In order to examine specific table its name have to be specified:
$devlink dpipe table show pci/:03:00.0 name erif
pci/:03:00.0:
name mlxsw_erif size 800 counters_enabled true
match:
type field_exact header mlxsw_meta field erif_port mapping ifindex
action:
type field_modify header mlxsw_meta field l3_forward
type field_modify header mlxsw_meta field l2_drop
To enable/disable counters on the table:
$devlink dpipe table set pci/:03:00.0 name erif counters enable
$devlink dpipe table set pci/:03:00.0 name erif counters disable
In order to see the current entries in the hardware for specific table:
$devlink dpipe table dump pci/:03:00.0 name erif
pci/:03:00.0:
index 0 counter 0
match_value:
type field_exact header mlxsw_meta field erif_port mapping ifindex
mapping_value 383 value 0
action_value:
type field_modify header mlxsw_meta field l3_forward value 1
index 1 counter 0
match_value:
type field_exact header mlxsw_meta field erif_port mapping ifindex
mapping_value 381 value 1
action_value:
type field_modify header mlxsw_meta field l3_forward value 1
In the above example the table contains two entries which does match
on erif port and forwards the packet or drop it (currently only the
forward count is implemented). The counter values are provided for
example. In case the counting is not enabled on the table the counters
will not be available.
Signed-off-by: Arkadi Sharshevsky
Signed-off-by: Jiri Pirko
---
devlink/devlink.c | 1413 +++
include/linux/devlink.h | 67 ++-
2 files changed, 1382 insertions(+), 98 deletions(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index e90226e..3d525ea 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -34,7 +34,15 @@
#define ESWITCH_INLINE_MODE_TRANSPORT "transport"
#define pr_err(args...) fprintf(stderr, ##args)
-#define pr_out(args...) fprintf(stdout, ##args)
+#define pr_out(args...)\
+ do {\
+ if (g_indent_newline) { \
+ fprintf(stdout, "%s", g_indent_str);\
+ g_indent_newline = false; \
+ } \
+ fprintf(stdout, ##args);\
+ } while (0)
+
#define pr_out_sp(num, args...)\
do {\
int ret = fprintf(stdout, ##args); \
@@ -42,6 +50,35 @@
fprintf(stdout, "%*s", num - ret, ""); \
} while (0)
+static int g_indent_level;
+static bool g_indent_newline;
+#define INDENT_STR_STEP 2
+#define INDENT_STR_MAXLEN 32
+static char g_indent_str[INDENT_STR_MAXLEN + 1] = "";
+
+static void __pr_out_indent_inc(void)
+{
+ if (g_indent_level + INDENT_STR_STEP > INDENT_STR_MAXLEN)
+ return;
+ g_indent_level += INDENT_STR_STEP;
+ memset(g_indent_str, ' ', sizeof(g_indent_str));
+ g_indent_str[g_indent_level] = '\0';
+}
+
+static void __pr_out_indent_dec(void)
+{
+ if (g_indent_level - INDENT_STR_STEP < 0)
+ return;
+ g_indent_level -= INDENT_STR_STEP;
+ g_indent_str[g_indent_level] = '\0';
+}
+
+static void __pr_out_newline(void)
+{
+ pr_out("\n");
+ g_indent_newline = true;
+}
+
static int _mnlg_socket_recv_run(struct mnlg_socket *nlg,
mnl_cb_t data_cb, void *data)
{
@@ -137,6 +174,8 @@ static void ifname_map_free(struct ifname_map *ifname_map)
#define DL_OPT_SB_TC BIT(10)
#define DL_OPT_ESWITCH_MODEBIT(11)
#define DL_OPT_ESWITC
