(no subject)

[Norbert Lange]

This is V2 as the first version had alot unnecessary whitespace changes
(otherwise empty lines with ws removed)

[PATCH v2 1/5] libcobalt: improve mutex autoinit support

[Norbert Lange]

contrary to some comments, mutexes are automatically
initialised on lock/unlock.
Correct the destroy method to not report an
error on such mutexes.

{get,set}prioceiling requires mutexes that were explicitely
initialised, so no change needed there

Signed-off-by: Norbert Lange <norbert.lange@andritz.com>
---
 lib/cobalt/mutex.c | 178 +++++++++++++++++++++++----------------------
 1 file changed, 92 insertions(+), 86 deletions(-)

diff --git a/lib/cobalt/mutex.c b/lib/cobalt/mutex.c
index ed32bba32..be9f6ed80 100644
--- a/lib/cobalt/mutex.c
+++ b/lib/cobalt/mutex.c
@@ -164,70 +164,51 @@ COBALT_IMPL(int, pthread_mutex_init, (pthread_mutex_t *mutex,
 }
 
 /**
- * Destroy a mutex.
- *
- * This service destroys the mutex @a mx, if it is unlocked and not referenced
- * by any condition variable. The mutex becomes invalid for all mutex services
- * (they all return the EINVAL error) except pthread_mutex_init().
- *
- * @param mutex the mutex to be destroyed.
- *
- * @return 0 on success,
- * @return an error number if:
- * - EINVAL, the mutex @a mx is invalid;
- * - EPERM, the mutex is not process-shared and does not belong to the current
- *   process;
- * - EBUSY, the mutex is locked, or used by a condition variable.
+ * Test if a mutex structure contains an valid autoinitializer.
  *
- * @see
- * <a href="http://www.opengroup.org/onlinepubs/000095399/functions/pthread_mutex_destroy.html">
- * Specification.</a>
- *
- * @apitags{thread-unrestricted}
+ * @return the mutex type on success,
+ * @return -1 if not in supported autoinitializer state
  */
-COBALT_IMPL(int, pthread_mutex_destroy, (pthread_mutex_t *mutex))
+static int __attribute__((cold)) _cobalt_mutex_autoinit_type(const pthread_mutex_t *mutex)
 {
-	struct cobalt_mutex_shadow *_mutex =
-		&((union cobalt_mutex_union *)mutex)->shadow_mutex;
-	int err;
-
-	if (_mutex->magic != COBALT_MUTEX_MAGIC)
-		return EINVAL;
-
-	err = XENOMAI_SYSCALL1(sc_cobalt_mutex_destroy, _mutex);
-
-	return -err;
-}
-
-static int __attribute__((cold)) cobalt_mutex_autoinit(pthread_mutex_t *mutex)
-{
-	static pthread_mutex_t uninit_normal_mutex =
-		PTHREAD_MUTEX_INITIALIZER;
+	static const pthread_mutex_t mutex_initializers[] = {
+#if HAVE_DECL_PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
+		PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP,
+#endif
 #if HAVE_DECL_PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
-	static pthread_mutex_t uninit_recursive_mutex =
-		PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
+		PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP,
 #endif
+		PTHREAD_MUTEX_INITIALIZER
+	};
+	static const int mutex_types[] =
+	{
 #if HAVE_DECL_PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
-	static pthread_mutex_t uninit_errorcheck_mutex =
-		PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+		PTHREAD_MUTEX_ERRORCHECK_NP,
 #endif
-	struct cobalt_mutex_shadow *_mutex =
-		&((union cobalt_mutex_union *)mutex)->shadow_mutex;
+#if HAVE_DECL_PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
+		PTHREAD_MUTEX_RECURSIVE_NP,
+#endif
+		PTHREAD_MUTEX_DEFAULT
+	};
+
+	unsigned i = sizeof(mutex_types) / sizeof(mutex_types[0]);
+	while (i-- > 0)
+	{
+		if (memcmp(mutex, &mutex_initializers[i], sizeof(mutex_initializers[0])) == 0)
+			return mutex_types[i];
+	}
+	return -1;
+}
+
+static int __attribute__((cold)) _cobalt_mutex_doautoinit(union cobalt_mutex_union *umutex)
+{
+	struct cobalt_mutex_shadow *_mutex = &umutex->shadow_mutex;
 	int err __attribute__((unused));
 	pthread_mutexattr_t mattr;
 	int ret = 0, type;
 
-	if (memcmp(mutex, &uninit_normal_mutex, sizeof(*mutex)) == 0)
-		type = PTHREAD_MUTEX_DEFAULT;
-#if HAVE_DECL_PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
-	else if (memcmp(mutex, &uninit_recursive_mutex, sizeof(*mutex)) == 0)
-		type = PTHREAD_MUTEX_RECURSIVE_NP;
-#endif
-#if HAVE_DECL_PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
-	else if (memcmp(mutex, &uninit_errorcheck_mutex, sizeof(*mutex)) == 0)
-		type = PTHREAD_MUTEX_ERRORCHECK_NP;
-#endif
-	else
+	type = _cobalt_mutex_autoinit_type(&umutex->native_mutex);
+	if (type < 0)
 		return EINVAL;
 
 	pthread_mutexattr_init(&mattr);
@@ -238,7 +219,7 @@ static int __attribute__((cold)) cobalt_mutex_autoinit(pthread_mutex_t *mutex)
 		goto out;
 	}
 	if (_mutex->magic != COBALT_MUTEX_MAGIC)
-		ret = __COBALT(pthread_mutex_init(mutex, &mattr));
+		ret = __COBALT(pthread_mutex_init(&umutex->native_mutex, &mattr));
 	err = __COBALT(pthread_mutex_unlock(cobalt_autoinit_mutex));
 	if (err) {
 		if (ret == 0)
@@ -251,6 +232,49 @@ static int __attribute__((cold)) cobalt_mutex_autoinit(pthread_mutex_t *mutex)
 	return ret;
 }
 
+static inline int _cobalt_mutex_autoinit(union cobalt_mutex_union *umutex)
+{
+	if (unlikely(umutex->shadow_mutex.magic != COBALT_MUTEX_MAGIC))
+		return _cobalt_mutex_doautoinit(umutex);
+	return 0;
+}
+
+/**
+ * Destroy a mutex.
+ *
+ * This service destroys the mutex @a mx, if it is unlocked and not referenced
+ * by any condition variable. The mutex becomes invalid for all mutex services
+ * (they all return the EINVAL error) except pthread_mutex_init().
+ *
+ * @param mutex the mutex to be destroyed.
+ *
+ * @return 0 on success,
+ * @return an error number if:
+ * - EINVAL, the mutex @a mx is invalid;
+ * - EPERM, the mutex is not process-shared and does not belong to the current
+ *   process;
+ * - EBUSY, the mutex is locked, or used by a condition variable.
+ *
+ * @see
+ * <a href="http://www.opengroup.org/onlinepubs/000095399/functions/pthread_mutex_destroy.html">
+ * Specification.</a>
+ *
+ * @apitags{thread-unrestricted}
+ */
+COBALT_IMPL(int, pthread_mutex_destroy, (pthread_mutex_t *mutex))
+{
+	struct cobalt_mutex_shadow *_mutex =
+		&((union cobalt_mutex_union *)mutex)->shadow_mutex;
+	int err;
+
+	if (unlikely(_mutex->magic != COBALT_MUTEX_MAGIC))
+		return (_cobalt_mutex_autoinit_type(mutex) < 0) ? EINVAL : 0;
+
+	err = XENOMAI_SYSCALL1(sc_cobalt_mutex_destroy, _mutex);
+
+	return -err;
+}
+
 /**
  * Lock a mutex.
  *
@@ -296,15 +320,15 @@ COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
 	if (cur == XN_NO_HANDLE)
 		return EPERM;
 
-	if (_mutex->magic != COBALT_MUTEX_MAGIC)
-		goto autoinit;
+	ret = _cobalt_mutex_autoinit((union cobalt_mutex_union *)mutex);
+	if (ret)
+		return ret;
 
 	/*
 	 * We track resource ownership for auto-relax of non real-time
 	 * shadows and some debug features, so we must always obtain
 	 * them via a syscall.
 	 */
-start:
 	status = cobalt_get_current_mode();
 	if ((status & (XNRELAX|XNWEAK|XNDEBUG)) == 0) {
 		if (_mutex->attr.protocol == PTHREAD_PRIO_PROTECT)
@@ -360,11 +384,6 @@ protect:
 	u_window->pp_pending = _mutex->handle;
 	lazy_protect = 1;
 	goto fast_path;
-autoinit:
-	ret = cobalt_mutex_autoinit(mutex);
-	if (ret)
-		return ret;
-	goto start;
 }
 
 /**
@@ -411,11 +430,11 @@ COBALT_IMPL(int, pthread_mutex_timedlock, (pthread_mutex_t *mutex,
 	if (cur == XN_NO_HANDLE)
 		return EPERM;
 
-	if (_mutex->magic != COBALT_MUTEX_MAGIC)
-		goto autoinit;
+	ret = _cobalt_mutex_autoinit((union cobalt_mutex_union *)mutex);
+	if (ret)
+		return ret;
 
 	/* See __cobalt_pthread_mutex_lock() */
-start:
 	status = cobalt_get_current_mode();
 	if ((status & (XNRELAX|XNWEAK|XNDEBUG)) == 0) {
 		if (_mutex->attr.protocol == PTHREAD_PRIO_PROTECT)
@@ -471,11 +490,6 @@ protect:
 	u_window->pp_pending = _mutex->handle;
 	lazy_protect = 1;
 	goto fast_path;
-autoinit:
-	ret = cobalt_mutex_autoinit(mutex);
-	if (ret)
-		return ret;
-	goto start;
 }
 
 /**
@@ -515,9 +529,10 @@ COBALT_IMPL(int, pthread_mutex_trylock, (pthread_mutex_t *mutex))
 	if (cur == XN_NO_HANDLE)
 		return EPERM;
 
-	if (_mutex->magic != COBALT_MUTEX_MAGIC)
-		goto autoinit;
-start:
+	ret = _cobalt_mutex_autoinit((union cobalt_mutex_union *)mutex);
+	if (ret)
+		return ret;
+
 	status = cobalt_get_current_mode();
 	if ((status & (XNRELAX|XNWEAK|XNDEBUG)) == 0) {
 		if (_mutex->attr.protocol == PTHREAD_PRIO_PROTECT)
@@ -558,11 +573,6 @@ slow_path:
 		_mutex->lockcnt = 1;
 
 	return -ret;
-autoinit:
-	ret = cobalt_mutex_autoinit(mutex);
-	if (ret)
-		return ret;
-	goto start;
 protect:
 	u_window = cobalt_get_current_window();
 	/*
@@ -611,9 +621,10 @@ COBALT_IMPL(int, pthread_mutex_unlock, (pthread_mutex_t *mutex))
 	xnhandle_t cur;
 	int err;
 
-	if (_mutex->magic != COBALT_MUTEX_MAGIC)
-		goto autoinit;
-start:
+	err = _cobalt_mutex_autoinit((union cobalt_mutex_union *)mutex);
+	if (err)
+		return err;
+
 	cur = cobalt_get_current();
 	if (cur == XN_NO_HANDLE)
 		return EPERM;
@@ -645,11 +656,6 @@ do_syscall:
 
 	return -err;
 
-autoinit:
-	err = cobalt_mutex_autoinit(mutex);
-	if (err)
-		return err;
-	goto start;
 unprotect:
 	u_window = cobalt_get_current_window();
 	u_window->pp_pending = XN_NO_HANDLE;
-- 
2.20.1

[PATCH v2 2/5] libcobalt: improve documentation regarding mutex initializers

[Norbert Lange]

Signed-off-by: Norbert Lange <norbert.lange@andritz.com>
---
 lib/cobalt/mutex.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/cobalt/mutex.c b/lib/cobalt/mutex.c
index be9f6ed80..125981d40 100644
--- a/lib/cobalt/mutex.c
+++ b/lib/cobalt/mutex.c
@@ -49,8 +49,12 @@
  * By default, Cobalt mutexes are of the normal type, use no
  * priority protocol and may not be shared between several processes.
  *
- * Note that only pthread_mutex_init() may be used to initialize a mutex, using
- * the static initializer @a PTHREAD_MUTEX_INITIALIZER is not supported.
+ * Note that pthread_mutex_init() should be used to initialize a mutex, using
+ * the static initializer @a PTHREAD_MUTEX_INITIALIZER will delay the
+ * initialization to the first method called on the mutex and will
+ * most likely introduce switches to secondary mode.
+ * The documentation (and specififcally api-tags) of the mutex services assumes
+ * a mutex was explicitely initialised with pthread_mutex_init().
  *
  *@{
  */
-- 
2.20.1

[PATCH v2 3/5] libcobalt: improve condvar autoinit support

[Norbert Lange]

contrary to some comments, condvars are automatically
initialised on signal/wait.
Correct the destroy method to not report an
error on such condvars.

Check whether the condition variable has the
static initializer is now more strict.

Signed-off-by: Norbert Lange <norbert.lange@andritz.com>
---
 lib/cobalt/cond.c | 93 ++++++++++++++++++++++++++---------------------
 1 file changed, 51 insertions(+), 42 deletions(-)

diff --git a/lib/cobalt/cond.c b/lib/cobalt/cond.c
index 00a201855..92eb230ff 100644
--- a/lib/cobalt/cond.c
+++ b/lib/cobalt/cond.c
@@ -142,6 +142,42 @@ COBALT_IMPL(int, pthread_cond_init, (pthread_cond_t *cond,
 	return 0;
 }
 
+static int __attribute__((cold)) _cobalt_cond_autoinit_type(const pthread_cond_t *cond)
+{
+	static const pthread_cond_t cond_initializers[] = {
+		PTHREAD_COND_INITIALIZER
+	};
+	static const int cond_types[] =
+	{
+		0
+	};
+
+	unsigned i = sizeof(cond_types) / sizeof(cond_types[0]);
+	while (i-- > 0)
+	{
+		if (memcmp(cond, &cond_initializers[i], sizeof(cond_initializers[0])) == 0)
+			return cond_types[i];
+	}
+	return -1;
+}
+
+static int __attribute__((cold)) _cobalt_cond_doautoinit(union cobalt_cond_union *ucond)
+{
+	int type;
+	type = _cobalt_cond_autoinit_type(&ucond->native_cond);
+	if (type < 0)
+		return EINVAL;
+
+	return __COBALT(pthread_cond_init(&ucond->native_cond, NULL));
+}
+
+static inline int _cobalt_cond_autoinit(union cobalt_cond_union *ucond)
+{
+	if (unlikely(ucond->shadow_cond.magic != COBALT_COND_MAGIC))
+		return _cobalt_cond_doautoinit(ucond);
+	return 0;
+}
+
 /**
  * @fn int pthread_cond_destroy(pthread_cond_t *cond)
  * @brief Destroy a condition variable
@@ -170,6 +206,9 @@ COBALT_IMPL(int, pthread_cond_destroy, (pthread_cond_t *cond))
 {
 	struct cobalt_cond_shadow *_cond = &((union cobalt_cond_union *)cond)->shadow_cond;
 
+	if (unlikely(_cond->magic != COBALT_COND_MAGIC))
+		return (_cobalt_cond_autoinit_type(cond) < 0) ? EINVAL : 0;
+
 	return -XENOMAI_SYSCALL1( sc_cobalt_cond_destroy, _cond);
 }
 
@@ -193,12 +232,6 @@ static void __pthread_cond_cleanup(void *data)
 	c->mutex->lockcnt = c->count;
 }
 
-static int __attribute__((cold)) cobalt_cond_autoinit(pthread_cond_t *cond)
-{
-	return __COBALT(pthread_cond_init(cond, NULL));
-}
-
-
 /**
  * Wait on a condition variable.
  *
@@ -262,10 +295,10 @@ COBALT_IMPL(int, pthread_cond_wait, (pthread_cond_t *cond, pthread_mutex_t *mute
 	if (_mx->magic != COBALT_MUTEX_MAGIC)
 		return EINVAL;
 
-	if (_cnd->magic != COBALT_COND_MAGIC)
-		goto autoinit;
+	err = _cobalt_cond_autoinit((union cobalt_cond_union *)cond);
+	if (err)
+		return err;
 
-  cont:
 	if (_mx->attr.type == PTHREAD_MUTEX_ERRORCHECK) {
 		xnhandle_t cur = cobalt_get_current();
 
@@ -297,12 +330,6 @@ COBALT_IMPL(int, pthread_cond_wait, (pthread_cond_t *cond, pthread_mutex_t *mute
 	pthread_testcancel();
 
 	return -err ?: -c.err;
-
-  autoinit:
-	err = cobalt_cond_autoinit(cond);
-	if (err)
-		return err;
-	goto cont;
 }
 
 /**
@@ -357,10 +384,10 @@ COBALT_IMPL(int, pthread_cond_timedwait, (pthread_cond_t *cond,
 	if (_mx->magic != COBALT_MUTEX_MAGIC)
 		return EINVAL;
 
-	if (_cnd->magic != COBALT_COND_MAGIC)
-		goto autoinit;
+	err = _cobalt_cond_autoinit((union cobalt_cond_union *)cond);
+	if (err)
+		return err;
 
-  cont:
 	if (_mx->attr.type == PTHREAD_MUTEX_ERRORCHECK) {
 		xnhandle_t cur = cobalt_get_current();
 
@@ -391,12 +418,6 @@ COBALT_IMPL(int, pthread_cond_timedwait, (pthread_cond_t *cond,
 	pthread_testcancel();
 
 	return -err ?: -c.err;
-
-  autoinit:
-	err = cobalt_cond_autoinit(cond);
-	if (err)
-		return err;
-	goto cont;
 }
 
 /**
@@ -431,10 +452,10 @@ COBALT_IMPL(int, pthread_cond_signal, (pthread_cond_t *cond))
 	__u32 flags;
 	int err;
 
-	if (_cnd->magic != COBALT_COND_MAGIC)
-		goto autoinit;
+	err = _cobalt_cond_autoinit((union cobalt_cond_union *)cond);
+	if (err)
+		return err;
 
-  cont:
 	mutex_state = get_mutex_state(_cnd);
 	if (mutex_state == NULL)
 		return 0;	/* Fast path, no waiter. */
@@ -455,12 +476,6 @@ COBALT_IMPL(int, pthread_cond_signal, (pthread_cond_t *cond))
 		cond_state->pending_signals = pending_signals + 1;
 
 	return 0;
-
-  autoinit:
-	err = cobalt_cond_autoinit(cond);
-	if (err)
-		return err;
-	goto cont;
 }
 
 /**
@@ -491,10 +506,10 @@ COBALT_IMPL(int, pthread_cond_broadcast, (pthread_cond_t *cond))
 	__u32 flags;
 	int err;
 
-	if (_cnd->magic != COBALT_COND_MAGIC)
-		goto autoinit;
+	err = _cobalt_cond_autoinit((union cobalt_cond_union *)cond);
+	if (err)
+		return err;
 
-  cont:
 	mutex_state = get_mutex_state(_cnd);
 	if (mutex_state == NULL)
 		return 0;
@@ -513,12 +528,6 @@ COBALT_IMPL(int, pthread_cond_broadcast, (pthread_cond_t *cond))
 	cond_state->pending_signals = ~0U;
 
 	return 0;
-
-  autoinit:
-	err = cobalt_cond_autoinit(cond);
-	if (err)
-		return err;
-	goto cont;
 }
 
 /**
-- 
2.20.1

[PATCH v2 4/5] libcobalt: improve documentation regarding condvar initializers

[Norbert Lange]

Signed-off-by: Norbert Lange <norbert.lange@andritz.com>
---
 lib/cobalt/cond.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/cobalt/cond.c b/lib/cobalt/cond.c
index 92eb230ff..cdbd512ae 100644
--- a/lib/cobalt/cond.c
+++ b/lib/cobalt/cond.c
@@ -47,9 +47,13 @@
  * several processes (it may not be shared by default, see
  * pthread_condattr_setpshared()).
  *
- * Note that only pthread_cond_init() may be used to initialize a condition
- * variable, using the static initializer @a PTHREAD_COND_INITIALIZER is
- * not supported.
+ * Note that pthread_cond_init() should be used to initialize a condition
+ * variable, using the static initializer @a PTHREAD_COND_INITIALIZER will
+ * delay the initialization to the first method called on the condition
+ * variable and will most likely introduce switches to secondary mode.
+ * The documentation (and specififcally api-tags) of the
+ * condition variable services assumes the condition variable was explicitely
+ * initialised with pthread_cond_init().
  *
  *@{
  */
-- 
2.20.1

[PATCH v2 5/5] smokey: add tests for mutex/condvar autoinit

[Norbert Lange]

add a few testcases where destroy is called as first function,
and test failure if the state is a non-standard initializer.

Signed-off-by: Norbert Lange <norbert.lange@andritz.com>
---
 testsuite/smokey/posix-cond/posix-cond.c   | 14 +++++++
 testsuite/smokey/posix-mutex/posix-mutex.c | 47 ++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/testsuite/smokey/posix-cond/posix-cond.c b/testsuite/smokey/posix-cond/posix-cond.c
index 153c64599..ad915c724 100644
--- a/testsuite/smokey/posix-cond/posix-cond.c
+++ b/testsuite/smokey/posix-cond/posix-cond.c
@@ -198,6 +198,19 @@ static void *cond_signaler(void *cookie)
 	return NULL;
 }
 
+static void autoinit_simple_conddestroy(void)
+{
+	pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
+	pthread_cond_t cond2 = PTHREAD_COND_INITIALIZER;
+	unsigned invalmagic = ~0x86860505; // ~COBALT_COND_MAGIC
+
+	memcpy((char *)&cond2 + sizeof(cond2) - sizeof(invalmagic), &invalmagic, sizeof(invalmagic));
+
+	smokey_trace("%s", __func__);
+	check("cond_destroy", cond_destroy(&cond), 0);
+	check("cond_destroy invalid", cond_destroy(&cond2), -EINVAL);
+}
+
 static void autoinit_simple_condwait(void)
 {
 	pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
@@ -731,6 +744,7 @@ int run_posix_cond(struct smokey_test *t, int argc, char *const argv[])
 	sparam.sched_priority = 2;
 	pthread_setschedparam(pthread_self(), SCHED_FIFO, &sparam);
 
+	autoinit_simple_conddestroy();
 	autoinit_simple_condwait();
 	simple_condwait();
 	relative_condwait();
diff --git a/testsuite/smokey/posix-mutex/posix-mutex.c b/testsuite/smokey/posix-mutex/posix-mutex.c
index 182f8c0e5..a42c52033 100644
--- a/testsuite/smokey/posix-mutex/posix-mutex.c
+++ b/testsuite/smokey/posix-mutex/posix-mutex.c
@@ -296,6 +296,28 @@ static int do_contend(pthread_mutex_t *mutex, int type)
 	return 0;
 }
 
+static int do_destroy(pthread_mutex_t *mutex, pthread_mutex_t *invalmutex, int type)
+{
+	int ret;
+	if (!__T(ret, pthread_mutex_destroy(mutex)))
+		return ret;
+	if (!__F(ret, pthread_mutex_destroy(invalmutex)) &&
+		__Tassert(ret == -EINVAL))
+		return -1;
+	return 0;
+}
+
+static int static_init_normal_destroy(void)
+{
+	pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
+	pthread_mutex_t invalmutex = PTHREAD_MUTEX_INITIALIZER;
+
+	unsigned invalmagic = ~0x86860303; // ~COBALT_MUTEX_MAGIC
+
+	memcpy((char *)&invalmutex + sizeof(invalmutex) - sizeof(invalmagic), &invalmagic, sizeof(invalmagic));
+	return do_destroy(&mutex, &invalmutex, PTHREAD_MUTEX_NORMAL);
+}
+
 static int static_init_normal_contend(void)
 {
 	pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
@@ -320,6 +342,17 @@ static int dynamic_init_normal_contend(void)
 	return __dynamic_init_contend(PTHREAD_MUTEX_NORMAL);
 }
 
+static int static_init_recursive_destroy(void)
+{
+	pthread_mutex_t mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
+	pthread_mutex_t invalmutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
+
+	unsigned invalmagic = ~0x86860303; // ~COBALT_MUTEX_MAGIC
+
+	memcpy((char *)&invalmutex + sizeof(invalmutex) - sizeof(invalmagic), &invalmagic, sizeof(invalmagic));
+	return do_destroy(&mutex, &invalmutex, PTHREAD_MUTEX_RECURSIVE);
+}
+
 static int static_init_recursive_contend(void)
 {
 	pthread_mutex_t mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
@@ -332,6 +365,17 @@ static int dynamic_init_recursive_contend(void)
 	return __dynamic_init_contend(PTHREAD_MUTEX_RECURSIVE);
 }
 
+static int static_init_errorcheck_destroy(void)
+{
+	pthread_mutex_t mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+	pthread_mutex_t invalmutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+
+	unsigned invalmagic = ~0x86860303; // ~COBALT_MUTEX_MAGIC
+
+	memcpy((char *)&invalmutex + sizeof(invalmutex) - sizeof(invalmagic), &invalmagic, sizeof(invalmagic));
+	return do_destroy(&mutex, &invalmutex, PTHREAD_MUTEX_ERRORCHECK);
+}
+
 static int static_init_errorcheck_contend(void)
 {
 	pthread_mutex_t mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
@@ -991,10 +1035,13 @@ static int run_posix_mutex(struct smokey_test *t, int argc, char *const argv[])
 					    SCHED_FIFO, &param)))
 		return ret;
 
+	do_test(static_init_normal_destroy, MAX_100_MS);
 	do_test(static_init_normal_contend, MAX_100_MS);
 	do_test(dynamic_init_normal_contend, MAX_100_MS);
+	do_test(static_init_recursive_destroy, MAX_100_MS);
 	do_test(static_init_recursive_contend, MAX_100_MS);
 	do_test(dynamic_init_recursive_contend, MAX_100_MS);
+	do_test(static_init_errorcheck_destroy, MAX_100_MS);
 	do_test(static_init_errorcheck_contend, MAX_100_MS);
 	do_test(dynamic_init_errorcheck_contend, MAX_100_MS);
 	do_test(timed_contend, MAX_100_MS);
-- 
2.20.1

Re: [PATCH v2 1/5] libcobalt: improve mutex autoinit support

[Jan Kiszka]

On 07.03.19 14:31, Norbert Lange via Xenomai wrote:
> contrary to some comments, mutexes are automatically
> initialised on lock/unlock.
> Correct the destroy method to not report an
> error on such mutexes.
>
> {get,set}prioceiling requires mutexes that were explicitely
> initialised, so no change needed there
>
> Signed-off-by: Norbert Lange <norbert.lange@andritz.com>
> ---
>   lib/cobalt/mutex.c | 178 +++++++++++++++++++++++----------------------
>   1 file changed, 92 insertions(+), 86 deletions(-)
>
> diff --git a/lib/cobalt/mutex.c b/lib/cobalt/mutex.c
> index ed32bba32..be9f6ed80 100644
> --- a/lib/cobalt/mutex.c
> +++ b/lib/cobalt/mutex.c
> @@ -164,70 +164,51 @@ COBALT_IMPL(int, pthread_mutex_init, (pthread_mutex_t *mutex,
>   }
>
>   /**
> - * Destroy a mutex.
> - *
> - * This service destroys the mutex @a mx, if it is unlocked and not referenced
> - * by any condition variable. The mutex becomes invalid for all mutex services
> - * (they all return the EINVAL error) except pthread_mutex_init().
> - *
> - * @param mutex the mutex to be destroyed.
> - *
> - * @return 0 on success,
> - * @return an error number if:
> - * - EINVAL, the mutex @a mx is invalid;
> - * - EPERM, the mutex is not process-shared and does not belong to the current
> - *   process;
> - * - EBUSY, the mutex is locked, or used by a condition variable.
> + * Test if a mutex structure contains an valid autoinitializer.
>    *
> - * @see
> - * <a href="http://www.opengroup.org/onlinepubs/000095399/functions/pthread_mutex_destroy.html">
> - * Specification.</a>
> - *
> - * @apitags{thread-unrestricted}
> + * @return the mutex type on success,
> + * @return -1 if not in supported autoinitializer state
>    */
> -COBALT_IMPL(int, pthread_mutex_destroy, (pthread_mutex_t *mutex))
> +static int __attribute__((cold)) _cobalt_mutex_autoinit_type(const pthread_mutex_t *mutex)
>   {
> -	struct cobalt_mutex_shadow *_mutex =
> -		&((union cobalt_mutex_union *)mutex)->shadow_mutex;
> -	int err;
> -
> -	if (_mutex->magic != COBALT_MUTEX_MAGIC)
> -		return EINVAL;
> -
> -	err = XENOMAI_SYSCALL1(sc_cobalt_mutex_destroy, _mutex);
> -
> -	return -err;
> -}
> -
> -static int __attribute__((cold)) cobalt_mutex_autoinit(pthread_mutex_t *mutex)
> -{
> -	static pthread_mutex_t uninit_normal_mutex =
> -		PTHREAD_MUTEX_INITIALIZER;
> +	static const pthread_mutex_t mutex_initializers[] = {
> +#if HAVE_DECL_PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
> +		PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP,
> +#endif
>   #if HAVE_DECL_PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
> -	static pthread_mutex_t uninit_recursive_mutex =
> -		PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
> +		PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP,
>   #endif
> +		PTHREAD_MUTEX_INITIALIZER
> +	};
> +	static const int mutex_types[] =
> +	{
>   #if HAVE_DECL_PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
> -	static pthread_mutex_t uninit_errorcheck_mutex =
> -		PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
> +		PTHREAD_MUTEX_ERRORCHECK_NP,
>   #endif
> -	struct cobalt_mutex_shadow *_mutex =
> -		&((union cobalt_mutex_union *)mutex)->shadow_mutex;
> +#if HAVE_DECL_PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
> +		PTHREAD_MUTEX_RECURSIVE_NP,
> +#endif
> +		PTHREAD_MUTEX_DEFAULT
> +	};
> +
> +	unsigned i = sizeof(mutex_types) / sizeof(mutex_types[0]);
> +	while (i-- > 0)
> +	{
> +		if (memcmp(mutex, &mutex_initializers[i], sizeof(mutex_initializers[0])) == 0)
> +			return mutex_types[i];
> +	}
> +	return -1;
> +}
> +
> +static int __attribute__((cold)) _cobalt_mutex_doautoinit(union cobalt_mutex_union *umutex)
> +{
> +	struct cobalt_mutex_shadow *_mutex = &umutex->shadow_mutex;
>   	int err __attribute__((unused));
>   	pthread_mutexattr_t mattr;
>   	int ret = 0, type;
>
> -	if (memcmp(mutex, &uninit_normal_mutex, sizeof(*mutex)) == 0)
> -		type = PTHREAD_MUTEX_DEFAULT;
> -#if HAVE_DECL_PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
> -	else if (memcmp(mutex, &uninit_recursive_mutex, sizeof(*mutex)) == 0)
> -		type = PTHREAD_MUTEX_RECURSIVE_NP;
> -#endif
> -#if HAVE_DECL_PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
> -	else if (memcmp(mutex, &uninit_errorcheck_mutex, sizeof(*mutex)) == 0)
> -		type = PTHREAD_MUTEX_ERRORCHECK_NP;
> -#endif
> -	else
> +	type = _cobalt_mutex_autoinit_type(&umutex->native_mutex);
> +	if (type < 0)
>   		return EINVAL;
>
>   	pthread_mutexattr_init(&mattr);
> @@ -238,7 +219,7 @@ static int __attribute__((cold)) cobalt_mutex_autoinit(pthread_mutex_t *mutex)
>   		goto out;
>   	}
>   	if (_mutex->magic != COBALT_MUTEX_MAGIC)
> -		ret = __COBALT(pthread_mutex_init(mutex, &mattr));
> +		ret = __COBALT(pthread_mutex_init(&umutex->native_mutex, &mattr));
>   	err = __COBALT(pthread_mutex_unlock(cobalt_autoinit_mutex));
>   	if (err) {
>   		if (ret == 0)
> @@ -251,6 +232,49 @@ static int __attribute__((cold)) cobalt_mutex_autoinit(pthread_mutex_t *mutex)
>   	return ret;
>   }
>
> +static inline int _cobalt_mutex_autoinit(union cobalt_mutex_union *umutex)
> +{
> +	if (unlikely(umutex->shadow_mutex.magic != COBALT_MUTEX_MAGIC))
> +		return _cobalt_mutex_doautoinit(umutex);
> +	return 0;
> +}
> +
> +/**
> + * Destroy a mutex.
> + *
> + * This service destroys the mutex @a mx, if it is unlocked and not referenced
> + * by any condition variable. The mutex becomes invalid for all mutex services
> + * (they all return the EINVAL error) except pthread_mutex_init().
> + *
> + * @param mutex the mutex to be destroyed.
> + *
> + * @return 0 on success,
> + * @return an error number if:
> + * - EINVAL, the mutex @a mx is invalid;
> + * - EPERM, the mutex is not process-shared and does not belong to the current
> + *   process;
> + * - EBUSY, the mutex is locked, or used by a condition variable.
> + *
> + * @see
> + * <a href="http://www.opengroup.org/onlinepubs/000095399/functions/pthread_mutex_destroy.html">
> + * Specification.</a>
> + *
> + * @apitags{thread-unrestricted}
> + */
> +COBALT_IMPL(int, pthread_mutex_destroy, (pthread_mutex_t *mutex))
> +{
> +	struct cobalt_mutex_shadow *_mutex =
> +		&((union cobalt_mutex_union *)mutex)->shadow_mutex;
> +	int err;
> +
> +	if (unlikely(_mutex->magic != COBALT_MUTEX_MAGIC))
> +		return (_cobalt_mutex_autoinit_type(mutex) < 0) ? EINVAL : 0;
> +
> +	err = XENOMAI_SYSCALL1(sc_cobalt_mutex_destroy, _mutex);
> +
> +	return -err;
> +}
> +
>   /**
>    * Lock a mutex.
>    *
> @@ -296,15 +320,15 @@ COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
>   	if (cur == XN_NO_HANDLE)
>   		return EPERM;
>
> -	if (_mutex->magic != COBALT_MUTEX_MAGIC)
> -		goto autoinit;
> +	ret = _cobalt_mutex_autoinit((union cobalt_mutex_union *)mutex);
> +	if (ret)
> +		return ret;
>
>   	/*
>   	 * We track resource ownership for auto-relax of non real-time
>   	 * shadows and some debug features, so we must always obtain
>   	 * them via a syscall.
>   	 */
> -start:
>   	status = cobalt_get_current_mode();
>   	if ((status & (XNRELAX|XNWEAK|XNDEBUG)) == 0) {
>   		if (_mutex->attr.protocol == PTHREAD_PRIO_PROTECT)
> @@ -360,11 +384,6 @@ protect:
>   	u_window->pp_pending = _mutex->handle;
>   	lazy_protect = 1;
>   	goto fast_path;
> -autoinit:
> -	ret = cobalt_mutex_autoinit(mutex);
> -	if (ret)
> -		return ret;
> -	goto start;
>   }
>
>   /**
> @@ -411,11 +430,11 @@ COBALT_IMPL(int, pthread_mutex_timedlock, (pthread_mutex_t *mutex,
>   	if (cur == XN_NO_HANDLE)
>   		return EPERM;
>
> -	if (_mutex->magic != COBALT_MUTEX_MAGIC)
> -		goto autoinit;
> +	ret = _cobalt_mutex_autoinit((union cobalt_mutex_union *)mutex);
> +	if (ret)
> +		return ret;
>
>   	/* See __cobalt_pthread_mutex_lock() */
> -start:
>   	status = cobalt_get_current_mode();
>   	if ((status & (XNRELAX|XNWEAK|XNDEBUG)) == 0) {
>   		if (_mutex->attr.protocol == PTHREAD_PRIO_PROTECT)
> @@ -471,11 +490,6 @@ protect:
>   	u_window->pp_pending = _mutex->handle;
>   	lazy_protect = 1;
>   	goto fast_path;
> -autoinit:
> -	ret = cobalt_mutex_autoinit(mutex);
> -	if (ret)
> -		return ret;
> -	goto start;
>   }
>
>   /**
> @@ -515,9 +529,10 @@ COBALT_IMPL(int, pthread_mutex_trylock, (pthread_mutex_t *mutex))
>   	if (cur == XN_NO_HANDLE)
>   		return EPERM;
>
> -	if (_mutex->magic != COBALT_MUTEX_MAGIC)
> -		goto autoinit;
> -start:
> +	ret = _cobalt_mutex_autoinit((union cobalt_mutex_union *)mutex);
> +	if (ret)
> +		return ret;
> +
>   	status = cobalt_get_current_mode();
>   	if ((status & (XNRELAX|XNWEAK|XNDEBUG)) == 0) {
>   		if (_mutex->attr.protocol == PTHREAD_PRIO_PROTECT)
> @@ -558,11 +573,6 @@ slow_path:
>   		_mutex->lockcnt = 1;
>
>   	return -ret;
> -autoinit:
> -	ret = cobalt_mutex_autoinit(mutex);
> -	if (ret)
> -		return ret;
> -	goto start;
>   protect:
>   	u_window = cobalt_get_current_window();
>   	/*
> @@ -611,9 +621,10 @@ COBALT_IMPL(int, pthread_mutex_unlock, (pthread_mutex_t *mutex))
>   	xnhandle_t cur;
>   	int err;
>
> -	if (_mutex->magic != COBALT_MUTEX_MAGIC)
> -		goto autoinit;
> -start:
> +	err = _cobalt_mutex_autoinit((union cobalt_mutex_union *)mutex);
> +	if (err)
> +		return err;
> +
>   	cur = cobalt_get_current();
>   	if (cur == XN_NO_HANDLE)
>   		return EPERM;
> @@ -645,11 +656,6 @@ do_syscall:
>
>   	return -err;
>
> -autoinit:
> -	err = cobalt_mutex_autoinit(mutex);
> -	if (err)
> -		return err;
> -	goto start;
>   unprotect:
>   	u_window = cobalt_get_current_window();
>   	u_window->pp_pending = XN_NO_HANDLE;
>

Hmpf, I commented on v1, sorry. If you have happened to address some of my
remarks here, just ignore them. But I see a couple of them are still relevant.

Jan