[dpdk-dev,v2] ether: add support for vtune task tracing

Message ID 1499359372-6658-1-git-send-email-ilia.kurakin@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation fail Compilation issues

Commit Message

ilia.kurakin@intel.com July 6, 2017, 4:42 p.m. UTC
  From: Ilia Kurakin <ilia.kurakin@intel.com>

The patch adds tracing of loop iterations that yielded no packets in a DPDK
application. It is using ITT task API:
    https://software.intel.com/en-us/node/544206

We suppose the flow of using this tracing would assume the user has ITT lib
and header on machine and re-build DPDK with additional make parameters:

    make EXTRA_CFLAGS=-I<path to ittnotify.h>
         EXTRA_LDLIBS="-L<path to libittnotify.a> -littnotify"

Signed-off-by: Ilia Kurakin <ilia.kurakin@intel.com>

---

-V2 change:
    ITT tasks collection is moved to rx callback


 config/common_base                    |   1 +
 lib/librte_ether/Makefile             |   2 +
 lib/librte_ether/rte_ethdev.c         |   9 +++
 lib/librte_ether/rte_ethdev_profile.h | 137 ++++++++++++++++++++++++++++++++++
 4 files changed, 149 insertions(+)
 create mode 100644 lib/librte_ether/rte_ethdev_profile.h
  

Comments

Jerin Jacob July 10, 2017, 12:30 p.m. UTC | #1
-----Original Message-----
> Date: Thu, 6 Jul 2017 19:42:52 +0300
> From: ilia.kurakin@intel.com
> To: dev@dpdk.org
> CC: jerin.jacob@caviumnetworks.com, konstantin.ananyev@intel.com,
>  keith.wiles@intel.com, dmitry.galanov@intel.com, Ilia Kurakin
>  <ilia.kurakin@intel.com>
> Subject: [PATCH v2] ether: add support for vtune task tracing
> X-Mailer: git-send-email 2.7.4
> 
> From: Ilia Kurakin <ilia.kurakin@intel.com>
> 
> The patch adds tracing of loop iterations that yielded no packets in a DPDK
> application. It is using ITT task API:
>     https://software.intel.com/en-us/node/544206
> 
> We suppose the flow of using this tracing would assume the user has ITT lib
> and header on machine and re-build DPDK with additional make parameters:
> 
>     make EXTRA_CFLAGS=-I<path to ittnotify.h>
>          EXTRA_LDLIBS="-L<path to libittnotify.a> -littnotify"
> 
> Signed-off-by: Ilia Kurakin <ilia.kurakin@intel.com>
> 
> ---
> 
> -V2 change:
>     ITT tasks collection is moved to rx callback
> 
> 
>  config/common_base                    |   1 +
>  lib/librte_ether/Makefile             |   2 +
>  lib/librte_ether/rte_ethdev.c         |   9 +++
>  lib/librte_ether/rte_ethdev_profile.h | 137 ++++++++++++++++++++++++++++++++++

Please create rte_ethdev_profile.c for the actual source code.

>  4 files changed, 149 insertions(+)
>  create mode 100644 lib/librte_ether/rte_ethdev_profile.h
> 
> diff --git a/config/common_base b/config/common_base
> index 660588a..8795a95 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -136,6 +136,7 @@ CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
>  CONFIG_RTE_LIBRTE_IEEE1588=n
>  CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
>  CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> +CONFIG_RTE_ETHDEV_TRACE_ITT_WASTED_RX_ITERATIONS=n

Should we change to CONFIG_RTE_ETHDEV_PROFILE_ITT_WASTED_RX_ITERATIONS
as its going to under rte_ethdev_profile.c

>  
>  #
>  # Turn off Tx preparation stage
> diff --git a/lib/librte_ether/Makefile b/lib/librte_ether/Makefile
> index 93fdde1..4d4017f 100644
> --- a/lib/librte_ether/Makefile
> +++ b/lib/librte_ether/Makefile
> @@ -56,5 +56,7 @@ SYMLINK-y-include += rte_eth_ctrl.h
>  SYMLINK-y-include += rte_dev_info.h
>  SYMLINK-y-include += rte_flow.h
>  SYMLINK-y-include += rte_flow_driver.h
> +SYMLINK-${CONFIG_RTE_ETHDEV_TRACE_ITT_WASTED_RX_ITERATIONS}-include \
> +	+= rte_ethdev_profile.h

With your current scheme,  you don't need to expose this header to
application as application is not directly including it.

One option is, Wherever there is reference to this header file in ethdev library, you
can update the CFLAGS in lib/librte_ether/Makefile

>  
>  include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 957ae2a..a99eeea 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -68,6 +68,10 @@
>  #include "rte_ether.h"
>  #include "rte_ethdev.h"
>  
> +#ifdef RTE_ETHDEV_TRACE_ITT_WASTED_RX_ITERATIONS

You can avoid conditional compilation here by including
rte_ethdev_profile.h always and ifdef in the rte_ethdev_profile.c
source code. With that scheme, rte_ethdev.c will be clean.

> +#include "rte_ethdev_profile.h"
> +#endif
> +
>  static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
>  struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS];
>  static struct rte_eth_dev_data *rte_eth_dev_data;
> @@ -825,6 +829,11 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>  		return diag;
>  	}
>  
> +#ifdef RTE_ETHDEV_TRACE_ITT_WASTED_RX_ITERATIONS

Same as above.

Call a generic function from here say(rte_eth_rx_profile_init()) and
add ifdef in rte_ethdev_profile.c

> +	/* See rte_ethdev_profile.h to find comments on code below. */
> +	rte_eth_init_itt(port_id, dev->data->name, nb_rx_q);
> +#endif
> +
>  	return 0;
>  }
>  

Other than the above comments, it looks good to me.
  

Patch

diff --git a/config/common_base b/config/common_base
index 660588a..8795a95 100644
--- a/config/common_base
+++ b/config/common_base
@@ -136,6 +136,7 @@  CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
 CONFIG_RTE_LIBRTE_IEEE1588=n
 CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
 CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
+CONFIG_RTE_ETHDEV_TRACE_ITT_WASTED_RX_ITERATIONS=n
 
 #
 # Turn off Tx preparation stage
diff --git a/lib/librte_ether/Makefile b/lib/librte_ether/Makefile
index 93fdde1..4d4017f 100644
--- a/lib/librte_ether/Makefile
+++ b/lib/librte_ether/Makefile
@@ -56,5 +56,7 @@  SYMLINK-y-include += rte_eth_ctrl.h
 SYMLINK-y-include += rte_dev_info.h
 SYMLINK-y-include += rte_flow.h
 SYMLINK-y-include += rte_flow_driver.h
+SYMLINK-${CONFIG_RTE_ETHDEV_TRACE_ITT_WASTED_RX_ITERATIONS}-include \
+	+= rte_ethdev_profile.h
 
 include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 957ae2a..a99eeea 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -68,6 +68,10 @@ 
 #include "rte_ether.h"
 #include "rte_ethdev.h"
 
+#ifdef RTE_ETHDEV_TRACE_ITT_WASTED_RX_ITERATIONS
+#include "rte_ethdev_profile.h"
+#endif
+
 static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
 struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS];
 static struct rte_eth_dev_data *rte_eth_dev_data;
@@ -825,6 +829,11 @@  rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		return diag;
 	}
 
+#ifdef RTE_ETHDEV_TRACE_ITT_WASTED_RX_ITERATIONS
+	/* See rte_ethdev_profile.h to find comments on code below. */
+	rte_eth_init_itt(port_id, dev->data->name, nb_rx_q);
+#endif
+
 	return 0;
 }
 
diff --git a/lib/librte_ether/rte_ethdev_profile.h b/lib/librte_ether/rte_ethdev_profile.h
new file mode 100644
index 0000000..95d49e9
--- /dev/null
+++ b/lib/librte_ether/rte_ethdev_profile.h
@@ -0,0 +1,137 @@ 
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2017 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_ETHDEV_PROFILE_H_
+#define _RTE_ETHDEV_PROFILE_H_
+
+#include <rte_config.h>
+#include <rte_ethdev.h>
+#include <ittnotify.h>
+
+#define ITT_MAX_NAME_LEN (100)
+
+/**
+ * Auxiliary ITT structure belonging to port and using to:
+ *   -  track queue state to determine whether it is wasting loop iterations
+ *   -  begin or end ITT task using task domain and name
+ */
+struct rte_eth_itt_aux_data {
+	/**
+	 * ITT domains for each queue.
+	 */
+	__itt_domain *wasted_iter_domains[RTE_MAX_QUEUES_PER_PORT];
+	/**
+	 * ITT task names for each queue.
+	 */
+	__itt_string_handle *wasted_iter_handles[RTE_MAX_QUEUES_PER_PORT];
+	/**
+	 * Flags indicating the queues state. Possible values:
+	 * 1 - queue is wasting iterations, 0 - otherwise.
+	 */
+	uint8_t queue_is_wasting_iters[RTE_MAX_QUEUES_PER_PORT];
+};
+
+/**
+ * The pool of *rte_eth_itt_aux_data* structures.
+ */
+struct rte_eth_itt_aux_data itt_aux_data[RTE_MAX_ETHPORTS];
+
+
+/**
+ * This callback function manages ITT tasks collection on given port and queue.
+ * It must be registered with rte_eth_add_rx_callback() to be called from
+ * rte_eth_rx_burst(). To find more comments see rte_rx_callback_fn function
+ * type declaration.
+ */
+static uint16_t
+collect_itt_rx_burst_cb(uint8_t port_id, uint16_t queue_id,
+	__rte_unused struct rte_mbuf *pkts[], uint16_t nb_pkts,
+	__rte_unused uint16_t max_pkts, __rte_unused void *user_param)
+{
+	if (unlikely(nb_pkts == 0)) {
+		if (!itt_aux_data[port_id].queue_is_wasting_iters[queue_id]) {
+			__itt_task_begin(
+				itt_aux_data[port_id].wasted_iter_domains[queue_id],
+				__itt_null, __itt_null,
+				itt_aux_data[port_id].wasted_iter_handles[queue_id]);
+			itt_aux_data[port_id].queue_is_wasting_iters[queue_id] = 1;
+		}
+	} else {
+		if (unlikely(itt_aux_data[port_id].queue_is_wasting_iters[queue_id])) {
+			__itt_task_end(
+				itt_aux_data[port_id].wasted_iter_domains[queue_id]);
+			itt_aux_data[port_id].queue_is_wasting_iters[queue_id] = 0;
+		}
+	}
+	return nb_pkts;
+}
+
+/**
+ * Initialization of rte_eth_itt_aux_data for a given port.
+ * This function must be invoked when ethernet device is being configured.
+ * Result will be stored in the global array *itt_aux_data*.
+ *
+ * @param port_id
+ *  The port identifier of the Ethernet device.
+ * @param port_name
+ *  The name of the Ethernet device.
+ * @param queue_num
+ *  The number of queues on specified port.
+ */
+static inline void
+rte_eth_init_itt(uint8_t port_id, char *port_name, uint8_t queue_num)
+{
+	uint16_t q_id;
+	for (q_id = 0; q_id < queue_num; ++q_id) {
+		char domain_name[ITT_MAX_NAME_LEN];
+		snprintf(domain_name, sizeof(domain_name),
+			"RXBurst.WastedIterations.Port_%s.Queue_%d",
+			port_name, q_id);
+		itt_aux_data[port_id].wasted_iter_domains[q_id]
+			= __itt_domain_create(domain_name);
+
+		char task_name[ITT_MAX_NAME_LEN];
+		snprintf(task_name, sizeof(task_name),
+			"port id: %d; queue id: %d",
+			port_id, q_id);
+		itt_aux_data[port_id].wasted_iter_handles[q_id]
+			= __itt_string_handle_create(task_name);
+
+		itt_aux_data[port_id].queue_is_wasting_iters[q_id] = 0;
+
+		rte_eth_add_rx_callback(
+			port_id, q_id, collect_itt_rx_burst_cb, NULL);
+	}
+}
+
+#endif