diff mbox series

[v4,1/3] ethdev: modify rte API for single flow dump

Message ID 1618389706-183883-2-git-send-email-haifeil@nvidia.com (mailing list archive)
State Superseded
Delegated to: Ferruh Yigit
Headers show
Series support single flow dump | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Haifei Luo April 14, 2021, 8:41 a.m. UTC
Previous implementations support dump all the flows. Add new arg
rte_flow in rte_flow_dev_dump to dump one flow.

Signed-off-by: Haifei Luo <haifeil@nvidia.com>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 app/test-pmd/config.c                |  2 +-
 doc/guides/nics/mlx5.rst             |  9 ++++++---
 doc/guides/prog_guide/rte_flow.rst   | 24 ++++++++++++++++++++++++
 drivers/net/mlx5/linux/mlx5_socket.c |  2 +-
 drivers/net/mlx5/mlx5.h              |  4 ++--
 drivers/net/mlx5/mlx5_flow.c         |  9 ++++++---
 drivers/net/octeontx2/otx2_flow.c    |  9 ++++++++-
 lib/librte_ethdev/rte_flow.c         |  5 +++--
 lib/librte_ethdev/rte_flow.h         |  5 ++++-
 lib/librte_ethdev/rte_flow_driver.h  |  1 +
 10 files changed, 56 insertions(+), 14 deletions(-)

Comments

Thomas Monjalon April 14, 2021, 8:57 a.m. UTC | #1
About the title, what is "rte API"?
I guess you mean DPDK API with rte prefix.
But given all DPDK API have rte prefix,
and this patch is for DPDK,
you can just say "API".

So the title can be:
	ethdev: modify API for single flow dump

But it can look as single flow dump was possible before,
which is wrong because it is a new feature.

Another idea is wrong:
The packets of the flow are not dumped,
it is only dumping the HW representation of the flow rule.

I propose to simply describe the new feature:
	ethdev: dump single flow rule


> + * @param[in] flow
> + *   The pointer of rte flow.

Please replace "rte flow" with 'flow rule".
Is it allowed to pass NULL? Will it dump all?
If yes, you can change to:
	Flow rule to dump.
	Dump all rules if NULL.
Thomas Monjalon April 14, 2021, 9:01 a.m. UTC | #2
14/04/2021 10:41, Haifei Luo:
> Previous implementations support dump all the flows. Add new arg
> rte_flow in rte_flow_dev_dump to dump one flow.
> 
> Signed-off-by: Haifei Luo <haifeil@nvidia.com>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Acked-by: Ori Kam <orika@nvidia.com>
[...]
> +Dump
> +~~~~~
> +
> +Dump information for all or one flows.
> +
> +.. code-block:: c
> +
> +   int
> +   rte_flow_dev_dump(uint16_t port_id, struct rte_flow *flow,
> +                     FILE *file,
> +                     struct rte_flow_error *error);
> +
> +Arguments:
> +
> +- ``port_id``: port identifier of Ethernet device.
> +- ``flow``: flow rule handle to dump. NULL to dump all.
> +- ``file``: a pointer to a file for output
> +- ``error``: perform verbose error reporting if not NULL. PMDs initialize
> +  this structure in case of error only.
> +
> +Return values:
> +
> +- 0 on success, a negative errno value otherwise and ``rte_errno`` is set.

I really don't understand why we continue duplicating rte_flow doxygen
in the guide. Please stop.
Do one good API description in doxygen,
and describe only the big picture and how-to in the guide.
Haifei Luo April 14, 2021, 9:07 a.m. UTC | #3
Hi Thomas,
    I will remove it from prog_guide/rte_flow.rst. Is it okay ? Thank you.

-----Original Message-----
From: Thomas Monjalon <thomas@monjalon.net> 
Sent: Wednesday, April 14, 2021 5:01 PM
To: Ori Kam <orika@nvidia.com>
Cc: dev@dpdk.org; Haifei Luo <haifeil@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>; Xueming(Steven) Li <xuemingl@nvidia.com>; Haifei Luo <haifeil@nvidia.com>; ajit.khaparde@broadcom.com; Xiaoyun Li <xiaoyun.li@intel.com>; Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Jerin Jacob <jerinj@marvell.com>; Nithin Dabilpuram <ndabilpuram@marvell.com>; Kiran Kumar K <kirankumark@marvell.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Subject: Re: [PATCH v4 1/3] ethdev: modify rte API for single flow dump

External email: Use caution opening links or attachments


14/04/2021 10:41, Haifei Luo:
> Previous implementations support dump all the flows. Add new arg 
> rte_flow in rte_flow_dev_dump to dump one flow.
>
> Signed-off-by: Haifei Luo <haifeil@nvidia.com>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Acked-by: Ori Kam <orika@nvidia.com>
[...]
> +Dump
> +~~~~~
> +
> +Dump information for all or one flows.
> +
> +.. code-block:: c
> +
> +   int
> +   rte_flow_dev_dump(uint16_t port_id, struct rte_flow *flow,
> +                     FILE *file,
> +                     struct rte_flow_error *error);
> +
> +Arguments:
> +
> +- ``port_id``: port identifier of Ethernet device.
> +- ``flow``: flow rule handle to dump. NULL to dump all.
> +- ``file``: a pointer to a file for output
> +- ``error``: perform verbose error reporting if not NULL. PMDs 
> +initialize
> +  this structure in case of error only.
> +
> +Return values:
> +
> +- 0 on success, a negative errno value otherwise and ``rte_errno`` is set.

I really don't understand why we continue duplicating rte_flow doxygen in the guide. Please stop.
Do one good API description in doxygen,
and describe only the big picture and how-to in the guide.
Haifei Luo April 14, 2021, 9:10 a.m. UTC | #4
HI Thomas,
    #1, okay , will modify title as " ethdev: dump single flow rule " .
    #2, yes, it can pass NULL. Will modify as you described.
Thank you so much for the comments.

-----Original Message-----
From: Thomas Monjalon <thomas@monjalon.net> 
Sent: Wednesday, April 14, 2021 4:58 PM
To: Haifei Luo <haifeil@nvidia.com>
Cc: dev@dpdk.org; Ori Kam <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>; Xueming(Steven) Li <xuemingl@nvidia.com>; Haifei Luo <haifeil@nvidia.com>; ajit.khaparde@broadcom.com; Xiaoyun Li <xiaoyun.li@intel.com>; Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Jerin Jacob <jerinj@marvell.com>; Nithin Dabilpuram <ndabilpuram@marvell.com>; Kiran Kumar K <kirankumark@marvell.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Subject: Re: [PATCH v4 1/3] ethdev: modify rte API for single flow dump

External email: Use caution opening links or attachments


About the title, what is "rte API"?
I guess you mean DPDK API with rte prefix.
But given all DPDK API have rte prefix,
and this patch is for DPDK,
you can just say "API".

So the title can be:
        ethdev: modify API for single flow dump

But it can look as single flow dump was possible before, which is wrong because it is a new feature.

Another idea is wrong:
The packets of the flow are not dumped,
it is only dumping the HW representation of the flow rule.

I propose to simply describe the new feature:
        ethdev: dump single flow rule


> + * @param[in] flow
> + *   The pointer of rte flow.

Please replace "rte flow" with 'flow rule".
Is it allowed to pass NULL? Will it dump all?
If yes, you can change to:
        Flow rule to dump.
        Dump all rules if NULL.
Thomas Monjalon April 14, 2021, 9:31 a.m. UTC | #5
14/04/2021 11:07, Haifei Luo:
> Hi Thomas,
>     I will remove it from prog_guide/rte_flow.rst. Is it okay ? Thank you.

This is more a question for Ori.
In case he is not available, yes I think it is better to skip
rte_flow.rst in this patch as it doesn't bring added value.

Note: please avoid top-post.



> From: Thomas Monjalon <thomas@monjalon.net> 
> 14/04/2021 10:41, Haifei Luo:
> > Previous implementations support dump all the flows. Add new arg 
> > rte_flow in rte_flow_dev_dump to dump one flow.
> >
> > Signed-off-by: Haifei Luo <haifeil@nvidia.com>
> > Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> > Acked-by: Ori Kam <orika@nvidia.com>
> [...]
> > +Dump
> > +~~~~~
> > +
> > +Dump information for all or one flows.
> > +
> > +.. code-block:: c
> > +
> > +   int
> > +   rte_flow_dev_dump(uint16_t port_id, struct rte_flow *flow,
> > +                     FILE *file,
> > +                     struct rte_flow_error *error);
> > +
> > +Arguments:
> > +
> > +- ``port_id``: port identifier of Ethernet device.
> > +- ``flow``: flow rule handle to dump. NULL to dump all.
> > +- ``file``: a pointer to a file for output
> > +- ``error``: perform verbose error reporting if not NULL. PMDs 
> > +initialize
> > +  this structure in case of error only.
> > +
> > +Return values:
> > +
> > +- 0 on success, a negative errno value otherwise and ``rte_errno`` is set.
> 
> I really don't understand why we continue duplicating rte_flow doxygen in the guide. Please stop.
> Do one good API description in doxygen,
> and describe only the big picture and how-to in the guide.
diff mbox series

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index a5e82b7..ca34a63 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1932,7 +1932,7 @@  struct rte_flow_shared_action *
 			return -errno;
 		}
 	}
-	ret = rte_flow_dev_dump(port_id, file, &error);
+	ret = rte_flow_dev_dump(port_id, NULL, file, &error);
 	if (ret) {
 		port_flow_complain(&error);
 		printf("Failed to dump flow: %s\n", strerror(-ret));
diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
index 490329a..7ff92b0 100644
--- a/doc/guides/nics/mlx5.rst
+++ b/doc/guides/nics/mlx5.rst
@@ -1837,13 +1837,16 @@  all flows with assistance of external tools.
 
    .. code-block:: console
 
-       testpmd> flow dump <port> <output_file>
+       To dump all flows:
+       testpmd> flow dump <port> all <output_file>
+       and dump one flow:
+       testpmd> flow dump <port> rule <rule_id> <output_file>
 
    - call rte_flow_dev_dump api:
 
    .. code-block:: console
 
-       rte_flow_dev_dump(port, file, NULL);
+       rte_flow_dev_dump(port, flow, file, NULL);
 
 #. Dump human-readable flows from raw file:
 
@@ -1851,4 +1854,4 @@  all flows with assistance of external tools.
 
    .. code-block:: console
 
-       mlx_steering_dump.py -f <output_file>
+       mlx_steering_dump.py -f <output_file> -flowptr <flow_ptr>
diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index e1b93ec..2ed149e 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -3025,6 +3025,30 @@  Return values:
 
 - 0 on success, a negative errno value otherwise and ``rte_errno`` is set.
 
+Dump
+~~~~~
+
+Dump information for all or one flows.
+
+.. code-block:: c
+
+   int
+   rte_flow_dev_dump(uint16_t port_id, struct rte_flow *flow,
+                     FILE *file,
+                     struct rte_flow_error *error);
+
+Arguments:
+
+- ``port_id``: port identifier of Ethernet device.
+- ``flow``: flow rule handle to dump. NULL to dump all.
+- ``file``: a pointer to a file for output
+- ``error``: perform verbose error reporting if not NULL. PMDs initialize
+  this structure in case of error only.
+
+Return values:
+
+- 0 on success, a negative errno value otherwise and ``rte_errno`` is set.
+
 Query
 ~~~~~
 
diff --git a/drivers/net/mlx5/linux/mlx5_socket.c b/drivers/net/mlx5/linux/mlx5_socket.c
index b1f41bc..6e354f4 100644
--- a/drivers/net/mlx5/linux/mlx5_socket.c
+++ b/drivers/net/mlx5/linux/mlx5_socket.c
@@ -84,7 +84,7 @@ 
 	}
 	/* Dump flow. */
 	dev = &rte_eth_devices[port_id];
-	ret = mlx5_flow_dev_dump(dev, file, NULL);
+	ret = mlx5_flow_dev_dump(dev, NULL, file, NULL);
 	/* Set-up the ancillary data and reply. */
 	msg.msg_controllen = 0;
 	msg.msg_control = NULL;
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 0f69f9d..e0f7101 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -1245,8 +1245,8 @@  void mlx5_flow_async_pool_query_handle(struct mlx5_dev_ctx_shared *sh,
 void mlx5_counter_free(struct rte_eth_dev *dev, uint32_t cnt);
 int mlx5_counter_query(struct rte_eth_dev *dev, uint32_t cnt,
 		       bool clear, uint64_t *pkts, uint64_t *bytes);
-int mlx5_flow_dev_dump(struct rte_eth_dev *dev, FILE *file,
-		       struct rte_flow_error *error);
+int mlx5_flow_dev_dump(struct rte_eth_dev *dev, struct rte_flow *flow,
+			FILE *file, struct rte_flow_error *error);
 void mlx5_flow_rxq_dynf_metadata_set(struct rte_eth_dev *dev);
 int mlx5_flow_get_aged_flows(struct rte_eth_dev *dev, void **contexts,
 			uint32_t nb_contexts, struct rte_flow_error *error);
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 668c32c..a8cf674 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -7154,7 +7154,7 @@  struct mlx5_meter_domains_infos *
  *   0 on success, a nagative value otherwise.
  */
 int
-mlx5_flow_dev_dump(struct rte_eth_dev *dev,
+mlx5_flow_dev_dump(struct rte_eth_dev *dev, struct rte_flow *flow_idx,
 		   FILE *file,
 		   struct rte_flow_error *error __rte_unused)
 {
@@ -7166,8 +7166,11 @@  struct mlx5_meter_domains_infos *
 			return -errno;
 		return -ENOTSUP;
 	}
-	return mlx5_devx_cmd_flow_dump(sh->fdb_domain, sh->rx_domain,
-				       sh->tx_domain, file);
+
+	if (!flow_idx)
+		return mlx5_devx_cmd_flow_dump(sh->fdb_domain,
+				sh->rx_domain, sh->tx_domain, file);
+	return -ENOTSUP;
 }
 
 /**
diff --git a/drivers/net/octeontx2/otx2_flow.c b/drivers/net/octeontx2/otx2_flow.c
index 14ac9bc..1c90d75 100644
--- a/drivers/net/octeontx2/otx2_flow.c
+++ b/drivers/net/octeontx2/otx2_flow.c
@@ -807,7 +807,7 @@ 
 
 static int
 otx2_flow_dev_dump(struct rte_eth_dev *dev,
-		  FILE *file,
+		  struct rte_flow *flow, FILE *file,
 		  struct rte_flow_error *error)
 {
 	struct otx2_eth_dev *hw = dev->data->dev_private;
@@ -822,6 +822,13 @@ 
 				   "Invalid file");
 		return -EINVAL;
 	}
+	if (flow != NULL) {
+		rte_flow_error_set(error, EINVAL,
+				   RTE_FLOW_ERROR_TYPE_HANDLE,
+				   NULL,
+				   "Invalid argument");
+		return -EINVAL;
+	}
 
 	max_prio = hw->npc_flow.flow_max_priority;
 
diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
index e07e617..7241f00 100644
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -1027,7 +1027,8 @@  enum rte_flow_conv_item_spec_type {
 }
 
 int
-rte_flow_dev_dump(uint16_t port_id, FILE *file, struct rte_flow_error *error)
+rte_flow_dev_dump(uint16_t port_id, struct rte_flow *flow,
+			FILE *file, struct rte_flow_error *error)
 {
 	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
 	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
@@ -1037,7 +1038,7 @@  enum rte_flow_conv_item_spec_type {
 		return -rte_errno;
 	if (likely(!!ops->dev_dump)) {
 		fts_enter(dev);
-		ret = ops->dev_dump(dev, file, error);
+		ret = ops->dev_dump(dev, flow, file, error);
 		fts_exit(dev);
 		return flow_err(port_id, ret, error);
 	}
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index c476a0f..53d1aa5 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -3232,6 +3232,8 @@  enum rte_flow_conv_op {
  *
  * @param[in] port_id
  *    The port identifier of the Ethernet device.
+ * @param[in] flow
+ *   The pointer of rte flow.
  * @param[in] file
  *   A pointer to a file for output.
  * @param[out] error
@@ -3242,7 +3244,8 @@  enum rte_flow_conv_op {
  */
 __rte_experimental
 int
-rte_flow_dev_dump(uint16_t port_id, FILE *file, struct rte_flow_error *error);
+rte_flow_dev_dump(uint16_t port_id, struct rte_flow *flow,
+		FILE *file, struct rte_flow_error *error);
 
 /**
  * Check if mbuf dynamic field for metadata is registered.
diff --git a/lib/librte_ethdev/rte_flow_driver.h b/lib/librte_ethdev/rte_flow_driver.h
index da594d9..6ae1f8c 100644
--- a/lib/librte_ethdev/rte_flow_driver.h
+++ b/lib/librte_ethdev/rte_flow_driver.h
@@ -75,6 +75,7 @@  struct rte_flow_ops {
 	/** See rte_flow_dev_dump(). */
 	int (*dev_dump)
 		(struct rte_eth_dev *dev,
+		 struct rte_flow *flow,
 		 FILE *file,
 		 struct rte_flow_error *error);
 	/** See rte_flow_get_aged_flows() */