[v9,07/10] eal: implement functions for mutex management
Checks
Commit Message
From: Narcisa Vasile <navasile@microsoft.com>
Add functions for mutex init, destroy, lock, unlock.
On Linux, static initialization of a mutex is possible
through PTHREAD_MUTEX_INITIALIZER.
Windows does not have a static initializer.
Initialization is only done through InitializeCriticalSection().
To simulate static initialization, a fake initializator has been added:
The rte_mutex_lock() function will verify if the mutex has been initialized
using this fake initializer and if so, it will perform additional
initialization.
Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
---
lib/eal/common/rte_thread.c | 24 ++++++
lib/eal/include/rte_thread.h | 53 ++++++++++++
lib/eal/include/rte_thread_types.h | 4 +
.../include/rte_windows_thread_types.h | 9 ++
lib/eal/windows/rte_thread.c | 83 ++++++++++++++++++-
5 files changed, 172 insertions(+), 1 deletion(-)
Comments
2021-06-04 16:44 (UTC-0700), Narcisa Ana Maria Vasile:
[...]
> diff --git a/lib/eal/include/rte_thread_types.h b/lib/eal/include/rte_thread_types.h
> index d67b24a563..7bb0d2948c 100644
> --- a/lib/eal/include/rte_thread_types.h
> +++ b/lib/eal/include/rte_thread_types.h
> @@ -7,4 +7,8 @@
>
> #include <pthread.h>
>
> +#define RTE_THREAD_MUTEX_INITIALIZER PTHREAD_MUTEX_INITIALIZER
> +
> +typedef pthread_mutex_t rte_thread_mutex_t;
> +
> #endif /* _RTE_THREAD_TYPES_H_ */
> diff --git a/lib/eal/windows/include/rte_windows_thread_types.h b/lib/eal/windows/include/rte_windows_thread_types.h
> index 60e6d94553..c6c8502bfb 100644
> --- a/lib/eal/windows/include/rte_windows_thread_types.h
> +++ b/lib/eal/windows/include/rte_windows_thread_types.h
> @@ -7,4 +7,13 @@
>
> #include <rte_windows.h>
>
> +#define WINDOWS_MUTEX_INITIALIZER (void*)-1
> +#define RTE_THREAD_MUTEX_INITIALIZER {WINDOWS_MUTEX_INITIALIZER}
> +
> +struct thread_mutex_t {
> + void* mutex_id;
> +};
> +
> +typedef struct thread_mutex_t rte_thread_mutex_t;
> +
> #endif /* _RTE_THREAD_TYPES_H_ */
In previous patches rte_thread content was made opaque and of equal size
for pthread (most implementations) and non-pthread variant.
AFAIU, we agree on the requirement of compatible ABI between variants,
that is, a compiled app can work with any threading variant of DPDK.
Above definition of `rte_thread_mutex_t` does not satisfy it.
Or do we only promise API compatibility?
This is the most important question now.
Also: DPDK should not export names without `rte_` prefix,
i. e. `WINDOWS_MUTEX_INITIALIZER` and `thread_mutex_t`.
Besides, why `_t`?
2021-06-09 02:04 (UTC+0300), Dmitry Kozlyuk:
> 2021-06-04 16:44 (UTC-0700), Narcisa Ana Maria Vasile:
> [...]
> > diff --git a/lib/eal/include/rte_thread_types.h b/lib/eal/include/rte_thread_types.h
> > index d67b24a563..7bb0d2948c 100644
> > --- a/lib/eal/include/rte_thread_types.h
> > +++ b/lib/eal/include/rte_thread_types.h
> > @@ -7,4 +7,8 @@
> >
> > #include <pthread.h>
> >
> > +#define RTE_THREAD_MUTEX_INITIALIZER PTHREAD_MUTEX_INITIALIZER
> > +
> > +typedef pthread_mutex_t rte_thread_mutex_t;
> > +
> > #endif /* _RTE_THREAD_TYPES_H_ */
> > diff --git a/lib/eal/windows/include/rte_windows_thread_types.h b/lib/eal/windows/include/rte_windows_thread_types.h
> > index 60e6d94553..c6c8502bfb 100644
> > --- a/lib/eal/windows/include/rte_windows_thread_types.h
> > +++ b/lib/eal/windows/include/rte_windows_thread_types.h
> > @@ -7,4 +7,13 @@
> >
> > #include <rte_windows.h>
> >
> > +#define WINDOWS_MUTEX_INITIALIZER (void*)-1
> > +#define RTE_THREAD_MUTEX_INITIALIZER {WINDOWS_MUTEX_INITIALIZER}
> > +
> > +struct thread_mutex_t {
> > + void* mutex_id;
> > +};
> > +
> > +typedef struct thread_mutex_t rte_thread_mutex_t;
> > +
> > #endif /* _RTE_THREAD_TYPES_H_ */
>
> In previous patches rte_thread content was made opaque and of equal size
> for pthread (most implementations) and non-pthread variant.
> AFAIU, we agree on the requirement of compatible ABI between variants,
> that is, a compiled app can work with any threading variant of DPDK.
> Above definition of `rte_thread_mutex_t` does not satisfy it.
> Or do we only promise API compatibility?
> This is the most important question now.
From Windows community call 2021-06-10, for everyone's information.
1. Yes, binary compatibility is a requirement.
2. Static mutex initializer for Windows is tricky (an old topic).
This patch proposes `rte_mutex` to hold a pointer to actual mutex
and use NULL as static initializer, then allocate on first use.
At the same time we want to use the same initializer for pthread variant.
This means it would also need indirection, allocation, and tricky logic.
My opinion:
New threading API can just be without static initilizer.
All it usages in DPDK could be converted to dynamic initialization
either in appropriate function or using `RTE_INIT`.
Maybe create a convenient macro to declare a static mutex and its
initialization code, what do others think?
RTE_STATIC_MUTEX(private_lock)
Expanding to:
static RTE_DECLARE_MUTEX(private_lock)
RTE_DEFINE_MUTEX(private_lock)
Expanding to:
static rte_mutex private_lock;
RTE_INIT(__rte_private_lock_init)
{
RTE_VERIFY(rte_thread_mutex_init(&private_lock));
}
As a bonus it removes the need of `rte_*_thread_types.h`.
On Thu, Jun 10, 2021 at 01:37:17AM +0300, Dmitry Kozlyuk wrote:
> 2021-06-09 02:04 (UTC+0300), Dmitry Kozlyuk:
> > 2021-06-04 16:44 (UTC-0700), Narcisa Ana Maria Vasile:
> > [...]
> > > diff --git a/lib/eal/include/rte_thread_types.h b/lib/eal/include/rte_thread_types.h
> > > index d67b24a563..7bb0d2948c 100644
> > > --- a/lib/eal/include/rte_thread_types.h
> > > +++ b/lib/eal/include/rte_thread_types.h
> > > @@ -7,4 +7,8 @@
> > >
> > > #include <pthread.h>
> > >
> > > +#define RTE_THREAD_MUTEX_INITIALIZER PTHREAD_MUTEX_INITIALIZER
> > > +
> > > +typedef pthread_mutex_t rte_thread_mutex_t;
> > > +
> > > #endif /* _RTE_THREAD_TYPES_H_ */
> > > diff --git a/lib/eal/windows/include/rte_windows_thread_types.h b/lib/eal/windows/include/rte_windows_thread_types.h
> > > index 60e6d94553..c6c8502bfb 100644
> > > --- a/lib/eal/windows/include/rte_windows_thread_types.h
> > > +++ b/lib/eal/windows/include/rte_windows_thread_types.h
> > > @@ -7,4 +7,13 @@
> > >
> > > #include <rte_windows.h>
> > >
> > > +#define WINDOWS_MUTEX_INITIALIZER (void*)-1
> > > +#define RTE_THREAD_MUTEX_INITIALIZER {WINDOWS_MUTEX_INITIALIZER}
> > > +
> > > +struct thread_mutex_t {
> > > + void* mutex_id;
> > > +};
> > > +
> > > +typedef struct thread_mutex_t rte_thread_mutex_t;
> > > +
> > > #endif /* _RTE_THREAD_TYPES_H_ */
> >
> > In previous patches rte_thread content was made opaque and of equal size
> > for pthread (most implementations) and non-pthread variant.
> > AFAIU, we agree on the requirement of compatible ABI between variants,
> > that is, a compiled app can work with any threading variant of DPDK.
> > Above definition of `rte_thread_mutex_t` does not satisfy it.
> > Or do we only promise API compatibility?
> > This is the most important question now.
>
> From Windows community call 2021-06-10, for everyone's information.
>
> 1. Yes, binary compatibility is a requirement.
>
> 2. Static mutex initializer for Windows is tricky (an old topic).
> This patch proposes `rte_mutex` to hold a pointer to actual mutex
> and use NULL as static initializer, then allocate on first use.
> At the same time we want to use the same initializer for pthread variant.
> This means it would also need indirection, allocation, and tricky logic.
>
> My opinion:
>
> New threading API can just be without static initilizer.
> All it usages in DPDK could be converted to dynamic initialization
> either in appropriate function or using `RTE_INIT`.
> Maybe create a convenient macro to declare a static mutex and its
> initialization code, what do others think?
>
> RTE_STATIC_MUTEX(private_lock)
>
> Expanding to:
>
> static RTE_DECLARE_MUTEX(private_lock)
> RTE_DEFINE_MUTEX(private_lock)
>
>
> Expanding to:
>
> static rte_mutex private_lock;
>
> RTE_INIT(__rte_private_lock_init)
> {
> RTE_VERIFY(rte_thread_mutex_init(&private_lock));
> }
>
> As a bonus it removes the need of `rte_*_thread_types.h`.
Thank you Dmitry, I think this is the best and most elegant solution.
I will use a pointer to represent the mutex:
typedef struct rte_thread_mutex_tag {
void* mutex_id;
} rte_thread_mutex;
..and use the macro for static initializations as you described.
@@ -249,6 +249,30 @@ rte_thread_join(rte_thread_t thread_id, int *value_ptr)
return 0;
}
+int
+rte_thread_mutex_init(rte_thread_mutex_t *mutex)
+{
+ return pthread_mutex_init(mutex, NULL);
+}
+
+int
+rte_thread_mutex_lock(rte_thread_mutex_t *mutex)
+{
+ return pthread_mutex_lock(mutex);
+}
+
+int
+rte_thread_mutex_unlock(rte_thread_mutex_t *mutex)
+{
+ return pthread_mutex_unlock(mutex);
+}
+
+int
+rte_thread_mutex_destroy(rte_thread_mutex_t *mutex)
+{
+ return pthread_mutex_destroy(mutex);
+}
+
int rte_thread_cancel(rte_thread_t thread_id)
{
/*
@@ -248,6 +248,58 @@ int rte_thread_create(rte_thread_t *thread_id,
__rte_experimental
int rte_thread_join(rte_thread_t thread_id, int *value_ptr);
+/**
+ * Initializes a mutex.
+ *
+ * @param mutex
+ * The mutex to be initialized.
+ *
+ * @return
+ * On success, return 0.
+ * On failure, return a positive errno-style error number.
+ */
+__rte_experimental
+int rte_thread_mutex_init(rte_thread_mutex_t *mutex);
+
+/**
+ * Locks a mutex.
+ *
+ * @param mutex
+ * The mutex to be locked.
+ *
+ * @return
+ * On success, return 0.
+ * On failure, return a positive errno-style error number.
+ */
+__rte_experimental
+int rte_thread_mutex_lock(rte_thread_mutex_t *mutex);
+
+/**
+ * Unlocks a mutex.
+ *
+ * @param mutex
+ * The mutex to be unlocked.
+ *
+ * @return
+ * On success, return 0.
+ * On failure, return a positive errno-style error number.
+ */
+__rte_experimental
+int rte_thread_mutex_unlock(rte_thread_mutex_t *mutex);
+
+/**
+ * Releases all resources associated with a mutex.
+ *
+ * @param mutex
+ * The mutex to be uninitialized.
+ *
+ * @return
+ * On success, return 0.
+ * On failure, return a positive errno-style error number.
+ */
+__rte_experimental
+int rte_thread_mutex_destroy(rte_thread_mutex_t *mutex);
+
/**
* Terminates a thread.
*
@@ -283,6 +335,7 @@ int rte_thread_detach(rte_thread_t thread_id);
*
* @param cpusetp
* Pointer to CPU affinity to set.
+ *
* @return
* On success, return 0; otherwise return -1;
*/
@@ -7,4 +7,8 @@
#include <pthread.h>
+#define RTE_THREAD_MUTEX_INITIALIZER PTHREAD_MUTEX_INITIALIZER
+
+typedef pthread_mutex_t rte_thread_mutex_t;
+
#endif /* _RTE_THREAD_TYPES_H_ */
@@ -7,4 +7,13 @@
#include <rte_windows.h>
+#define WINDOWS_MUTEX_INITIALIZER (void*)-1
+#define RTE_THREAD_MUTEX_INITIALIZER {WINDOWS_MUTEX_INITIALIZER}
+
+struct thread_mutex_t {
+ void* mutex_id;
+};
+
+typedef struct thread_mutex_t rte_thread_mutex_t;
+
#endif /* _RTE_THREAD_TYPES_H_ */
@@ -470,6 +470,88 @@ rte_thread_join(rte_thread_t thread_id, int *value_ptr)
return ret;
}
+int
+rte_thread_mutex_init(rte_thread_mutex_t *mutex)
+{
+ int ret = 0;
+ CRITICAL_SECTION *m = NULL;
+
+ RTE_VERIFY(mutex != NULL);
+
+ m = calloc(1, sizeof(*m));
+ if (m == NULL) {
+ RTE_LOG(DEBUG, EAL, "Unable to initialize mutex. Insufficient memory!\n");
+ ret = ENOMEM;
+ goto cleanup;
+ }
+
+ InitializeCriticalSection(m);
+ mutex->mutex_id = m;
+ m = NULL;
+
+cleanup:
+ return ret;
+}
+
+int
+rte_thread_mutex_lock(rte_thread_mutex_t *mutex)
+{
+ int ret = 0;
+ void* id = 0;
+ rte_thread_mutex_t m;
+
+ RTE_VERIFY(mutex != NULL);
+
+ /* Check if mutex has been statically initialized */
+ id = InterlockedCompareExchangePointer(&mutex->mutex_id, mutex->mutex_id, WINDOWS_MUTEX_INITIALIZER);
+ /* If mutex has been statically initialized */
+ if (id == WINDOWS_MUTEX_INITIALIZER) {
+ ret = rte_thread_mutex_init(&m);
+ if (ret != 0) {
+ return ret;
+ }
+
+ id = InterlockedCompareExchangePointer(&mutex->mutex_id, m.mutex_id, WINDOWS_MUTEX_INITIALIZER);
+ /* If meanwhile the mutex was initialized by a different thread,
+ * destroy the local initialization.
+ */
+ if (id != WINDOWS_MUTEX_INITIALIZER) {
+ rte_thread_mutex_destroy(&m);
+ }
+ }
+
+ EnterCriticalSection(mutex->mutex_id);
+
+ return 0;
+}
+
+int
+rte_thread_mutex_unlock(rte_thread_mutex_t *mutex)
+{
+ RTE_VERIFY(mutex != NULL);
+
+ LeaveCriticalSection(mutex->mutex_id);
+ return 0;
+}
+
+int
+rte_thread_mutex_destroy(rte_thread_mutex_t *mutex)
+{
+ RTE_VERIFY(mutex != NULL);
+
+ if (mutex->mutex_id == WINDOWS_MUTEX_INITIALIZER) {
+ goto cleanup;
+ }
+
+ DeleteCriticalSection(mutex->mutex_id);
+ free(mutex->mutex_id);
+
+cleanup:
+ mutex->mutex_id = NULL;
+
+ return 0;
+}
+
int
rte_thread_cancel(rte_thread_t thread_id)
{
@@ -549,7 +631,6 @@ rte_thread_key_delete(rte_thread_key key)
int
rte_thread_value_set(rte_thread_key key, const void *value)
{
- int ret;
char *p;
if (key == NULL) {