Re: [PATCH v18 8/8] eal: implement functions for mutex management

Re: [PATCH v18 8/8] eal: implement functions for mutex management

[Ananyev, Konstantin]

> Add functions for mutex init, destroy, lock, unlock, trylock.
> 
> Windows does not have a static initializer. Initialization
> is only done through InitializeCriticalSection(). To overcome this,
> RTE_INIT_MUTEX macro is added to replace static initialization
> of mutexes. The macro calls rte_thread_mutex_init().
> 
> Add unit tests to verify that the mutex correctly locks/unlocks
> and protects the data. Check both static and dynamic mutexes.
> Signed-off-by: Narcisa Vasile <navasile@microsoft.com>

Few comments from me below.
I am not sure was such approach already discussed,
if so - apologies for repetition. 

> ---
>  app/test/test_threads.c      | 106 +++++++++++++++++++++++++++++++++++
>  lib/eal/common/rte_thread.c  |  69 +++++++++++++++++++++++
>  lib/eal/include/rte_thread.h |  85 ++++++++++++++++++++++++++++
>  lib/eal/version.map          |   5 ++
>  lib/eal/windows/rte_thread.c |  64 +++++++++++++++++++++
>  5 files changed, 329 insertions(+)
> 
>  };
> diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c
> index d30a8a7ca3..4a9a1b6e07 100644
> --- a/lib/eal/common/rte_thread.c
> +++ b/lib/eal/common/rte_thread.c
> @@ -309,6 +309,75 @@ rte_thread_detach(rte_thread_t thread_id)
>  	return pthread_detach((pthread_t)thread_id.opaque_id);
>  }
> 
> +int
> +rte_thread_mutex_init(rte_thread_mutex *mutex)

Don't we need some sort of mutex_attr here too?
To be able to create PROCESS_SHARED mutexes?

> +{
> +	int ret = 0;
> +	pthread_mutex_t *m = NULL;
> +
> +	RTE_VERIFY(mutex != NULL);
> +
> +	m = calloc(1, sizeof(*m));

But is that what we really want for the mutexes?
It means actual mutex will always be allocated on process heap,
away from the data it is supposed to guard.
Even if we'll put performance considerations away,
that wouldn't work for MP case.
Is that considered as ok?

> +	if (m == NULL) {
> +		RTE_LOG(DEBUG, EAL, "Unable to initialize mutex. Insufficient memory!\n");
> +		ret = ENOMEM;
> +		goto cleanup;
> +	}
> +
> +	ret = pthread_mutex_init(m, NULL);
> +	if (ret != 0) {
> +		RTE_LOG(DEBUG, EAL, "Failed to init mutex. ret = %d\n", ret);
> +		goto cleanup;
> +	}
> +
> +	mutex->mutex_id = m;
> +	m = NULL;
> +
> +cleanup:
> +	free(m);
> +	return ret;
> +}
> +
> +int
> +rte_thread_mutex_lock(rte_thread_mutex *mutex)
> +{
> +	RTE_VERIFY(mutex != NULL);
> +
> +	return pthread_mutex_lock((pthread_mutex_t *)mutex->mutex_id);
> +}
> +
> +int
> +rte_thread_mutex_unlock(rte_thread_mutex *mutex)
> +{
> +	RTE_VERIFY(mutex != NULL);
> +
> +	return pthread_mutex_unlock((pthread_mutex_t *)mutex->mutex_id);
> +}
> +
> +int
> +rte_thread_mutex_try_lock(rte_thread_mutex *mutex)
> +{
> +	RTE_VERIFY(mutex != NULL);
> +
> +	return pthread_mutex_trylock((pthread_mutex_t *)mutex->mutex_id);
> +}
> +
> +int
> +rte_thread_mutex_destroy(rte_thread_mutex *mutex)
> +{
> +	int ret = 0;
> +	RTE_VERIFY(mutex != NULL);
> +
> +	ret = pthread_mutex_destroy((pthread_mutex_t *)mutex->mutex_id);
> +	if (ret != 0)
> +		RTE_LOG(DEBUG, EAL, "Unable to destroy mutex, ret = %d\n", ret);
> +
> +	free(mutex->mutex_id);
> +	mutex->mutex_id = NULL;
> +
> +	return ret;
> +}
> +
>  int
>  rte_thread_barrier_init(rte_thread_barrier *barrier, int count)
>  {
> diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> index 7c84e32988..09a5fd8add 100644
> --- a/lib/eal/include/rte_thread.h
> +++ b/lib/eal/include/rte_thread.h
> @@ -54,6 +54,25 @@ typedef struct {
> 
>  #endif /* RTE_HAS_CPUSET */
> 
> +#define RTE_DECLARE_MUTEX(private_lock)          rte_thread_mutex private_lock
> +
> +#define RTE_DEFINE_MUTEX(private_lock)\
> +RTE_INIT(__rte_ ## private_lock ## _init)\
> +{\
> +	RTE_VERIFY(rte_thread_mutex_init(&private_lock) == 0);\
> +}
> +
> +#define RTE_INIT_MUTEX(private_lock)\
> +static RTE_DECLARE_MUTEX(private_lock);\
> +RTE_DEFINE_MUTEX(private_lock)

Hmm, but that way we can't use  RTE_INIT_MUTEX()
within functions, right?

> +
> +/**
> + * Thread mutex representation.
> + */
> +typedef struct rte_thread_mutex_tag {
> +	void *mutex_id;  /**< mutex identifier */
> +} rte_thread_mutex;

I wonder can't we have something like that instead:

for posix:
typedef pthread_mutex_t rte_thread_mutex_t;
for windows: 
typedef struct rte_thread_mutex {
	int initialized;
	CRITICAL_SECTION cs;
}  rte_thread_mutex_t;

Then for posix:
#define RTE_INIT_MUTEX(mx) do {\
	*(mx) = PTHREAD_MUTEX_INITIALIZER; \
} while(0)

#define RTE_DESTROY_MUTEX(mx)	do {} while (0); /*empty */

For windows:
#define RTE_INIT_MUTEX(mx) do {\
	If ((mx)->initialized == 0) {
		InitializeCriticalSection((mx)->cs);
		(mx)->initialized = 1;
	}
} while (0)

#define RTE_DESTROY_MUTEX(mx)	do {
	if ((mx)->initialized != 0) { \
		DeleteCriticalSection((mx)->cs);
	}
} while (0)

That way we'll keep ability for static initialization on posix systems,
and would avoid need to allocate actual mutex object from the heap. 

>  /**
>   * Returned by rte_thread_barrier_wait() when call is successful.
>   */
> @@ -314,6 +333,72 @@ __rte_experimental
>  int rte_thread_set_priority(rte_thread_t thread_id,
>  		enum rte_thread_priority priority);
> 
> +/**
> + * 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 *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 *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 *mutex);
> +
> +/**
> + * Tries to lock a mutex.If the mutex is already held by a different thread,
> + * the function returns without blocking.
> + *
> + * @param mutex
> + *    The mutex that will be acquired, if not already locked.
> + *
> + * @return
> + *   On success, if the mutex is acquired, return 0.
> + *   On failure, return a positive errno-style error number.
> + */
> +__rte_experimental
> +int rte_thread_mutex_try_lock(rte_thread_mutex *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 *mutex);
> +
>  /**
>   * Initializes a synchronization barrier.
>   *

RE: [PATCH v18 8/8] eal: implement functions for mutex management

[Ananyev, Konstantin]

> > +
> > +/**
> > + * Thread mutex representation.
> > + */
> > +typedef struct rte_thread_mutex_tag {
> > +	void *mutex_id;  /**< mutex identifier */
> > +} rte_thread_mutex;
> 
> I wonder can't we have something like that instead:
> 
> for posix:
> typedef pthread_mutex_t rte_thread_mutex_t;
> for windows:
> typedef struct rte_thread_mutex {
> 	int initialized;
> 	CRITICAL_SECTION cs;
> }  rte_thread_mutex_t;
> 
> Then for posix:
> #define RTE_INIT_MUTEX(mx) do {\
> 	*(mx) = PTHREAD_MUTEX_INITIALIZER; \
> } while(0)
> 
> #define RTE_DESTROY_MUTEX(mx)	do {} while (0); /*empty */
> 
> For windows:
> #define RTE_INIT_MUTEX(mx) do {\
> 	If ((mx)->initialized == 0) {
> 		InitializeCriticalSection((mx)->cs);
> 		(mx)->initialized = 1;
> 	}
> } while (0)
> 
> #define RTE_DESTROY_MUTEX(mx)	do {
> 	if ((mx)->initialized != 0) { \
> 		DeleteCriticalSection((mx)->cs);
> 	}
> } while (0)
> 

Actually, please scrap that comment.
Obviously it wouldn't work for static variables, 
and doesn't make much sense.
Though few thoughts remain:
for posix we probably don't need an indirection and
rte_thread_mutex can be just typedef of pthread_mutex_t.
also for posix we don't need RTE_INIT constructor for each
static mutex initialization.
Something like:
#define RTE_STATIC_INITIALIZED_MUTEX(mx) \
	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
should work, I think.
Konstantin

Re: [PATCH v18 8/8] eal: implement functions for mutex management

[Narcisa Ana Maria Vasile]

On Tue, Feb 08, 2022 at 02:21:49AM +0000, Ananyev, Konstantin wrote:
> 
> 
> > > +
> > > +/**
> > > + * Thread mutex representation.
> > 
> 
> Actually, please scrap that comment.
> Obviously it wouldn't work for static variables, 
> and doesn't make much sense.
> Though few thoughts remain:
> for posix we probably don't need an indirection and
> rte_thread_mutex can be just typedef of pthread_mutex_t.
> also for posix we don't need RTE_INIT constructor for each
> static mutex initialization.
> Something like:
> #define RTE_STATIC_INITIALIZED_MUTEX(mx) \
> 	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
> should work, I think.
> Konstantin

Thank you for reviewing, Konstantin!
Some context for the current representation of mutex
can be found in v9, patch 7/10 of this patchset.

Originally we've typedef'ed the pthread_mutex_t on POSIX, just
like you are suggesting here.
However, on Windows there's no static initializer similar to the pthread
one. Still, we want ABI compatibility and same thread behavior between
platforms. The most elegant solution we found was the current representation,
as suggested by Dmitry K.

I will address your other comments on the other thread.

Link to v9: http://patchwork.dpdk.org/project/dpdk/patch/1622850274-6946-8-git-send-email-navasile@linux.microsoft.com/
> 
> 

RE: [PATCH v18 8/8] eal: implement functions for mutex management

[Ananyev, Konstantin]

> > Actually, please scrap that comment.
> > Obviously it wouldn't work for static variables,
> > and doesn't make much sense.
> > Though few thoughts remain:
> > for posix we probably don't need an indirection and
> > rte_thread_mutex can be just typedef of pthread_mutex_t.
> > also for posix we don't need RTE_INIT constructor for each
> > static mutex initialization.
> > Something like:
> > #define RTE_STATIC_INITIALIZED_MUTEX(mx) \
> > 	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
> > should work, I think.
> > Konstantin
> 
> Thank you for reviewing, Konstantin!
> Some context for the current representation of mutex
> can be found in v9, patch 7/10 of this patchset.
> 
> Originally we've typedef'ed the pthread_mutex_t on POSIX, just
> like you are suggesting here.
> However, on Windows there's no static initializer similar to the pthread
> one. Still, we want ABI compatibility and same thread behavior between
> platforms. The most elegant solution we found was the current representation,
> as suggested by Dmitry K.

Yes, I agree it is a problem with Windows for static initializer.
But why we can't have different structs typedef for mutex 
for posix and windows platforms?
On posix it would be:

typedef pthread_mutex_t rte_thread_mutex_t;
#define RTE_STATIC_INITIALIZED_MUTEX(mx)   rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER 

On windows it could be what Dimitry suggested:
 
typedef struct rte_thread_mutex {
        void *mutex_id;  /**< mutex identifier */
} rte_thread_mutex_t;

#define RTE_STATIC_INITIALIZED_MUTEX(private_lock)   \
rte_thread_mutex_t private_lock; \
RTE_INIT(__rte_ ## private_lock ## _init)\
{\
        RTE_VERIFY(rte_thread_mutex_init(&private_lock) == 0);\
}

API would remain the same, though it would be different underneath.
Yes, on Windows rte_thread_mutex still wouldn't work for MP,
but that's the same as with current design.
 
> I will address your other comments on the other thread.
> 
> Link to v9: http://patchwork.dpdk.org/project/dpdk/patch/1622850274-6946-8-git-send-email-navasile@linux.microsoft.com/

Re: [PATCH v18 8/8] eal: implement functions for mutex management

[Dmitry Kozlyuk]

2022-02-09 13:57 (UTC+0000), Ananyev, Konstantin:
> > > Actually, please scrap that comment.
> > > Obviously it wouldn't work for static variables,
> > > and doesn't make much sense.
> > > Though few thoughts remain:
> > > for posix we probably don't need an indirection and
> > > rte_thread_mutex can be just typedef of pthread_mutex_t.
> > > also for posix we don't need RTE_INIT constructor for each
> > > static mutex initialization.
> > > Something like:
> > > #define RTE_STATIC_INITIALIZED_MUTEX(mx) \
> > > 	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
> > > should work, I think.
> > > Konstantin  
> > 
> > Thank you for reviewing, Konstantin!
> > Some context for the current representation of mutex
> > can be found in v9, patch 7/10 of this patchset.
> > 
> > Originally we've typedef'ed the pthread_mutex_t on POSIX, just
> > like you are suggesting here.
> > However, on Windows there's no static initializer similar to the pthread
> > one. Still, we want ABI compatibility and same thread behavior between
> > platforms. The most elegant solution we found was the current representation,
> > as suggested by Dmitry K.  
> 
> Yes, I agree it is a problem with Windows for static initializer.
> But why we can't have different structs typedef for mutex 
> for posix and windows platforms?

Yes, I agree that having different mutex types on *nix and Windows
is a great idea. It will avoid ABI change for *nix
and will guarantee no performance impact.

Maybe wrap pthread_mutex_t into a struct to have a distinct type
and to force using only DPDK API with it?

[...]
> Yes, on Windows rte_thread_mutex still wouldn't work for MP,
> but that's the same as with current design.

MP support is not planned for Windows and it is unknown if it ever will be,
so it's not an issue.
Data location is.
The reason rte_thread_mutex_t is not a typedef of CRITICAL_SECTION
(akin to pthread_mutex_t) is to avoid including Windows headers
into DPDK public headers, because Windows headers can break user code
by some macros they define.
Maybe instead of a pointer it could be an opaque array:

	#define RTE_PTHREAD_MUTEX_SIZE 40

	struct rte_pthread_mutex_t {
		uint8_t opaque[RTE_PTHREAD_MUTEX_SIZE];
	};

where RTE_PTHREAD_MUTEX_SIZE is actually sizeof(CRITICAL_SECTION).
Win32 ABI is remarkably stable, I don't think this constant will ever change,
it would break all the Windows user space.
Naty, DmitryM, Tyler, what do you think?

Re: [PATCH v18 8/8] eal: implement functions for mutex management

[Dmitry Kozlyuk]

2022-02-21 00:56 (UTC+0300), Dmitry Kozlyuk:
> 2022-02-09 13:57 (UTC+0000), Ananyev, Konstantin:
> > > > Actually, please scrap that comment.
> > > > Obviously it wouldn't work for static variables,
> > > > and doesn't make much sense.
> > > > Though few thoughts remain:
> > > > for posix we probably don't need an indirection and
> > > > rte_thread_mutex can be just typedef of pthread_mutex_t.
> > > > also for posix we don't need RTE_INIT constructor for each
> > > > static mutex initialization.
> > > > Something like:
> > > > #define RTE_STATIC_INITIALIZED_MUTEX(mx) \
> > > > 	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
> > > > should work, I think.
> > > > Konstantin    
> > > 
> > > Thank you for reviewing, Konstantin!
> > > Some context for the current representation of mutex
> > > can be found in v9, patch 7/10 of this patchset.
> > > 
> > > Originally we've typedef'ed the pthread_mutex_t on POSIX, just
> > > like you are suggesting here.
> > > However, on Windows there's no static initializer similar to the pthread
> > > one. Still, we want ABI compatibility and same thread behavior between
> > > platforms. The most elegant solution we found was the current representation,
> > > as suggested by Dmitry K.    
> > 
> > Yes, I agree it is a problem with Windows for static initializer.
> > But why we can't have different structs typedef for mutex 
> > for posix and windows platforms?  
> 
> Yes, I agree that having different mutex types on *nix and Windows
> is a great idea. It will avoid ABI change for *nix
> and will guarantee no performance impact.
> 
> Maybe wrap pthread_mutex_t into a struct to have a distinct type
> and to force using only DPDK API with it?
> 
> [...]
> > Yes, on Windows rte_thread_mutex still wouldn't work for MP,
> > but that's the same as with current design.  
> 
> MP support is not planned for Windows and it is unknown if it ever will be,
> so it's not an issue.
> Data location is.
> The reason rte_thread_mutex_t is not a typedef of CRITICAL_SECTION
> (akin to pthread_mutex_t) is to avoid including Windows headers
> into DPDK public headers, because Windows headers can break user code
> by some macros they define.
> Maybe instead of a pointer it could be an opaque array:
> 
> 	#define RTE_PTHREAD_MUTEX_SIZE 40
> 
> 	struct rte_pthread_mutex_t {
> 		uint8_t opaque[RTE_PTHREAD_MUTEX_SIZE];
> 	};
> 
> where RTE_PTHREAD_MUTEX_SIZE is actually sizeof(CRITICAL_SECTION).
> Win32 ABI is remarkably stable, I don't think this constant will ever change,
> it would break all the Windows user space.
> Naty, DmitryM, Tyler, what do you think?

Conclusion from offline call: yes, this is OK to do so.

However, DmitryM suggested using Slim Reader-Writer lock (SRW):
https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks
instead of CRITICAL_SECTION.
It seems to be a much better option:

* sizeof(SRWLOCK) == 8 (technically "size of a pointer"),
  same as sizeof(pthread_mutex_t) on a typical Linux.
  Layout of data structures containing rte_thread_mutex_t
  can be the same on Windows and Unix,
  which simplifies design and promises similar less performance differences.

* Can be taken by multiple readers and one writer,
  which is semantically similar to pthread_mutex_t
  (CRITICAL_SECTION can only be taken by a single thread).

Technically it can be a "typedef uintptr_t" or a structure wrapping it.

RE: [PATCH v18 8/8] eal: implement functions for mutex management

[Ananyev, Konstantin]

Hi Dmitry,

> 2022-02-21 00:56 (UTC+0300), Dmitry Kozlyuk:
> > 2022-02-09 13:57 (UTC+0000), Ananyev, Konstantin:
> > > > > Actually, please scrap that comment.
> > > > > Obviously it wouldn't work for static variables,
> > > > > and doesn't make much sense.
> > > > > Though few thoughts remain:
> > > > > for posix we probably don't need an indirection and
> > > > > rte_thread_mutex can be just typedef of pthread_mutex_t.
> > > > > also for posix we don't need RTE_INIT constructor for each
> > > > > static mutex initialization.
> > > > > Something like:
> > > > > #define RTE_STATIC_INITIALIZED_MUTEX(mx) \
> > > > > 	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
> > > > > should work, I think.
> > > > > Konstantin
> > > >
> > > > Thank you for reviewing, Konstantin!
> > > > Some context for the current representation of mutex
> > > > can be found in v9, patch 7/10 of this patchset.
> > > >
> > > > Originally we've typedef'ed the pthread_mutex_t on POSIX, just
> > > > like you are suggesting here.
> > > > However, on Windows there's no static initializer similar to the pthread
> > > > one. Still, we want ABI compatibility and same thread behavior between
> > > > platforms. The most elegant solution we found was the current representation,
> > > > as suggested by Dmitry K.
> > >
> > > Yes, I agree it is a problem with Windows for static initializer.
> > > But why we can't have different structs typedef for mutex
> > > for posix and windows platforms?
> >
> > Yes, I agree that having different mutex types on *nix and Windows
> > is a great idea. It will avoid ABI change for *nix
> > and will guarantee no performance impact.
> >
> > Maybe wrap pthread_mutex_t into a struct to have a distinct type
> > and to force using only DPDK API with it?
> >
> > [...]
> > > Yes, on Windows rte_thread_mutex still wouldn't work for MP,
> > > but that's the same as with current design.
> >
> > MP support is not planned for Windows and it is unknown if it ever will be,
> > so it's not an issue.
> > Data location is.
> > The reason rte_thread_mutex_t is not a typedef of CRITICAL_SECTION
> > (akin to pthread_mutex_t) is to avoid including Windows headers
> > into DPDK public headers, because Windows headers can break user code
> > by some macros they define.
> > Maybe instead of a pointer it could be an opaque array:
> >
> > 	#define RTE_PTHREAD_MUTEX_SIZE 40
> >
> > 	struct rte_pthread_mutex_t {
> > 		uint8_t opaque[RTE_PTHREAD_MUTEX_SIZE];
> > 	};
> >
> > where RTE_PTHREAD_MUTEX_SIZE is actually sizeof(CRITICAL_SECTION).
> > Win32 ABI is remarkably stable, I don't think this constant will ever change,
> > it would break all the Windows user space.
> > Naty, DmitryM, Tyler, what do you think?
> 
> Conclusion from offline call: yes, this is OK to do so.
> 
> However, DmitryM suggested using Slim Reader-Writer lock (SRW):
> https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks
> instead of CRITICAL_SECTION.
> It seems to be a much better option:
> 
> * sizeof(SRWLOCK) == 8 (technically "size of a pointer"),
>   same as sizeof(pthread_mutex_t) on a typical Linux.
>   Layout of data structures containing rte_thread_mutex_t
>   can be the same on Windows and Unix,
>   which simplifies design and promises similar less performance differences.
> 
> * Can be taken by multiple readers and one writer,
>   which is semantically similar to pthread_mutex_t

Not sure I understand you here:
pthread_mutex provides only mutually-exclusive lock semantics.
For RW lock there exists: pthread_rwlock_t.
Off-course you can use rwlock fo exclusive locking too -
if all callers will use only writer locks, but usually that's no point to do that -
mutexes are simpler and faster.
That's for posix-like systems, don't know much about Windows environment :)

>   (CRITICAL_SECTION can only be taken by a single thread).
> 
> Technically it can be a "typedef uintptr_t" or a structure wrapping it.

Again can't say much about Windows, but pthread_mutex_t
can (and is) bigger then then 8 bytes. 

Re: [PATCH v18 8/8] eal: implement functions for mutex management

[Stephen Hemminger]

On Thu, 24 Feb 2022 17:29:03 +0000
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:

> Hi Dmitry,
> 
> > 2022-02-21 00:56 (UTC+0300), Dmitry Kozlyuk:  
> > > 2022-02-09 13:57 (UTC+0000), Ananyev, Konstantin:  
> > > > > > Actually, please scrap that comment.
> > > > > > Obviously it wouldn't work for static variables,
> > > > > > and doesn't make much sense.
> > > > > > Though few thoughts remain:
> > > > > > for posix we probably don't need an indirection and
> > > > > > rte_thread_mutex can be just typedef of pthread_mutex_t.
> > > > > > also for posix we don't need RTE_INIT constructor for each
> > > > > > static mutex initialization.
> > > > > > Something like:
> > > > > > #define RTE_STATIC_INITIALIZED_MUTEX(mx) \
> > > > > > 	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
> > > > > > should work, I think.
> > > > > > Konstantin  
> > > > >
> > > > > Thank you for reviewing, Konstantin!
> > > > > Some context for the current representation of mutex
> > > > > can be found in v9, patch 7/10 of this patchset.
> > > > >
> > > > > Originally we've typedef'ed the pthread_mutex_t on POSIX, just
> > > > > like you are suggesting here.
> > > > > However, on Windows there's no static initializer similar to the pthread
> > > > > one. Still, we want ABI compatibility and same thread behavior between
> > > > > platforms. The most elegant solution we found was the current representation,
> > > > > as suggested by Dmitry K.  
> > > >
> > > > Yes, I agree it is a problem with Windows for static initializer.
> > > > But why we can't have different structs typedef for mutex
> > > > for posix and windows platforms?  
> > >
> > > Yes, I agree that having different mutex types on *nix and Windows
> > > is a great idea. It will avoid ABI change for *nix
> > > and will guarantee no performance impact.
> > >
> > > Maybe wrap pthread_mutex_t into a struct to have a distinct type
> > > and to force using only DPDK API with it?
> > >
> > > [...]  
> > > > Yes, on Windows rte_thread_mutex still wouldn't work for MP,
> > > > but that's the same as with current design.  
> > >
> > > MP support is not planned for Windows and it is unknown if it ever will be,
> > > so it's not an issue.
> > > Data location is.
> > > The reason rte_thread_mutex_t is not a typedef of CRITICAL_SECTION
> > > (akin to pthread_mutex_t) is to avoid including Windows headers
> > > into DPDK public headers, because Windows headers can break user code
> > > by some macros they define.
> > > Maybe instead of a pointer it could be an opaque array:
> > >
> > > 	#define RTE_PTHREAD_MUTEX_SIZE 40
> > >
> > > 	struct rte_pthread_mutex_t {
> > > 		uint8_t opaque[RTE_PTHREAD_MUTEX_SIZE];
> > > 	};
> > >
> > > where RTE_PTHREAD_MUTEX_SIZE is actually sizeof(CRITICAL_SECTION).
> > > Win32 ABI is remarkably stable, I don't think this constant will ever change,
> > > it would break all the Windows user space.
> > > Naty, DmitryM, Tyler, what do you think?  
> > 
> > Conclusion from offline call: yes, this is OK to do so.
> > 
> > However, DmitryM suggested using Slim Reader-Writer lock (SRW):
> > https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks
> > instead of CRITICAL_SECTION.
> > It seems to be a much better option:
> > 
> > * sizeof(SRWLOCK) == 8 (technically "size of a pointer"),
> >   same as sizeof(pthread_mutex_t) on a typical Linux.
> >   Layout of data structures containing rte_thread_mutex_t
> >   can be the same on Windows and Unix,
> >   which simplifies design and promises similar less performance differences.
> > 
> > * Can be taken by multiple readers and one writer,
> >   which is semantically similar to pthread_mutex_t  
> 
> Not sure I understand you here:
> pthread_mutex provides only mutually-exclusive lock semantics.
> For RW lock there exists: pthread_rwlock_t.
> Off-course you can use rwlock fo exclusive locking too -
> if all callers will use only writer locks, but usually that's no point to do that -
> mutexes are simpler and faster.
> That's for posix-like systems, don't know much about Windows environment :)
> 
> >   (CRITICAL_SECTION can only be taken by a single thread).
> > 
> > Technically it can be a "typedef uintptr_t" or a structure wrapping it.  
> 
> Again can't say much about Windows, but pthread_mutex_t
> can (and is) bigger then then 8 bytes. 
> 
> 


There seems to be some confusion here:
   pthread_mutex put thread to sleep if contended and on linux are built on the futex system call.
   pthread_rwlock are the reader/writer versions of these.

The DPDK has primitives for multiple types of locks: spinlock, rwlock, ticketlock, pflock, etc
   these are build using atomic primitives (no syscall).
   these are platform independent
   these spin if contended

Not sure about Windows, but it looks like slim rwlocks came from Windows NT and are an implementation
of the same kind of spinning lock DPDK already has.

Re: [PATCH v18 8/8] eal: implement functions for mutex management

[Dmitry Kozlyuk]

Hi Stephen,

2022-02-24 09:44 (UTC-0800), Stephen Hemminger:
> There seems to be some confusion here:
>    pthread_mutex put thread to sleep if contended and on linux are built on the futex system call.
>    pthread_rwlock are the reader/writer versions of these.
> 
> The DPDK has primitives for multiple types of locks: spinlock, rwlock, ticketlock, pflock, etc
>    these are build using atomic primitives (no syscall).
>    these are platform independent
>    these spin if contended
> 
> Not sure about Windows, but it looks like slim rwlocks came from Windows NT and are an implementation
> of the same kind of spinning lock DPDK already has.

Both CRITICAL_SECTION and SRW lock spin shortly before going into kernel,
but both do this eventually---SRW lock is not a purely user space primitive.

Re: [PATCH v18 8/8] eal: implement functions for mutex management

[Dmitry Kozlyuk]

Hi Konstantin,

2022-02-24 17:29 (UTC+0000), Ananyev, Konstantin:
[...]
> > However, DmitryM suggested using Slim Reader-Writer lock (SRW):
> > https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks
> > instead of CRITICAL_SECTION.
> > It seems to be a much better option:
> > 
> > * sizeof(SRWLOCK) == 8 (technically "size of a pointer"),
> >   same as sizeof(pthread_mutex_t) on a typical Linux.
> >   Layout of data structures containing rte_thread_mutex_t
> >   can be the same on Windows and Unix,
> >   which simplifies design and promises similar less performance differences.
> > 
> > * Can be taken by multiple readers and one writer,
> >   which is semantically similar to pthread_mutex_t  
> 
> Not sure I understand you here:
> pthread_mutex provides only mutually-exclusive lock semantics.
> For RW lock there exists: pthread_rwlock_t.
> Off-course you can use rwlock fo exclusive locking too -
> if all callers will use only writer locks, but usually that's no point to do that -
> mutexes are simpler and faster.
> That's for posix-like systems, don't know much about Windows environment :)

It is different on Windows.
Multiple sources claim SRW lock is faster than CRITICAL_SECTION
even when used only for exclusive locking.
SRW locks do not support recursive locking,
while CRITICAL_SECTION is always recursive.
I see PTHREAD_MUTEX_RECURSIVE used in net/failsafe and raw/ifpga.
CRITICAL_SECTION also keeps debug information to analyze deadlocks
(can't say much here, never used this feature).

> > Technically it can be a "typedef uintptr_t" or a structure wrapping it.  
> 
> Again can't say much about Windows, but pthread_mutex_t
> can (and is) bigger then then 8 bytes. 

My bad, I measured incorrectly: sizeof(pthread_mutex_t) is 40 on my system.

Re: [PATCH v18 8/8] eal: implement functions for mutex management

[Narcisa Ana Maria Vasile]

On Mon, Feb 07, 2022 at 04:02:54PM +0000, Ananyev, Konstantin wrote:
> > Add functions for mutex init, destroy, lock, unlock, trylock.
> > 
> > Windows does not have a static initializer. Initialization
> > is only done through InitializeCriticalSection(). To overcome this,
> > RTE_INIT_MUTEX macro is added to replace static initialization
> > of mutexes. The macro calls rte_thread_mutex_init().
> > 
> > Add unit tests to verify that the mutex correctly locks/unlocks
> > and protects the data. Check both static and dynamic mutexes.
> > Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> 
> Few comments from me below.
> I am not sure was such approach already discussed,
> if so - apologies for repetition. 
> 

No worries, I appreciate your review!

> > ---
> >  app/test/test_threads.c      | 106 +++++++++++++++++++++++++++++++++++
> >  lib/eal/common/rte_thread.c  |  69 +++++++++++++++++++++++
> >  lib/eal/include/rte_thread.h |  85 ++++++++++++++++++++++++++++
> >  lib/eal/version.map          |   5 ++
> >  lib/eal/windows/rte_thread.c |  64 +++++++++++++++++++++
> >  5 files changed, 329 insertions(+)
> > 
> >  };
> > diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c
> > index d30a8a7ca3..4a9a1b6e07 100644
> > --- a/lib/eal/common/rte_thread.c
> > +++ b/lib/eal/common/rte_thread.c
> > @@ -309,6 +309,75 @@ rte_thread_detach(rte_thread_t thread_id)
> >  	return pthread_detach((pthread_t)thread_id.opaque_id);
> >  }
> > 
> > +int
> > +rte_thread_mutex_init(rte_thread_mutex *mutex)
> 
> Don't we need some sort of mutex_attr here too?
> To be able to create PROCESS_SHARED mutexes?

Attributes are tricky to implement on Windows.
In order to not overcomplicate this patchset and since the drivers
that need them don't compile on Windows anyway, I decided to omit
them from this patchset. In the future, after enabling the new thread API,
we can consider implementing them as well.

> 
> > +{
> > +	int ret = 0;
> > +	pthread_mutex_t *m = NULL;
> > +
> > +	RTE_VERIFY(mutex != NULL);
> > +
> > +	m = calloc(1, sizeof(*m));
> 
> But is that what we really want for the mutexes?
> It means actual mutex will always be allocated on process heap,
> away from the data it is supposed to guard.
> Even if we'll put performance considerations away,
> that wouldn't work for MP case.
> Is that considered as ok?

Are you refering to the fact that all mutexes will be dynamically allocated,
due to the static intializer calling _mutex_init() in the background?
Why wouldn't it work in the MP case?

> 
> > +	if (m == NULL) {
> > +		RTE_LOG(DEBUG, EAL, "Unable to initialize mutex. Insufficient memory!\n");
> > +		ret = ENOMEM;
> > +		goto cleanup;
> > +	}
> > +
> > +
> > +	return ret;
> > +}
> > +	return pthread_mutex_trylock((pthread_mutex_t *)mutex->mutex_id);

RE: [PATCH v18 8/8] eal: implement functions for mutex management

[Ananyev, Konstantin]

> On Mon, Feb 07, 2022 at 04:02:54PM +0000, Ananyev, Konstantin wrote:
> > > Add functions for mutex init, destroy, lock, unlock, trylock.
> > >
> > > Windows does not have a static initializer. Initialization
> > > is only done through InitializeCriticalSection(). To overcome this,
> > > RTE_INIT_MUTEX macro is added to replace static initialization
> > > of mutexes. The macro calls rte_thread_mutex_init().
> > >
> > > Add unit tests to verify that the mutex correctly locks/unlocks
> > > and protects the data. Check both static and dynamic mutexes.
> > > Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> >
> > Few comments from me below.
> > I am not sure was such approach already discussed,
> > if so - apologies for repetition.
> >
> 
> No worries, I appreciate your review!
> 
> > > ---
> > >  app/test/test_threads.c      | 106 +++++++++++++++++++++++++++++++++++
> > >  lib/eal/common/rte_thread.c  |  69 +++++++++++++++++++++++
> > >  lib/eal/include/rte_thread.h |  85 ++++++++++++++++++++++++++++
> > >  lib/eal/version.map          |   5 ++
> > >  lib/eal/windows/rte_thread.c |  64 +++++++++++++++++++++
> > >  5 files changed, 329 insertions(+)
> > >
> > >  };
> > > diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c
> > > index d30a8a7ca3..4a9a1b6e07 100644
> > > --- a/lib/eal/common/rte_thread.c
> > > +++ b/lib/eal/common/rte_thread.c
> > > @@ -309,6 +309,75 @@ rte_thread_detach(rte_thread_t thread_id)
> > >  	return pthread_detach((pthread_t)thread_id.opaque_id);
> > >  }
> > >
> > > +int
> > > +rte_thread_mutex_init(rte_thread_mutex *mutex)
> >
> > Don't we need some sort of mutex_attr here too?
> > To be able to create PROCESS_SHARED mutexes?
> 
> Attributes are tricky to implement on Windows.
> In order to not overcomplicate this patchset and since the drivers
> that need them don't compile on Windows anyway, I decided to omit
> them from this patchset. In the future, after enabling the new thread API,
> we can consider implementing them as well.

But it could just return ENOTSUP for Windows if 'attr' parameter is not NULL, no?

> 
> >
> > > +{
> > > +	int ret = 0;
> > > +	pthread_mutex_t *m = NULL;
> > > +
> > > +	RTE_VERIFY(mutex != NULL);
> > > +
> > > +	m = calloc(1, sizeof(*m));
> >
> > But is that what we really want for the mutexes?
> > It means actual mutex will always be allocated on process heap,
> > away from the data it is supposed to guard.
> > Even if we'll put performance considerations away,
> > that wouldn't work for MP case.
> > Is that considered as ok?
> 
> Are you refering to the fact that all mutexes will be dynamically allocated,
> due to the static intializer calling _mutex_init() in the background?
> Why wouldn't it work in the MP case?

No, I am talking about another case:
suppose we allocate a structure (with a mutex) inside shared memory
and plan to use it for inter-process communication.
But with rte_thread_mutex_init() implementation actual mutex object will 
be allocated on process heap (private area) and wouldn't be accessible by other
processes.  
As an example:

struct shared {
	rte_thread_mutex lock;
	uint32_t val;
};

...
struct shared *p = rte_zmalloc(NULL, sizeof(*p), RTE_CACHE_LINE_SIZE);
rte_thread_mutex_init(&p->lock);
	 
Now p->lock is in shared memory, but actual mutex object:
p->lock.mutex_id is within process private memory.


> 
> >
> > > +	if (m == NULL) {
> > > +		RTE_LOG(DEBUG, EAL, "Unable to initialize mutex. Insufficient memory!\n");
> > > +		ret = ENOMEM;
> > > +		goto cleanup;
> > > +	}
> > > +
> > > +
> > > +	return ret;
> > > +}
> > > +	return pthread_mutex_trylock((pthread_mutex_t *)mutex->mutex_id);

[dpdk-dev] [PATCH v17 00/13] eal: Add EAL API for threading

[Narcisa Ana Maria Vasile]

From: Narcisa Vasile <navasile@microsoft.com>

EAL thread API

**Problem Statement**
DPDK currently uses the pthread interface to create and manage threads.
Windows does not support the POSIX thread programming model,
so it currently
relies on a header file that hides the Windows calls under
pthread matched interfaces. Given that EAL should isolate the environment
specifics from the applications and libraries and mediate
all the communication with the operating systems, a new EAL interface
is needed for thread management.

**Goals**
* Introduce a generic EAL API for threading support that will remove
  the current Windows pthread.h shim.
* Replace references to pthread_* across the DPDK codebase with the new
  RTE_THREAD_* API.
* Allow users to choose between using the RTE_THREAD_* API or a
  3rd party thread library through a configuration option.

**Design plan**
New API main files:
* rte_thread.h (librte_eal/include)
* rte_thread.c (librte_eal/windows)
* rte_thread.c (librte_eal/common)

**A schematic example of the design**
--------------------------------------------------
lib/librte_eal/include/rte_thread.h
int rte_thread_create();

lib/librte_eal/common/rte_thread.c
int rte_thread_create() 
{
	return pthread_create();
}

lib/librte_eal/windows/rte_thread.c
int rte_thread_create() 
{
	return CreateThread();
}
-----------------------------------------------------

**Thread attributes**

When or after a thread is created, specific characteristics of the thread
can be adjusted. Currently in DPDK most threads operate at the OS-default
priority level but there are cases when increasing the priority is useful.
For example, high-performance applications require elevated priority to
avoid being preempted by other threads on the system.
The following structure that represents thread attributes has been
defined:

typedef struct
{
	enum rte_thread_priority priority;
	rte_cpuset_t cpuset;
} rte_thread_attr_t;

The *rte_thread_create()* function can optionally receive
an rte_thread_attr_t
object that will cause the thread to be created with the
affinity and priority
described by the attributes object. If no rte_thread_attr_t is passed
(parameter is NULL), the default affinity and priority are used.
An rte_thread_attr_t object can also be set to the default values
by calling *rte_thread_attr_init()*.

*Priority* is represented through an enum that currently advertises
two values for priority:
	- RTE_THREAD_PRIORITY_NORMAL
	- RTE_THREAD_PRIORITY_REALTIME_CRITICAL
The enum can be extended to allow for multiple priority levels.
rte_thread_set_priority      - sets the priority of a thread
rte_thread_get_priority      - retrieves the priority of a thread
                               from the OS
rte_thread_attr_set_priority - updates an rte_thread_attr_t object
                               with a new value for priority

*Affinity* is described by the already known “rte_cpuset_t” type.
rte_thread_attr_set/get_affinity - sets/gets the affinity field in a
                                   rte_thread_attr_t object
rte_thread_set/get_affinity      – sets/gets the affinity of a thread

**Errors**
As different platforms have different error codes, the approach here
is to translate the Windows error to POSIX-style ones to have
uniformity over the values returned. 

**Future work**
The long term plan is for EAL to provide full threading support:
* Add support for conditional variables
* Additional functionality offered by pthread_*
  (such as pthread_setname_np, etc.)

v17:
 - Move unrelated changes to the correct patch.
 - Rename RTE_STATIC_MUTEX to avoid confusion, since
   the mutex is still dynamically initialized behind the scenes.
 - Break down the unit tests into smaller patches and reorder them.
 - Remove duplicated code in header.
 - Improve commit messages and cover letter.

v16:
- Fix warning on freebsd by adding cast
- Change affinity unit test to consider ases when the requested CPU
  are not available on the system.
- Fix priority unit test to avoid termination of thread before the
  priority is checked.

v15:
- Add try_lock mutex functionality. If the mutex is already owned by a
  different thread, the function returns immediately. Otherwise, 
  the mutex will be acquired.
- Add function for getting the priority of a thread.
  An auxiliary function that translates the OS priority to the
  EAL accepted ones is added.
- Fix unit tests logging, add descriptive asserts that mark test failures.
  Verify mutex locking, verify barrier return values. Add test for
  statically initialized mutexes.
- Fix Alpine build by removing the use of pthread_attr_set_affinity() and
  using pthread_set_affinity() after the thread is created.

v14:
- Remove patch "eal: add EAL argument for setting thread priority"
  This will be added later when enabling the new threading API.
- Remove priority enum value "_UNDEFINED". NORMAL is used
  as the default.
- Fix issue with thread return value.

v13:
 - Fix syntax error in unit tests

v12:
 - Fix freebsd warning about initializer in unit tests

v11:
 - Add unit tests for thread API
 - Rebase

v10:
 - Remove patch no. 10. It will be broken down in subpatches 
   and sent as a different patchset that depends on this one.
   This is done due to the ABI breaks that would be caused by patch 10.
 - Replace unix/rte_thread.c with common/rte_thread.c
 - Remove initializations that may prevent compiler from issuing useful
   warnings.
 - Remove rte_thread_types.h and rte_windows_thread_types.h
 - Remove unneeded priority macros (EAL_THREAD_PRIORITY*)
 - Remove functions that retrieves thread handle from process handle
 - Remove rte_thread_cancel() until same behavior is obtained on
   all platforms.
 - Fix rte_thread_detach() function description,
   return value and remove empty line.
 - Reimplement mutex functions. Add compatible representation for mutex
   identifier. Add macro to replace static mutex initialization instances.
 - Fix commit messages (lines too long, remove unicode symbols)

v9:
- Sign patches

v8:
- Rebase
- Add rte_thread_detach() API
- Set default priority, when user did not specify a value

v7:
Based on DmitryK's review:
- Change thread id representation
- Change mutex id representation
- Implement static mutex inititalizer for Windows
- Change barrier identifier representation
- Improve commit messages
- Add missing doxygen comments
- Split error translation function
- Improve name for affinity function
- Remove cpuset_size parameter
- Fix eal_create_cpu_map function
- Map EAL priority values to OS specific values
- Add thread wrapper for start routine
- Do not export rte_thread_cancel() on Windows
- Cleanup, fix comments, fix typos.

v6:
- improve error-translation function
- call the error translation function in rte_thread_value_get()

v5:
- update cover letter with more details on the priority argument

v4:
- fix function description
- rebase

v3:
- rebase

v2:
- revert changes that break ABI 
- break up changes into smaller patches
- fix coding style issues
- fix issues with errors
- fix parameter type in examples/kni.c

Narcisa Vasile (13):
  eal: add basic threading functions
  eal: add thread attributes
  eal/windows: translate Windows errors to errno-style errors
  eal: implement functions for thread affinity management
  eal: implement thread priority management functions
  eal: add thread lifetime management
  app/test: add unit tests for rte_thread_self
  app/test: add unit tests for thread attributes
  app/test: add unit tests for thread lifetime management
  eal: implement functions for thread barrier management
  app/test: add unit tests for barrier
  eal: implement functions for mutex management
  app/test: add unit tests for mutex

 app/test/meson.build            |   2 +
 app/test/test_threads.c         | 372 ++++++++++++++++++
 lib/eal/common/meson.build      |   1 +
 lib/eal/common/rte_thread.c     | 497 ++++++++++++++++++++++++
 lib/eal/include/rte_thread.h    | 412 +++++++++++++++++++-
 lib/eal/unix/meson.build        |   1 -
 lib/eal/unix/rte_thread.c       |  92 -----
 lib/eal/version.map             |  22 ++
 lib/eal/windows/eal_lcore.c     | 176 ++++++---
 lib/eal/windows/eal_windows.h   |  10 +
 lib/eal/windows/include/sched.h |   2 +-
 lib/eal/windows/rte_thread.c    | 656 ++++++++++++++++++++++++++++++--
 12 files changed, 2070 insertions(+), 173 deletions(-)
 create mode 100644 app/test/test_threads.c
 create mode 100644 lib/eal/common/rte_thread.c
 delete mode 100644 lib/eal/unix/rte_thread.c

-- 
2.31.0.vfs.0.1

[PATCH v18 0/8] eal: Add EAL API for threading

[Narcisa Ana Maria Vasile]

From: Narcisa Vasile <navasile@microsoft.com>

EAL thread API

**Problem Statement**
DPDK currently uses the pthread interface to create and manage threads.
Windows does not support the POSIX thread programming model,
so it currently
relies on a header file that hides the Windows calls under
pthread matched interfaces. Given that EAL should isolate the environment
specifics from the applications and libraries and mediate
all the communication with the operating systems, a new EAL interface
is needed for thread management.

**Goals**
* Introduce a generic EAL API for threading support that will remove
  the current Windows pthread.h shim.
* Replace references to pthread_* across the DPDK codebase with the new
  RTE_THREAD_* API.
* Allow users to choose between using the RTE_THREAD_* API or a
  3rd party thread library through a configuration option.

**Design plan**
New API main files:
* rte_thread.h (librte_eal/include)
* rte_thread.c (librte_eal/windows)
* rte_thread.c (librte_eal/common)

**A schematic example of the design**
--------------------------------------------------
lib/librte_eal/include/rte_thread.h
int rte_thread_create();

lib/librte_eal/common/rte_thread.c
int rte_thread_create() 
{
	return pthread_create();
}

lib/librte_eal/windows/rte_thread.c
int rte_thread_create() 
{
	return CreateThread();
}
-----------------------------------------------------

**Thread attributes**

When or after a thread is created, specific characteristics of the thread
can be adjusted. Currently in DPDK most threads operate at the OS-default
priority level but there are cases when increasing the priority is useful.
For example, high-performance applications require elevated priority to
avoid being preempted by other threads on the system.
The following structure that represents thread attributes has been
defined:

typedef struct
{
	enum rte_thread_priority priority;
	rte_cpuset_t cpuset;
} rte_thread_attr_t;

The *rte_thread_create()* function can optionally receive
an rte_thread_attr_t
object that will cause the thread to be created with the
affinity and priority
described by the attributes object. If no rte_thread_attr_t is passed
(parameter is NULL), the default affinity and priority are used.
An rte_thread_attr_t object can also be set to the default values
by calling *rte_thread_attr_init()*.

*Priority* is represented through an enum that currently advertises
two values for priority:
	- RTE_THREAD_PRIORITY_NORMAL
	- RTE_THREAD_PRIORITY_REALTIME_CRITICAL
The enum can be extended to allow for multiple priority levels.
rte_thread_set_priority      - sets the priority of a thread
rte_thread_get_priority      - retrieves the priority of a thread
                               from the OS
rte_thread_attr_set_priority - updates an rte_thread_attr_t object
                               with a new value for priority

*Affinity* is described by the already known “rte_cpuset_t” type.
rte_thread_attr_set/get_affinity - sets/gets the affinity field in a
                                   rte_thread_attr_t object
rte_thread_set/get_affinity      – sets/gets the affinity of a thread

**Errors**
As different platforms have different error codes, the approach here
is to translate the Windows error to POSIX-style ones to have
uniformity over the values returned. 

**Future work**
The long term plan is for EAL to provide full threading support:
* Add support for conditional variables
* Additional functionality offered by pthread_*
  (such as pthread_setname_np, etc.)

v18:
 - Squash unit tests in corresponding patches.
 - Prevent priority to be set to realtime on non-Windows systems.
 - Use already existing affinity function in rte_thread_create()

v17:
 - Move unrelated changes to the correct patch.
 - Rename RTE_STATIC_MUTEX to avoid confusion, since
   the mutex is still dynamically initialized behind the scenes.
 - Break down the unit tests into smaller patches and reorder them.
 - Remove duplicated code in header
 - Improve commit messages and cover letter.

v16:
- Fix warning on freebsd by adding cast
- Change affinity unit test to consider ases when the requested CPU
  are not available on the system.
- Fix priority unit test to avoid termination of thread before the
  priority is checked.

v15:
- Add try_lock mutex functionality. If the mutex is already owned by a
  different thread, the function returns immediately. Otherwise, 
  the mutex will be acquired.
- Add function for getting the priority of a thread.
  An auxiliary function that translates the OS priority to the
  EAL accepted ones is added.
- Fix unit tests logging, add descriptive asserts that mark test failures.
  Verify mutex locking, verify barrier return values. Add test for
  statically initialized mutexes.
- Fix Alpine build by removing the use of pthread_attr_set_affinity() and
  using pthread_set_affinity() after the thread is created.

v14:
- Remove patch "eal: add EAL argument for setting thread priority"
  This will be added later when enabling the new threading API.
- Remove priority enum value "_UNDEFINED". NORMAL is used
  as the default.
- Fix issue with thread return value.

v13:
 - Fix syntax error in unit tests

v12:
 - Fix freebsd warning about initializer in unit tests

v11:
 - Add unit tests for thread API
 - Rebase

v10:
 - Remove patch no. 10. It will be broken down in subpatches 
   and sent as a different patchset that depends on this one.
   This is done due to the ABI breaks that would be caused by patch 10.
 - Replace unix/rte_thread.c with common/rte_thread.c
 - Remove initializations that may prevent compiler from issuing useful
   warnings.
 - Remove rte_thread_types.h and rte_windows_thread_types.h
 - Remove unneeded priority macros (EAL_THREAD_PRIORITY*)
 - Remove functions that retrieves thread handle from process handle
 - Remove rte_thread_cancel() until same behavior is obtained on
   all platforms.
 - Fix rte_thread_detach() function description,
   return value and remove empty line.
 - Reimplement mutex functions. Add compatible representation for mutex
   identifier. Add macro to replace static mutex initialization instances.
 - Fix commit messages (lines too long, remove unicode symbols)

v9:
- Sign patches

v8:
- Rebase
- Add rte_thread_detach() API
- Set default priority, when user did not specify a value

v7:
Based on DmitryK's review:
- Change thread id representation
- Change mutex id representation
- Implement static mutex inititalizer for Windows
- Change barrier identifier representation
- Improve commit messages
- Add missing doxygen comments
- Split error translation function
- Improve name for affinity function
- Remove cpuset_size parameter
- Fix eal_create_cpu_map function
- Map EAL priority values to OS specific values
- Add thread wrapper for start routine
- Do not export rte_thread_cancel() on Windows
- Cleanup, fix comments, fix typos.

v6:
- improve error-translation function
- call the error translation function in rte_thread_value_get()

v5:
- update cover letter with more details on the priority argument

v4:
- fix function description
- rebase

v3:
- rebase

v2:
- revert changes that break ABI 
- break up changes into smaller patches
- fix coding style issues
- fix issues with errors
- fix parameter type in examples/kni.c

Narcisa Vasile (8):
  eal: add basic threading functions
  eal: add thread attributes
  eal/windows: translate Windows errors to errno-style errors
  eal: implement functions for thread affinity management
  eal: implement thread priority management functions
  eal: add thread lifetime management
  eal: implement functions for thread barrier management
  eal: implement functions for mutex management

 app/test/meson.build            |   2 +
 app/test/test_threads.c         | 372 ++++++++++++++++++
 lib/eal/common/meson.build      |   1 +
 lib/eal/common/rte_thread.c     | 511 +++++++++++++++++++++++++
 lib/eal/include/rte_thread.h    | 412 +++++++++++++++++++-
 lib/eal/unix/meson.build        |   1 -
 lib/eal/unix/rte_thread.c       |  92 -----
 lib/eal/version.map             |  22 ++
 lib/eal/windows/eal_lcore.c     | 176 ++++++---
 lib/eal/windows/eal_windows.h   |  10 +
 lib/eal/windows/include/sched.h |   2 +-
 lib/eal/windows/rte_thread.c    | 656 ++++++++++++++++++++++++++++++--
 12 files changed, 2084 insertions(+), 173 deletions(-)
 create mode 100644 app/test/test_threads.c
 create mode 100644 lib/eal/common/rte_thread.c
 delete mode 100644 lib/eal/unix/rte_thread.c

-- 
2.31.0.vfs.0.1

[PATCH v18 8/8] eal: implement functions for mutex management

[Narcisa Ana Maria Vasile]

From: Narcisa Vasile <navasile@microsoft.com>

Add functions for mutex init, destroy, lock, unlock, trylock.

Windows does not have a static initializer. Initialization
is only done through InitializeCriticalSection(). To overcome this,
RTE_INIT_MUTEX macro is added to replace static initialization
of mutexes. The macro calls rte_thread_mutex_init().

Add unit tests to verify that the mutex correctly locks/unlocks
and protects the data. Check both static and dynamic mutexes.

Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
---
 app/test/test_threads.c      | 106 +++++++++++++++++++++++++++++++++++
 lib/eal/common/rte_thread.c  |  69 +++++++++++++++++++++++
 lib/eal/include/rte_thread.h |  85 ++++++++++++++++++++++++++++
 lib/eal/version.map          |   5 ++
 lib/eal/windows/rte_thread.c |  64 +++++++++++++++++++++
 5 files changed, 329 insertions(+)

diff --git a/app/test/test_threads.c b/app/test/test_threads.c
index 00f604ab7e..91155a04e3 100644
--- a/app/test/test_threads.c
+++ b/app/test/test_threads.c
@@ -243,6 +243,110 @@ test_thread_barrier(void)
 	return 0;
 }
 
+RTE_INIT_MUTEX(static_mutex);
+
+struct mutex_loop_args {
+	rte_thread_barrier *barrier;
+	rte_thread_mutex *mutex;
+	unsigned long result_A;
+	unsigned long result_B;
+};
+
+static void *
+thread_loop_mutex_B(void *arg)
+{
+	struct mutex_loop_args *args = arg;
+
+	if (rte_thread_mutex_try_lock(args->mutex) == 0) {
+		rte_thread_barrier_wait(args->barrier);
+		rte_thread_mutex_unlock(args->mutex);
+		args->result_B = 1;
+	} else {
+		rte_thread_barrier_wait(args->barrier);
+		args->result_B = 2;
+	}
+
+	return NULL;
+}
+
+static void *
+thread_loop_mutex_A(void *arg)
+{
+	struct mutex_loop_args *args = arg;
+
+	if (rte_thread_mutex_try_lock(args->mutex) != 0) {
+		rte_thread_barrier_wait(args->barrier);
+		args->result_A = 2;
+	} else {
+		rte_thread_barrier_wait(args->barrier);
+		rte_thread_mutex_unlock(args->mutex);
+		args->result_A = 1;
+	}
+
+	return NULL;
+}
+
+static int
+test_thread_mutex(rte_thread_mutex *pmutex)
+{
+	rte_thread_t thread_A;
+	rte_thread_t thread_B;
+	rte_thread_mutex mutex;
+	rte_thread_barrier barrier;
+	struct mutex_loop_args args;
+	int ret = 0;
+
+	/* If mutex is not statically initialized */
+	if (pmutex == NULL) {
+		ret = rte_thread_mutex_init(&mutex);
+		RTE_TEST_ASSERT(ret == 0, "Failed to initialize mutex!");
+	} else
+		mutex = *pmutex;
+
+	ret = rte_thread_barrier_init(&barrier, 2);
+	RTE_TEST_ASSERT(ret == 0, "Failed to initialize barrier!");
+
+	args.mutex = &mutex;
+	args.barrier = &barrier;
+
+	ret = rte_thread_create(&thread_A, NULL, thread_loop_mutex_A, &args);
+	RTE_TEST_ASSERT(ret == 0, "Failed to create thread!");
+
+	ret = rte_thread_create(&thread_B, NULL, thread_loop_mutex_B, &args);
+	RTE_TEST_ASSERT(ret == 0, "Failed to create thread!");
+
+	ret = rte_thread_join(thread_A, NULL);
+	RTE_TEST_ASSERT(ret == 0, "Failed to join thread!");
+
+	ret = rte_thread_join(thread_B, NULL);
+	RTE_TEST_ASSERT(ret == 0, "Failed to join thread!");
+
+	RTE_TEST_ASSERT(args.result_A != args.result_B, "Mutex failed to be acquired or was acquired by both threads!");
+
+	/* Destroy if dynamically initialized */
+	if (pmutex == NULL) {
+		ret = rte_thread_mutex_destroy(&mutex);
+		RTE_TEST_ASSERT(ret == 0, "Failed to destroy mutex!");
+	}
+
+	ret = rte_thread_barrier_destroy(&barrier);
+	RTE_TEST_ASSERT(ret == 0, "Failed to destroy barrier!");
+
+	return ret;
+}
+
+static int
+test_thread_mutex_static(void)
+{
+	return test_thread_mutex(&static_mutex);
+}
+
+static int
+test_thread_mutex_dynamic(void)
+{
+	return test_thread_mutex(NULL);
+}
+
 static struct unit_test_suite threads_test_suite = {
 	.suite_name = "threads autotest",
 	.setup = NULL,
@@ -253,6 +357,8 @@ static struct unit_test_suite threads_test_suite = {
 			TEST_CASE(test_thread_attributes_priority),
 			TEST_CASE(test_thread_detach),
 			TEST_CASE(test_thread_barrier),
+			TEST_CASE(test_thread_mutex_static),
+			TEST_CASE(test_thread_mutex_dynamic),
 			TEST_CASES_END()
 	}
 };
diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c
index d30a8a7ca3..4a9a1b6e07 100644
--- a/lib/eal/common/rte_thread.c
+++ b/lib/eal/common/rte_thread.c
@@ -309,6 +309,75 @@ rte_thread_detach(rte_thread_t thread_id)
 	return pthread_detach((pthread_t)thread_id.opaque_id);
 }
 
+int
+rte_thread_mutex_init(rte_thread_mutex *mutex)
+{
+	int ret = 0;
+	pthread_mutex_t *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;
+	}
+
+	ret = pthread_mutex_init(m, NULL);
+	if (ret != 0) {
+		RTE_LOG(DEBUG, EAL, "Failed to init mutex. ret = %d\n", ret);
+		goto cleanup;
+	}
+
+	mutex->mutex_id = m;
+	m = NULL;
+
+cleanup:
+	free(m);
+	return ret;
+}
+
+int
+rte_thread_mutex_lock(rte_thread_mutex *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	return pthread_mutex_lock((pthread_mutex_t *)mutex->mutex_id);
+}
+
+int
+rte_thread_mutex_unlock(rte_thread_mutex *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	return pthread_mutex_unlock((pthread_mutex_t *)mutex->mutex_id);
+}
+
+int
+rte_thread_mutex_try_lock(rte_thread_mutex *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	return pthread_mutex_trylock((pthread_mutex_t *)mutex->mutex_id);
+}
+
+int
+rte_thread_mutex_destroy(rte_thread_mutex *mutex)
+{
+	int ret = 0;
+	RTE_VERIFY(mutex != NULL);
+
+	ret = pthread_mutex_destroy((pthread_mutex_t *)mutex->mutex_id);
+	if (ret != 0)
+		RTE_LOG(DEBUG, EAL, "Unable to destroy mutex, ret = %d\n", ret);
+
+	free(mutex->mutex_id);
+	mutex->mutex_id = NULL;
+
+	return ret;
+}
+
 int
 rte_thread_barrier_init(rte_thread_barrier *barrier, int count)
 {
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
index 7c84e32988..09a5fd8add 100644
--- a/lib/eal/include/rte_thread.h
+++ b/lib/eal/include/rte_thread.h
@@ -54,6 +54,25 @@ typedef struct {
 
 #endif /* RTE_HAS_CPUSET */
 
+#define RTE_DECLARE_MUTEX(private_lock)          rte_thread_mutex private_lock
+
+#define RTE_DEFINE_MUTEX(private_lock)\
+RTE_INIT(__rte_ ## private_lock ## _init)\
+{\
+	RTE_VERIFY(rte_thread_mutex_init(&private_lock) == 0);\
+}
+
+#define RTE_INIT_MUTEX(private_lock)\
+static RTE_DECLARE_MUTEX(private_lock);\
+RTE_DEFINE_MUTEX(private_lock)
+
+/**
+ * Thread mutex representation.
+ */
+typedef struct rte_thread_mutex_tag {
+	void *mutex_id;  /**< mutex identifier */
+} rte_thread_mutex;
+
 /**
  * Returned by rte_thread_barrier_wait() when call is successful.
  */
@@ -314,6 +333,72 @@ __rte_experimental
 int rte_thread_set_priority(rte_thread_t thread_id,
 		enum rte_thread_priority priority);
 
+/**
+ * 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 *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 *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 *mutex);
+
+/**
+ * Tries to lock a mutex.If the mutex is already held by a different thread,
+ * the function returns without blocking.
+ *
+ * @param mutex
+ *    The mutex that will be acquired, if not already locked.
+ *
+ * @return
+ *   On success, if the mutex is acquired, return 0.
+ *   On failure, return a positive errno-style error number.
+ */
+__rte_experimental
+int rte_thread_mutex_try_lock(rte_thread_mutex *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 *mutex);
+
 /**
  * Initializes a synchronization barrier.
  *
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 06e5f82da2..e80eea4316 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -431,6 +431,11 @@ EXPERIMENTAL {
 	rte_thread_barrier_wait;
 	rte_thread_barrier_destroy;
 	rte_thread_get_affinity_by_id;
+	rte_thread_mutex_init;
+	rte_thread_mutex_lock;
+	rte_thread_mutex_unlock;
+	rte_thread_mutex_try_lock;
+	rte_thread_mutex_destroy;
 	rte_thread_set_affinity_by_id;
 	rte_thread_get_priority;
 	rte_thread_set_priority;
diff --git a/lib/eal/windows/rte_thread.c b/lib/eal/windows/rte_thread.c
index 3f72bbf716..11b4863fe8 100644
--- a/lib/eal/windows/rte_thread.c
+++ b/lib/eal/windows/rte_thread.c
@@ -504,6 +504,70 @@ rte_thread_detach(rte_thread_t thread_id)
 	return 0;
 }
 
+int
+rte_thread_mutex_init(rte_thread_mutex *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 *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	EnterCriticalSection(mutex->mutex_id);
+	return 0;
+}
+
+int
+rte_thread_mutex_unlock(rte_thread_mutex *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	LeaveCriticalSection(mutex->mutex_id);
+	return 0;
+}
+
+int
+rte_thread_mutex_try_lock(rte_thread_mutex *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	if (TryEnterCriticalSection(mutex->mutex_id) != 0)
+		return 0;
+
+	return EBUSY;
+}
+
+int
+rte_thread_mutex_destroy(rte_thread_mutex *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	DeleteCriticalSection(mutex->mutex_id);
+	free(mutex->mutex_id);
+	mutex->mutex_id = NULL;
+
+	return 0;
+}
+
 int
 rte_thread_barrier_init(rte_thread_barrier *barrier, int count)
 {
-- 
2.31.0.vfs.0.1

Re: [PATCH v18 8/8] eal: implement functions for mutex management

[Narcisa Ana Maria Vasile]

On Wed, Nov 10, 2021 at 05:33:45PM -0800, Narcisa Ana Maria Vasile wrote:
> From: Narcisa Vasile <navasile@microsoft.com>
> 
> Add functions for mutex init, destroy, lock, unlock, trylock.
> 
> Windows does not have a static initializer. Initialization
> is only done through InitializeCriticalSection(). To overcome this,
> RTE_INIT_MUTEX macro is added to replace static initialization
> of mutexes. The macro calls rte_thread_mutex_init().
> 
> Add unit tests to verify that the mutex correctly locks/unlocks
> and protects the data. Check both static and dynamic mutexes.
> 
> Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> ---

Hi Bruce R., Stephen H., I've followed Bruce's suggestion and changed this patchset
to return ENOTSUP for realtime priority on Linux. Can you please take a look at
the latest version of the patchset?
Let me know if it needs any changes or if it's ready to be merged. Thank you!