Userspace RCU library internal error handling

Hi,

Currently, liburcu calls "exit(-1)" upon internal consistency error. This is not pretty, and usually frowned upon in libraries.

One example of failure path where we use this is if pthread_mutex_lock() would happen to fail within synchronize_rcu(). Clearly, this should _never_ happen: it would typically be triggered only by memory corruption (or other terrible things like that). That being said, we clearly don't want to make "synchronize_rcu()" return errors like that to the application, because it would complexify the application error handling needlessly.

So instead of calling exit(-1), one possibility would be to do something like this:

#include <signal.h>
#include <pthread.h>
#include <stdio.h>

#define urcu_die(fmt, ...)                      \
        do {    \
                fprintf(stderr, fmt, ##__VA_ARGS__);    \
                (void) pthread_kill(pthread_self(), SIGBUS);    \
        } while (0)

and call urcu_die(); in those "unrecoverable error" cases, instead of calling exit(-1). Therefore, if an application chooses to trap those signals, it can, which is otherwise not possible with a direct call to exit().

Currently, liburcu calls "exit(-1)" upon internal consistency error. This is not pretty, and usually frowned upon in libraries.

One example of failure path where we use this is if pthread_mutex_lock() would happen to fail within synchronize_rcu(). Clearly, this should _never_ happen: it would typically be triggered only by memory corruption (or other terrible things like that). That being said, we clearly don't want to make "synchronize_rcu()" return errors like that to the application, because it would complexify the application error handling needlessly.

So instead of calling exit(-1), one possibility would be to do something like this:

#include <signal.h>
#include <pthread.h>
#include <stdio.h>

#define urcu_die(fmt, ...)                      \
        do {    \
                fprintf(stderr, fmt, ##__VA_ARGS__);    \
                (void) pthread_kill(pthread_self(), SIGBUS);    \
        } while (0)

and call urcu_die(); in those "unrecoverable error" cases, instead of calling exit(-1). Therefore, if an application chooses to trap those signals, it can, which is otherwise not possible with a direct call to exit().

Quoted 2 lines · Mathieu Desnoyers

Currently, liburcu calls "exit(-1)" upon internal consistency error. This is not pretty, and usually frowned upon in libraries.

Agreed.

Quoted 7 lines

One example of failure path where we use this is if pthread_mutex_lock() would happen to fail within synchronize_rcu(). Clearly, this should _never_ happen: it would typically be triggered only by memory corruption (or other terrible things like that). That being said, we clearly don't want to make "synchronize_rcu()" return errors like that to the application, because it would complexify the application error handling needlessly.

I think you can safely ignore any error conditions you know you can't trigger. pthread_mutex_lock can only return an error under two
conditions: an uninitialized mutex, or an error-checking mutex already locked by the current thread. Neither of those can happen in this case. Given that, I'd suggest either calling pthread_mutex_lock and ignoring any possibility of error, or adding an assert.

Quoted 13 lines

So instead of calling exit(-1), one possibility would be to do something like this:

#include <signal.h>
#include <pthread.h>
#include <stdio.h>

#define urcu_die(fmt, ...)                      \
        do {    \
                fprintf(stderr, fmt, ##__VA_ARGS__);    \
                (void) pthread_kill(pthread_self(), SIGBUS);    \
        } while (0)

Quoted 4 lines

and call urcu_die(); in those "unrecoverable error" cases, instead of calling exit(-1). Therefore, if an application chooses to trap those signals, it can, which is otherwise not possible with a direct call to exit().

It looks like you want to use signals as a kind of exception mechanism, to allow the application to clean up (though not to recover). assert seems much clearer to me for "this can't happen" cases, and assert also generates a signal that the application can catch and clean up.

Quoted 39 lines · * Josh Triplett ([email protected]) wrote:

Quoted 2 lines · Mathieu Desnoyers

Currently, liburcu calls "exit(-1)" upon internal consistency error. This is not pretty, and usually frowned upon in libraries.

Agreed.

Quoted 7 lines

One example of failure path where we use this is if pthread_mutex_lock() would happen to fail within synchronize_rcu(). Clearly, this should _never_ happen: it would typically be triggered only by memory corruption (or other terrible things like that). That being said, we clearly don't want to make "synchronize_rcu()" return errors like that to the application, because it would complexify the application error handling needlessly.

I think you can safely ignore any error conditions you know you can't trigger. pthread_mutex_lock can only return an error under two
conditions: an uninitialized mutex, or an error-checking mutex already locked by the current thread. Neither of those can happen in this case. Given that, I'd suggest either calling pthread_mutex_lock and ignoring any possibility of error, or adding an assert.

Quoted 13 lines

So instead of calling exit(-1), one possibility would be to do something like this:

#include <signal.h>
#include <pthread.h>
#include <stdio.h>

#define urcu_die(fmt, ...)                      \
        do {    \
                fprintf(stderr, fmt, ##__VA_ARGS__);    \
                (void) pthread_kill(pthread_self(), SIGBUS);    \
        } while (0)

Quoted 4 lines

and call urcu_die(); in those "unrecoverable error" cases, instead of calling exit(-1). Therefore, if an application chooses to trap those signals, it can, which is otherwise not possible with a direct call to exit().

Quoted 4 lines

It looks like you want to use signals as a kind of exception mechanism, to allow the application to clean up (though not to recover). assert seems much clearer to me for "this can't happen" cases, and assert also generates a signal that the application can catch and clean up.

Within discussions with other LTTng developers, we considered the the assert, but the thought that this case might be silently ignored on production systems (which compile with assertions disabled) makes me uncomfortable. This is why I would prefer a SIGBUS to an assertion.

Using assert() would be similar to turning the Linux kernel BUG_ON() mechanism into no-ops on production systems because "it should never happen" (tm) ;-)

Quoted 39 lines · * Josh Triplett ([email protected]) wrote:

Quoted 2 lines · Mathieu Desnoyers

Currently, liburcu calls "exit(-1)" upon internal consistency error. This is not pretty, and usually frowned upon in libraries.

Agreed.

Quoted 7 lines

One example of failure path where we use this is if pthread_mutex_lock() would happen to fail within synchronize_rcu(). Clearly, this should _never_ happen: it would typically be triggered only by memory corruption (or other terrible things like that). That being said, we clearly don't want to make "synchronize_rcu()" return errors like that to the application, because it would complexify the application error handling needlessly.

I think you can safely ignore any error conditions you know you can't trigger. pthread_mutex_lock can only return an error under two
conditions: an uninitialized mutex, or an error-checking mutex already locked by the current thread. Neither of those can happen in this case. Given that, I'd suggest either calling pthread_mutex_lock and ignoring any possibility of error, or adding an assert.

Quoted 13 lines

So instead of calling exit(-1), one possibility would be to do something like this:

#include <signal.h>
#include <pthread.h>
#include <stdio.h>

#define urcu_die(fmt, ...)                      \
        do {    \
                fprintf(stderr, fmt, ##__VA_ARGS__);    \
                (void) pthread_kill(pthread_self(), SIGBUS);    \
        } while (0)

Quoted 4 lines

and call urcu_die(); in those "unrecoverable error" cases, instead of calling exit(-1). Therefore, if an application chooses to trap those signals, it can, which is otherwise not possible with a direct call to exit().

Quoted 4 lines

It looks like you want to use signals as a kind of exception mechanism, to allow the application to clean up (though not to recover). assert seems much clearer to me for "this can't happen" cases, and assert also generates a signal that the application can catch and clean up.

Within discussions with other LTTng developers, we considered the the assert, but the thought that this case might be silently ignored on production systems (which compile with assertions disabled) makes me uncomfortable. This is why I would prefer a SIGBUS to an assertion.

Using assert() would be similar to turning the Linux kernel BUG_ON() mechanism into no-ops on production systems because "it should never happen" (tm) ;-)

Quoted 50 lines · Mathieu Desnoyers

Quoted 39 lines · * Josh Triplett ([email protected]) wrote:

Quoted 2 lines · Mathieu Desnoyers

Currently, liburcu calls "exit(-1)" upon internal consistency error. This is not pretty, and usually frowned upon in libraries.

Agreed.

Quoted 7 lines

One example of failure path where we use this is if pthread_mutex_lock() would happen to fail within synchronize_rcu(). Clearly, this should _never_ happen: it would typically be triggered only by memory corruption (or other terrible things like that). That being said, we clearly don't want to make "synchronize_rcu()" return errors like that to the application, because it would complexify the application error handling needlessly.

I think you can safely ignore any error conditions you know you can't trigger. pthread_mutex_lock can only return an error under two
conditions: an uninitialized mutex, or an error-checking mutex already locked by the current thread. Neither of those can happen in this case. Given that, I'd suggest either calling pthread_mutex_lock and ignoring any possibility of error, or adding an assert.

Quoted 13 lines

So instead of calling exit(-1), one possibility would be to do something like this:

#include <signal.h>
#include <pthread.h>
#include <stdio.h>

#define urcu_die(fmt, ...)                      \
        do {    \
                fprintf(stderr, fmt, ##__VA_ARGS__);    \
                (void) pthread_kill(pthread_self(), SIGBUS);    \
        } while (0)

Quoted 4 lines

and call urcu_die(); in those "unrecoverable error" cases, instead of calling exit(-1). Therefore, if an application chooses to trap those signals, it can, which is otherwise not possible with a direct call to exit().

Quoted 4 lines

It looks like you want to use signals as a kind of exception mechanism, to allow the application to clean up (though not to recover). assert seems much clearer to me for "this can't happen" cases, and assert also generates a signal that the application can catch and clean up.

Within discussions with other LTTng developers, we considered the the assert, but the thought that this case might be silently ignored on production systems (which compile with assertions disabled) makes me uncomfortable. This is why I would prefer a SIGBUS to an assertion.

Quoted 3 lines

Using assert() would be similar to turning the Linux kernel BUG_ON() mechanism into no-ops on production systems because "it should never happen" (tm) ;-)

Just don't define NDEBUG then. :)

Quoted 55 lines · * Josh Triplett ([email protected]) wrote:

Quoted 53 lines · Mathieu Desnoyers

Quoted 39 lines · * Josh Triplett ([email protected]) wrote:

Quoted 2 lines · Mathieu Desnoyers

Currently, liburcu calls "exit(-1)" upon internal consistency error. This is not pretty, and usually frowned upon in libraries.

Agreed.

Quoted 7 lines

One example of failure path where we use this is if pthread_mutex_lock() would happen to fail within synchronize_rcu(). Clearly, this should _never_ happen: it would typically be triggered only by memory corruption (or other terrible things like that). That being said, we clearly don't want to make "synchronize_rcu()" return errors like that to the application, because it would complexify the application error handling needlessly.

I think you can safely ignore any error conditions you know you can't trigger. pthread_mutex_lock can only return an error under two
conditions: an uninitialized mutex, or an error-checking mutex already locked by the current thread. Neither of those can happen in this case. Given that, I'd suggest either calling pthread_mutex_lock and ignoring any possibility of error, or adding an assert.

Quoted 13 lines

So instead of calling exit(-1), one possibility would be to do something like this:

#include <signal.h>
#include <pthread.h>
#include <stdio.h>

#define urcu_die(fmt, ...)                      \
        do {    \
                fprintf(stderr, fmt, ##__VA_ARGS__);    \
                (void) pthread_kill(pthread_self(), SIGBUS);    \
        } while (0)

Quoted 4 lines

and call urcu_die(); in those "unrecoverable error" cases, instead of calling exit(-1). Therefore, if an application chooses to trap those signals, it can, which is otherwise not possible with a direct call to exit().

Quoted 4 lines

It looks like you want to use signals as a kind of exception mechanism, to allow the application to clean up (though not to recover). assert seems much clearer to me for "this can't happen" cases, and assert also generates a signal that the application can catch and clean up.

Within discussions with other LTTng developers, we considered the the assert, but the thought that this case might be silently ignored on production systems (which compile with assertions disabled) makes me uncomfortable. This is why I would prefer a SIGBUS to an assertion.

Using assert() would be similar to turning the Linux kernel BUG_ON() mechanism into no-ops on production systems because "it should never happen" (tm) ;-)

Quoted 1 line

Just don't define NDEBUG then. :)

Well, AFAIK, it is usual for some distribution packages to define NDEBUG (maybe distro maintainers reading lttng-dev could confirm or infirm this assumption ?). So in a context where upstream does not have total control on the specific tweaks done by distro packages, I prefer not to rely on NDEBUG not being defined to catch internal consistency errors in the wild.

Quoted 56 lines · Mathieu Desnoyers

Quoted 55 lines · * Josh Triplett ([email protected]) wrote:

Quoted 53 lines · Mathieu Desnoyers

Quoted 39 lines · * Josh Triplett ([email protected]) wrote:

Quoted 2 lines · Mathieu Desnoyers

Currently, liburcu calls "exit(-1)" upon internal consistency error. This is not pretty, and usually frowned upon in libraries.

Agreed.

Quoted 7 lines

One example of failure path where we use this is if pthread_mutex_lock() would happen to fail within synchronize_rcu(). Clearly, this should _never_ happen: it would typically be triggered only by memory corruption (or other terrible things like that). That being said, we clearly don't want to make "synchronize_rcu()" return errors like that to the application, because it would complexify the application error handling needlessly.

I think you can safely ignore any error conditions you know you can't trigger. pthread_mutex_lock can only return an error under two
conditions: an uninitialized mutex, or an error-checking mutex already locked by the current thread. Neither of those can happen in this case. Given that, I'd suggest either calling pthread_mutex_lock and ignoring any possibility of error, or adding an assert.

Quoted 13 lines

So instead of calling exit(-1), one possibility would be to do something like this:

#include <signal.h>
#include <pthread.h>
#include <stdio.h>

#define urcu_die(fmt, ...)                      \
        do {    \
                fprintf(stderr, fmt, ##__VA_ARGS__);    \
                (void) pthread_kill(pthread_self(), SIGBUS);    \
        } while (0)

Quoted 4 lines

and call urcu_die(); in those "unrecoverable error" cases, instead of calling exit(-1). Therefore, if an application chooses to trap those signals, it can, which is otherwise not possible with a direct call to exit().

Quoted 4 lines

It looks like you want to use signals as a kind of exception mechanism, to allow the application to clean up (though not to recover). assert seems much clearer to me for "this can't happen" cases, and assert also generates a signal that the application can catch and clean up.

Within discussions with other LTTng developers, we considered the the assert, but the thought that this case might be silently ignored on production systems (which compile with assertions disabled) makes me uncomfortable. This is why I would prefer a SIGBUS to an assertion.

Using assert() would be similar to turning the Linux kernel BUG_ON() mechanism into no-ops on production systems because "it should never happen" (tm) ;-)

Quoted 8 lines

Quoted 1 line

Just don't define NDEBUG then. :)

Well, AFAIK, it is usual for some distribution packages to define NDEBUG (maybe distro maintainers reading lttng-dev could confirm or infirm this assumption ?). So in a context where upstream does not have total control on the specific tweaks done by distro packages, I prefer not to rely on NDEBUG not being defined to catch internal consistency errors in the wild.

#undef NDEBUG
#include <assert.h>

Or if you don't consider that sufficient for some reason, you could define your own assert(), but that seems like an odd thing to not count on. Nonetheless, if you define your own assert, I'd still suggest making it look as much like assert() as possible, including the call to abort().

Quoted 8 lines · * Josh Triplett ([email protected]) wrote:

#undef NDEBUG
#include <assert.h>

Or if you don't consider that sufficient for some reason, you could define your own assert(), but that seems like an odd thing to not count on. Nonetheless, if you define your own assert, I'd still suggest making it look as much like assert() as possible, including the call to abort().

#undef NDEBUG is unwanted, due to its side-effects. We use "assert()" in other locations of the code, for which we want the assertion check to be disabled if NDEBUG is defined in production.

I agree with you that calling "abort()" is exactly what we want, and it's much more standard that sending a signal chosen with a fair roll of dices. How about the following ?

[...]

urcu-die.h new file +28 -0

new file mode 100644
index 0000000..227c8dc
@@ -0,0 +1,37 @@
+#ifndef _URCU_DIE_H
+#define _URCU_DIE_H
+
+/*
+ * urcu-die.h
+ *
+ * Userspace RCU library unrecoverable error handling
+ *
+ * Copyright (c) 2012 Mathieu Desnoyers <[email protected]>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>