posix: mlockall() causing problems for non-IOCs

Bug #1539791 reported by mdavidsaver
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
EPICS Base
Fix Released
Low
mdavidsaver

Bug Description

A change introduced in 3.15 series such that, when built with USE_POSIX_THREAD_PRIORITY_SCHEDULING=YES (the default), "mlockall(MCL_CURRENT | MCL_FUTURE)" will be called for *any* process with sufficient permission to use RT scheduling. This will either result in a warning if unsuccessful, which is a nuisance when running eg. caget as root.

http://www.aps.anl.gov/epics/tech-talk/2015/msg01811.php

or can cause problems for client programs using dynamic loading.

http://www.aps.anl.gov/epics/tech-talk/2016/msg00202.php

Related branches

Revision history for this message
mdavidsaver (mdavidsaver) wrote :
Revision history for this message
mdavidsaver (mdavidsaver) wrote :

The attached patch move the mlockall() call to the end of iocRun(), and makes it conditional on an iocsh variable dbRTPrepare (name provisional subject to Andrew).

Changed in epics-base:
milestone: 3.14.branch → 3.15.branch
Revision history for this message
Andrew Johnson (anj) wrote :

As written the patch has some minor issues in addition to the names, the main one being that if you do iocPause and then re-start the IOC with iocRun() it will call mlockall() again. There is no need to delay the call until iocRun() since we do provide the flag MCL_FUTURE, even though there are probably some minor performance reasons for calling it as late as possible. I'm going to guess that we probably don't want/need to enable real-time operations in iocBuildIsolated(), so I propose moving the call into iocBuild().

As Michael suspected I don't like the names epicsRTPrepare() or dbRTPrepare, they don't give many clues as to what they actually do. The epicsRTPrepare() routine only affects the epicsThread subsystem, so the name needs to start with that. It just calls memlock() for now, so a name like epicsThreadMemlock() might be appropriate, but doesn't leave much room for future additions. The name epicsThreadRealtimeLock() seems appropriate, any better ideas out there?

I have removed the "Successfully locked memory" status message, which is mostly just noise — I wonder whether the routine should return a status value to let the application know if the OS reported any problems.

Should any of the epicsThread*Test programs such as epicsThreadPriorityTest.cpp be calling epicsThreadRealtimeLock() as well?

New patch attached, with the above changes and a Release Notes entry.

Revision history for this message
Andrew Johnson (anj) wrote :

... except that I forgot to move the call in iocInit.c and it's not obvious exactly where it should go...

Revision history for this message
Andrew Johnson (anj) wrote :

Updated patch with iocInit fix.

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

> As Michael suspected I don't like the names ...

Suspicion implies doubt, I had none ;)

> whether the routine should return a status value

This would be a good idea.

> Updated patch with iocInit fix.

Moving the mlockall() call just after initHookAfterIocBuilt seems reasonable.

That said, I've given some thought to Till's comment that RT scheduling should be runtime controlled as well. This decision can't be deferred to iocInit(). So I'm considering having a global variable, initialized from and env. variable, to determine if RT priorities/. This variable would be exported for iocsh as well. Thoughts?

Revision history for this message
Andrew Johnson (anj) wrote : Re: [Bug 1539791] Re: posix: mlockall() causing problems for non-IOCs

> That said, I've given some thought to Till's comment that RT scheduling
> should be runtime controlled as well. This decision can't be deferred
> to iocInit(). So I'm considering having a global variable, initialized
> from an env. variable, to determine if RT priorities/. This variable
> would be exported for iocsh as well. Thoughts?

If we added an environment variable, why would we need a global variable
as well? An iocsh script can set env-vars (although by then it's too
late to change the RT prioritization), and I'd much rather have just one
configuration variable than two. Wouldn't we implement this by just
adding an env-var check to the existing code that sets the priorities
and calls mlockall() from the posix/osdThread.c::once() routine and
continue to rely on the MCL_FUTURE flag as we do now? I don't see a need
for epicsThreadRealtimeLock() at all in that case.

To be honest though I'm not really sure whether Till's idea makes sense
or not. Setting the env-var to enable prioritization only actually does
so if the program is run as root or with the right capabilities and the
soft-limit is set correctly. Users will try to turn prioritization on
without understanding the other requirements, so this could be a source
of FAQs. In that respect an env-var could only ever be a real-time
disable setting, not an enable, but I do hate making people have to
understand double negatives in APIs (disable = false).

The bottom line is that running CA client programs as root shouldn't
cause the issue that SPEC saw. CA should be able to start real-time
priority threads when they are available to it without having to
explicitly enable them in the client program, but the mlockall() call
should only be done when a program explicitly asks for it. A SPEC user
shouldn't have to set special built-time or even run-time flags to
prevent libCom from making an mlockall() call.

For that reason I think your idea of separating the two issues makes the
most sense. I also don't see the need for a variable that disables
real-time prioritization of threads; nobody is actually complaining
about that anyway.

Ralph, do you have any thoughts on this?

- Andrew

--
There are only two hard problems in distributed systems:
  2. Exactly-once delivery
  1. Guaranteed order of messages
  2. Exactly-once delivery
 -- Mathias Verraes

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

> why would we need a global variable as well?

My thinking was to allow an application to override the environment variable.

> An iocsh script can set env-vars

I'm thinking of applications other than IOCs. Someday Gerry may give us SPECRT.

> The bottom line is that running CA client programs as root shouldn't cause the issue that SPEC saw.

I don't believe there is no disagreement on this. I'd be happy to see your patch with only a change to hide epicsThreadRealtimeLock() from public headers (in 3.15 at least) until we figure out if/how non-IOC RT applications should be configured.

> ... I also don't see the need for a variable that disables
> real-time prioritization of threads; nobody is actually complaining
> about that anyway.

Well... this also hasn't been enabled by default until recently.

Andrew Johnson (anj)
Changed in epics-base:
status: Confirmed → Fix Committed
Changed in epics-base:
milestone: 3.15.branch → 3.15.4
Changed in epics-base:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.