[ovs-dev] [PATCH V12] Function tracer to trace all function calls

Ben Pfaff blp at ovn.org
Sun Jul 3 04:09:24 UTC 2016


On Tue, Jun 28, 2016 at 07:02:06PM -0700, nghosh at us.ibm.com wrote:
> From: Nirapada Ghosh <nghosh at us.ibm.com>
> 
> In some circumstances, we might need to figure out where in
> code, the CPU time is being spent most, so as to pinpoint
> the bottleneck and thereby resolve it with proper changes.
> Using '-finstrument-functions' flag, that can be achieved, and
> this patch exactly does that.
> 
> There is a python file [generate_ft_report.py] with the patch,
> that may be used to convert this trace output to a human readable
> format with symbol names instead of address and their execution
> times. This tool uses addr2line that expects the executable to
> be built with -g flag.
> 
> To enable this feature, ovs needs needs to be configured with
> "--enable-ft" command line argument [i.e. configure --enable-ft]
> 
> This instrumentation logs the tracing output in separate log files
> namely func_trace_<pid>.log. It does not use VLOG mechanism for
> logging as that will make the patch very complicated to avoid
> recursion in the trace routine.
> 
> This feature starts dumping output, only in debug mode, which means
> ovs-appctl -t <module> vlog/set any:any:dbg should be used to enable
> this logging.
> 
> Currently, only ovn-northd, ovn-controller, vswitchd are instrumented.
> 
> It is intended to be used for debugging purposes.
> Signed-off-by: Nirapada Ghosh <nghosh at us.ibm.com>

Thanks for the patch!  I have some comments.

We usually only put trivial invocations of macros directly into
configure.ac.  Please put your new code into a macro and invoke it from
configure.ac.

The customary form for this usage of Autoconf would look more like this:

AC_ARG_ENABLE([ft],
  [AC_HELP_STRING([--enable-ft], [Turn on function tracing])],
  [case "${enableval}" in
    (yes) ft=true ;;
    (no)  ft=false ;;
    (*) AC_MSG_ERROR([bad value ${enableval} for --enable-ft]) ;;
  esac],
  [ft=false])
AM_CONDITIONAL([ENABLE_FT], [test x$ft = xtrue])

This patch adds raw use of __attribute__ to various .c and .h files.
That will break with MSVC, the compiler that OVS uses on Windows.
Please use the preprocessor appropriately to avoid the problem.

The vlog_directory() function seems really confused.  It appears to
open-code ovs_strlcpy(), twice.

The name is_cmd_line_args_parsed() is terrible grammar.

Why is there a g_ prefix on some names?  It doesn't follow any OVS
convention for names.

I really don't like this new is_cmd_line_args_parsed() interface.  Can
you figure out another way that doesn't add new code to each
instrumented daemon?

Why does this add a file in the third-party/ directory?  This directory
is only for patches to third-party programs.  Anything that is compiled
into OVS itself should by definition not go in there.

This includes no comments or documentation.  It needs some.  An
explanation in INSTALL.md would be a place to start.

Thanks,

Ben.



More information about the dev mailing list