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

[Jerin Jacob]

-----Original Message-----
> Date: Thu, 6 Jul 2017 19:42:52 +0300
> From: ilia.kurakin at intel.com
> To: dev at dpdk.org
> CC: jerin.jacob at caviumnetworks.com, konstantin.ananyev at intel.com,
>  keith.wiles at intel.com, dmitry.galanov at intel.com, Ilia Kurakin
>  <ilia.kurakin at 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 at 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 at 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.