Details

Reviewers

markj
kib
brooks
val_packett.cool
dchagin

Summary

The timerfd_create(), timerfd_gettime() and timerfd_settime()
functions are not traditional syscalls because they're exposed
through the specialfd interface and ioctl().

These calls have no entry in the syscall table, so their stubs are not
auto generated. Instead, use a libc wrapper to expose the timerfd
user API.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

jfree created this revision.Feb 9 2023, 5:48 AM

Herald added a subscriber: imp. · View Herald TranscriptFeb 9 2023, 5:48 AM

jfree requested review of this revision.Feb 9 2023, 5:48 AM

jfree added a parent revision: D38459: timerfd: Add native support for Linux's timerfd.Feb 12 2023, 4:22 AM

Update syscall wrappers for timerfd_gettime() and timerfd_settime()
to use an ioctl() entry point.

Use TFD_SETFLAGS ioctl() to set tfd flags during timerfd_settime()

kib added inline comments.Feb 13 2023, 10:26 PM

lib/libc/gen/Symbol.map
441	Order alphabetically
lib/libc/gen/timerfd.c
30	Not needed anymore, remove these two lines
35	No need for the blank line
37	Same
39	I am not sure about the purpose of the empty namespace/un-namespace block. What functions did you tried to call using internal bindings?
42
51
56

Remove unnecessary headers
Fix style(9) formatting

Is the race between TFD_GETTIME and TFD_SETTIME in timerfd_settime acceptable? I wonder if TFD_SETFLAGS should be _IOWR and always return the previous value.

lib/libc/gen/timerfd.c
2	While common in the tree the -FreeBSD variant is deprecated.

In D38460#879425, @brooks wrote:

Is the race between TFD_GETTIME and TFD_SETTIME in timerfd_settime acceptable? I wonder if TFD_SETFLAGS should be _IOWR and always return the previous value.

Will execution continue in userspace before the kernel returns at timerfd.c:56? I'm not completely clear on how synchronicity works in the kernel for situations like this. I assumed the userspace thread would wait for the kernel to return before continuing execution. I don't see a reason for TFD_SETFLAGS to read the current flags considering Linux doesn't support similar functionality. Are you suggesting this to check for TFD_GETTIME completion?

Change file license from BSD-2-Clause-FreeBSD to BSD-2-Clause

In D38460#879734, @jfree wrote:

In D38460#879425, @brooks wrote:

Is the race between TFD_GETTIME and TFD_SETTIME in timerfd_settime acceptable? I wonder if TFD_SETFLAGS should be _IOWR and always return the previous value.

Will execution continue in userspace before the kernel returns at timerfd.c:56? I'm not completely clear on how synchronicity works in the kernel for situations like this. I assumed the userspace thread would wait for the kernel to return before continuing execution. I don't see a reason for TFD_SETFLAGS to read the current flags considering Linux doesn't support similar functionality. Are you suggesting this to check for TFD_GETTIME completion?

Sorry, a cut-and-paste error made that make no sense. My initial worry was about TFD_GETTIME and TFD_SETTIME, but actually it's both. Consider the following scenario:

thread0       thread1
TFD_GETTIME
TFD_SETFLAGS
              TFD_GETTIME
              TFD_SETFLAGS
              TFD_SETTIME
TFD_SETTIME

I'm not sure how important it is to prevent this problem in practice, but multiple system calls seem to make problems more likely.

Sorry, a cut-and-paste error made that make no sense. My initial worry was about TFD_GETTIME and TFD_SETTIME, but actually it's both. Consider the following scenario:
thread0       thread1
TFD_GETTIME
TFD_SETFLAGS
              TFD_GETTIME
              TFD_SETFLAGS
              TFD_SETTIME
TFD_SETTIME
I'm not sure how important it is to prevent this problem in practice, but multiple system calls seem to make problems more likely.

Yeah, that is definitely an issue. I originally made the ioctl()s take in struct timerfd_gettime_args and struct timerfd_settime_args. This solution is rather clumsy because you need to have two single-use structs declared in sys/timerfd.h. I assume this also makes compat32 quite a bit more difficult. I'll give it some thought and try to come up with a better solution. We may just have to compromise on having a struct timerfd_settime_args. Definitely tell if you have any suggestions.

The TFD_SETTIME ioctl() now returns old_time in an effort to avoid race conditions between threads using the same timerfd fd. This reduces the number of kernel traps from 3 to 2 in timerfd_settime().

kib added inline comments.Feb 18 2023, 6:45 AM

lib/libc/gen/timerfd.c
61	Why do you need the ex variable? Can you pass new_value pointer directly to the ioctl? Is it to handle aliasing issues between new_value and old_value? IMO it is better to extend TFD_SETFLAGS argument to take two itimerspec structures to take a new value from one, and to return the old value in another. Then why not pack TFD_SETFLAGS there as well?

jfree added inline comments.Feb 18 2023, 5:36 PM

lib/libc/gen/timerfd.c
61	Why do you need the ex variable? Can you pass new_value pointer directly to the ioctl? Is it to handle aliasing issues between new_value and old_value? The Linux API expects `new_value` to be `const`. It wouldn't be correct to pass `new_value` into the ioctl() where it gets overwritten with the old_value. IMO it is better to extend TFD_SETFLAGS argument to take two itimerspec structures to take a new value from one, and to return the old value in another. Then why not pack TFD_SETFLAGS there as well? There is no reason to do this because there is no equivalent Linux functionality. `TFD_SETFLAGS` exists simply because I didn't want to pack args for `TFD_SETTIME`. Did you mean `TFD_SETTIME`? If so, I don't see what benefit that'll provide over this implementation. If anything, I like this solution better because it doesn't require a `struct timerfd_settime_args` declaration.

jfree added inline comments.Feb 19 2023, 5:12 AM

lib/libc/gen/timerfd.c
61	If so, I don't see what benefit that'll provide over this implementation. If anything, I like this solution better because it doesn't require a `struct timerfd_settime_args` declaration. After some thought, the packed args approach for `TFD_SETTIME` would eliminate all race conditions. Is that worth the extra `timerfd_settime_args` declaration?