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

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

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.