Pls sanity check my semtimedop(2) implementation

[Michael B Allen]

On Fri, Jul 18, 2008 at 11:58 AM, Jilles Tjoelker <jilles at stack.nl> wrote:
> On Sat, Jul 12, 2008 at 07:11:26PM -0400, Michael B Allen wrote:
>> Below is a semtimedop(2) implementation that I'm using for FreeBSD. I
>> was hoping someone could look it over and tell me if they think the
>> implementation is sound.
>
>> [snip semtimedop implementation that uses SIGALRM and relies on EINTR]
>
>> The code seems to work ok but when stressing the FreeBSD build of my app
>> I have managed to provoke errors related to concurrency (usually when a
>> SIGALRM goes off). The Linux build works flawlessesly so I'm wondering
>> about this one critical function that is different.
>
> In your implementation, the SIGALRM signal may happen before you even
> call semop(2). If so, most likely the semop(2) will hang arbitrarily
> long.

Indeed. And I reconnoiter that condition is likely to happen if called
with a sufficiently small timeout value.

> Another dirty fix is to try non-blocking semop(2) several times with
> sleeps in between.

Actually that seems to work pretty well.

If I force the nanosleep codepath to be used always, the application
actually works pretty well under load. I wonder if the overhead of
managing the signal and timer is worth the trouble.

For posterity, below is the current implementation of semtimedop(2)
for FreeBSD. Further ideas are welcome.

void
_timeval_diff(struct timeval *tv1, struct timeval *tv2, struct timeval *tvd)
{
    tvd->tv_sec = tv1->tv_sec - tv2->tv_sec;
    tvd->tv_usec = tv1->tv_usec - tv2->tv_usec;
    if (tvd->tv_usec < 0) {
        tvd->tv_usec = 1000000 - tvd->tv_usec;
        tvd->tv_sec--;
    }
}
void
signal_ignore(int s, siginfo_t *si, void *ctx)
{
}
int
_semtimedop(int semid, struct sembuf *array, size_t nops, struct
timespec *_timeout)
{
    struct timeval timeout, before, after;
    struct itimerval value, ovalue;
    struct sigaction sa, osa;
    int ret;

    if (_timeout) {
        timeout.tv_sec = _timeout->tv_sec;
        timeout.tv_usec = _timeout->tv_nsec / 1000;

        if (gettimeofday(&before, NULL) == -1) {
            errno = EFAULT;
            return -1;
        }

        if (timeout.tv_sec == 0 && timeout.tv_usec < 5000) {
            struct timeval tsleep, tend;
            struct sembuf wait;

            wait = *array;
            wait.sem_flg |= IPC_NOWAIT;

            tsleep.tv_sec = 0;
            tsleep.tv_usec = 1;

            timeradd(&before, &timeout, &tend);

            for ( ;; ) {
                struct timeval tnow, tleft;
                struct timespec ts;

                ret = semop(semid, &wait, nops);
                if (ret == 0) {
                    return 0;
                } else if (errno != EAGAIN) {
                    break;
                }

                if (gettimeofday(&tnow, NULL) == -1) {
                    errno = EFAULT;
                    break;
                }

                if (timercmp(&tnow, &tend, >=)) {
                    errno = EAGAIN;
                    break;
                }

                timersub(&tend, &tnow, &tleft);

                if (tsleep.tv_usec > tleft.tv_usec)
                    tsleep.tv_usec = tleft.tv_usec;

                ts.tv_sec = 0;
                ts.tv_nsec = tsleep.tv_usec * 1000;
                nanosleep(&ts, NULL);

                tsleep.tv_usec *= 10;
            }

            return -1;
        }

        memset(&value, 0, sizeof value);
        value.it_value = timeout;

        memset(&sa, 0, sizeof sa);
        sa.sa_sigaction = signal_ignore;
        sa.sa_flags = SA_SIGINFO;
        sigemptyset(&sa.sa_mask);
        sigaction(SIGALRM, &sa, &osa);

        if (setitimer(ITIMER_REAL, &value, &ovalue) == -1) {
            sigaction(SIGALRM, &osa, NULL);
            return -1;
        }
    }

    ret = semop(semid, array, nops);

    if (_timeout) {
        ret = setitimer(ITIMER_REAL, &ovalue, NULL);
        if (ret < 0)
            errno = EFAULT;

        sigaction(SIGALRM, &osa, NULL);
    }

    if (ret == -1) {
        if (_timeout) {
            struct timeval elapsed;

            if (gettimeofday(&after, NULL) == -1) {
            } else {

                _timeval_diff(&after, &before, &elapsed);

                if (timercmp(&elapsed, &timeout, >=))
                    errno = EAGAIN;
            }
        }

        return -1;
    }

    return 0;
}