Re: [PATCH v3] ether: add support for vtune task tracing

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Tue, 11 Jul 2017 20:48:06 +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 v3] 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"
> 

Plenty of checkpatch errors found in the patch. Verify the checkpatch
issues with ./devtools/checkpatches.sh

I think, this vtune tracing support deserves to added a section in
http://dpdk.org/doc/guides/prog_guide/profile_app.html


> Signed-off-by: Ilia Kurakin <ilia.kurakin@intel.com>
> 
> ---
> 
> -V2 change:
>     ITT tasks collection is moved to rx callback
> 
> -V3 change:
>     rte_ethdev_profile.c created, all profile specific code moved there.
> 
>     Added generic profile function
> 
> 
>  config/common_base                    |   1 +
>  lib/librte_ether/Makefile             |   1 +
>  lib/librte_ether/rte_ethdev.c         |   4 +
>  lib/librte_ether/rte_ethdev_profile.c | 150 ++++++++++++++++++++++++++++++++++
>  lib/librte_ether/rte_ethdev_profile.h |  51 ++++++++++++
>  5 files changed, 207 insertions(+)
>  create mode 100644 lib/librte_ether/rte_ethdev_profile.c
>  create mode 100644 lib/librte_ether/rte_ethdev_profile.h
> 
> diff --git a/config/common_base b/config/common_base
> index 8ae6e92..dda51db 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_PROFILE_ITT_WASTED_RX_ITERATIONS=n
>  
>  #
>  # Turn off Tx preparation stage
> diff --git a/lib/librte_ether/Makefile b/lib/librte_ether/Makefile
> index 93fdde1..3c86ec6 100644
> --- a/lib/librte_ether/Makefile
> +++ b/lib/librte_ether/Makefile
> @@ -45,6 +45,7 @@ LIBABIVER := 6
>  
>  SRCS-y += rte_ethdev.c
>  SRCS-y += rte_flow.c
> +SRCS-y += rte_ethdev_profile.c
>  
>  #
>  # Export include files
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 76179fd..f4ec119 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -67,6 +67,7 @@
>  
>  #include "rte_ether.h"
>  #include "rte_ethdev.h"
> +#include "rte_ethdev_profile.h"
>  
>  static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
>  struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS];
> @@ -825,6 +826,9 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>  		return diag;
>  	}
>  
> +	/* See rte_ethdev_profile.h to find comments on code below. */
> +	rte_eth_rx_profile_init(port_id, dev);

I prefer to change to rte_eth_profile_rx_init for better name space.

> +
>  	return 0;
>  }
>  
> diff --git a/lib/librte_ether/rte_ethdev_profile.c b/lib/librte_ether/rte_ethdev_profile.c
> new file mode 100644
> index 0000000..029fbba
> --- /dev/null
> +++ b/lib/librte_ether/rte_ethdev_profile.c
> @@ -0,0 +1,150 @@
> +/*-
> + *   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.
> + */
> +
> +#include "rte_ethdev_profile.h"
> +
> +/**
> + * This conditional block is responsible for RX queues profiling using the
> + * Instrumentation and Tracing Technology (ITT) API, employed by the
> + * Intel VTune TM Amplifier.
> + */
> +#ifdef RTE_ETHDEV_PROFILE_ITT_WASTED_RX_ITERATIONS
> +
> +#include <rte_config.h>

Shouldn't the rte_config.h moved up to bring
RTE_ETHDEV_PROFILE_ITT_WASTED_RX_ITERATIONS definition.

> +#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 rx_queue_num
> + *  The number of RX queues on specified port.
> + */
> +static inline void
> +rte_eth_init_itt(uint8_t port_id, char *port_name, uint8_t rx_queue_num)
> +{
> +	uint16_t q_id;
> +	for (q_id = 0; q_id < rx_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 /* RTE_ETHDEV_PROFILE_ITT_WASTED_RX_ITERATIONS */
> +
> +void
> +rte_eth_rx_profile_init(__rte_unused uint8_t port_id,
> +	__rte_unused struct rte_eth_dev *dev)
> +{
> +#ifdef RTE_ETHDEV_PROFILE_ITT_WASTED_RX_ITERATIONS
> +	rte_eth_init_itt(port_id, dev->data->name, dev->data->nb_rx_queues);

You may not need to start with rte_ for location functions and i think,
it is better to reference rx in this function name.