diff mbox series

[1/5] ethdev: add API to dump device internal flow info

Message ID dfb4ae762a7c19031d26c1f664707f4c7823617b.1579168182.git.jackmin@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: add API to dump device internal flow info |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Xiaoyu Min Jan. 16, 2020, 10:14 a.m. UTC 
  Introduce an API which dump the device's internal representation
information of rte flows in hardware.

Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
---
 lib/librte_ethdev/rte_ethdev_version.map |  3 +++
 lib/librte_ethdev/rte_flow.c             | 16 ++++++++++++++++
 lib/librte_ethdev/rte_flow.h             | 21 +++++++++++++++++++++
 lib/librte_ethdev/rte_flow_driver.h      |  5 +++++
 4 files changed, 45 insertions(+)

Comments

Jerin Jacob Jan. 16, 2020, 10:37 a.m. UTC | #1 
On Thu, Jan 16, 2020 at 3:45 PM Xiaoyu Min <jackmin@mellanox.com> wrote:
>
> Introduce an API which dump the device's internal representation
> information of rte flows in hardware.
>
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>

Looks good to me.

> ---
>  lib/librte_ethdev/rte_ethdev_version.map |  3 +++
>  lib/librte_ethdev/rte_flow.c             | 16 ++++++++++++++++
>  lib/librte_ethdev/rte_flow.h             | 21 +++++++++++++++++++++
>  lib/librte_ethdev/rte_flow_driver.h      |  5 +++++
>  4 files changed, 45 insertions(+)
>
> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
> index a7dacf2cf2..3f32fdecf7 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -227,4 +227,7 @@ EXPERIMENTAL {
>         rte_flow_dynf_metadata_mask;
>         rte_flow_dynf_metadata_register;
>         rte_eth_dev_set_ptypes;
> +
> +       # added in 20.02
> +       rte_flow_dev_dump;
>  };
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 87a3e8c4c6..751ce721b2 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -1212,3 +1212,19 @@ rte_flow_expand_rss(struct rte_flow_expand_rss *buf, size_t size,
>         }
>         return lsize;
>  }
> +
> +int
> +rte_flow_dev_dump(uint16_t port_id, 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);
> +
> +       if (unlikely(!ops))
> +               return -rte_errno;
> +       if (likely(!!ops->dev_dump))
> +               return flow_err(port_id, ops->dev_dump(dev, file, error),
> +                               error);
> +       return rte_flow_error_set(error, ENOSYS,
> +                                 RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> +                                 NULL, rte_strerror(ENOSYS));
> +}
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 58b50265d2..cf7cf61ae8 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -2790,6 +2790,27 @@ enum rte_flow_conv_op {
>         RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
>  };
>
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Dump hardware internal representation information of
> + * rte flow to file.
> + *
> + * @param[in] port_id
> + *    The port identifier of the Ethernet device.
> + * @param[in] file
> + *   A pointer to a file for output.
> + * @param[out] error
> + *   Perform verbose error reporting if not NULL. PMDs initialize this
> + *   structure in case of error only.
> + * @return
> + *   0 on success, a nagative value otherwise.
> + */
> +__rte_experimental
> +int
> +rte_flow_dev_dump(uint16_t port_id, 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 a0359853e6..51a9a57a0f 100644
> --- a/lib/librte_ethdev/rte_flow_driver.h
> +++ b/lib/librte_ethdev/rte_flow_driver.h
> @@ -96,6 +96,11 @@ struct rte_flow_ops {
>                 (struct rte_eth_dev *,
>                  int,
>                  struct rte_flow_error *);
> +       /** See rte_flow_dev_dump(). */
> +       int (*dev_dump)
> +               (struct rte_eth_dev *dev,
> +                FILE *file,
> +                struct rte_flow_error *error);
>  };
>
>  /**
> --
> 2.24.1
>
Ori Kam Jan. 16, 2020, 1:29 p.m. UTC | #2 
> -----Original Message-----
> <adrien.mazarguil@6wind.com>; dpdk-dev <dev@dpdk.org>
> Subject: Re: [PATCH 1/5] ethdev: add API to dump device internal flow info
> 
> On Thu, Jan 16, 2020 at 3:45 PM Xiaoyu Min <jackmin@mellanox.com> wrote:
> >
> > Introduce an API which dump the device's internal representation
> > information of rte flows in hardware.
> >
> > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> 
> Looks good to me.

Acked-by: Ori Kam <orika@mellanox.com>
Thanks,
Ori
> 
> > ---
> >  lib/librte_ethdev/rte_ethdev_version.map |  3 +++
> >  lib/librte_ethdev/rte_flow.c             | 16 ++++++++++++++++
> >  lib/librte_ethdev/rte_flow.h             | 21 +++++++++++++++++++++
> >  lib/librte_ethdev/rte_flow_driver.h      |  5 +++++
> >  4 files changed, 45 insertions(+)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev_version.map
> b/lib/librte_ethdev/rte_ethdev_version.map
> > index a7dacf2cf2..3f32fdecf7 100644
> > --- a/lib/librte_ethdev/rte_ethdev_version.map
> > +++ b/lib/librte_ethdev/rte_ethdev_version.map
> > @@ -227,4 +227,7 @@ EXPERIMENTAL {
> >         rte_flow_dynf_metadata_mask;
> >         rte_flow_dynf_metadata_register;
> >         rte_eth_dev_set_ptypes;
> > +
> > +       # added in 20.02
> > +       rte_flow_dev_dump;
> >  };
> > diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> > index 87a3e8c4c6..751ce721b2 100644
> > --- a/lib/librte_ethdev/rte_flow.c
> > +++ b/lib/librte_ethdev/rte_flow.c
> > @@ -1212,3 +1212,19 @@ rte_flow_expand_rss(struct
> rte_flow_expand_rss *buf, size_t size,
> >         }
> >         return lsize;
> >  }
> > +
> > +int
> > +rte_flow_dev_dump(uint16_t port_id, 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);
> > +
> > +       if (unlikely(!ops))
> > +               return -rte_errno;
> > +       if (likely(!!ops->dev_dump))
> > +               return flow_err(port_id, ops->dev_dump(dev, file, error),
> > +                               error);
> > +       return rte_flow_error_set(error, ENOSYS,
> > +                                 RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > +                                 NULL, rte_strerror(ENOSYS));
> > +}
> > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > index 58b50265d2..cf7cf61ae8 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -2790,6 +2790,27 @@ enum rte_flow_conv_op {
> >         RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
> >  };
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Dump hardware internal representation information of
> > + * rte flow to file.
> > + *
> > + * @param[in] port_id
> > + *    The port identifier of the Ethernet device.
> > + * @param[in] file
> > + *   A pointer to a file for output.
> > + * @param[out] error
> > + *   Perform verbose error reporting if not NULL. PMDs initialize this
> > + *   structure in case of error only.
> > + * @return
> > + *   0 on success, a nagative value otherwise.
> > + */
> > +__rte_experimental
> > +int
> > +rte_flow_dev_dump(uint16_t port_id, 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 a0359853e6..51a9a57a0f 100644
> > --- a/lib/librte_ethdev/rte_flow_driver.h
> > +++ b/lib/librte_ethdev/rte_flow_driver.h
> > @@ -96,6 +96,11 @@ struct rte_flow_ops {
> >                 (struct rte_eth_dev *,
> >                  int,
> >                  struct rte_flow_error *);
> > +       /** See rte_flow_dev_dump(). */
> > +       int (*dev_dump)
> > +               (struct rte_eth_dev *dev,
> > +                FILE *file,
> > +                struct rte_flow_error *error);
> >  };
> >
> >  /**
> > --
> > 2.24.1
> >
Ferruh Yigit Jan. 16, 2020, 8:37 p.m. UTC | #3 
On 1/16/2020 10:14 AM, Xiaoyu Min wrote:
> Introduce an API which dump the device's internal representation
> information of rte flows in hardware.
> 
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>

<...>

> @@ -1212,3 +1212,19 @@ rte_flow_expand_rss(struct rte_flow_expand_rss *buf, size_t size,
>  	}
>  	return lsize;
>  }
> +
> +int
> +rte_flow_dev_dump(uint16_t port_id, 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);
> +
> +	if (unlikely(!ops))
> +		return -rte_errno;
> +	if (likely(!!ops->dev_dump))
> +		return flow_err(port_id, ops->dev_dump(dev, file, error),
> +				error);
> +	return rte_flow_error_set(error, ENOSYS,
> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> +				  NULL, rte_strerror(ENOSYS));
> +}

Should API validate user provided input "FILE *file" ?
Stephen Hemminger Jan. 16, 2020, 10:56 p.m. UTC | #4 
On Thu, 16 Jan 2020 20:37:36 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/16/2020 10:14 AM, Xiaoyu Min wrote:
> > Introduce an API which dump the device's internal representation
> > information of rte flows in hardware.
> > 
> > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>  
> 
> <...>
> 
> > @@ -1212,3 +1212,19 @@ rte_flow_expand_rss(struct rte_flow_expand_rss *buf, size_t size,
> >  	}
> >  	return lsize;
> >  }
> > +
> > +int
> > +rte_flow_dev_dump(uint16_t port_id, 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);
> > +
> > +	if (unlikely(!ops))
> > +		return -rte_errno;
> > +	if (likely(!!ops->dev_dump))
> > +		return flow_err(port_id, ops->dev_dump(dev, file, error),
> > +				error);
> > +	return rte_flow_error_set(error, ENOSYS,
> > +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > +				  NULL, rte_strerror(ENOSYS));
> > +}  
> 
> Should API validate user provided input "FILE *file" ?

None of the other DPDK dump routines do.
Ferruh Yigit Jan. 17, 2020, 9:20 a.m. UTC | #5 
On 1/16/2020 10:56 PM, Stephen Hemminger wrote:
> On Thu, 16 Jan 2020 20:37:36 +0000
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> On 1/16/2020 10:14 AM, Xiaoyu Min wrote:
>>> Introduce an API which dump the device's internal representation
>>> information of rte flows in hardware.
>>>
>>> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>  
>>
>> <...>
>>
>>> @@ -1212,3 +1212,19 @@ rte_flow_expand_rss(struct rte_flow_expand_rss *buf, size_t size,
>>>  	}
>>>  	return lsize;
>>>  }
>>> +
>>> +int
>>> +rte_flow_dev_dump(uint16_t port_id, 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);
>>> +
>>> +	if (unlikely(!ops))
>>> +		return -rte_errno;
>>> +	if (likely(!!ops->dev_dump))
>>> +		return flow_err(port_id, ops->dev_dump(dev, file, error),
>>> +				error);
>>> +	return rte_flow_error_set(error, ENOSYS,
>>> +				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>> +				  NULL, rte_strerror(ENOSYS));
>>> +}  
>>
>> Should API validate user provided input "FILE *file" ?
> 
> None of the other DPDK dump routines do.
> 

You are right, a few samples I checked doesn't validating it, I wonder if this
is intentional decision or just missed the user input validation.
For me, APIs should validate user input.
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index a7dacf2cf2..3f32fdecf7 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -227,4 +227,7 @@  EXPERIMENTAL {
 	rte_flow_dynf_metadata_mask;
 	rte_flow_dynf_metadata_register;
 	rte_eth_dev_set_ptypes;
+
+	# added in 20.02
+	rte_flow_dev_dump;
 };
diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
index 87a3e8c4c6..751ce721b2 100644
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -1212,3 +1212,19 @@  rte_flow_expand_rss(struct rte_flow_expand_rss *buf, size_t size,
 	}
 	return lsize;
 }
+
+int
+rte_flow_dev_dump(uint16_t port_id, 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);
+
+	if (unlikely(!ops))
+		return -rte_errno;
+	if (likely(!!ops->dev_dump))
+		return flow_err(port_id, ops->dev_dump(dev, file, error),
+				error);
+	return rte_flow_error_set(error, ENOSYS,
+				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+				  NULL, rte_strerror(ENOSYS));
+}
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index 58b50265d2..cf7cf61ae8 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -2790,6 +2790,27 @@  enum rte_flow_conv_op {
 	RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
 };
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Dump hardware internal representation information of
+ * rte flow to file.
+ *
+ * @param[in] port_id
+ *    The port identifier of the Ethernet device.
+ * @param[in] file
+ *   A pointer to a file for output.
+ * @param[out] error
+ *   Perform verbose error reporting if not NULL. PMDs initialize this
+ *   structure in case of error only.
+ * @return
+ *   0 on success, a nagative value otherwise.
+ */
+__rte_experimental
+int
+rte_flow_dev_dump(uint16_t port_id, 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 a0359853e6..51a9a57a0f 100644
--- a/lib/librte_ethdev/rte_flow_driver.h
+++ b/lib/librte_ethdev/rte_flow_driver.h
@@ -96,6 +96,11 @@  struct rte_flow_ops {
 		(struct rte_eth_dev *,
 		 int,
 		 struct rte_flow_error *);
+	/** See rte_flow_dev_dump(). */
+	int (*dev_dump)
+		(struct rte_eth_dev *dev,
+		 FILE *file,
+		 struct rte_flow_error *error);
 };
 
 /**