mheily / libkqueue

kqueue(2) compatibility library
Other
236 stars 77 forks source link

EVFILT_TIMER for Linux/POSIX Does Not Correctly Honor Event Data Changes across Calls to kqueue #116

Closed gerickson closed 2 years ago

gerickson commented 2 years ago

In contrast to the kernel-native implementation on BSD-based systems such as Darwin, libkqueue deviates from that/those implementations and the specification, insofar as the man page is a proxy for one in that not all changes to extant entries in the queue are not honored across calls to kqueue:

The kqueue man page cites:

All changes contained in the changelist are applied before any pending events are read from the queue.

This is particularly evident with EVFILT_TIMER in which the only opportunity to influence and set the timeout value is in evfilt_timer_knote_create in src/linux/timer.c.

This is demonstrated with the attached sample program, compiled and executed with:

% clang -Wall -Werror -fsanitize=address kqueue-timer-example.c -o kqueue-timer-example && ./kqueue-timer-example

on Darwin and with:

% clang-11 -Wall -Werror -fsanitize=address -I/usr/local/include/kqueue kqueue-timer-example.c -L/usr/local/lib -lkqueue -o kqueue-timer-example && ./kqueue-timer-example

on Linux. On Darwin, the changes to the EVFILT_TIMER data value are observed across kqueue calls and the wait correctly expires four (4) times at the second of the two data values pushed for the event, as expected. However, on Linux, using libkqueue-2.5.1, the wait only expires two (2) of the four (4) expected and does so at the first of the two data values pushed for those events.

The as-implemented behavior makes using kqueue via libkqueue for CFRunLoop timer support in opencflite problematic when multiple timers are at play when a later-added timer has a smaller or larger expiry than the prior timer, requiring the timer event in the kqueue to be adjusted downward or upward.

mheily commented 2 years ago

Thanks for reporting this, and I do think that libkqueue should change it's behavior to match the native behavior of kqueue. Here's my quick take on what's happening. Feel free to correct any misunderstanding I may have.

In your test program, the timer is created in one call to kevent(), and then in a second call to kevent() the data value is changed. Here's the relevant lines of code:

   intptr_t data[2][4] = {
        { 250, 333, 750, 500 },
        { 300, 400, 800, 600 }
    };

        // Enable and arm the timer in the queue.

        EV_SET(&input[0], (uintptr_t)&input[0], EVFILT_TIMER, (EV_ADD | EV_ENABLE |EV_ONESHOT), 0, data[0][i], NULL);

        status = kevent(queue, &input[0], 1, NULL, 0, NULL);
        if (status == -1) {
            status = EXIT_FAILURE;
            goto done;
        }

        EV_SET(&input[0], (uintptr_t)&input[0], EVFILT_TIMER, (EV_ADD | EV_ENABLE |EV_ONESHOT), 0, data[1][i], NULL);

        status = kevent(queue, &input[0], 1, NULL, 0, NULL);
        if (status == -1) {
            status = EXIT_FAILURE;
            goto done;
        }

The bug in libkqueue is that we haven't implemented the evfilt_timer_knote_modify function, so the underlying Linux timerfd is never updated. Here's the stub function as currently implemented:

int
evfilt_timer_knote_modify(struct filter *filt, struct knote *kn,
        const struct kevent *kev)
{
    (void)filt;
    (void)kn;
    (void)kev;
    return (0); /* STUB */
}

@gerickson I would like to add your test program to the libkqueue testsuite. Would you mind submitting an updated version that includes a copyright and license statement at the top in the following format:

/*
 * Copyright (c) 2021 $YOUR_NAME_HERE <$YOUR_EMAIL_HERE>
 *
 * Permission to use, copy, modify, and distribute this software for any
 * purpose with or without fee is hereby granted, provided that the above
 * copyright notice and this permission notice appear in all copies.
 *
 * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
 * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
 * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
 * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
 * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
 * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 */
arr2036 commented 2 years ago

@mheily may still be worth pulling this in as a test case. Most of the test program is duplicating functions we already have in the test suite so I'd rather not pull it in in its entirety.