diff mbox series

[ovs-dev] ovs-thread: Log pthread failures.

Message ID 20240215120005.1780470-1-i.maximets@ovn.org
State Accepted
Commit 46159983d949094e041f6b32cad2c8d6e35084d6
Delegated to: Ilya Maximets
Headers show
Series [ovs-dev] ovs-thread: Log pthread failures. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Ilya Maximets Feb. 15, 2024, noon UTC 
Currently, failures of pthread_* functions are printed to stderr
only and then OVS aborts.  These error messages are hard to find
and may be even just lost.

Use VLOG_ABORT() instead.  It will do the same thing, but will try
to log the error to the log file and syslog first, if configured.

Using VLOG_ABORT() instead of VLOG_FATAL() to preserve the abort()
logic and not just exit with a failure code, because it's likely
we want a core dump if one of these function failed.  For example,
we would like to have a stack trace in a core dump in case a mutex
lock failed with 'deadlock avoided'.

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---

Another thing we can do to improve debugability is to also dump
the stack trace on ovs_abort.  It will also help with debugging
assertions on systems where coredumps are disabled for some reason.
But it's a separate topic.

 lib/ovs-thread.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

Comments

Simon Horman Feb. 15, 2024, 1:38 p.m. UTC | #1 
On Thu, Feb 15, 2024 at 01:00:05PM +0100, Ilya Maximets wrote:
> Currently, failures of pthread_* functions are printed to stderr
> only and then OVS aborts.  These error messages are hard to find
> and may be even just lost.
> 
> Use VLOG_ABORT() instead.  It will do the same thing, but will try
> to log the error to the log file and syslog first, if configured.
> 
> Using VLOG_ABORT() instead of VLOG_FATAL() to preserve the abort()
> logic and not just exit with a failure code, because it's likely
> we want a core dump if one of these function failed.  For example,
> we would like to have a stack trace in a core dump in case a mutex
> lock failed with 'deadlock avoided'.
> 
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
> 
> Another thing we can do to improve debugability is to also dump
> the stack trace on ovs_abort.  It will also help with debugging
> assertions on systems where coredumps are disabled for some reason.
> But it's a separate topic.

Acked-by: Simon Horman <horms@ovn.org>

...
Eelco Chaudron Feb. 21, 2024, 2:34 p.m. UTC | #2 
On 15 Feb 2024, at 13:00, Ilya Maximets wrote:

> Currently, failures of pthread_* functions are printed to stderr
> only and then OVS aborts.  These error messages are hard to find
> and may be even just lost.
>
> Use VLOG_ABORT() instead.  It will do the same thing, but will try
> to log the error to the log file and syslog first, if configured.
>
> Using VLOG_ABORT() instead of VLOG_FATAL() to preserve the abort()
> logic and not just exit with a failure code, because it's likely
> we want a core dump if one of these function failed.  For example,
> we would like to have a stack trace in a core dump in case a mutex
> lock failed with 'deadlock avoided'.
>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Changes look good to me!

Acked-by: Eelco Chaudron <echaudro@redhat.com>

//Eelco
Ilya Maximets Feb. 21, 2024, 10:55 p.m. UTC | #3 
On 2/21/24 15:34, Eelco Chaudron wrote:
> 
> 
> On 15 Feb 2024, at 13:00, Ilya Maximets wrote:
> 
>> Currently, failures of pthread_* functions are printed to stderr
>> only and then OVS aborts.  These error messages are hard to find
>> and may be even just lost.
>>
>> Use VLOG_ABORT() instead.  It will do the same thing, but will try
>> to log the error to the log file and syslog first, if configured.
>>
>> Using VLOG_ABORT() instead of VLOG_FATAL() to preserve the abort()
>> logic and not just exit with a failure code, because it's likely
>> we want a core dump if one of these function failed.  For example,
>> we would like to have a stack trace in a core dump in case a mutex
>> lock failed with 'deadlock avoided'.
>>
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> 
> Changes look good to me!
> 
> Acked-by: Eelco Chaudron <echaudro@redhat.com>

Thanks, Eelco and Simon!

Applied.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
index ac5d2c3d0..f80008061 100644
--- a/lib/ovs-thread.c
+++ b/lib/ovs-thread.c
@@ -63,13 +63,14 @@  static bool multithreaded;
  \
         /* Verify that 'l' was initialized. */ \
         if (OVS_UNLIKELY(!l->where)) { \
-            ovs_abort(0, "%s: %s() passed uninitialized ovs_"#TYPE, \
-                      where, __func__); \
+            VLOG_ABORT("%s: %s() passed uninitialized ovs_"#TYPE, \
+                       where, __func__); \
         } \
  \
         error = pthread_##TYPE##_##FUN(&l->lock); \
         if (OVS_UNLIKELY(error)) { \
-            ovs_abort(error, "%s: pthread_%s_%s failed", where, #TYPE, #FUN); \
+            VLOG_ABORT("%s: pthread_%s_%s failed: %s", where, #TYPE, #FUN, \
+                       ovs_strerror(error)); \
         } \
         l->where = where; \
  }
@@ -91,13 +92,14 @@  LOCK_FUNCTION(spin, lock);
  \
         /* Verify that 'l' was initialized. */ \
         if (OVS_UNLIKELY(!l->where)) { \
-            ovs_abort(0, "%s: %s() passed uninitialized ovs_"#TYPE, \
-                      where, __func__); \
+            VLOG_ABORT("%s: %s() passed uninitialized ovs_"#TYPE, \
+                       where, __func__); \
         } \
  \
         error = pthread_##TYPE##_##FUN(&l->lock); \
         if (OVS_UNLIKELY(error) && error != EBUSY) { \
-            ovs_abort(error, "%s: pthread_%s_%s failed", where, #TYPE, #FUN); \
+            VLOG_ABORT("%s: pthread_%s_%s failed: %s", where, #TYPE, #FUN, \
+                       ovs_strerror(error)); \
         } \
         if (!error) { \
             l->where = where; \
@@ -125,7 +127,8 @@  TRY_LOCK_FUNCTION(spin, trylock);
         l->where = WHERE; \
         error = pthread_##TYPE##_##FUN(&l->lock); \
         if (OVS_UNLIKELY(error)) { \
-            ovs_abort(error, "pthread_%s_%s failed", #TYPE, #FUN); \
+            VLOG_ABORT("%s: pthread_%s_%s failed: %s", l->where, #TYPE, #FUN, \
+                       ovs_strerror(error)); \
         } \
     }
 UNLOCK_FUNCTION(mutex, unlock, "<unlocked>");
@@ -143,7 +146,8 @@  UNLOCK_FUNCTION(spin, destroy, NULL);
     {                                                   \
         int error = FUNCTION(arg1);                     \
         if (OVS_UNLIKELY(error)) {                      \
-            ovs_abort(error, "%s failed", #FUNCTION);   \
+            VLOG_ABORT("%s failed: %s", #FUNCTION,      \
+                       ovs_strerror(error));            \
         }                                               \
     }
 #define XPTHREAD_FUNC2(FUNCTION, PARAM1, PARAM2)        \
@@ -152,7 +156,8 @@  UNLOCK_FUNCTION(spin, destroy, NULL);
     {                                                   \
         int error = FUNCTION(arg1, arg2);               \
         if (OVS_UNLIKELY(error)) {                      \
-            ovs_abort(error, "%s failed", #FUNCTION);   \
+            VLOG_ABORT("%s failed: %s", #FUNCTION,      \
+                       ovs_strerror(error));            \
         }                                               \
     }
 #define XPTHREAD_FUNC3(FUNCTION, PARAM1, PARAM2, PARAM3)\
@@ -161,7 +166,8 @@  UNLOCK_FUNCTION(spin, destroy, NULL);
     {                                                   \
         int error = FUNCTION(arg1, arg2, arg3);         \
         if (OVS_UNLIKELY(error)) {                      \
-            ovs_abort(error, "%s failed", #FUNCTION);   \
+            VLOG_ABORT("%s failed: %s", #FUNCTION,      \
+                       ovs_strerror(error));            \
         }                                               \
     }
 
@@ -204,7 +210,7 @@  ovs_mutex_init__(const struct ovs_mutex *l_, int type)
     xpthread_mutexattr_settype(&attr, type);
     error = pthread_mutex_init(&l->lock, &attr);
     if (OVS_UNLIKELY(error)) {
-        ovs_abort(error, "pthread_mutex_init failed");
+        VLOG_ABORT("pthread_mutex_init failed: %s", ovs_strerror(error));
     }
     xpthread_mutexattr_destroy(&attr);
 }
@@ -257,7 +263,7 @@  ovs_rwlock_init(const struct ovs_rwlock *l_)
 #endif
 
     if (OVS_UNLIKELY(error)) {
-        ovs_abort(error, "pthread_rwlock_init failed");
+        VLOG_ABORT("pthread_rwlock_init failed: %s", ovs_strerror(error));
     }
 }
 
@@ -275,7 +281,7 @@  ovs_mutex_cond_wait(pthread_cond_t *cond, const struct ovs_mutex *mutex_)
     error = pthread_cond_wait(cond, &mutex->lock);
 
     if (OVS_UNLIKELY(error)) {
-        ovs_abort(error, "pthread_cond_wait failed");
+        VLOG_ABORT("pthread_cond_wait failed: %s", ovs_strerror(error));
     }
 }
 
@@ -289,7 +295,7 @@  ovs_spin_init__(const struct ovs_spin *l_, int pshared)
     l->where = "<unlocked>";
     error = pthread_spin_init(&l->lock, pshared);
     if (OVS_UNLIKELY(error)) {
-        ovs_abort(error, "pthread_spin_init failed");
+        VLOG_ABORT("pthread_spin_init failed: %s", ovs_strerror(error));
     }
 }
 
@@ -431,13 +437,15 @@  set_min_stack_size(pthread_attr_t *attr, size_t min_stacksize)
 
     error = pthread_attr_getstacksize(attr, &stacksize);
     if (error) {
-        ovs_abort(error, "pthread_attr_getstacksize failed");
+        VLOG_ABORT("pthread_attr_getstacksize failed: %s",
+                   ovs_strerror(error));
     }
 
     if (stacksize < min_stacksize) {
         error = pthread_attr_setstacksize(attr, min_stacksize);
         if (error) {
-            ovs_abort(error, "pthread_attr_setstacksize failed");
+            VLOG_ABORT("pthread_attr_setstacksize failed: %s",
+                       ovs_strerror(error));
         }
     }
 }
@@ -486,7 +494,7 @@  ovs_thread_create(const char *name, void *(*start)(void *), void *arg)
 
     error = pthread_create(&thread, &attr, ovsthread_wrapper, aux);
     if (error) {
-        ovs_abort(error, "pthread_create failed");
+        VLOG_ABORT("pthread_create failed: %s", ovs_strerror(error));
     }
     pthread_attr_destroy(&attr);
     return thread;