Experimental Physics and
Industrial Control System

Andrew Johnson <[email protected]> · Tue, 03 Dec 2013 19:35:23 -0000

Review: Needs Fixing

For future reference: With almost 2**10 lines of diff this branch mixes together (admittedly related) changes to several different subsystems, which makes it hard to review (Jeff's 3.15_libcom_from_cvs_trunk branch has the same problem). The easier you make it to review a branch, the sooner and quicker it will likely be reviewed.

The documentation branch does not cover all the changes made here, it only talks about the modifications to the callback subsystem.

Is it wise to provide public APIs to all these Shutdown functions? I'm not so much concerned that this code doesn't work as whether by making them available we're letting ourselves in for a maintenance nightmare in the future. Do all these subsystems *need* to be shut down and restarted? What are the criteria for choosing which ones should and shouldn't be included? Could we shut everything down by calling epicsExitCallAtExits() instead, and just ensure that each system's registered shutdown function leaves the subsystem in a state that can be properly restarted?

The idea of resetting OnceFlags makes me cringe; they were never designed for that, and I'm not convinced that doing so is completely safe. The state change from EPICS_THREAD_ONCE_INIT through <active> to EPICS_THREAD_ONCE_DONE happens within the protection of a mutex inside the epicsThreadOnce() routines, the mutex providing a memory barrier on SMP. The reversals here are unprotected by either, and don't check that the subsystems have even been initialized either — could some other thread such as a time provider on a different CPU call a subsystem function at the wrong time while the subsystem is shutting down and crash it?

I'm not convinced that restarting an IOC inside the same process is a good idea, given the code we have (I know you're trying to change that, but I think it needs a properly planned solution, which this isn't [yet]). Just making the shutdown APIs available will mean that someone will want/try to use them on a running hard IOC, which is not going to work since we can't communicate the shutdown request to any device support or driver threads (other than by calling their epicsExitCallAtExits() routines, but existing support don't use that to prepare themselves for a restart).

IMHO if you need to test two independent IOCs, they need to be in two separate processes. You could split them into two test programs, or write the test as a Perl script which starts, controls and stops the IOCs as sub-processes; I have a Perl module which can do that on a host OS and could be extended to control an embedded IOC.

This isn't a definite NO, but I will need convincing that this is the right way to go.

- Andrew

-- 
https://code.launchpad.net/~epics-core/epics-base/ioc-shutdown/+merge/190512
Your team EPICS Core Developers is subscribed to branch lp:epics-base.

Experimental Physics and Industrial Control System

Experimental Physics and
Industrial Control System