What's the correct way of implementing lockless asynPortDriver?

[ericonr]

By lockless asynPortDriver, I mean that the port lock isn't acquired before entering any of the write* and read* methods.

We observed this need in two AreaDetector drivers, but it's entirely caused by the implementation of asynPortDriver, so I'm discussing it here instead of in ADCore.

There are probably other situations where this is relevant, but we have observed it in AD drivers where the detector itself doesn't implement multiple acquisition, so a loop is implemented manually in an acquisition task as part of the driver, and the port lock has to be held during most of the implementation of the acquisition. For these drivers, the lock is only released during the stopEvent.wait(acquire_period - time_taken) call. Some examples below:

ADRIXSCam

	this->unlock();
	stopEventStatus = epicsEventWaitWithTimeout(this->stopEventId, delay);
	this->lock();

ADTLBC2

        /* If delay is exactly 0 then this thread never sleeps and the mutex is locked again
         * before any of the other threads can lock it. This means that the background thread
         * which runs the writeInt32 function can never trigger stop_acquire_event and
         * acquisition is never stopped. To mitigate this we set a minimal delay which is
         * imperceptible but still causes the current thread to sleep, allowing the other
         * thread to reliably wake up and trigger the stop_acquire_event */
        if (delay < 0.) delay = 0.001;

        unlock();
        /* .wait() returns true if the event was triggered, false if it timed out */
        bool stop_acquisition = stop_acquire_event.wait(delay);
        lock();

ADTLBC2 already includes a workaround for the issue described here. What happens is that, if acquireTime and acquirePeriod are sufficiently close, the calculated delay would be zero, and there wouldn't be enough time for any other thread to wake up and grab the lock before the acquisition thread can grab it back. This includes the port thread, when someone might have run caput $DETECTOR:Acquire 0 and be trying to stop the acquisition: that event would never be received, unless one gets very lucky with the kernel scheduling. ADTLBC2 uses an empirically determined value that's worked for us, but it's not guaranteed, and means adding a hardcoded (if low) delay to our acquisition loop. A user can also manually work around this by setting a sufficient buffer between acquireTime and acquirePeriod, but that means decreasing their max frame rate, complicates usage, and the correct time might vary depending on multiple factors.

We have reproduced this behavior on Windows and Linux, and it's caused by the synchronization primitives of those platforms not being fair. Using fair locking could be a solution, but the implementations I could find would add allocation and de-allocation in the lock and unlock functions, and most references recommend instead adopting a better design, which I believe could be possible in asynPortDriver!

If it wasn't necessary to grab a lock before calling e.g. asynPortDriver::writeInt32 (and all other functions), the function could be called, simply trigger the stopEvent, and return, without touching anything that needs to be protected by a lock. I could see two ways of implementing this: a new ASYN_LOCKLESS flag or overwriting the writeInt32 function in the int32interface.

ASYN_LOCKLESS flag:

extern "C" {static asynStatus writeInt32(void *drvPvt, asynUser *pasynUser,
                            epicsInt32 value)
{
    if (!drvPvt) {
        return asynDisabled;
    }

    asynPortDriver *pPvt = (asynPortDriver *)drvPvt;
    asynStatus status;

    if (!(flags & ASYN_LOCKLESS)) pPvt->lock();
    status = pPvt->writeInt32(pasynUser, value);
    if (!(flags & ASYN_LOCKLESS)) pPvt->unlock();
    return status;
}}

overwriting writeInt32 function (in the driver implementation):

LocklessDriver(...)
{
    pasynStdInterfaces->int32.pinterface.writeInt32 = writeInt32Lockless;
}

I greatly dislike the latter option, to be honest.

For drivers which opted into lockless interfaces, they could simply add a scoped guard locking the port for all functions where they touch the parameter list, and definitely before calling any functions from superclasses.

asynStatus LocklessDriver::writeInt32(asynUser *pasynUser, epicsInt32 value)
{
    if (pasynUser->reason == ADAcquire && value == 0) {
        stopEvent.trigger();
        return asynSuccess;
    }
    // other things which need to be done without locks

    std::lock_guard<asynPortDriver> lock(*this);
    // rest of implementation, following usual patterns
}