New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: Add -finstrument-functions flag to OSD code #15055

Merged
merged 5 commits into from May 28, 2017

Conversation

Projects
None yet
5 participants
@mogeb
Contributor

mogeb commented May 11, 2017

Done using -DWITH_OSD_INSTRUMENT_FUNCTIONS=ON to CMake.

@liewegas liewegas changed the title from Add -finstrument-functions flag to OSD code to cmake: Add -finstrument-functions flag to OSD code May 18, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented May 18, 2017

@alimaredia mind taking a quick look at this?

@tchaikov tchaikov self-requested a review May 18, 2017

@alimaredia

This comment has been minimized.

Contributor

alimaredia commented May 18, 2017

retest this please

@alimaredia

This comment has been minimized.

Contributor

alimaredia commented May 18, 2017

@liewegas upon a quick glance it looks fine to be as long as it builds and passes make check. @mogeb If I build this PR just to make sure it build, I'd only need the WITH_LTTNG and WITH_OSD_INSTRUMENT_FUNCTIONS to be ON

@mogeb

This comment has been minimized.

Contributor

mogeb commented May 18, 2017

@alimaredia that is correct

@@ -56,6 +56,10 @@ TracepointProvider::Traits osd_tracepoint_traits("libosd_tp.so",
"osd_tracing");
TracepointProvider::Traits os_tracepoint_traits("libos_tp.so",
"osd_objectstore_tracing");
#ifdef WITH_OSD_INSTRUMENT_FUNCTIONS
TracepointProvider::Traits cyg_profile_traits("libcyg_profile_tp.so",

This comment has been minimized.

@tchaikov

tchaikov May 18, 2017

Contributor

@mogeb i am curious about what "cyg" stands for? =)

This comment has been minimized.

@mogeb

mogeb May 18, 2017

Contributor

@tchaikov GCC names the added functions "__cyg_profile_func_enter" and "__cyg_profile_func_exit". By looking online, it seems to be because it was added by Cygnus Solutions into GCC. So pretty much an arbitrary name. :)

This comment has been minimized.

@tchaikov

tchaikov May 18, 2017

Contributor

oic, thanks for the explanation!

@@ -437,6 +437,9 @@ if(${WITH_LTTNG})
endif()
endif(${WITH_LTTNG})
#option for OSD function instrumentation
option(WITH_OSD_INSTRUMENT_FUNCTIONS "Enable function instrumentation for OSD code" OFF)

This comment has been minimized.

@tchaikov

tchaikov May 18, 2017

Contributor

this comment is not necessary, imho. the code comments itself.

This comment has been minimized.

@mogeb

mogeb May 18, 2017

Contributor

I will remove it shortly

if(WITH_OSD_INSTRUMENT_FUNCTIONS)
set(GCC_C_FLAGS "-finstrument-functions")
set(GCC_C_FLAGS "${GCC_C_FLAGS} -finstrument-functions-exclude-function-list=_mm_loadu_si128,_mm_cmpeq_epi32,_mm_movemask_epi8")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${GCC_C_FLAGS}")

This comment has been minimized.

@cbodley

cbodley May 18, 2017

Contributor

are these flags specific to gcc, or does clang support them too?

This comment has been minimized.

@mogeb

mogeb May 18, 2017

Contributor

@cbodley I hadn't checked that, good point! I'm not too familiar with clang, but it seems -finstrument-functions is supported and behaves the same way based on quick runs I did. However, it doesn't support -finstrument-functions-exclude-function-list yet. Should I add make a check for GCC around this whole WITH_OSD_INSTRUMENT_FUNCTIONS config?

This comment has been minimized.

@cbodley

cbodley May 19, 2017

Contributor

i guess the criteria here would be: is the feature useful for clang without the -exclude part? if not, gate the whole thing on gcc

This comment has been minimized.

@mogeb

mogeb May 23, 2017

Contributor

The -exclude part is to resolve a link-time issue. The functions listed in the -exclude part are inline, but GCC tries to instrument them as well. I updated the code and documentation of this PR to support WITH_OSD_INSTRUMENT_FUNCTIONS on Clang, but with no effect. Clang might eventually support the -exclude option (there are patches for it online).

@alimaredia

This comment has been minimized.

Contributor

alimaredia commented May 19, 2017

The PR built just fine for me with the two options I specified above, so I don't see any reason not to merge it from a build perspective.

@mogeb do you have any instructions for building it from source in a new VM to running it and viewing tracepoints?

@mogeb

This comment has been minimized.

Contributor

mogeb commented May 23, 2017

@alimaredia yes, given you configured with WITH_LTTNG=ON and WITH_OSD_INSTRUMENT_FUNCTIONS=ON. Add the following to your ceph.conf:

[osd]
osd tracing = true
osd function tracing = true

You should have LTTng installed. Make sure the LTTng daemon is running, otherwise start it with:

$> sudo lttng-sessiond -d

After vstarting Ceph, make sure that LTTng can list the function entry and exit tracepoints (one entry/exit per OSD), as follows:

$> lttng list -u | grep cyg
      lttng_ust_cyg_profile:func_exit (loglevel: TRACE_DEBUG_LINE (13)) (type: tracepoint)
      lttng_ust_cyg_profile:func_entry (loglevel: TRACE_DEBUG_LINE (13)) (type: tracepoint)
      lttng_ust_cyg_profile:func_exit (loglevel: TRACE_DEBUG_LINE (13)) (type: tracepoint)
      lttng_ust_cyg_profile:func_entry (loglevel: TRACE_DEBUG_LINE (13)) (type: tracepoint)
      lttng_ust_cyg_profile:func_exit (loglevel: TRACE_DEBUG_LINE (13)) (type: tracepoint)
      lttng_ust_cyg_profile:func_entry (loglevel: TRACE_DEBUG_LINE (13)) (type: tracepoint)

I was planning to add a test for this following this PR.

@alimaredia

This comment has been minimized.

Contributor

alimaredia commented May 25, 2017

@tchaikov @cbodley anything else blocking this from merging?

@tchaikov

modulo the nit, lgtm.

@@ -437,6 +437,9 @@ if(${WITH_LTTNG})
endif()
endif(${WITH_LTTNG})
#option for OSD function instrumentation

This comment has been minimized.

@tchaikov

tchaikov May 25, 2017

Contributor

nit, i'd recommend drop this comment.

This comment has been minimized.

@mogeb

mogeb May 25, 2017

Contributor

@tchaikov done

mogeb added some commits May 11, 2017

cmake: configuration for OSD function instrumentation
Signed-off-by: Mohamad Gebai <mgebai@suse.com>
tracing: add cyg_profile tracepoint provider
Signed-off-by: Mohamad Gebai <mgebai@suse.com>
tracing: add -finstrument-functions for OSD
Signed-off-by: Mohamad Gebai <mgebai@suse.com>
tracing: add documentation for function instrumentation
Signed-off-by: Mohamad Gebai <mgebai@suse.com>
tracing: remove -pie if using function instrumentation
Signed-off-by: Mohamad Gebai <mgebai@suse.com>

@tchaikov tchaikov merged commit ef9d93b into ceph:master May 28, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@mogeb mogeb deleted the mogeb:wip-with-instrument-functions branch May 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment