From b8320b8021856ae61b3012b82c2ae96df97e3ec4 Mon Sep 17 00:00:00 2001 From: Yabin Cui Date: Tue, 21 Jul 2015 17:27:54 -0700 Subject: [PATCH] Don't abort when failed to write tracing message. Also make the code thread-safe with lock. Bug: 20666100 Change-Id: I0f331a617b75280f36179c187418450230d713ef (cherry picked from commit 166112531558a1d4ea179c29147f27db7045db22) --- libc/bionic/bionic_systrace.cpp | 85 ++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 38 deletions(-) diff --git a/libc/bionic/bionic_systrace.cpp b/libc/bionic/bionic_systrace.cpp index 10521cf01..103aa8f71 100644 --- a/libc/bionic/bionic_systrace.cpp +++ b/libc/bionic/bionic_systrace.cpp @@ -21,6 +21,7 @@ #include #include +#include "private/bionic_lock.h" #include "private/bionic_systrace.h" #include "private/libc_logging.h" @@ -29,12 +30,17 @@ #define WRITE_OFFSET 32 -static const prop_info* g_pinfo = NULL; +constexpr char SYSTRACE_PROPERTY_NAME[] = "debug.atrace.tags.enableflags"; + +static Lock g_lock; +static const prop_info* g_pinfo; static uint32_t g_serial = -1; -static uint64_t g_tags = 0; +static uint64_t g_tags; static int g_trace_marker_fd = -1; static bool should_trace() { + bool result = false; + g_lock.lock(); // If g_pinfo is null, this means that systrace hasn't been run and it's safe to // assume that no trace writing will need to take place. However, to avoid running // this costly find check each time, we set it to a non-tracing value so that next @@ -42,32 +48,39 @@ static bool should_trace() { // this function also deals with the bootup case, during which the call to property // set will fail if the property server hasn't yet started. if (g_pinfo == NULL) { - g_pinfo = __system_property_find("debug.atrace.tags.enableflags"); + g_pinfo = __system_property_find(SYSTRACE_PROPERTY_NAME); if (g_pinfo == NULL) { - __system_property_set("debug.atrace.tags.enableflags", "0"); - g_pinfo = __system_property_find("debug.atrace.tags.enableflags"); - if (g_pinfo == NULL) { - return false; - } + __system_property_set(SYSTRACE_PROPERTY_NAME, "0"); + g_pinfo = __system_property_find(SYSTRACE_PROPERTY_NAME); } } - - // Find out which tags have been enabled on the command line and set - // the value of tags accordingly. If the value of the property changes, - // the serial will also change, so the costly system_property_read function - // can be avoided by calling the much cheaper system_property_serial - // first. The values within pinfo may change, but its location is guaranteed - // not to move. - const uint32_t cur_serial = __system_property_serial(g_pinfo); - if (cur_serial != g_serial) { - g_serial = cur_serial; - char value[PROP_VALUE_MAX]; - __system_property_read(g_pinfo, 0, value); - g_tags = strtoull(value, NULL, 0); + if (g_pinfo != NULL) { + // Find out which tags have been enabled on the command line and set + // the value of tags accordingly. If the value of the property changes, + // the serial will also change, so the costly system_property_read function + // can be avoided by calling the much cheaper system_property_serial + // first. The values within pinfo may change, but its location is guaranteed + // not to move. + uint32_t cur_serial = __system_property_serial(g_pinfo); + if (cur_serial != g_serial) { + g_serial = cur_serial; + char value[PROP_VALUE_MAX]; + __system_property_read(g_pinfo, 0, value); + g_tags = strtoull(value, NULL, 0); + } + result = ((g_tags & ATRACE_TAG_BIONIC) != 0); } + g_lock.unlock(); + return result; +} - // Finally, verify that this tag value enables bionic tracing. - return ((g_tags & ATRACE_TAG_BIONIC) != 0); +static int get_trace_marker_fd() { + g_lock.lock(); + if (g_trace_marker_fd == -1) { + g_trace_marker_fd = open("/sys/kernel/debug/tracing/trace_marker", O_CLOEXEC | O_WRONLY); + } + g_lock.unlock(); + return g_trace_marker_fd; } ScopedTrace::ScopedTrace(const char* message) { @@ -75,11 +88,9 @@ ScopedTrace::ScopedTrace(const char* message) { return; } - if (g_trace_marker_fd == -1) { - g_trace_marker_fd = open("/sys/kernel/debug/tracing/trace_marker", O_CLOEXEC | O_WRONLY); - if (g_trace_marker_fd == -1) { - __libc_fatal("Could not open kernel trace file: %s\n", strerror(errno)); - } + int trace_marker_fd = get_trace_marker_fd(); + if (trace_marker_fd == -1) { + return; } // If bionic tracing has been enabled, then write the message to the @@ -87,12 +98,10 @@ ScopedTrace::ScopedTrace(const char* message) { int length = strlen(message); char buf[length + WRITE_OFFSET]; size_t len = snprintf(buf, length + WRITE_OFFSET, "B|%d|%s", getpid(), message); - ssize_t wbytes = TEMP_FAILURE_RETRY(write(g_trace_marker_fd, buf, len)); - // Error while writing - if (static_cast(wbytes) != len) { - __libc_fatal("Could not write to kernel trace file: %s\n", strerror(errno)); - } + // Tracing may stop just after checking property and before writing the message. + // So the write is acceptable to fail. See b/20666100. + TEMP_FAILURE_RETRY(write(trace_marker_fd, buf, len)); } ScopedTrace::~ScopedTrace() { @@ -100,10 +109,10 @@ ScopedTrace::~ScopedTrace() { return; } - ssize_t wbytes = TEMP_FAILURE_RETRY(write(g_trace_marker_fd, "E", 1)); - - // Error while writing - if (static_cast(wbytes) != 1) { - __libc_fatal("Could not write to kernel trace file: %s\n", strerror(errno)); + int trace_marker_fd = get_trace_marker_fd(); + if (trace_marker_fd == -1) { + return; } + + TEMP_FAILURE_RETRY(write(trace_marker_fd, "E", 1)); }