[RFC,2/4] app/testpmd: new flow dump CLI
diff mbox series

Message ID 620a2408b0dd370b8483c452d2b5957b3144fac7.1578969179.git.jackmin@mellanox.com
State Superseded
Delegated to: Ferruh Yigit
Headers show
Series
  • net/mlx5: dump software steering flows in HW
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Xiaoyu Min Jan. 14, 2020, 3:45 a.m. UTC
From: Xueming Li <xuemingl@mellanox.com>

New flow dump CLI to dump MLX5 PMD specific flows into screen.

Signed-off-by: Xueming Li <xuemingl@mellanox.com>
Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
---
 app/test-pmd/Makefile       |  4 ++
 app/test-pmd/cmdline_flow.c | 91 +++++++++++++++++++++++++++++++++++++
 app/test-pmd/config.c       | 33 ++++++++++++++
 app/test-pmd/meson.build    |  3 ++
 app/test-pmd/testpmd.h      |  1 +
 5 files changed, 132 insertions(+)

Comments

Jerin Jacob Jan. 14, 2020, 4:31 a.m. UTC | #1
On Tue, Jan 14, 2020 at 9:15 AM Xiaoyu Min <jackmin@mellanox.com> wrote:
>
> From: Xueming Li <xuemingl@mellanox.com>
>
> New flow dump CLI to dump MLX5 PMD specific flows into screen.
>
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> ---
>  app/test-pmd/Makefile       |  4 ++
>  app/test-pmd/cmdline_flow.c | 91 +++++++++++++++++++++++++++++++++++++
>  app/test-pmd/config.c       | 33 ++++++++++++++
>  app/test-pmd/meson.build    |  3 ++
>  app/test-pmd/testpmd.h      |  1 +
>  5 files changed, 132 insertions(+)
>

>
> +/** Dump all flow rules. */
> +int
> +port_flow_dump(portid_t port_id __rte_unused,
> +              const char *file_name __rte_unused)
> +{
> +       int ret = 0;
> +#ifdef RTE_LIBRTE_MLX5_PMD

IMO, It should be the last resort to add driver-specific symbols in testpmd.
Why not introduce rte_flow_dump() and hook the MLX driver underneath?

> +       FILE * file = stdout;
> +
> +       if (file_name && strlen(file_name)) {
> +               file = fopen(file_name, "w");
> +               if (!file) {
> +                       printf("Failed to create file %s: %s\n", file_name,
> +                              strerror(errno));
> +                       return -errno;
> +               }
> +       }
> +       ret = rte_pmd_mlx5_flow_dump(port_id, file);
> +       if (ret)
> +               printf("Failed to dump flow: %s\n", strerror(-ret));
> +       else
> +               printf("Flow dump finished\n");
> +       if (file_name && strlen(file_name))
> +               fclose(file);
> +#else
> +       printf("MLX5 PMD driver disabled\n");
> +#endif
> +       return ret;
> +}
Xiaoyu Min Jan. 14, 2020, 10:15 a.m. UTC | #2
On Tue, 20-01-14, 10:01, Jerin Jacob wrote:
> On Tue, Jan 14, 2020 at 9:15 AM Xiaoyu Min <jackmin@mellanox.com> wrote:
> >
> > From: Xueming Li <xuemingl@mellanox.com>
> >
> > New flow dump CLI to dump MLX5 PMD specific flows into screen.
> >
> > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> > ---
> >  app/test-pmd/Makefile       |  4 ++
> >  app/test-pmd/cmdline_flow.c | 91 +++++++++++++++++++++++++++++++++++++
> >  app/test-pmd/config.c       | 33 ++++++++++++++
> >  app/test-pmd/meson.build    |  3 ++
> >  app/test-pmd/testpmd.h      |  1 +
> >  5 files changed, 132 insertions(+)
> >
> 
> >
> > +/** Dump all flow rules. */
> > +int
> > +port_flow_dump(portid_t port_id __rte_unused,
> > +              const char *file_name __rte_unused)
> > +{
> > +       int ret = 0;
> > +#ifdef RTE_LIBRTE_MLX5_PMD
> 
> IMO, It should be the last resort to add driver-specific symbols in testpmd.
> Why not introduce rte_flow_dump() and hook the MLX driver underneath?
> 
Hey Jerin,

Thanks for you comments.

What my understanding is this flow dump is very Mellanox specific, it will dump
all flows in Mellanox HW using Mellanox format. They are hardware flows in
short, which are different from rte flow.

I don't know whether other vendor has the similar functionality and needs
so an rte flow level API could be helpful.

And basically rte flow API is based on rte_flow, a dump function probabily means
dump rte_flow itself (i.e flow->attr, pattern, actions).

This is the reason a private API is choosen and driver-specific symbols added
in testpmd as result.

-Jack
Jerin Jacob Jan. 14, 2020, 2 p.m. UTC | #3
On Tue, Jan 14, 2020 at 3:45 PM Jack Min <jackmin@mellanox.com> wrote:
>
> On Tue, 20-01-14, 10:01, Jerin Jacob wrote:
> > On Tue, Jan 14, 2020 at 9:15 AM Xiaoyu Min <jackmin@mellanox.com> wrote:
> > >
> > > From: Xueming Li <xuemingl@mellanox.com>
> > >
> > > New flow dump CLI to dump MLX5 PMD specific flows into screen.
> > >
> > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> > > ---
> > >  app/test-pmd/Makefile       |  4 ++
> > >  app/test-pmd/cmdline_flow.c | 91 +++++++++++++++++++++++++++++++++++++
> > >  app/test-pmd/config.c       | 33 ++++++++++++++
> > >  app/test-pmd/meson.build    |  3 ++
> > >  app/test-pmd/testpmd.h      |  1 +
> > >  5 files changed, 132 insertions(+)
> > >
> >
> > >
> > > +/** Dump all flow rules. */
> > > +int
> > > +port_flow_dump(portid_t port_id __rte_unused,
> > > +              const char *file_name __rte_unused)
> > > +{
> > > +       int ret = 0;
> > > +#ifdef RTE_LIBRTE_MLX5_PMD
> >
> > IMO, It should be the last resort to add driver-specific symbols in testpmd.
> > Why not introduce rte_flow_dump() and hook the MLX driver underneath?
> >
> Hey Jerin,

Hi Jack.

>
> Thanks for you comments.
>
> What my understanding is this flow dump is very Mellanox specific, it will dump
> all flows in Mellanox HW using Mellanox format. They are hardware flows in
> short, which are different from rte flow.

We do have a similar API for other drivers to represent the internal info.
I think, similar API[1] would suffice for your case. We should not
standardize the output of dump, instead, it will provide a generic API
to dump the internal representation of flow HW to file/stdout.

Octeontx2 MACM HW (rte_flow) has similar internal information,
We could implement the driver API if it is a generic API.

[1]
See the below example for eventdev driver.

/**
 * Dump internal information about *dev_id* to the FILE* provided in *f*.
 *
 * @param dev_id
 *   The identifier of the device.
 *
 * @param f
 *   A pointer to a file for output
 *
 * @return
 *   - 0: on success
 *   - <0: on failure.
 */
int
rte_event_dev_dump(uint8_t dev_id, FILE *f);


>
> I don't know whether other vendor has the similar functionality and needs
> so an rte flow level API could be helpful.
>
> And basically rte flow API is based on rte_flow, a dump function probabily means
> dump rte_flow itself (i.e flow->attr, pattern, actions).
>
> This is the reason a private API is choosen and driver-specific symbols added
> in testpmd as result.
>
> -Jack
Xiaoyu Min Jan. 15, 2020, 12:49 p.m. UTC | #4
On Tue, 20-01-14, 19:30, Jerin Jacob wrote:
> On Tue, Jan 14, 2020 at 3:45 PM Jack Min <jackmin@mellanox.com> wrote:
> >
> > On Tue, 20-01-14, 10:01, Jerin Jacob wrote:
> > > On Tue, Jan 14, 2020 at 9:15 AM Xiaoyu Min <jackmin@mellanox.com> wrote:
> > > >
> > > > From: Xueming Li <xuemingl@mellanox.com>
> > > >
> > > > New flow dump CLI to dump MLX5 PMD specific flows into screen.
> > > >
> > > > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > > > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> > > > ---
> > > >  app/test-pmd/Makefile       |  4 ++
> > > >  app/test-pmd/cmdline_flow.c | 91 +++++++++++++++++++++++++++++++++++++
> > > >  app/test-pmd/config.c       | 33 ++++++++++++++
> > > >  app/test-pmd/meson.build    |  3 ++
> > > >  app/test-pmd/testpmd.h      |  1 +
> > > >  5 files changed, 132 insertions(+)
> > > >
> > >
> > > >
> > > > +/** Dump all flow rules. */
> > > > +int
> > > > +port_flow_dump(portid_t port_id __rte_unused,
> > > > +              const char *file_name __rte_unused)
> > > > +{
> > > > +       int ret = 0;
> > > > +#ifdef RTE_LIBRTE_MLX5_PMD
> > >
> > > IMO, It should be the last resort to add driver-specific symbols in testpmd.
> > > Why not introduce rte_flow_dump() and hook the MLX driver underneath?
> > >
> > Hey Jerin,
> 
> Hi Jack.
> 
> >
> > Thanks for you comments.
> >
> > What my understanding is this flow dump is very Mellanox specific, it will dump
> > all flows in Mellanox HW using Mellanox format. They are hardware flows in
> > short, which are different from rte flow.
> 
> We do have a similar API for other drivers to represent the internal info.
> I think, similar API[1] would suffice for your case. We should not
> standardize the output of dump, instead, it will provide a generic API
> to dump the internal representation of flow HW to file/stdout.
> 
> Octeontx2 MACM HW (rte_flow) has similar internal information,
> We could implement the driver API if it is a generic API.
Yes, a generic rte_flow API (as similar as [1])can benifit both of us. :-)
I'll propose a new rte_flow API for this in v2.

Thanks,
-Jack

> 
> [1]
> See the below example for eventdev driver.
> 
> /**
>  * Dump internal information about *dev_id* to the FILE* provided in *f*.
>  *
>  * @param dev_id
>  *   The identifier of the device.
>  *
>  * @param f
>  *   A pointer to a file for output
>  *
>  * @return
>  *   - 0: on success
>  *   - <0: on failure.
>  */
> int
> rte_event_dev_dump(uint8_t dev_id, FILE *f);
> 
> 
> >
> > I don't know whether other vendor has the similar functionality and needs
> > so an rte flow level API could be helpful.
> >
> > And basically rte flow API is based on rte_flow, a dump function probabily means
> > dump rte_flow itself (i.e flow->attr, pattern, actions).
> >
> > This is the reason a private API is choosen and driver-specific symbols added
> > in testpmd as result.
> >
> > -Jack

Patch
diff mbox series

diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile
index d5258eae4a..e60c8ecf63 100644
--- a/app/test-pmd/Makefile
+++ b/app/test-pmd/Makefile
@@ -70,6 +70,10 @@  ifeq ($(CONFIG_RTE_LIBRTE_PMD_SOFTNIC),y)
 LDLIBS += -lrte_pmd_softnic
 endif
 
+ifeq ($(CONFIG_RTE_LIBRTE_MLX5_PMD),y)
+LDLIBS += -lrte_pmd_mlx5
+endif
+
 endif
 
 include $(RTE_SDK)/mk/rte.app.mk
diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 99dade7d8c..19336e5d42 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -41,6 +41,7 @@  enum index {
 	BOOLEAN,
 	STRING,
 	HEX,
+	FILE_PATH,
 	MAC_ADDR,
 	IPV4_ADDR,
 	IPV6_ADDR,
@@ -63,6 +64,7 @@  enum index {
 	CREATE,
 	DESTROY,
 	FLUSH,
+	DUMP,
 	QUERY,
 	LIST,
 	ISOLATE,
@@ -631,6 +633,9 @@  struct buffer {
 			uint32_t *rule;
 			uint32_t rule_n;
 		} destroy; /**< Destroy arguments. */
+		struct {
+			char file[128];
+		} dump; /**< Dump arguments. */
 		struct {
 			uint32_t rule;
 			struct rte_flow_action action;
@@ -685,6 +690,12 @@  static const enum index next_destroy_attr[] = {
 	ZERO,
 };
 
+static const enum index next_dump_attr[] = {
+	FILE_PATH,
+	END,
+	ZERO,
+};
+
 static const enum index next_list_attr[] = {
 	LIST_GROUP,
 	END,
@@ -1374,6 +1385,9 @@  static int parse_destroy(struct context *, const struct token *,
 static int parse_flush(struct context *, const struct token *,
 		       const char *, unsigned int,
 		       void *, unsigned int);
+static int parse_dump(struct context *, const struct token *,
+		      const char *, unsigned int,
+		      void *, unsigned int);
 static int parse_query(struct context *, const struct token *,
 		       const char *, unsigned int,
 		       void *, unsigned int);
@@ -1401,6 +1415,9 @@  static int parse_string(struct context *, const struct token *,
 static int parse_hex(struct context *ctx, const struct token *token,
 			const char *str, unsigned int len,
 			void *buf, unsigned int size);
+static int parse_string0(struct context *, const struct token *,
+			const char *, unsigned int,
+			void *, unsigned int);
 static int parse_mac_addr(struct context *, const struct token *,
 			  const char *, unsigned int,
 			  void *, unsigned int);
@@ -1494,6 +1511,12 @@  static const struct token token_list[] = {
 		.type = "HEX",
 		.help = "fixed string",
 		.call = parse_hex,
+	},
+	[FILE_PATH] = {
+		.name = "{file path}",
+		.type = "STRING",
+		.help = "file path",
+		.call = parse_string0,
 		.comp = comp_none,
 	},
 	[MAC_ADDR] = {
@@ -1555,6 +1578,7 @@  static const struct token token_list[] = {
 			      CREATE,
 			      DESTROY,
 			      FLUSH,
+			      DUMP,
 			      LIST,
 			      QUERY,
 			      ISOLATE)),
@@ -1589,6 +1613,14 @@  static const struct token token_list[] = {
 		.args = ARGS(ARGS_ENTRY(struct buffer, port)),
 		.call = parse_flush,
 	},
+	[DUMP] = {
+		.name = "dump",
+		.help = "dump all flow rules to file",
+		.next = NEXT(next_dump_attr, NEXT_ENTRY(PORT_ID)),
+		.args = ARGS(ARGS_ENTRY(struct buffer, args.dump.file),
+			     ARGS_ENTRY(struct buffer, port)),
+		.call = parse_dump,
+	},
 	[QUERY] = {
 		.name = "query",
 		.help = "query an existing flow rule",
@@ -5012,6 +5044,33 @@  parse_flush(struct context *ctx, const struct token *token,
 	return len;
 }
 
+/** Parse tokens for dump command. */
+static int
+parse_dump(struct context *ctx, const struct token *token,
+	    const char *str, unsigned int len,
+	    void *buf, unsigned int size)
+{
+	struct buffer *out = buf;
+
+	/* Token name must match. */
+	if (parse_default(ctx, token, str, len, NULL, 0) < 0)
+		return -1;
+	/* Nothing else to do if there is no buffer. */
+	if (!out)
+		return len;
+	if (!out->command) {
+		if (ctx->curr != DUMP)
+			return -1;
+		if (sizeof(*out) > size)
+			return -1;
+		out->command = ctx->curr;
+		ctx->objdata = 0;
+		ctx->object = out;
+		ctx->objmask = NULL;
+	}
+	return len;
+}
+
 /** Parse tokens for query command. */
 static int
 parse_query(struct context *ctx, const struct token *token,
@@ -5409,6 +5468,35 @@  parse_hex(struct context *ctx, const struct token *token,
 
 }
 
+/**
+ * Parse a zero-ended string.
+ */
+static int
+parse_string0(struct context *ctx, const struct token *token __rte_unused,
+	     const char *str, unsigned int len,
+	     void *buf, unsigned int size)
+{
+	const struct arg *arg_data = pop_args(ctx);
+
+	/* Arguments are expected. */
+	if (!arg_data)
+		return -1;
+	size = arg_data->size;
+	/* Bit-mask fill is not supported. */
+	if (arg_data->mask || size < len + 1)
+		goto error;
+	if (!ctx->object)
+		return len;
+	buf = (uint8_t *)ctx->object + arg_data->offset;
+	strncpy(buf, str, len);
+	if (ctx->objmask)
+		memset((uint8_t *)ctx->objmask + arg_data->offset, 0xff, len);
+	return len;
+error:
+	push_args(ctx, arg_data);
+	return -1;
+}
+
 /**
  * Parse a MAC address.
  *
@@ -6068,6 +6156,9 @@  cmd_flow_parsed(const struct buffer *in)
 	case FLUSH:
 		port_flow_flush(in->port);
 		break;
+	case DUMP:
+		port_flow_dump(in->port, in->args.dump.file);
+		break;
 	case QUERY:
 		port_flow_query(in->port, in->args.query.rule,
 				&in->args.query.action);
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 9da1ffb034..b5a9915df9 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -48,6 +48,9 @@ 
 #ifdef RTE_LIBRTE_BNXT_PMD
 #include <rte_pmd_bnxt.h>
 #endif
+#ifdef RTE_LIBRTE_MLX5_PMD
+#include <rte_pmd_mlx5.h>
+#endif
 #include <rte_gro.h>
 #include <rte_config.h>
 
@@ -1441,6 +1444,36 @@  port_flow_flush(portid_t port_id)
 	return ret;
 }
 
+/** Dump all flow rules. */
+int
+port_flow_dump(portid_t port_id __rte_unused,
+	       const char *file_name __rte_unused)
+{
+	int ret = 0;
+#ifdef RTE_LIBRTE_MLX5_PMD
+	FILE * file = stdout;
+
+	if (file_name && strlen(file_name)) {
+		file = fopen(file_name, "w");
+		if (!file) {
+			printf("Failed to create file %s: %s\n", file_name,
+			       strerror(errno));
+			return -errno;
+		}
+	}
+	ret = rte_pmd_mlx5_flow_dump(port_id, file);
+	if (ret)
+		printf("Failed to dump flow: %s\n", strerror(-ret));
+	else
+		printf("Flow dump finished\n");
+	if (file_name && strlen(file_name))
+		fclose(file);
+#else
+	printf("MLX5 PMD driver disabled\n");
+#endif
+	return ret;
+}
+
 /** Query a flow rule. */
 int
 port_flow_query(portid_t port_id, uint32_t rule,
diff --git a/app/test-pmd/meson.build b/app/test-pmd/meson.build
index 6006c60f99..a71e0a0cd1 100644
--- a/app/test-pmd/meson.build
+++ b/app/test-pmd/meson.build
@@ -48,3 +48,6 @@  if dpdk_conf.has('RTE_LIBRTE_BPF')
 	sources += files('bpf_cmd.c')
 	deps += 'bpf'
 endif
+if dpdk_conf.has('RTE_LIBRTE_MLX5_PMD')
+	deps += 'pmd_mlx5'
+endif
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 857a11f8de..e1b9aba360 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -734,6 +734,7 @@  int port_flow_create(portid_t port_id,
 		     const struct rte_flow_action *actions);
 int port_flow_destroy(portid_t port_id, uint32_t n, const uint32_t *rule);
 int port_flow_flush(portid_t port_id);
+int port_flow_dump(portid_t port_id, const char *file_name);
 int port_flow_query(portid_t port_id, uint32_t rule,
 		    const struct rte_flow_action *action);
 void port_flow_list(portid_t port_id, uint32_t n, const uint32_t *group);