Merge ~dirk.zimoch/epics-base:epicsMutexPriorityInheritance into ~epics-core/epics-base/+git/epics-base:7.0

[Dirk Zimoch]

This change enables priority inheritance for posix mutexes and thus prevents priority inversion problems on real-time Linux systems.

For backward compatibility reasons, this feature must be explicitly enabled by setting the environment variable EPICS_MUTEX_USE_PRIORITY_INHERITANCE=YES before libCom is loaded.

Is only meaningful when running with SCHED_FIFO scheduling, which requires sufficient priviledges to set. That typically means to run the IOC as root.

[Dirk Zimoch (dirk.zimoch)]

Test does not succeed when not running as root or EPICS_MUTEX_USE_PRIORITY_INHERITANCE is not set before.

review: Needs Fixing

e8a0c17... by Till Straumann on 2020-11-23

Support run-time enabled priority-inheritance for epicsMutex, epicsEvent etc.

[Ralph Lange (ralph-lange)]

> ...SCHED_FIFO scheduling, which requires sufficient privileges to set. That typically means to run the IOC as root.

FWIW, IOCs should *not* be run as root.
The EPICS website has detailed instructions at
https://epics-controls.org/resources-and-support/documents/howto-documents/posix-thread-priority/

[mdavidsaver (mdavidsaver)]

I see in posix/osdMutex.c that

> /* Until these can be demonstrated to work leave them undefined*/
> /* On solaris 8 _POSIX_THREAD_PRIO_INHERIT fails*/
> #undef _POSIX_THREAD_PROCESS_SHARED
> #undef _POSIX_THREAD_PRIO_INHERIT

has been present since 2002. I would naively think that the various sites using RTLinux flavors which have noticed quickly if epicsMutex didn't have priority inheritance.

Can Dirk (or anyone) give some background on why this change is being proposed now?

Also, some significant complexity is being added in the !HAVE_RECURSIVE_MUTEX case. Is this implementation actually used by any currently supported targets?

[Dirk Zimoch (dirk.zimoch)]

We found that EPICS was not behaving as "real-time" as expected even though SCHED_FIFO was used. A deeper investigation found that this was due to prioriy inversion. When reading the source code, we found that "Until these can be demonstrated to work leave them undefined" has been in for a long time and nobody seemed to have cared, which had surprized us a lot. I think this wither got forgotten somehow and nobody really needed it or noticed it or maybe people simply removed those #undefs (which is not sufficient because other code used posix semaphores directly instead of using epicsMutexes).

I propose this change now, because we have fixed it now.

Till needs to comment on details of the implementation...

[Dirk Zimoch (dirk.zimoch)]

> this wither got forgotten
* either

[mdavidsaver (mdavidsaver)]

I tried the simply minded (early morning) test of removing the two #undef lines. It seems like to have the desired effect. pthread_mutexattr_setprotocol(..., PTHREAD_PRIO_INHERIT) succeeds with for a normal unprivleged user. I added some prints to confirm that status==0.

Am I missing something obvious?

I'm testing with Debian 10, where _XOPEN_SOURCE=700 (by way of -D_GNU_SOURCE).

Also, what is the motivation to economize pthread_mutexattr_t? I see __SIZEOF_PTHREAD_MUTEXATTR_T==4 (on a 64-bit host).

[Till Straumann (tillstraumann)]

> We found that EPICS was not behaving as "real-time" as expected even though
> SCHED_FIFO was used. A deeper investigation found that this was due to prioriy
> inversion. When reading the source code, we found that "Until these can be
> demonstrated to work leave them undefined" has been in for a long time and
> nobody seemed to have cared, which had surprized us a lot. I think this wither
> got forgotten somehow and nobody really needed it or noticed it or maybe
> people simply removed those #undefs (which is not sufficient because other
> code used posix semaphores directly instead of using epicsMutexes).
>
> I propose this change now, because we have fixed it now.
>
> Till needs to comment on details of the implementation...

At SLAC we had -- for years -- patched this old #undef. What I only noticed recently is that even if you patch the #undef EPICS still uses non-PI mutexes in some other places (e.g., the mutex protecting the condition variable in epicsEvent) and this is what this particular contribution addresses.

Re - 'root': it is absolutely correct that you don't have to be root to use SCHED_FIFO (and at SLAC we had followed the guidelines linked by Ralph). Using 'root' is just the easiest way to get there...

[mdavidsaver (mdavidsaver)]

Another question, what motivates the addition of epicsPosixMutex*()? At first glance, I'm not enthused to add an OS specific extension to an OS independent API?

review: Needs Fixing

[mdavidsaver (mdavidsaver)]

> ... still uses non-PI mutexes in some other places ...

Ah, I see. pthread_mutex_init() currently appears in posix/osdEvent.c, osdMutex.c, osdSpin.c, and osdThread.c.

[mdavidsaver (mdavidsaver)]

> I tried the simply minded (early morning) test of removing the two #undef lines.

Turns out I have a quick answer for this. Some ~emulated environments don't fully support the PI futex() operations. (eg. rr-project.org) In these cases pthread_mutex_init() fails with ENOTSUP.

It seems like the right approach is to probe, as is done in osdThread.c, to see if PI mutex is available.

[Andrew Johnson (anj)]

Names beginning with epics are generally intended for public use, which the epicsPosixMutex stuff shouldn't be. If I've understood the code correctly you aren't replacing the pthread_mutex_t type but are providing a wrapper for pthread_mutex_init() that creates one, and you're also moving the need for the caller to set mutex attributes on the new mutex. Any new publicly visible symbols that are meant to be internal to a libCom implementation should begin with "osd", so maybe osd_mutex_init()?

I've read the essay in the comments just below epicsMutexOsdShow(), but I'm not sure how it helps future maintainers to understand this file. The discussion there doesn't seem appropriate in source code comments.

Is the new test program generic? It has some CPU-affinity stuff in it which is conditional on glibc; what happens on other OSs (Windows, MacOS, RTEMS-5, embedded Linux) that can also do SMP and don't use glibc?

I'm not sure I like the new environment variable or understand what effect not defining it will have. Is there a good reason to continue to support the previous mode if in practice it's broken? As long as we document the changes in behavior when SCHED_FIFO is available (I'm assuming nothing should change when it isn't) I'm not convinced we need the env switch, which would resolve Dirk's question about how to set an environment variable before running a test program.

However I wonder if it would be desirable to run this new test (and maybe some of the other thread-related test programs too) twice, once with the default scheduler and once under SCHED_FIFO?

Finally I think we would probably want to delay merging this until after Heinz' RTEMS-5 code has been pulled in, that might require some changes to this MR because he uses the Posix APIs in the new RTEMS port.

review: Needs Fixing

[Dirk Zimoch (dirk.zimoch)]

> Also, what is the motivation to economize pthread_mutexattr_t? I see
> __SIZEOF_PTHREAD_MUTEXATTR_T==4 (on a 64-bit host).

It's simply not necessary that each mutex creates its own attribute.
From the pthread_mutexattr_init manual page:
"After a mutex attributes object has been used to initialize one or more mutexes, any function affecting the attributes object (including destruction) shall not affect any previously initialized mutexes."
Thus the same attribute can be used for all mutexes and the mutex does not use it any more after creation. It is a waste of not only space but also time and error handling complexity to have (and keep!) one in each mutex.

[mdavidsaver (mdavidsaver)]

> ... intended for public use, which the epicsPosixMutex stuff shouldn't be ...

Ok, as internal helpers, calls like these (with an ANJ compliant name) are perfectly reasonable. I would like to see them moved to a private header. Maybe 'os/posix/osdMutexPvt.h'?

> ... I'm not convinced we need the env switch ...

Likewise. We already have the build time $(USE_POSIX_THREAD_PRIORITY_SCHEDULING) switch.

> ... I'm not sure how it helps future maintainers to understand this file ...

I'll go further. In-code comments should provide meaningful context for the code around them. This discussion would make a nice core-talk (or maybe blog) post, but is a distraction here.

> ... I think we would probably want to delay merging this until after Heinz' RTEMS-5 code has been pulled in ...

I don't like this. To my mind, we merge whichever is ready first, and the second needs to be reworked to resolve any conflicts. I think this gives a health incentive for promptness.

[Dirk Zimoch (dirk.zimoch)]

> Also, what is the motivation to economize pthread_mutexattr_t?

Should I make a separate merge request that just handles the removal of per-mutex attributes?
This may allow to merge the cleanup before before agreeing on the new features.

[mdavidsaver (mdavidsaver)]

I think a smaller diff would help move the process faster, as would more targeted changes.

Personally, I'm not sure I'll accept a global pthread_mutexattr_t. I think this should become a local in epicsMutexOsdCreate() (or equivalent helper).

As a general rule, I prefer locality for various reasons. Modern processors with deep caches generally reward memory locality. Mostly though, it is a case of readability. I find it easier to understand code which doesn't reference globals.

[Andrew Johnson (anj)]

Ack on Michael's private header idea since I believe these definitions should only be needed when compiling the code in libCom itself, not by the client application.

Looking at the code again, there is only one call to epicsPosixMutexMustInit(), from a place where it isn't actually needed. The epicsMutexOsdCreate() routine is supposed to return NULL if it can't finish (not suspend the thread) so calling a MustInit() routine there is wrong and the code for it can be removed.

Similarly there is only one call to epicsPosixMutexAttrGet(), from the epicsPosixMutexInit() routine. The two pthread_mutexattr_t objects are really just constants passed into pthread_mutex_init() that we have to look up the values of at run-time, and they aren't globally visible anyway. The osd_mutex_init() routine should create a static instance the first time each one is asked for, or create both the first time it's called. Till's use of pthread_once() is fine for doing that, but moving it into the _init() routine would be better.

Hmm, according to the online spec (and both manpages I just checked) passing a NULL attribute into pthread_mutex_init() is legal and is equivalent to a default-initialized mutex, so we only actually need to create the recursive one.

I'm also not terribly keen on adding our own enum for selecting the mutex type either, why not just use the mutex type constants PTHREAD_MUTEX_DEFAULT and PTHREAD_MUTEX_RECURSIVE as the second parameter to osd_mutex_init() to switch on? The pthread.h header is required to define those, and to be honest I don't believe that any of our supported targets don't support both (does anyone know of a supported Posix target that doesn't have recursive mutexes?). I wonder how much of the other conditional code could we drop too, just putting a check somewhere so the build will fail on any Posix target that doesn't provide the features we need?

We don't support Solaris any more (especially Solaris 8), so those comments can go too.

I withdraw my "after RTEMS-5" suggesting, if this becomes ready first it should get merged. It looks like the two won't conflict anyway though.

[Dirk Zimoch (dirk.zimoch)]

I am taking the changes apart now...

[Dirk Zimoch (dirk.zimoch)]

> Names beginning with epics are generally intended for public use, which the
> epicsPosixMutex stuff shouldn't be. If I've understood the code correctly you
> aren't replacing the pthread_mutex_t type but are providing a wrapper for
> pthread_mutex_init() that creates one, and you're also moving the need for the
> caller to set mutex attributes on the new mutex. Any new publicly visible
> symbols that are meant to be internal to a libCom implementation should begin
> with "osd", so maybe osd_mutex_init()?

I think something with "posix" in the name would be good as it only affects posix mutexes?
But maybe Till had more in mind: An OSI interface to ask for recursive or (maybe cheaper) non-recursive mutexes, no matter how they are implemented and what the OS is. Could be semM vs semB in vxworks or whatever in Windows. Of course the header should not be "osd" in that case.
Is there a need to ask for (possibly cheaper) non-recursive mutexes? They are now only used in osdSpin.c (only if no posix spin locks are available), in osdEvent.c and osdThread.c. Everything else uses recursive mutexes.

>
> I've read the essay in the comments just below epicsMutexOsdShow(), but I'm
> not sure how it helps future maintainers to understand this file. The
> discussion there doesn't seem appropriate in source code comments.

I will remove the essay.

>
> Is the new test program generic? It has some CPU-affinity stuff in it which is
> conditional on glibc; what happens on other OSs (Windows, MacOS, RTEMS-5,
> embedded Linux) that can also do SMP and don't use glibc?

The change is in posix only, not Windows or RTEMS. The test is Linux only. Maybe it should be posix only as well. The CPU affinities are used to guarantee CPU starvarion of the high priority thread on multi-core systems as well. Otherwise the test may show a false success. Should be possible to run the test on all posix architectures.

Do we need a cpu affinity OSI API?

>
> I'm not sure I like the new environment variable or understand what effect not
> defining it will have. Is there a good reason to continue to support the
> previous mode if in practice it's broken? As long as we document the changes
> in behavior when SCHED_FIFO is available (I'm assuming nothing should change
> when it isn't) I'm not convinced we need the env switch, which would resolve
> Dirk's question about how to set an environment variable before running a test
> program.

The previous implementation is a bit "faster on average" because the mutexes are simpler. But "fast" only counts in RT context, and there "faster on average" basically means "wrong".

I can take the environment variable out. It was in so we can easily switch back and forth to see the difference. And as an anti change-panic device.

>
> However I wonder if it would be desirable to run this new test (and maybe some
> of the other thread-related test programs too) twice, once with the default
> scheduler and once under SCHED_FIFO?

You mean expected failure is success? How to program that?

9f08d6d... by Dirk Zimoch on 2020-11-25

removed the in-code essay

45856b0... by Dirk Zimoch on 2020-11-25

remove epicsMutexMustInit, instead return NULL on failure

5ee60ae... by Dirk Zimoch on 2020-11-25

remove epicsPosixMutexAttrGet() enum EpicsPosixMutexProperty

b4afc97... by Dirk Zimoch on 2020-11-25

rename epicsPosixMutexInit to osdPosixMutexInit

[Andrew Johnson (anj)]

> I think something with "posix" in the name would be good as it only affects posix mutexes?

Whether a mutex is "posix" or not is a characteristic of the target being built for, not the kind of mutex being created. There are no posix mutexes for our Windows or VxWorks implementations so that makes no sense to me. I suggested using a more pthreads-style name osd_mutex_init() because the thing it returns is an actual pthread_mutex_t which gets passed into other pthread_*() routines. Passing something apparently created by a non-pthreads routine into a pthreads routine is a bit of a code smell, it looks wrong even though it isn't really.

> You mean expected failure is success? How to program that?

There are some problems with the test program; it always defines TEST_PLAN as 8, but the number of test results that will be reported will not be the same for all builds. There are several tests which won't report any result if HAVE_CPU_AFFINITY is undefined. This situation is why the unitTest API provides testSkip(), which lets a program declare that it can't run some number of tests in its current configuration or on this target. That is how running the same test with both schedulers would work, at run-time the code detects whether the tests should pass or not and calls testSkip() instead if not. Please don't try to adjust the test plan at runtime instead, it's much easier to read test code if a particular group of tests always generates the same number of results.

My suggestion of running the thread test programs a second time on Linux was really an idea for Michael and Ralph to consider: On the CI runners that give us sudo it ought to be possible to configure our account's maxrtprio hard limit somewhere in the job setup, then we would add linux-only scripts such as epicsThreadPriorityRT.plt which run the program again using say
        prlimit -r 80 ./epicsThreadPriority

b611d31... by Dirk Zimoch on 2020-11-26

moved osdPosixMutexInit to private header

e41fa06... by Dirk Zimoch on 2020-11-26

remove check of environment variable EPICS_MUTEX_USE_PRIORITY_INHERITANCE

20c0dba... by Dirk Zimoch on 2020-11-26

removed comment about outdated solaris version

[Dirk Zimoch (dirk.zimoch)]

Is there any posix implementation which does not support PTHREAD_MUTEX_RECURSIVE? That would be systems with _XOPEN_SOURCE < 500, that is Posix 1995.

I mean: do we still need the alternative implementation?

I have 12 different Linux architectures, some quite old, and none uses that compilation path.

If we do, should _XOPEN_SOURCE be the thing to be tested or rather #ifdef PTHREAD_MUTEX_RECURSIVE ?

[Dirk Zimoch (dirk.zimoch)]

> If we do, should _XOPEN_SOURCE be the thing to be tested or rather #ifdef
> PTHREAD_MUTEX_RECURSIVE ?

I just see that PTHREAD_MUTEX_RECURSIVE is an enum, not a #define. Thus, no way to check it with #ifdef.

[Dirk Zimoch (dirk.zimoch)]

Found a problem in one of my earlier changes. Will changes that and force-push soon....

f2bd8ec... by Dirk Zimoch on 2020-11-27

PTHREAD_MUTEX_NORMAL may not be defined when Posix version is too old (before 1995)

3c7596d... by Dirk Zimoch on 2020-11-27

removed unreachable code

7e1812e... by Dirk Zimoch on 2020-11-27

added missing macro in case USE_POSIX_THREAD_PRIORITY_SCHEDULING=NO

5703c77... by Dirk Zimoch on 2020-11-27

removed warning which interfered with tests

[Dirk Zimoch (dirk.zimoch)]

Problem was not as big as I first thought. No force-push necessary.

Still the question remains: Sould we support _XOPEN_SOURCE < 500 ?

[mdavidsaver (mdavidsaver)]

I'm inclined to say no. Of course our problem is how to decide.

Technically, I find references in 'man feature_test_macros' to _XOPEN_SOURCE>=600 being recognized by glibc 2.2 (circa 2000). So at least on Linux I think the <500 case stopped being used years ago. Since we no longer support solaris, that leaves the *BSDs, and OSX.

Practically, if the change passes the Linux and OSX CI builders, I'd be happy to accept the simplification.

[mdavidsaver (mdavidsaver)]

Also, it may be worth repeating. Even on modern Linux, support for PTHREAD_PRIO_INHERIT can't be taken for granted as some (partially) emulated environments don't implement/allow all of the syscalls necessary for PI mutex operations.

So I think it is necessary to probe for support at runtime. eg. with glibc, an error will be returned by pthread_mutex_init().

The obvious place to do this would be in your thread once function. Try to create a mutex with PTHREAD_PRIO_INHERIT, the fallback to PTHREAD_PRIO_NONE if this isn't possible. This would also help convince me that a thread once is the better approach.

[Dirk Zimoch (dirk.zimoch)]

Sure, run time failure needs to be considered. Falling back from PTHREAD_PRIO_INHERIT to PTHREAD_PRIO_NONE is relatively safe.

What we really need is recursive mutexes. Removing the pre 1995 compatible code (older than VxWorks 5.5, yay) would simplify the implementation a lot. As long as we document the drop of support, I think we are fine.

4039d97... by Dirk Zimoch on 2020-11-30

drop workaround for missing PTHREAD_MUTEX_RECURSIVE support

[Dirk Zimoch (dirk.zimoch)]

I have now dropped the workaround for missing PTHREAD_MUTEX_RECURSIVE in pre 2001 Posix.

Falling back from PTHREAD_PRIO_INHERIT to PTHREAD_PRIO_NONE is still supported of course. Line 60 pthread_mutexattr_settype() will fail but that will simply result in a default mutex attribute and an error message (if errVerbose is set).

Setting PTHREAD_MUTEX_RECURSIVE in line 70 must not fail. That should not be a problem, even for rr or other special systems.

The only new external function is now called osdPosixMutexInit (not starting with "epics") and is in the private header file osdPosixMutexPriv.h which does not get installed. It is used inside libCom only.

I am not yet happy with the test. I need to find out how to use testSkip() properly when SCHED_FIFO is not available or the prority range is too limited.

There seems to be no TESTPROD_HOST_Posix (instead of TESTPROD_HOST_Linux) to run the test on all posix architectures. How to do that in the Makefile?

4197954... by Dirk Zimoch on 2020-11-30

remove unnecessary setAttrDefaults() function

6d54127... by Dirk Zimoch on 2020-11-30

run epicsMutexPriorityInversionTest for all Posix architectures

[Andrew Johnson (anj)]

A couple more small changes that I noticed last week (see my inline comments), these may be the last changes we ask for – thanks!

9eda0c0... by Dirk Zimoch on 2020-12-18

removed useless epicsShare macros from osdPosixMutexInit

[Dirk Zimoch (dirk.zimoch)]

Done. I would like to fix the test so that it does not complain when RT prios are not available....

f003ed6... by Dirk Zimoch on 2020-12-18

Do not fail (but skip) epicsMutexPriorityInversionTest when SCHED_FIFO is unavailable

03b909b... by Dirk Zimoch on 2020-12-18

do not count as test that posix functions are actually working

d5dbd83... by Dirk Zimoch on 2020-12-18

run epicsMutexPriorityInversionTest even when CPU affinity is not available for old glibc because those systems likely have only one CPU core

a09e429... by Dirk Zimoch on 2020-12-18

relax epicsMutexPriorityInversionTest timing a bit

[Dirk Zimoch (dirk.zimoch)]

I think this is done.

review: Approve

[mdavidsaver (mdavidsaver)]

I pushed your latest to github for testing. There are a couple of failures with RTEMS and OSX.

https://github.com/mdavidsaver/epics-base/actions/runs/430893895

I still think it is necessary for the new globalAttrInit() to probe for runtime support of PTHREAD_PRIO_INHERIT. I've included a sketch of this as a diff comment.

Unless I'm misreading, it looks like only the recursive implementation would have PTHREAD_PRIO_INHERIT set. So the mutex used internally in osdThread and osdSpin would continue to be non-PI.

review: Needs Fixing

[Heinz Junkes (junkes)]

Yes, even RTEMS5 without the new libbsd-network-stack has not defined _XOPEN_SOURCE and asks for the old implementation. I can set _XOPEN_SOURCE by hand and it works so far with it. But what I noticed in this context and I think that the mutexes may be used incorrectly in some modules and therefore it comes to problems such as: "mutex not owned by the task".

I saw the whole thing in the context of MotorRecords (OMS):

https://github.com/epics-modules/motor/issues/172

I see a general problem that probably in some places mutexes are created in the iocInit task and then used afterwards in other tasks although the iocInit task has been terminated.

As I understand it, mutexes should generally be created in the tasks in which they are also used? Or with shared memory the creating task must remain however existing?

[Heinz Junkes (junkes)]

> Yes, even RTEMS5 without the new libbsd-network-stack has not defined
> _XOPEN_SOURCE and asks for the old implementation. I can set _XOPEN_SOURCE by
> hand and it works so far with it. But what I noticed in this context and I
> think that the mutexes may be used incorrectly in some modules and therefore
> it comes to problems such as: "mutex not owned by the task".
>
> I saw the whole thing in the context of MotorRecords (OMS):
>
> https://github.com/epics-modules/motor/issues/172
>
> I see a general problem that probably in some places mutexes are created in
> the iocInit task and then used afterwards in other tasks although the iocInit
> task has been terminated.
>
> As I understand it, mutexes should generally be created in the tasks in which
> they are also used? Or with shared memory the creating task must remain
> however existing?

If you build RTEMS with the POSIX_FLAGS like in Linux (-D_GNU_SOURCE -D_DEFAULT_SOURCE) then with the help of includes/sys/features.h _XOPENS_SOURCE is defined to 700.

[Dirk Zimoch (dirk.zimoch)]

Now I have some time again to return to this one...

Michael, if I understand correctly, you would like to see a run-time test that the attribute PTHREAD_PRIO_INHERIT can actually be used to init a mutex? Have you really seen that pthread_mutexattr_setprotocol(&globalAttrRecursive, PTHREAD_PRIO_INHERIT) succeeds but the resulting globalAttrRecursive cannot be used to init a mutex? If yes, I find that behavior strange and unexpected.

Heinz, do you think I should add #define D_GNU_SOURCE somewhere in the code or should that be done in the configuration?

Concerning "mutex not owned by task": As a mutex is used to synchronize different tasks, there is no specific task "owning" it permanently. The task that has last locked it owns it, independent of the creator task. The only situation where I would expect such a message is if task A has locked it but task B tries to unlock it. Or no task has locked it but now it is unlocked. That has nothing to do which who created it. The problem in the MotorRecord originated in the wrong check of TryLock and thus a confusion which task owned the mutex at run time.

30a2421... by Dirk Zimoch on 2021-01-26

check at run-time if PTHREAD_PRIO_INHERIT actually works

b0f1271... by Dirk Zimoch on 2021-01-26

define _GNU_SOURCE for all platforms to make sure all needed posix functions are available

855519c... by Dirk Zimoch on 2021-01-26

No need to include features.h directly. And may fail on some OS.

[Heinz Junkes (junkes)]

I added the following to RTEMS:
in configure/os/CONFIG.Common.RTEMS
POSIX_CPPFLAGS = -D_GNU_SOURCE -D_DEFAULT_SOURCE

Just like it is already done for Linux:
in configure/os/CONFIG.COMMON.linuxCommon you can find
# Define _GNU_SOURCE and _DEFAULT_SOURCE for maximum portability
POSIX_CPPFLAGS = -D_GNU_SOURCE -D_DEFAULT_SOURCE

> Now I have some time again to return to this one...
>
> Michael, if I understand correctly, you would like to see a run-time test that
> the attribute PTHREAD_PRIO_INHERIT can actually be used to init a mutex? Have
> you really seen that pthread_mutexattr_setprotocol(&globalAttrRecursive,
> PTHREAD_PRIO_INHERIT) succeeds but the resulting globalAttrRecursive cannot be
> used to init a mutex? If yes, I find that behavior strange and unexpected.
>
> Heinz, do you think I should add #define D_GNU_SOURCE somewhere in the code or
> should that be done in the configuration?
>
> Concerning "mutex not owned by task": As a mutex is used to synchronize
> different tasks, there is no specific task "owning" it permanently. The task
> that has last locked it owns it, independent of the creator task. The only
> situation where I would expect such a message is if task A has locked it but
> task B tries to unlock it. Or no task has locked it but now it is unlocked.
> That has nothing to do which who created it. The problem in the MotorRecord
> originated in the wrong check of TryLock and thus a confusion which task owned
> the mutex at run time.

[mdavidsaver (mdavidsaver)]

> ... Have you really seen ...

Yes Dirk, really really. cf. https://github.com/rr-debugger/rr/issues/2750

If we open the source it's easy enough to see what glibc (at least) is doing.

https://github.com/bminor/glibc/blob/76ea70c613cee23a1846b9605e6433c1fa8baea7/nptl/pthread_mutexattr_setprotocol.c#L25

So pthread_mutexattr_setprotocol() will only fail if passed an unknown flag.

https://github.com/bminor/glibc/blob/76ea70c613cee23a1846b9605e6433c1fa8baea7/nptl/pthread_mutex_init.c#L76-L78

And here is pthread_mutex_init() testing for OS support, and returning ENOTSUP if this isn't found.

If anything, I'm left with some gratitude that glibc does this test in pthread_mutex_init() instead of letting it be discovered during pthread_mutex_lock().

Also, seeing how pthread_mutexattr_t is only a bit mask makes me skeptical of economizing.

[Dirk Zimoch (dirk.zimoch)]

I have now added the run-time test, removed the unnecessary direct include of features.h and added _GNU_SOURCE to the source code. I am now rebasing everything onto the current 7.0.

b410ce8... by Dirk Zimoch on 2021-01-26

don't destroy mutex when init failed

[mdavidsaver (mdavidsaver)]

I think this leaves only the question of osdPosixMutexInit() and PTHREAD_MUTEX_DEFAULT. As it stands now, the mutexs created through epicsMutex can have PI enabled, but those created elsewhere (eg. osdSpin) will not.

wrt. _GNU_SOURCE I don't think this should be defined in osdMutex.c, or any source file. This is easily removed later though. Also, it is my turn to be surprised to see #ifdef _GNU_SOURCE appearing in RTEMS (which uses newlib, not glibc).

[Dirk Zimoch (dirk.zimoch)]

> As it stands now, the mutexs created through epicsMutex can have PI enabled, but those created elsewhere (eg. osdSpin) will not.

Oops, you are right. I got confused by Andrew's earlier remark:

> Hmm, according to the online spec (and both manpages I just checked) passing a NULL attribute into pthread_mutex_init() is legal and is equivalent to a default-initialized mutex, so we only actually need to create the recursive one.

Of course, a non-recursive mutex with PI enabled is not a default mutex and we need a mutex attribute for those.

53fcc93... by Dirk Zimoch on 2021-02-01

fix bug in commit 5ee60a: We need a mutex attribute for non-recursive, PI enabled mutexes

[Dirk Zimoch (dirk.zimoch)]

Concerning _GNU_SOURCE, this is what gnu.org has to say about this:

"You should define these macros by using ‘#define’ preprocessor directives at the top of your source code files. These directives must come before any #include of a system header file. It is best to make them the very first thing in the file, preceded only by comments. You could also use the ‘-D’ option to GCC, but it’s better if you make the source files indicate their own meaning in a self-contained way."

So better put it in the source files, not in the Makefile.

[mdavidsaver (mdavidsaver)]

Ok, I think this covers all of the issues which I can see.

I've pushed your latest to my github to trigger CI builds, and a couple are failing with your favorite implicit declaration error.

https://github.com/mdavidsaver/epics-base/actions/runs/528387798

> ../epicsMutexPriorityInversionTest.c:87: error: implicit declaration of function 'pthread_getschedparam'

I think that explicitly including pthread.h from epicsMutexPriorityInversionTest.c should resolve this.

[Dirk Zimoch (dirk.zimoch)]

The failing MacOS compilation have this error:
  ../epicsMutexPriorityInversionTest.c:19:54: error: function-like macro '__GLIBC_PREREQ' is not defined
  #if defined(__GLIBC__) && defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2,4)

It first tests that __GLIBC_PREREQ is defined and then caomplains that it is not defined?
Is it defined as a non-function-like macro on that platform?

The RTEMS compilations fail in the same line but with another error:
  ../epicsMutexPriorityInversionTest.c:19:68: error: missing binary operator before token "("

Again, it seems to have problems with __GLIBC_PREREQ.

The C Processor manual says that && and || "obey the usual short-circuiting rules of standard C." Thus __GLIBC_PREREQ(2,4) should never be evalueted if __GLIBC_PREREQ is not defined.

Anyway, I will try to fix this...

a97e5af... by Dirk Zimoch on 2021-02-02

Do not rely on __GLIBC_PREREQ macro

[Dirk Zimoch (dirk.zimoch)]

The CI environments should make sure to run the test with sufficient privileges to use RT priorities.

[mdavidsaver (mdavidsaver)]

... __GLIBC_PREREQ

Ah, sorry I didn't catch this. There is a difference in how CPP treats undefined "variable" vs. "function" macros. eg. an undefined __GNUC__ will silently expand to zero. An undefined __GLIBC_PREREQ() is an error.

What you have is fine, and no further change is needed. So I think this change is now ready to merge.

I will note that this could also be expressed using the VERSION_INT() macro from epicsVersion.h as

> #ifdef __GLIBC__
> #define GLIBC_VERSION VERSION_INT(__GLIBC__, __GLIBC_MINOR__, 0, 0)
> #endif

and then

> #if GLIBC_VERSION >= VERSION_INT(2,4,0,0)

This works since an undefined GLIBC_VERSION is treated as a zero.

> ... The CI environments should ...

This is currently being controlled by '.github/workflows/ci-scripts-build.yml'. As a first pass, I should be sufficient to apply 'sudo' to the following line.

> run: python .ci/cue.py test

review: Approve

[Dirk Zimoch (dirk.zimoch)]

Ohhh, I still have a typo:
if __GLIBC__ * 100 * __GLIBC_MINOR__ >= 204

should be + __GLIBC_MINOR__ of course.

b324abe... by Dirk Zimoch on 2021-02-02

fix typo in GLIBC version check

[mdavidsaver (mdavidsaver)]

> ... I think this change is now ready to merge.

... once the CI builds pass.

https://github.com/mdavidsaver/epics-base/runs/1816607672?check_suite_focus=true

RTEMS 4.10

  ../epicsMutexPriorityInversionTest.c:87: error: implicit declaration of function 'pthread_self'

Missing an include of <pthread.h>

OSX

  ../epicsMutexPriorityInversionTest.c:226:10: error: implicit declaration of function 'sched_setscheduler' is invalid in C99 [-Werror,-Wimplicit-function-

Not sure about this one as <sched.h> is being included. Can someone with a Mac see if/where this is defined?

review: Needs Fixing

[mdavidsaver (mdavidsaver)]

Ok, maybe time to take a(nother) step back and ask if epicsMutexPriorityInversionTest should be run by default.

> testOk1( hiPriStalledTimeUs < 1000 );

This will almost certainly fail randomly with the CI builders since they don't have a PREEMPT_RT kernel, and are anyway timeshare VMs.

I don't see a need for Base to test whether PTHREAD_PRIO_INHERIT functions correctly. This seems like testing malloc(). The critical question which a user will have is whether PTHREAD_PRIO_INHERIT is being set. This seems more like asking whether OS priorities are being set at all. This is complicated since, unlike priority or mlock(), I don't know of a straightforward way to verify this externally. (I would resort to strace, which is complicated to interpret)

The question of OS priorities is answered by epicsThreadShowAll(). So maybe it would be sufficient for epicsMutexShowAll() to print a line showing if PTHREAD_PRIO_INHERIT is being set?

[Andrew Johnson (anj)]

OSX:

woz$ man sched_setscheduler
No manual entry for sched_setscheduler

This routine doesn't exist on macOS (Google confirms).

5ff7658... by Dirk Zimoch on 2021-02-03

add missing pthread.h

[Dirk Zimoch (dirk.zimoch)]

Does that mean that OSX cannot use RT priorities?

786de29... by Dirk Zimoch on 2021-02-03

run epicsMutexPriorityInversionTest only if we have priority scheduling enabled

[Dirk Zimoch (dirk.zimoch)]

I have modified the Makefilt to run (even compile) the test only if priority scheduling is available. Otherwise the test would be rather pointless.

[mdavidsaver (mdavidsaver)]

The OSX build still fails.

https://github.com/mdavidsaver/epics-base/actions/runs/534660122

fyi. Base uses the pthread_setschedparam() instead of sched_setscheduler(). Combining with pthread_self() should accomplish what you intend.

[Dirk Zimoch (dirk.zimoch)]

Base also uses sched_setscheduler() in modules/libcom/src/osi/os/posix/osdProcess.c line 104. How does that work with OSX?

4855d9e... by Dirk Zimoch on 2021-02-08

use pthread_setschedparam() instead of sched_setscheduler() because of OSX

[Andrew Johnson (anj)]

#define _POSIX_THREAD_PRIORITY_SCHEDULING (-1) /* [TPS] */

[mdavidsaver (mdavidsaver)]

https://github.com/mdavidsaver/epics-base/runs/1856569036?check_suite_focus=true

Progress. Everything builds, but the new epicsMutexPriorityInversionTest fails of OSX. Of course, it only "succeeds" on the Linux builds due to lack of permission.

I think this change is basically done and ready to go except for the new test. Is this test so essential that it needs to be an obstacle to merging?

Maybe it would be sufficient for epicsMutexShowAll() to print a line showing if PTHREAD_PRIO_INHERIT is being set?

[mdavidsaver (mdavidsaver)]

@Dirk, unless you object, I'm going to merge this work excluding epicsMutexPriorityInversionTest. This test can be added later when it has been perfected.

[Dirk Zimoch (dirk.zimoch)]

Feel free to remove the test.

[mdavidsaver (mdavidsaver)]

I squashed the series and applied it as 5a8b6e4111b869e8913b7bfaff4c56b546fb8879, I also changed epicsMutexShowAll() to print "PI is enabled" if mutexes are being created with PTHREAD_PRIO_INHERIT.

[Dirk Zimoch (dirk.zimoch)]

Unfortunately you introduced a bug in epicsMutexOsdShowAll(). Not all implementations have pthread_mutexattr_getprotocol and PTHREAD_PRIO_INHERIT. Earlier in that file you find `#if defined _POSIX_THREAD_PRIO_INHERIT`. The same is needed here.

[mdavidsaver (mdavidsaver)]

Corrected with 3c46542630823a272001aaab4e6fc265c7e03046.

Which targets don't define _POSIX_THREAD_PRIO_INHERIT?

review: Approve

[Andrew Johnson (anj)]

There's also an issue that Brendan found and fixed in his rtems5 branch, the assumption in calls to osdPosixMutexInit() in osdEvent.c, osdSpin.c and osdThread.c that PTHREAD_MUTEX_DEFAULT always has the value 0. That fix will come in when we merge his branch though, so it probably isn't necessary to cherry-pick it into 7.0.

[mdavidsaver (mdavidsaver)]

To expand, I take it you're referring to the change which replaces

> osdPosixMutexInit(&spin->lock, 0);

calls with

> osdPosixMutexInit(&spin->lock, PTHREAD_MUTEX_DEFAULT);

Where RTEMS 5 has

> .../include/sys/_pthreadtypes.h:#define PTHREAD_MUTEX_DEFAULT 3

This is a simple enough change, and the exact timetable for RTEMS5 support is uncertain enough to me, that I think it should be applied on 7.0 soon. Do you (ANJ) want to do this? If not, then I will.

[Andrew Johnson (anj)]

Yup. I was assuming that no other target would need those changes before RTEMS-5 since our CI jobs haven't failed, although I guess freebsd might (could we add that to our GitHub-Actions jobs?). I'm a bit snowed under right now so if you think this needs to go in before the rest of RTEMS-5 go ahead and cherry-pick Brendan's change.

[mdavidsaver (mdavidsaver)]

Looking at the other open libc impls. uclibc has almost the same enum{} as glibc, so PTHREAD_MUTEX_DEFAULT is zero. musl explicitly defines PTHREAD_MUTEX_DEFAULT as zero. It looks like newlib is the exception.

> ... since our CI jobs haven't failed ...

This would depend on what 0 actually means.

> ... could we add that to our GitHub-Actions jobs?

Maybe? This will probably wait for someone with sufficient interest in epics on *BSD.

> ... go ahead and cherry-pick Brendan's change.

Ok. Simple as it may be, I'll wait for a CI run before pushing to the 7.0 branch.

https://github.com/mdavidsaver/epics-base/actions/runs/615581362

[Dirk Zimoch (dirk.zimoch)]

Michael, the target without _POSIX_THREAD_PRIO_INHERIT is some old embedded Linux based on Montavista Linux 4.0 that came with a serial interface box from Moxa.

[Dirk Zimoch (dirk.zimoch)]

I am now testing the PTHREAD_MUTEX_DEFAULT fix ....

[Dirk Zimoch (dirk.zimoch)]

Compiles for all my targets.

[mdavidsaver (mdavidsaver)]

Not directly related to Dirk's change, but I want to note it somewhere. The fallback implementation in posix/epicsAtomicOSD.cpp is using a statically initialized, not RT, mutex. Obviously only relevant to older posix targets without atomic builtins.

diff --git a/documentation/RELEASE_NOTES.md b/documentation/RELEASE_NOTES.md
index 1679526..1ef81a4 100644
--- a/documentation/RELEASE_NOTES.md
+++ b/documentation/RELEASE_NOTES.md
@@ -17,6 +17,7 @@ should also be read to understand what has changed since earlier releases.
 
 <!-- Insert new items immediately below here ... -->
 
+<<<<<<< documentation/RELEASE_NOTES.md
 
 
 ### Build System: New `VALID_BUILDS` type "Command"
@@ -36,6 +37,14 @@ cross-compiled targets though since `CONFIG.Common.UnixCommon` sets it.
 The other `VALID_BUILDS` types are "Host" for target architectures that can
 compile and run their own programs (`PROD_HOST` etc.), and "Ioc" for targets
 that can run IOCs (`PROD_IOC` etc.).
+=======
+### Priority inversion safe posix mutexes
+
+On Posix systems, epicsMutex now support priority inheritance if available.
+The IOC needs to run with SCHED_FIFO engaged.
+Support for Posix implementations before POSIX.1-2001 (_XOPEN_SOURCE < 500,
+glibc version < 2.3.3) has been dropped.
+>>>>>>> documentation/RELEASE_NOTES.md
 
 ### Support for JSON5
 
diff --git a/modules/libcom/src/osi/Makefile b/modules/libcom/src/osi/Makefile
index f0108e6..ce9dcc6 100644
--- a/modules/libcom/src/osi/Makefile
+++ b/modules/libcom/src/osi/Makefile
@@ -112,6 +112,7 @@ Com_SRCS += osdStdio.c
 THREAD_CPPFLAGS_NO += -DDONT_USE_POSIX_THREAD_PRIORITY_SCHEDULING
 osdThread_CPPFLAGS += $(THREAD_CPPFLAGS_$(USE_POSIX_THREAD_PRIORITY_SCHEDULING))
 osdSpin_CPPFLAGS += $(THREAD_CPPFLAGS_$(USE_POSIX_THREAD_PRIORITY_SCHEDULING))
+osdMutex_CPPFLAGS += $(THREAD_CPPFLAGS_$(USE_POSIX_THREAD_PRIORITY_SCHEDULING))
 
 Com_SRCS += osdThread.c
 Com_SRCS += osdThreadExtra.c
diff --git a/modules/libcom/src/osi/os/posix/osdEvent.c b/modules/libcom/src/osi/os/posix/osdEvent.c
index 251f35d..db61c24 100644
--- a/modules/libcom/src/osi/os/posix/osdEvent.c
+++ b/modules/libcom/src/osi/os/posix/osdEvent.c
@@ -23,6 +23,7 @@
 #include "epicsEvent.h"
 #include "epicsTime.h"
 #include "errlog.h"
+#include "osdPosixMutexPriv.h"
 
 struct epicsEventOSD {
     pthread_mutex_t mutex;
@@ -50,11 +51,11 @@ LIBCOM_API epicsEventId epicsEventCreate(epicsEventInitialState init)
     epicsEventId pevent = malloc(sizeof(*pevent));
 
     if (pevent) {
-        int status = pthread_mutex_init(&pevent->mutex, 0);
+        int status = osdPosixMutexInit(&pevent->mutex, 0);
 
         pevent->isFull = (init == epicsEventFull);
         if (status) {
-            printStatus(status, "pthread_mutex_init", "epicsEventCreate");
+            printStatus(status, "osdPosixMutexInit", "epicsEventCreate");
         } else {
             status = pthread_cond_init(&pevent->cond, 0);
             if (!status)
diff --git a/modules/libcom/src/osi/os/posix/osdMutex.c b/modules/libcom/src/osi/os/posix/osdMutex.c
index 707ae63..6279c57 100644
--- a/modules/libcom/src/osi/os/posix/osdMutex.c
+++ b/modules/libcom/src/osi/os/posix/osdMutex.c
@@ -11,6 +11,10 @@
 
 /* Author:  Marty Kraimer Date:    13AUG1999 */
 
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+
 #include <stddef.h>
 #include <stdlib.h>
 #include <stdio.h>
@@ -21,6 +25,7 @@
 #include <pthread.h>
 
 #include "epicsMutex.h"
+#include "osdPosixMutexPriv.h"
 #include "cantProceed.h"
 #include "epicsTime.h"
 #include "errlog.h"
@@ -38,6 +43,69 @@
         cantProceed((method)); \
     }
 
+#if defined(DONT_USE_POSIX_THREAD_PRIORITY_SCHEDULING)
+#undef _POSIX_THREAD_PRIO_INHERIT
+#endif
+
+/* Global var - pthread_once does not support passing args but it is more efficient
+ * than epicsThreadOnce which always acquires a mutex.
+ */
+static pthread_mutexattr_t globalAttrDefault;
+static pthread_mutexattr_t globalAttrRecursive;
+static pthread_once_t      globalAttrInitOnce = PTHREAD_ONCE_INIT;
+
+static void globalAttrInit()
+{
+    int status;
+
+    status = pthread_mutexattr_init(&globalAttrDefault);
+    checkStatusQuit(status,"pthread_mutexattr_init(&globalAttrDefault)","globalAttrInit");
+    status = pthread_mutexattr_init(&globalAttrRecursive);
+    checkStatusQuit(status,"pthread_mutexattr_init(&globalAttrRecursive)","globalAttrInit");
+    status = pthread_mutexattr_settype(&globalAttrRecursive, PTHREAD_MUTEX_RECURSIVE);
+    checkStatusQuit(status, "pthread_mutexattr_settype(&globalAttrRecursive, PTHREAD_MUTEX_RECURSIVE)", "globalAttrInit");
+#if defined _POSIX_THREAD_PRIO_INHERIT
+    status = pthread_mutexattr_setprotocol(&globalAttrDefault, PTHREAD_PRIO_INHERIT);
+    if (errVerbose) checkStatus(status, "pthread_mutexattr_setprotocol(&globalAttrDefault, PTHREAD_PRIO_INHERIT)");
+    status = pthread_mutexattr_setprotocol(&globalAttrRecursive, PTHREAD_PRIO_INHERIT);
+    if (errVerbose) checkStatus(status, "pthread_mutexattr_setprotocol(&globalAttrRecursive, PTHREAD_PRIO_INHERIT)");
+    if (status == 0) {
+        /* Can we really use PTHREAD_PRIO_INHERIT? */
+        pthread_mutex_t temp;
+        status = pthread_mutex_init(&temp, &globalAttrRecursive);
+        if (errVerbose) checkStatus(status, "pthread_mutex_init(&temp, &globalAttrRecursive)");
+        if (status != 0) {
+            /* No, PTHREAD_PRIO_INHERIT does not work, fall back to PTHREAD_PRIO_NONE */;
+            pthread_mutexattr_setprotocol(&globalAttrDefault, PTHREAD_PRIO_NONE);
+            pthread_mutexattr_setprotocol(&globalAttrRecursive, PTHREAD_PRIO_NONE);
+        } else {
+            pthread_mutex_destroy(&temp);
+        }
+    }
+#endif
+}
+
+int osdPosixMutexInit (pthread_mutex_t *m, int mutextype)
+{
+    pthread_mutexattr_t *atts;
+    int status;
+
+    status = pthread_once( &globalAttrInitOnce, globalAttrInit );
+    checkStatusQuit(status,"pthread_once","epicsPosixMutexAttrGet");
+
+    switch (mutextype) {
+        case PTHREAD_MUTEX_DEFAULT:
+            atts = &globalAttrDefault;
+            break;
+        case PTHREAD_MUTEX_RECURSIVE:
+            atts = &globalAttrRecursive;
+            break;
+        default:
+            return ENOTSUP;
+    }
+    return pthread_mutex_init(m, atts);
+}
+
 static int mutexLock(pthread_mutex_t *id)
 {
     int status;
@@ -48,23 +116,8 @@ static int mutexLock(pthread_mutex_t *id)
     return status;
 }
 
-/* Until these can be demonstrated to work leave them undefined*/
-/* On solaris 8 _POSIX_THREAD_PRIO_INHERIT fails*/
-#undef _POSIX_THREAD_PROCESS_SHARED
-#undef _POSIX_THREAD_PRIO_INHERIT
-
-/* Two completely different implementations are provided below
- * If support is available for PTHREAD_MUTEX_RECURSIVE then
- *      only pthread_mutex is used.
- * If support is not available for PTHREAD_MUTEX_RECURSIVE then
- *      a much more complicated solution is required
- */
-
-

-#if defined(_XOPEN_SOURCE) && (_XOPEN_SOURCE)>=500
 typedef struct epicsMutexOSD {
     pthread_mutex_t     lock;
-    pthread_mutexattr_t mutexAttr;
 } epicsMutexOSD;
 
 epicsMutexOSD * epicsMutexOsdCreate(void) {
@@ -73,32 +126,12 @@ epicsMutexOSD * epicsMutexOsdCreate(void) {
 
     pmutex = calloc(1, sizeof(*pmutex));
     if(!pmutex)
-        goto fail;
-
-    status = pthread_mutexattr_init(&pmutex->mutexAttr);
-    if (status)
-        goto fail;
-
-#if defined(_POSIX_THREAD_PRIO_INHERIT) && _POSIX_THREAD_PRIO_INHERIT > 0
-    status = pthread_mutexattr_setprotocol(&pmutex->mutexAttr,
-        PTHREAD_PRIO_INHERIT);
-    if (errVerbose) checkStatus(status, "pthread_mutexattr_setprotocal");
-#endif /*_POSIX_THREAD_PRIO_INHERIT*/
-
-    status = pthread_mutexattr_settype(&pmutex->mutexAttr,
-        PTHREAD_MUTEX_RECURSIVE);
-    checkStatus(status, "pthread_mutexattr_settype");
-    if (status)
-        goto fail;
+        return NULL;
 
-    status = pthread_mutex_init(&pmutex->lock, &pmutex->mutexAttr);
-    if (status)
-        goto dattr;
-    return pmutex;
+    status = osdPosixMutexInit(&pmutex->lock, PTHREAD_MUTEX_RECURSIVE);
+    if (!status)
+        return pmutex;
 
-dattr:
-    pthread_mutexattr_destroy(&pmutex->mutexAttr);
-fail:
     free(pmutex);
     return NULL;
 }
@@ -109,11 +142,9 @@ void epicsMutexOsdDestroy(struct epicsMutexOSD * pmutex)
 
     status = pthread_mutex_destroy(&pmutex->lock);
     checkStatus(status, "pthread_mutex_destroy");
-    status = pthread_mutexattr_destroy(&pmutex->mutexAttr);
-    checkStatus(status, "pthread_mutexattr_destroy");
     free(pmutex);
 }
-

+
 void epicsMutexOsdUnlock(struct epicsMutexOSD * pmutex)
 {
     int status;
@@ -157,178 +188,3 @@ void epicsMutexOsdShow(struct epicsMutexOSD * pmutex, unsigned int level)
      */
     printf("    pthread_mutex_t* uaddr=%p\n", &pmutex->lock);
 }
-

-#else /*defined(_XOPEN_SOURCE) && (_XOPEN_SOURCE)>=500 */
-
-typedef struct epicsMutexOSD {
-    pthread_mutex_t     lock;
-    pthread_mutexattr_t mutexAttr;
-    pthread_cond_t      waitToBeOwner;
-#if defined(_POSIX_THREAD_PROCESS_SHARED) && _POSIX_THREAD_PROCESS_SHARED > 0
-    pthread_condattr_t  condAttr;
-#endif /*_POSIX_THREAD_PROCESS_SHARED*/
-    int                 count;
-    int                 owned;  /* TRUE | FALSE */
-    pthread_t           ownerTid;
-} epicsMutexOSD;
-
-epicsMutexOSD * epicsMutexOsdCreate(void) {
-    epicsMutexOSD *pmutex;
-    int           status;
-
-    pmutex = calloc(1, sizeof(*pmutex));
-    if(!pmutex)
-        return NULL;
-
-    status = pthread_mutexattr_init(&pmutex->mutexAttr);
-    if(status)
-        goto fail;
-
-#if defined(_POSIX_THREAD_PRIO_INHERIT) && _POSIX_THREAD_PRIO_INHERIT > 0
-    status = pthread_mutexattr_setprotocol(
-        &pmutex->mutexAttr,PTHREAD_PRIO_INHERIT);
-    if (errVerbose) checkStatus(status, "pthread_mutexattr_setprotocal");
-#endif /*_POSIX_THREAD_PRIO_INHERIT*/
-
-    status = pthread_mutex_init(&pmutex->lock, &pmutex->mutexAttr);
-    if(status)
-        goto dattr;
-
-#if defined(_POSIX_THREAD_PROCESS_SHARED) && _POSIX_THREAD_PROCESS_SHARED > 0
-    status = pthread_condattr_init(&pmutex->condAttr);
-    checkStatus(status, "pthread_condattr_init");
-    status = pthread_condattr_setpshared(&pmutex->condAttr,
-        PTHREAD_PROCESS_PRIVATE);
-    checkStatus(status, "pthread_condattr_setpshared");
-    status = pthread_cond_init(&pmutex->waitToBeOwner, &pmutex->condAttr);
-#else
-    status = pthread_cond_init(&pmutex->waitToBeOwner, 0);
-#endif /*_POSIX_THREAD_PROCESS_SHARED*/
-    if(status)
-        goto dmutex;
-
-    return pmutex;
-
-dmutex:
-    pthread_mutex_destroy(&pmutex->lock);
-dattr:
-    pthread_mutexattr_destroy(&pmutex->mutexAttr);
-fail:
-    free(pmutex);
-    return NULL;
-}
-
-void epicsMutexOsdDestroy(struct epicsMutexOSD * pmutex)
-{
-    int   status;
-
-    status = pthread_cond_destroy(&pmutex->waitToBeOwner);
-    checkStatus(status, "pthread_cond_destroy");
-#if defined(_POSIX_THREAD_PROCESS_SHARED) && _POSIX_THREAD_PROCESS_SHARED > 0
-    status = pthread_condattr_destroy(&pmutex->condAttr);
-#endif /*_POSIX_THREAD_PROCESS_SHARED*/
-    status = pthread_mutex_destroy(&pmutex->lock);
-    checkStatus(status, "pthread_mutex_destroy");
-    status = pthread_mutexattr_destroy(&pmutex->mutexAttr);
-    checkStatus(status, "pthread_mutexattr_destroy");
-    free(pmutex);
-}
-

-void epicsMutexOsdUnlock(struct epicsMutexOSD * pmutex)
-{
-    int status;
-
-    status = mutexLock(&pmutex->lock);
-    checkStatus(status, "pthread_mutex_lock epicsMutexOsdUnlock");
-    if(status)
-        return;
-
-    if ((pmutex->count <= 0) || (pmutex->ownerTid != pthread_self())) {
-        pthread_mutex_unlock(&pmutex->lock);
-        checkStatus(status, "pthread_mutex_unlock epicsMutexOsdUnlock");
-        errlogPrintf("epicsMutexOsdUnlock but caller is not owner\n");
-        cantProceed("epicsMutexOsdUnlock but caller is not owner");
-        return;
-    }
-
-    pmutex->count--;
-    if (pmutex->count == 0) {
-        pmutex->owned = 0;
-        pmutex->ownerTid = 0;
-        status = pthread_cond_signal(&pmutex->waitToBeOwner);
-        checkStatusQuit(status, "pthread_cond_signal epicsMutexOsdUnlock", "epicsMutexOsdUnlock");
-    }
-
-    status = pthread_mutex_unlock(&pmutex->lock);
-    checkStatus(status, "pthread_mutex_unlock epicsMutexOsdUnlock");
-}
-
-static int condWait(pthread_cond_t *condId, pthread_mutex_t *mutexId)
-{
-    int status;
-
-    while ((status = pthread_cond_wait(condId, mutexId)) == EINTR) {
-        errlogPrintf("pthread_cond_wait returned EINTR. Violates SUSv3\n");
-    }
-    return status;
-}
-
-epicsMutexLockStatus epicsMutexOsdLock(struct epicsMutexOSD * pmutex)
-{
-    pthread_t tid = pthread_self();
-    int status;
-
-    if (!pmutex || !tid) return epicsMutexLockError;
-    status = mutexLock(&pmutex->lock);
-    if (status == EINVAL) return epicsMutexLockError;
-    checkStatus(status, "pthread_mutex_lock epicsMutexOsdLock");
-    if(status)
-        return epicsMutexLockError;
-
-    while (pmutex->owned && !pthread_equal(pmutex->ownerTid, tid))
-        condWait(&pmutex->waitToBeOwner, &pmutex->lock);
-    pmutex->ownerTid = tid;
-    pmutex->owned = 1;
-    pmutex->count++;
-
-    status = pthread_mutex_unlock(&pmutex->lock);
-    checkStatus(status, "pthread_mutex_unlock epicsMutexOsdLock");
-    if(status)
-        return epicsMutexLockError;
-    return epicsMutexLockOK;
-}
-
-epicsMutexLockStatus epicsMutexOsdTryLock(struct epicsMutexOSD * pmutex)
-{
-    pthread_t tid = pthread_self();
-    epicsMutexLockStatus result;
-    int status;
-
-    status = mutexLock(&pmutex->lock);
-    if (status == EINVAL) return epicsMutexLockError;
-    checkStatus(status, "pthread_mutex_lock epicsMutexOsdTryLock");
-    if(status)
-        return epicsMutexLockError;
-
-    if (!pmutex->owned || pthread_equal(pmutex->ownerTid, tid)) {
-        pmutex->ownerTid = tid;
-        pmutex->owned = 1;
-        pmutex->count++;
-        result = epicsMutexLockOK;
-    } else {
-        result = epicsMutexLockTimeout;
-    }
-
-    status = pthread_mutex_unlock(&pmutex->lock);
-    checkStatus(status, "pthread_mutex_unlock epicsMutexOsdTryLock");
-    if(status)
-        return epicsMutexLockError;
-    return result;
-}
-
-void epicsMutexOsdShow(struct epicsMutexOSD *pmutex,unsigned int level)
-{
-    printf("ownerTid %p count %d owned %d\n",
-        (void *)pmutex->ownerTid, pmutex->count, pmutex->owned);
-}
-#endif /*defined(_XOPEN_SOURCE) && (_XOPEN_SOURCE)>=500 */
diff --git a/modules/libcom/src/osi/os/posix/osdPosixMutexPriv.h b/modules/libcom/src/osi/os/posix/osdPosixMutexPriv.h
new file mode 100644
index 0000000..2b6846c
--- /dev/null
+++ b/modules/libcom/src/osi/os/posix/osdPosixMutexPriv.h
@@ -0,0 +1,28 @@
+/*************************************************************************\
+* Copyright (c) 2002 The University of Chicago, as Operator of Argonne
+*     National Laboratory.
+* Copyright (c) 2002 The Regents of the University of California, as
+*     Operator of Los Alamos National Laboratory.
+* SPDX-License-Identifier: EPICS
+* EPICS Base is distributed subject to a Software License Agreement found
+* in file LICENSE that is included with this distribution.
+\*************************************************************************/
+
+#ifndef osdPosixMutexPrivh
+#define osdPosixMutexPrivh
+
+#include <pthread.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Returns ENOTSUP if requested mutextype is not supported */
+/* At the moment, only 0 (default non recursive mutex) and PTHREAD_MUTEX_RECURSIVE are supported */
+int osdPosixMutexInit(pthread_mutex_t *,int mutextype);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* osdPosixMutexPrivh */
diff --git a/modules/libcom/src/osi/os/posix/osdSpin.c b/modules/libcom/src/osi/os/posix/osdSpin.c
index 1f8434b..f039e7c 100644
--- a/modules/libcom/src/osi/os/posix/osdSpin.c
+++ b/modules/libcom/src/osi/os/posix/osdSpin.c
@@ -109,6 +109,8 @@ void epicsSpinUnlock(epicsSpinId spin) {
  *  POSIX MUTEX IMPLEMENTATION
  */
 
+#include <osdPosixMutexPriv.h>
+
 typedef struct epicsSpin {
     pthread_mutex_t lock;
 } epicsSpin;
@@ -121,8 +123,8 @@ epicsSpinId epicsSpinCreate(void) {
     if (!spin)
         goto fail;
 
-    status = pthread_mutex_init(&spin->lock, NULL);
-    checkStatus(status, "pthread_mutex_init");
+    status = osdPosixMutexInit(&spin->lock, 0);
+    checkStatus(status, "osdPosixMutexInit");
     if (status)
         goto fail;
 
diff --git a/modules/libcom/src/osi/os/posix/osdThread.c b/modules/libcom/src/osi/os/posix/osdThread.c
index fe375fd..4525588 100644
--- a/modules/libcom/src/osi/os/posix/osdThread.c
+++ b/modules/libcom/src/osi/os/posix/osdThread.c
@@ -32,6 +32,7 @@
 #include "ellLib.h"
 #include "epicsEvent.h"
 #include "epicsMutex.h"
+#include "osdPosixMutexPriv.h"
 #include "epicsString.h"
 #include "epicsThread.h"
 #include "cantProceed.h"
@@ -327,10 +328,10 @@ static void once(void)
     int status;
 
     pthread_key_create(&getpthreadInfo,0);
-    status = pthread_mutex_init(&onceLock,0);
-    checkStatusOnceQuit(status,"pthread_mutex_init","epicsThreadInit");
-    status = pthread_mutex_init(&listLock,0);
-    checkStatusOnceQuit(status,"pthread_mutex_init","epicsThreadInit");
+    status = osdPosixMutexInit(&onceLock,0);
+    checkStatusOnceQuit(status,"osdPosixMutexInit","epicsThreadInit");
+    status = osdPosixMutexInit(&listLock,0);
+    checkStatusOnceQuit(status,"osdPosixMutexInit","epicsThreadInit");
     pcommonAttr = calloc(1,sizeof(commonAttr));
     if(!pcommonAttr) checkStatusOnceQuit(errno,"calloc","epicsThreadInit");
     status = pthread_attr_init(&pcommonAttr->attr);
diff --git a/modules/libcom/test/Makefile b/modules/libcom/test/Makefile
index 3ad869a..c49ac94 100755
--- a/modules/libcom/test/Makefile
+++ b/modules/libcom/test/Makefile
@@ -333,6 +333,15 @@ TESTS += nonEpicsThreadPriorityTest
 endif
 endif
 
+ifeq ($(POSIX),YES)
+ifeq ($(USE_POSIX_THREAD_PRIORITY_SCHEDULING),YES)
+TESTPROD_HOST += epicsMutexPriorityInversionTest
+epicsMutexPriorityInversionTest_SRCS += epicsMutexPriorityInversionTest.c
+epicsMutexPriorityInversionTest_SYS_LIBS_Linux += pthread rt
+TESTSCRIPTS_HOST+=epicsMutexPriorityInversionTest.t
+endif
+endif
+
 include $(TOP)/configure/RULES
 
 rtemsTestData.c : $(TESTFILES) $(TOOLS)/epicsMakeMemFs.pl
diff --git a/modules/libcom/test/epicsMutexPriorityInversionTest.c b/modules/libcom/test/epicsMutexPriorityInversionTest.c
new file mode 100644
index 0000000..8d7c6f2
--- /dev/null
+++ b/modules/libcom/test/epicsMutexPriorityInversionTest.c
@@ -0,0 +1,258 @@
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+#include <sched.h>
+#include <epicsThread.h>
+#include <epicsEvent.h>
+#include <epicsMutex.h>
+#include <epicsUnitTest.h>
+#include <errlog.h>
+#include <testMain.h>
+#include <time.h>
+#include <envDefs.h>
+#include <stdio.h>
+#include <pthread.h>
+
+#define  THE_CPU   0
+
+#define  SPIN_SECS 5000000
+
+#if __GLIBC__ * 100 + __GLIBC_MINOR__ >= 204
+#define HAVE_CPU_AFFINITY
+#else
+#undef  HAVE_CPU_AFFINITY
+#endif
+
+typedef struct ThreadArg_ {
+    epicsEventId           sync;
+    epicsEventId           done;
+    epicsMutexId           mtx;
+    volatile unsigned long end;
+    unsigned long          lim;
+    int                   pri[3];
+} ThreadArg;
+
+static unsigned long getClockUs()
+{
+    struct timespec now;
+    if ( clock_gettime( CLOCK_MONOTONIC, &now ) ) {
+        testAbort("clock_gettime(CLOCK_MONOTONIC) failed");
+    }
+    return ((unsigned long)now.tv_sec)*1000000 + ((unsigned long)now.tv_nsec)/1000;
+}
+
+/*
+ * Set affinity of executing thread to 'cpu'
+ */
+static void setThreadAffinity(int cpu)
+{
+#ifdef HAVE_CPU_AFFINITY
+    cpu_set_t cset;
+
+    CPU_ZERO( &cset );
+    CPU_SET( cpu, &cset );
+    pthread_setaffinity_np( pthread_self(), sizeof(cset), &cset );
+#endif
+}
+
+/*
+ * Ensure only 'cpu' is set in executing thread's affinity mask
+ */
+static void checkAffinity(int cpu)
+{
+#ifdef HAVE_CPU_AFFINITY
+    cpu_set_t cset;
+
+    if ( pthread_getaffinity_np( pthread_self(), sizeof(cset), &cset) ) {
+        testDiag("pthread_getaffinity_np FAILED");
+        return;
+    }
+    if ( 1 != CPU_COUNT( &cset ) || ! CPU_ISSET( cpu, &cset ) ) {
+        testDiag( "Thread has unexpected CPU affinity mask" );
+    }
+#endif
+}
+
+
+/*
+ * Make sure executing thread uses SCHED_FIFO and
+ * store its priority a->pri[idx]
+ *
+ * RETURNS: 0 if SCHED_FIFO engaged, nonzero otherwise.
+ */
+static int checkThreadPri(ThreadArg *a, unsigned idx)
+{
+    int                pol;
+    struct sched_param p;
+
+    if ( pthread_getschedparam( pthread_self(), &pol, &p ) ) {
+        testFail("pthread_getschedparam FAILED");
+        return 1;
+    }
+    if ( a && idx < sizeof(a->pri)/sizeof(a->pri[0]) ) {
+        a->pri[idx] = p.sched_priority;
+    }
+    if ( SCHED_FIFO != pol ) {
+        testDiag( "Thread uses unexpected scheduler" );
+    }
+    return (SCHED_FIFO != pol);
+}
+
+
+/*
+ * Low-priority thread.
+ *
+ * Lock mutex and signal to the medium-priority thread
+ * that it may proceed.
+ *
+ * Since all three (high-, medium- and low-priority threads)
+ * execute on the same CPU (via affinity) the medium-
+ * priority thread will then take over the CPU and prevent
+ * (unless priority-inheritance is enabled, that is)
+ * the low-priority thread from continueing on to unlock
+ * the mutex.
+ */
+static void loPriThread(void *parm)
+{
+    ThreadArg *a = (ThreadArg*)parm;
+
+    checkAffinity( THE_CPU );
+
+    checkThreadPri( a, 0 );
+
+    epicsMutexMustLock( a->mtx );
+
+    epicsEventSignal( a->sync );
+
+    /* medium-priority thread takes over CPU and spins
+     * while the high-priority thread waits for the mutex.
+     * With priority-inheritance enabled this thread's
+     * priority will be increased so that the medium-
+     * priority thread can be preempted and we proceed
+     * to release the mutex.
+     */
+
+    epicsMutexUnlock( a->mtx );
+}
+
+static void hiPriThread(void *parm)
+{
+    ThreadArg  *a = (ThreadArg*)parm;
+
+    /* Try to get the mutex */
+    epicsMutexMustLock( a->mtx );
+    /* Record the time when we obtained the mutex */
+    a->end = getClockUs();
+    epicsMutexUnlock( a->mtx );
+    /* Tell the main thread that the test done */
+    epicsEventSignal( a->done );
+}
+
+static void miPriThread(void *parm)
+{
+    ThreadArg  *a = (ThreadArg*)parm;
+
+    /* Basic checks:
+     *  - affinity must be set to use single CPU for all threads
+     *  - SCHED_FIFO must be in effect
+     */
+    checkAffinity( THE_CPU );
+    checkThreadPri( a, 1 );
+
+    /* Create the low-priority thread. */
+    epicsThreadMustCreate("testLo",
+                          a->pri[0],
+                          epicsThreadGetStackSize( epicsThreadStackMedium ),
+                          loPriThread,
+                          a);
+
+    /* Wait until low-priority thread has taken the mutex */
+    epicsEventMustWait( a->sync );
+
+    /* Compute the end-time for our spinning loop */
+    a->lim = getClockUs() + SPIN_SECS;
+    a->end = 0;
+
+    /* Create the high-priority thread. The hiPri thread will
+     * block for the mutex and
+     *  if priority-inheritance is available:
+     *    increase the low-priority thread's priority temporarily
+     *    so it can proceed to release the mutex and hand it to
+     *    the high-priority thread.
+     *  if priority-inheritance is not available:
+     *    the high-priority thread will have to wait until we are
+     *    done spinning and the low-priority thread is scheduled
+     *    again.
+     */
+    epicsThreadMustCreate("testHi",
+                          a->pri[2],
+                          epicsThreadGetStackSize( epicsThreadStackMedium ),
+                          hiPriThread,
+                          a);
+
+    /* Spin for some time; the goal is hogging the CPU and thus preventing
+     * the low-priority thread from being scheduled.
+     * If priority-inheritance is enabled then the low-priority thread's
+     * priority is temporarily increased so that we can be preempted. This
+     * then causes the high-priority thread to record the 'end' time which
+     * tells us that we can terminate early...
+     */
+    while ( 0 == a->end && getClockUs() < a->lim )
+        /* spin */;
+
+    /* w/o priority-inheritance the low-priority thread may proceed at
+     * this point and release the mutex to the high-priority thread.
+     */
+}
+
+
+#define NUM_TESTS 2
+
+MAIN(epicsMutexPriorityInversionTest)
+{
+    ThreadArg a;
+    long      hiPriStalledTimeUs;
+    struct    sched_param p_pri;
+
+    a.mtx  = epicsMutexMustCreate();
+    a.sync = epicsEventMustCreate( epicsEventEmpty );
+    a.done = epicsEventMustCreate( epicsEventEmpty );
+    a.pri[0] = epicsThreadPriorityLow;
+    a.pri[1] = epicsThreadPriorityMedium;
+    a.pri[2] = epicsThreadPriorityHigh;
+
+    testPlan(NUM_TESTS);
+
+    p_pri.sched_priority = sched_get_priority_min( SCHED_FIFO );
+    if ( pthread_setschedparam(pthread_self(), SCHED_FIFO, &p_pri) ) {
+        testDiag("SCHED_FIFO not engaged - cannot run this test");
+        testSkip( NUM_TESTS, "SCHED_FIFO not engaged" );
+        return testDone();
+    } else {
+        testDiag("SCHED_FIFO is in use");
+    }
+
+    setThreadAffinity( THE_CPU );
+    /* created threads should inherit CPU affinity mask */
+
+    epicsThreadMustCreate("testMi",
+                          a.pri[1],
+                          epicsThreadGetStackSize( epicsThreadStackMedium ),
+                          miPriThread,
+                          &a);
+
+    epicsEventMustWait( a.done );
+
+    testOk1( (a.pri[0] < a.pri[1]) && (a.pri[1] < a.pri[2]) );
+
+    hiPriStalledTimeUs = a.end - (a.lim - SPIN_SECS);
+
+    testDiag("High-priority thread stalled for %li us\n", hiPriStalledTimeUs);
+    testOk1( hiPriStalledTimeUs < 1000 );
+
+    epicsEventDestroy( a.done );
+    epicsEventDestroy( a.sync );
+    epicsMutexDestroy( a.mtx  );
+
+    return testDone();
+}