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

[Stephen Hemminger <stephen@networkplumber.org>]

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.