[ovs-dev] [PATCH] fatal-signal: Skip acquiring log fd lock.

William Tu u9012063 at gmail.com
Tue Mar 24 04:26:03 UTC 2020


On Mon, Mar 23, 2020 at 4:13 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>
> On 3/23/20 11:34 PM, William Tu wrote:
> > On Mon, Mar 23, 2020 at 2:58 PM William Tu <u9012063 at gmail.com> wrote:
> >>
> >> On Mon, Mar 23, 2020 at 2:34 PM Ben Pfaff <blp at ovn.org> wrote:
> >>>
> >>> On Mon, Mar 23, 2020 at 02:22:17PM -0700, William Tu wrote:
> >>>> Due to not acquiring lock, clang reports:
> >>>>   lib/vlog.c:618:12: error: reading variable 'log_fd' requires holding mutex
> >>>>   'log_file_mutex' [-Werror,-Wthread-safety-analysis]
> >>>>   return log_fd;
> >>>> The patch fixes by skipping it using OVS_NO_THREAD_SAFETY_ANALYSIS.
> >>>>
> >>>> Tested-at: https://travis-ci.org/github/williamtu/ovs-travis/builds/666051210
> >>>> Fixex: ecd4a8fcdff2 ("fatal-signal: Log backtrace when no monitor daemon.")
> >>>> Signed-off-by: William Tu <u9012063 at gmail.com>
> >>>
> >>> You can't just mark the existing send_backtrace_to_monitor()
> >>> OVS_NO_THREAD_SAFETY_ANALYSIS?  It seems like the easiest change.
> >> OK
> >> that will also work.
> >>>
> >>> Also, the OVS_REQUIRES should also go in vlog.h.
> >
> > Adding 'OVS_REQUIRES(log_file_mutext)' to vlog.h has error like this:
> > ./include/openvswitch/vlog.h:147:36: error: use of undeclared
> > identifier 'log_file_mutex'
> > int vlog_get_fd(void) OVS_REQUIRES(log_file_mutex);
> >
> > I will have to move log_file_mutex and pattern_rwlock to vlog.h.
> > Do we want this?
> > Does it make difference if OVS_REQUIRES is only used in C file
> > but not in header file?
>
> To avoid exposure of the vlog internals, maybe we could create a new function like:
>
> /*
>  * This function writes directly to log file without using async writer or
>  * taking a lock.  Caller must hold 'log_file_mutex' or be sure that it's
>  * not necessary.  Could be used in exceptional cases like dumping of backtrace
>  * on fatal signals.
>  */
> void
> vlog_direct_write_to_log_file_unsafe(const char *s)
>     OVS_NO_THREAD_SAFETY_ANALYSIS
> {
>     if (log_fd >= 0) {
>         ignore(write(log_fd, s, strlen(s)));
>     }
> }
>

Hi Ilya,

Do you know any restrictions when calling functions in signal handler?
When use this approach, the backtrace does not print anything.
I'm still debugging it with the below patch:

diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
index 476bf3d6d5b4..886fce5e0fd5 100644
--- a/include/openvswitch/vlog.h
+++ b/include/openvswitch/vlog.h
@@ -143,8 +143,8 @@ void vlog_set_syslog_method(const char *method);
 /* Configure syslog target. */
 void vlog_set_syslog_target(const char *target);

-/* Return the log_fd. */
-int vlog_get_fd(void);
+/* Write directly to log file. */
+void vlog_direct_write_to_log_file_unsafe(const char *s);

 /* Initialization. */
 void vlog_init(void);
diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c
index 4965c1ae82c0..51cf628d994e 100644
--- a/lib/fatal-signal.c
+++ b/lib/fatal-signal.c
@@ -197,11 +197,7 @@ send_backtrace_to_monitor(void) {
          */
         char str[] = "SIGSEGV detected, backtrace:\n";

-        if (vlog_get_fd() < 0) {
-            return;
-        }
-
-        ignore(write(vlog_get_fd(), str, strlen(str)));
+        vlog_direct_write_to_log_file_unsafe(str);

         for (int i = 0; i < dep; i++) {
             char line[64];
@@ -210,7 +206,7 @@ send_backtrace_to_monitor(void) {
                      unw_bt[i].ip,
                      unw_bt[i].func,
                      unw_bt[i].offset);
-            ignore(write(vlog_get_fd(), line, strlen(line)));
+            vlog_direct_write_to_log_file_unsafe(line);
         }
     }
 }
diff --git a/lib/vlog.c b/lib/vlog.c
index 502b33fc831e..6d17d4837e9c 100644
--- a/lib/vlog.c
+++ b/lib/vlog.c
@@ -612,10 +612,19 @@ vlog_set_syslog_target(const char *target)
     ovs_rwlock_unlock(&pattern_rwlock);
 }

-int
-vlog_get_fd(void)
+/*
+ * This function writes directly to log file without using async writer or
+ * taking a lock.  Caller must hold 'log_file_mutex' or be sure that it's
+ * not necessary.  Could be used in exceptional cases like dumping of backtrace
+ * on fatal signals.
+ */
+void
+vlog_direct_write_to_log_file_unsafe(const char *s)
+    OVS_NO_THREAD_SAFETY_ANALYSIS
 {
-    return log_fd;
+    if (log_fd >= 0) {
+        ignore(write(log_fd, s, strlen(s)));
+    }
 }

 /* Returns 'false' if 'facility' is not a valid string. If 'facility'


More information about the dev mailing list