[dpdk-dev,v5,01/13] port: added structures for port stats and config option
Commit Message
Added common data structures for port statistics. Added config option to
enable stats collecting.
Signed-off-by: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
---
config/common_bsdapp | 1 +
config/common_linuxapp | 1 +
lib/librte_port/rte_port.h | 60 ++++++++++++++++++++++++++++++++++++++++----
3 files changed, 57 insertions(+), 5 deletions(-)
Comments
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maciej Gajdzica
> Sent: Friday, June 19, 2015 10:41 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v5 01/13] port: added structures for port stats
> and config option
>
> Added common data structures for port statistics. Added config option to
> enable stats collecting.
>
> Signed-off-by: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
> ---
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
2015-06-19 11:41, Maciej Gajdzica:
> /** Input port interface defining the input port operation */
> struct rte_port_in_ops {
> rte_port_in_op_create f_create; /**< Create */
> rte_port_in_op_free f_free; /**< Free */
> rte_port_in_op_rx f_rx; /**< Packet RX (packet burst) */
> + rte_port_in_op_stats_read f_stats; /**< Stats */
> };
Isn't it breaking an ABI?
> struct rte_port_out_ops {
> - rte_port_out_op_create f_create; /**< Create */
> - rte_port_out_op_free f_free; /**< Free */
> - rte_port_out_op_tx f_tx; /**< Packet TX (single packet) */
> - rte_port_out_op_tx_bulk f_tx_bulk; /**< Packet TX (packet burst) */
> - rte_port_out_op_flush f_flush; /**< Flush */
> + rte_port_out_op_create f_create; /**< Create */
> + rte_port_out_op_free f_free; /**< Free */
> + rte_port_out_op_tx f_tx; /**< Packet TX (single packet) */
> + rte_port_out_op_tx_bulk f_tx_bulk; /**< Packet TX (packet burst) */
> + rte_port_out_op_flush f_flush; /**< Flush */
What is the goal of this change? Breaking the alignment?
> + rte_port_out_op_stats_read f_stats; /**< Stats */
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, June 23, 2015 2:55 PM
> To: Gajdzica, MaciejX T
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 01/13] port: added structures for port
> stats and config option
>
> 2015-06-19 11:41, Maciej Gajdzica:
> > /** Input port interface defining the input port operation */
> > struct rte_port_in_ops {
> > rte_port_in_op_create f_create; /**< Create */
> > rte_port_in_op_free f_free; /**< Free */
> > rte_port_in_op_rx f_rx; /**< Packet RX (packet burst) */
> > + rte_port_in_op_stats_read f_stats; /**< Stats */
> > };
>
> Isn't it breaking an ABI?
This is simply adding a field at the end of the API structure. This structure is instantiated per each port type and its role is very similar to a driver ops structure, for example:
in file "rte_port_ethdev.h": extern struct rte_port_out_ops rte_port_ethdev_writer_ops;
in file "rte_port_ring.h": extern struct rte_port_out_ops rte_port_ring_writer_nodrop_ops;
Typically, instances of these structures are only referenced through pointers by application code (and other libraries, like librte_pipeline), so code that is not aware of this last field in the structure will still continue to work.
The only case I see possible when code will break is if somebody would create an array of such structures, but I think this is not a realistic scenario. Instances of this structure are infrequent: once per port type in librte_port, and new instances are only created when the user wants to create new port type. Basically, instances of this structure are created in isolation and not in bulk (arrays).
Due to this, I do not see this as breaking the API. I think this is the most it could be done to minimize the effect on the ABI will still adding new functionality. Please let me know what you think.
>
> > struct rte_port_out_ops {
> > - rte_port_out_op_create f_create; /**< Create */
> > - rte_port_out_op_free f_free; /**< Free */
> > - rte_port_out_op_tx f_tx; /**< Packet TX (single packet) */
> > - rte_port_out_op_tx_bulk f_tx_bulk; /**< Packet TX (packet burst)
> */
> > - rte_port_out_op_flush f_flush; /**< Flush */
> > + rte_port_out_op_create f_create; /**< Create */
> > + rte_port_out_op_free f_free; /**< Free */
> > + rte_port_out_op_tx f_tx; /**< Packet
> TX (single packet) */
> > + rte_port_out_op_tx_bulk f_tx_bulk; /**< Packet TX
> (packet burst) */
> > + rte_port_out_op_flush f_flush; /**< Flush */
>
> What is the goal of this change? Breaking the alignment?
Shall we submit a new patch revision to fix the alignment of the comments?
>
> > + rte_port_out_op_stats_read f_stats; /**< Stats */
2015-06-23 14:30, Dumitrescu, Cristian:
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > 2015-06-19 11:41, Maciej Gajdzica:
> > > /** Input port interface defining the input port operation */
> > > struct rte_port_in_ops {
> > > rte_port_in_op_create f_create; /**< Create */
> > > rte_port_in_op_free f_free; /**< Free */
> > > rte_port_in_op_rx f_rx; /**< Packet RX (packet burst) */
> > > + rte_port_in_op_stats_read f_stats; /**< Stats */
> > > };
> >
> > Isn't it breaking an ABI?
>
> This is simply adding a field at the end of the API structure. This structure is instantiated per each port type and its role is very similar to a driver ops structure, for example:
>
> in file "rte_port_ethdev.h": extern struct rte_port_out_ops rte_port_ethdev_writer_ops;
> in file "rte_port_ring.h": extern struct rte_port_out_ops rte_port_ring_writer_nodrop_ops;
>
> Typically, instances of these structures are only referenced through pointers by application code (and other libraries, like librte_pipeline), so code that is not aware of this last field in the structure will still continue to work.
>
> The only case I see possible when code will break is if somebody would create an array of such structures, but I think this is not a realistic scenario. Instances of this structure are infrequent: once per port type in librte_port, and new instances are only created when the user wants to create new port type. Basically, instances of this structure are created in isolation and not in bulk (arrays).
Why wouldn't it be a problem even for single instance?
If the application allocates one with old sizeof and the lib is trying to write
in the new field, there can be a problem, no?
Maybe Neil has an opinion?
> Due to this, I do not see this as breaking the API. I think this is the most it could be done to minimize the effect on the ABI will still adding new functionality. Please let me know what you think.
>
> >
> > > struct rte_port_out_ops {
> > > - rte_port_out_op_create f_create; /**< Create */
> > > - rte_port_out_op_free f_free; /**< Free */
> > > - rte_port_out_op_tx f_tx; /**< Packet TX (single packet) */
> > > - rte_port_out_op_tx_bulk f_tx_bulk; /**< Packet TX (packet burst)
> > */
> > > - rte_port_out_op_flush f_flush; /**< Flush */
> > > + rte_port_out_op_create f_create; /**< Create */
> > > + rte_port_out_op_free f_free; /**< Free */
> > > + rte_port_out_op_tx f_tx; /**< Packet
> > TX (single packet) */
> > > + rte_port_out_op_tx_bulk f_tx_bulk; /**< Packet TX
> > (packet burst) */
> > > + rte_port_out_op_flush f_flush; /**< Flush */
> >
> > What is the goal of this change? Breaking the alignment?
>
> Shall we submit a new patch revision to fix the alignment of the comments?
Yes using spaces for alignment would be better.
On Tue, Jun 23, 2015 at 03:55:25PM +0200, Thomas Monjalon wrote:
> 2015-06-19 11:41, Maciej Gajdzica:
> > /** Input port interface defining the input port operation */
> > struct rte_port_in_ops {
> > rte_port_in_op_create f_create; /**< Create */
> > rte_port_in_op_free f_free; /**< Free */
> > rte_port_in_op_rx f_rx; /**< Packet RX (packet burst) */
> > + rte_port_in_op_stats_read f_stats; /**< Stats */
> > };
>
> Isn't it breaking an ABI?
>
This is an interesting question. Strictly speaking, yes it breaks ABI because
we're adding space, and if older applications statically allocate this
structure, it will be smaller than the port library expects, potentially
scribbling over someone elses memory. That said, I'm not sure this structure is
meant to be accessed outside of the library. If it isn't, then we can feel
comfortable that no one is going to access the data structure from code that was
compiled out of sync with the defining library.
The implication if thats true of course is that we should make this structure
opaque outside of the library with a structure prototype and move its definition
into the library private namespace, but I'm fine with doing that at a later date
if the intention is not to have applications touch this structure.
Regards
Neil
> > struct rte_port_out_ops {
> > - rte_port_out_op_create f_create; /**< Create */
> > - rte_port_out_op_free f_free; /**< Free */
> > - rte_port_out_op_tx f_tx; /**< Packet TX (single packet) */
> > - rte_port_out_op_tx_bulk f_tx_bulk; /**< Packet TX (packet burst) */
> > - rte_port_out_op_flush f_flush; /**< Flush */
> > + rte_port_out_op_create f_create; /**< Create */
> > + rte_port_out_op_free f_free; /**< Free */
> > + rte_port_out_op_tx f_tx; /**< Packet TX (single packet) */
> > + rte_port_out_op_tx_bulk f_tx_bulk; /**< Packet TX (packet burst) */
> > + rte_port_out_op_flush f_flush; /**< Flush */
>
> What is the goal of this change? Breaking the alignment?
>
> > + rte_port_out_op_stats_read f_stats; /**< Stats */
>
>
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, June 23, 2015 3:55 PM
> To: Dumitrescu, Cristian
> Cc: Gajdzica, MaciejX T; dev@dpdk.org; nhorman@tuxdriver.com
> Subject: Re: [dpdk-dev] [PATCH v5 01/13] port: added structures for port
> stats and config option
>
> 2015-06-23 14:30, Dumitrescu, Cristian:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > > 2015-06-19 11:41, Maciej Gajdzica:
> > > > /** Input port interface defining the input port operation */
> > > > struct rte_port_in_ops {
> > > > rte_port_in_op_create f_create; /**< Create */
> > > > rte_port_in_op_free f_free; /**< Free */
> > > > rte_port_in_op_rx f_rx; /**< Packet RX (packet burst) */
> > > > + rte_port_in_op_stats_read f_stats; /**< Stats */
> > > > };
> > >
> > > Isn't it breaking an ABI?
> >
> > This is simply adding a field at the end of the API structure. This structure is
> instantiated per each port type and its role is very similar to a driver ops
> structure, for example:
> >
> > in file "rte_port_ethdev.h": extern struct rte_port_out_ops
> rte_port_ethdev_writer_ops;
> > in file "rte_port_ring.h": extern struct rte_port_out_ops
> rte_port_ring_writer_nodrop_ops;
> >
> > Typically, instances of these structures are only referenced through
> pointers by application code (and other libraries, like librte_pipeline), so code
> that is not aware of this last field in the structure will still continue to work.
> >
> > The only case I see possible when code will break is if somebody would
> create an array of such structures, but I think this is not a realistic scenario.
> Instances of this structure are infrequent: once per port type in librte_port,
> and new instances are only created when the user wants to create new port
> type. Basically, instances of this structure are created in isolation and not in
> bulk (arrays).
>
> Why wouldn't it be a problem even for single instance?
> If the application allocates one with old sizeof and the lib is trying to write
> in the new field, there can be a problem, no?
>
The only case when the application is required to create a new instance of this structure is when the application is defining a new port type (unlikely). In this case, the application using the old structure layout is not aware about the statistics functionality, so it will not invoke it, so the library will not attempt to read the f_stats structure field. Since this field is immediately after the old structure layout, the other fields in the structure are not disturbed, so the application works just fine.
The only case when the application using the old structure layout is impacted is when the position of the old structure fields changes, and this can only happen when an array of such structures is created. To my earlier point, this is not realistic, as instances of this structure are created in isolation (single instance, not array of instances) and are accessed through pointers.
> Maybe Neil has an opinion?
>
> > Due to this, I do not see this as breaking the API. I think this is the most it
> could be done to minimize the effect on the ABI will still adding new
> functionality. Please let me know what you think.
> >
> > >
> > > > struct rte_port_out_ops {
> > > > - rte_port_out_op_create f_create; /**< Create */
> > > > - rte_port_out_op_free f_free; /**< Free */
> > > > - rte_port_out_op_tx f_tx; /**< Packet TX (single packet) */
> > > > - rte_port_out_op_tx_bulk f_tx_bulk; /**< Packet TX (packet burst)
> > > */
> > > > - rte_port_out_op_flush f_flush; /**< Flush */
> > > > + rte_port_out_op_create f_create; /**< Create */
> > > > + rte_port_out_op_free f_free; /**< Free */
> > > > + rte_port_out_op_tx f_tx; /**< Packet
> > > TX (single packet) */
> > > > + rte_port_out_op_tx_bulk f_tx_bulk; /**< Packet TX
> > > (packet burst) */
> > > > + rte_port_out_op_flush f_flush; /**< Flush */
> > >
> > > What is the goal of this change? Breaking the alignment?
> >
> > Shall we submit a new patch revision to fix the alignment of the
> comments?
>
> Yes using spaces for alignment would be better.
@@ -383,6 +383,7 @@ CONFIG_RTE_LIBRTE_REORDER=y
# Compile librte_port
#
CONFIG_RTE_LIBRTE_PORT=y
+CONFIG_RTE_PORT_STATS_COLLECT=n
#
# Compile librte_table
@@ -390,6 +390,7 @@ CONFIG_RTE_LIBRTE_REORDER=y
# Compile librte_port
#
CONFIG_RTE_LIBRTE_PORT=y
+CONFIG_RTE_PORT_STATS_COLLECT=n
#
# Compile librte_table
@@ -81,6 +81,12 @@ extern "C" {
Cannot be changed. */
#define RTE_PORT_IN_BURST_SIZE_MAX 64
+/** Input port statistics */
+struct rte_port_in_stats {
+ uint64_t n_pkts_in;
+ uint64_t n_pkts_drop;
+};
+
/**
* Input port create
*
@@ -120,17 +126,42 @@ typedef int (*rte_port_in_op_rx)(
struct rte_mbuf **pkts,
uint32_t n_pkts);
+/**
+ * Input port stats get
+ *
+ * @param port
+ * Handle to output port instance
+ * @param stats
+ * Handle to port_in stats struct to copy data
+ * @param clear
+ * Flag indicating that stats should be cleared after read
+ *
+ * @return
+ * Error code or 0 on success.
+ */
+typedef int (*rte_port_in_op_stats_read)(
+ void *port,
+ struct rte_port_in_stats *stats,
+ int clear);
+
/** Input port interface defining the input port operation */
struct rte_port_in_ops {
rte_port_in_op_create f_create; /**< Create */
rte_port_in_op_free f_free; /**< Free */
rte_port_in_op_rx f_rx; /**< Packet RX (packet burst) */
+ rte_port_in_op_stats_read f_stats; /**< Stats */
};
/*
* Port OUT
*
*/
+/** Output port statistics */
+struct rte_port_out_stats {
+ uint64_t n_pkts_in;
+ uint64_t n_pkts_drop;
+};
+
/**
* Output port create
*
@@ -197,13 +228,32 @@ typedef int (*rte_port_out_op_tx_bulk)(
*/
typedef int (*rte_port_out_op_flush)(void *port);
+/**
+ * Output port stats read
+ *
+ * @param port
+ * Handle to output port instance
+ * @param stats
+ * Handle to port_out stats struct to copy data
+ * @param clear
+ * Flag indicating that stats should be cleared after read
+ *
+ * @return
+ * Error code or 0 on success.
+ */
+typedef int (*rte_port_out_op_stats_read)(
+ void *port,
+ struct rte_port_out_stats *stats,
+ int clear);
+
/** Output port interface defining the output port operation */
struct rte_port_out_ops {
- rte_port_out_op_create f_create; /**< Create */
- rte_port_out_op_free f_free; /**< Free */
- rte_port_out_op_tx f_tx; /**< Packet TX (single packet) */
- rte_port_out_op_tx_bulk f_tx_bulk; /**< Packet TX (packet burst) */
- rte_port_out_op_flush f_flush; /**< Flush */
+ rte_port_out_op_create f_create; /**< Create */
+ rte_port_out_op_free f_free; /**< Free */
+ rte_port_out_op_tx f_tx; /**< Packet TX (single packet) */
+ rte_port_out_op_tx_bulk f_tx_bulk; /**< Packet TX (packet burst) */
+ rte_port_out_op_flush f_flush; /**< Flush */
+ rte_port_out_op_stats_read f_stats; /**< Stats */
};
#ifdef __cplusplus