Changeset View
Standalone View
sys/kern/sys_timerfd.c
- This file was added.
/*- | ||||||||||||||||||||||||||
* SPDX-License-Identifier: BSD-2-Clause-FreeBSD | ||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||
* Copyright (c) 2014 Dmitry Chagin <dchagin@FreeBSD.org> | ||||||||||||||||||||||||||
* Copyright (c) 2023 Jake Freeland <jfree@FreeBSD.org> | ||||||||||||||||||||||||||
dchagin: should be removed, Roman was involved in the epoll part
| ||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||
* Redistribution and use in source and binary forms, with or without | ||||||||||||||||||||||||||
* modification, are permitted provided that the following conditions | ||||||||||||||||||||||||||
* are met: | ||||||||||||||||||||||||||
* 1. Redistributions of source code must retain the above copyright | ||||||||||||||||||||||||||
* notice, this list of conditions and the following disclaimer. | ||||||||||||||||||||||||||
* 2. Redistributions in binary form must reproduce the above copyright | ||||||||||||||||||||||||||
* notice, this list of conditions and the following disclaimer in the | ||||||||||||||||||||||||||
* documentation and/or other materials provided with the distribution. | ||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND | ||||||||||||||||||||||||||
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE | ||||||||||||||||||||||||||
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE | ||||||||||||||||||||||||||
* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE | ||||||||||||||||||||||||||
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL | ||||||||||||||||||||||||||
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS | ||||||||||||||||||||||||||
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) | ||||||||||||||||||||||||||
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT | ||||||||||||||||||||||||||
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY | ||||||||||||||||||||||||||
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF | ||||||||||||||||||||||||||
* SUCH DAMAGE. | ||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||
#include <sys/cdefs.h> | ||||||||||||||||||||||||||
__FBSDID("$FreeBSD$"); | ||||||||||||||||||||||||||
Done Inline ActionsDid that came back? I do not remember if I asked to remove the $FreeBSD$ from the new files. kib: Did that came back? I do not remember if I asked to remove the $FreeBSD$ from the new files. | ||||||||||||||||||||||||||
Done Inline Actions
You asked on https://reviews.freebsd.org/D38460. I was unsure if it was still required for kernel files, so I just left it in here. I take it that __FSDID() is obsolete everywhere in the source tree? jfree: > Did that came back? I do not remember if I asked to remove the $FreeBSD$ from the new files. | ||||||||||||||||||||||||||
Not Done Inline ActionsCurrent policy is to not introduce these into new files, but also not to remove existing IDs. The idea was that a single sweep should get rid of them. Might be, some day. kib: Current policy is to not introduce these into new files, but also not to remove existing IDs. | ||||||||||||||||||||||||||
#include <sys/param.h> | ||||||||||||||||||||||||||
#include <sys/systm.h> | ||||||||||||||||||||||||||
#include <sys/callout.h> | ||||||||||||||||||||||||||
#include <sys/fcntl.h> | ||||||||||||||||||||||||||
#include <sys/file.h> | ||||||||||||||||||||||||||
#include <sys/filedesc.h> | ||||||||||||||||||||||||||
#include <sys/filio.h> | ||||||||||||||||||||||||||
#include <sys/kernel.h> | ||||||||||||||||||||||||||
#include <sys/lock.h> | ||||||||||||||||||||||||||
#include <sys/malloc.h> | ||||||||||||||||||||||||||
#include <sys/mutex.h> | ||||||||||||||||||||||||||
#include <sys/poll.h> | ||||||||||||||||||||||||||
#include <sys/proc.h> | ||||||||||||||||||||||||||
#include <sys/selinfo.h> | ||||||||||||||||||||||||||
#include <sys/stat.h> | ||||||||||||||||||||||||||
#include <sys/timerfd.h> | ||||||||||||||||||||||||||
#include <sys/timespec.h> | ||||||||||||||||||||||||||
#include <sys/uio.h> | ||||||||||||||||||||||||||
#include <sys/user.h> | ||||||||||||||||||||||||||
#include <security/audit/audit.h> | ||||||||||||||||||||||||||
Done Inline Actions
brooks: | ||||||||||||||||||||||||||
MALLOC_DEFINE(M_TIMERFD, "timerfd", "timerfd structures"); | ||||||||||||||||||||||||||
static fo_rdwr_t timerfd_read; | ||||||||||||||||||||||||||
Done Inline ActionsBTW, could this define be static? kib: BTW, could this define be static? | ||||||||||||||||||||||||||
static fo_ioctl_t timerfd_ioctl; | ||||||||||||||||||||||||||
static fo_poll_t timerfd_poll; | ||||||||||||||||||||||||||
static fo_kqfilter_t timerfd_kqfilter; | ||||||||||||||||||||||||||
static fo_stat_t timerfd_stat; | ||||||||||||||||||||||||||
static fo_close_t timerfd_close; | ||||||||||||||||||||||||||
static fo_fill_kinfo_t timerfd_fill_kinfo; | ||||||||||||||||||||||||||
static struct fileops timerfdops = { | ||||||||||||||||||||||||||
.fo_read = timerfd_read, | ||||||||||||||||||||||||||
.fo_write = invfo_rdwr, | ||||||||||||||||||||||||||
.fo_truncate = invfo_truncate, | ||||||||||||||||||||||||||
.fo_ioctl = timerfd_ioctl, | ||||||||||||||||||||||||||
.fo_poll = timerfd_poll, | ||||||||||||||||||||||||||
.fo_kqfilter = timerfd_kqfilter, | ||||||||||||||||||||||||||
.fo_stat = timerfd_stat, | ||||||||||||||||||||||||||
.fo_close = timerfd_close, | ||||||||||||||||||||||||||
.fo_chmod = invfo_chmod, | ||||||||||||||||||||||||||
.fo_chown = invfo_chown, | ||||||||||||||||||||||||||
.fo_sendfile = invfo_sendfile, | ||||||||||||||||||||||||||
.fo_fill_kinfo = timerfd_fill_kinfo, | ||||||||||||||||||||||||||
.fo_flags = DFLAG_PASSABLE, | ||||||||||||||||||||||||||
Done Inline ActionsWhen using designated initializers for structs, please put the ending comma at all lines. kib: When using designated initializers for structs, please put the ending comma at all lines. | ||||||||||||||||||||||||||
Done Inline Actions
style(9) has an example of a designated initializer that doesn't have a comma at the end. Looking at other source files, I appear to be finding more instances of no comma at the end, as well. I am going to leave this style choice how it is. jfree: > When using designated initializers for structs, please put the ending comma at all lines. | ||||||||||||||||||||||||||
Done Inline ActionsI agree with kib on this one. We aren't generally going around churning the repo to update things, but this prevents future churn when adding new entries. brooks: I agree with kib on this one. We aren't generally going around churning the repo to update… | ||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||
static void filt_timerfddetach(struct knote *kn); | ||||||||||||||||||||||||||
static int filt_timerfdread(struct knote *kn, long hint); | ||||||||||||||||||||||||||
static struct filterops timerfd_rfiltops = { | ||||||||||||||||||||||||||
.f_isfd = 1, | ||||||||||||||||||||||||||
.f_detach = filt_timerfddetach, | ||||||||||||||||||||||||||
.f_event = filt_timerfdread, | ||||||||||||||||||||||||||
Done Inline ActionsThere too kib: There too | ||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||
struct timerfd { | ||||||||||||||||||||||||||
clockid_t tfd_clockid; | ||||||||||||||||||||||||||
struct itimerspec tfd_time; | ||||||||||||||||||||||||||
Done Inline ActionsAnd where is this unr initialized? kib: And where is this unr initialized? | ||||||||||||||||||||||||||
Done Inline Actions
new_unrhdr64() in timerfd_create_file(). jfree: > And where is this unr initialized?
`new_unrhdr64()` in `timerfd_create_file()`. | ||||||||||||||||||||||||||
Done Inline ActionsThis still does not make a sense. The unrhdr64 var needs to be initialized once, globally. Instead you re-initialize it on each open. kib: This still does not make a sense. The unrhdr64 var needs to be initialized once, globally. | ||||||||||||||||||||||||||
struct callout tfd_callout; | ||||||||||||||||||||||||||
timerfd_t tfd_count; | ||||||||||||||||||||||||||
bool tfd_canceled; | ||||||||||||||||||||||||||
struct selinfo tfd_sel; | ||||||||||||||||||||||||||
struct mtx tfd_lock; | ||||||||||||||||||||||||||
int tfd_flags; | ||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||
static void timerfd_expire(void *); | ||||||||||||||||||||||||||
static void timerfd_curval(struct timerfd *, struct itimerspec *); | ||||||||||||||||||||||||||
int | ||||||||||||||||||||||||||
timerfd_create_file(struct thread *td, int clockid, int flags) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
struct timerfd *tfd; | ||||||||||||||||||||||||||
struct file *fp; | ||||||||||||||||||||||||||
int error, fd, fflags = 0; | ||||||||||||||||||||||||||
AUDIT_ARG_FFLAGS(flags); | ||||||||||||||||||||||||||
AUDIT_ARG_VALUE(clockid); | ||||||||||||||||||||||||||
if (clockid != CLOCK_REALTIME && clockid != CLOCK_MONOTONIC) | ||||||||||||||||||||||||||
return (EINVAL); | ||||||||||||||||||||||||||
if ((flags & ~TFD_CREATE_FLAGS) != 0) | ||||||||||||||||||||||||||
return (EINVAL); | ||||||||||||||||||||||||||
if ((flags & TFD_CLOEXEC) != 0) | ||||||||||||||||||||||||||
Done Inline ActionsWhy? This must be a static data, so that each timerfd gets unique ino. Right now you are generating the same ino for each timerfd. kib: Why? This must be a static data, so that each timerfd gets unique ino. Right now you are… | ||||||||||||||||||||||||||
fflags |= O_CLOEXEC; | ||||||||||||||||||||||||||
error = falloc(td, &fp, &fd, fflags); | ||||||||||||||||||||||||||
if (error != 0) | ||||||||||||||||||||||||||
return (error); | ||||||||||||||||||||||||||
Done Inline ActionsIf you put SYSINIT right after the timerfd_init(), you can avoid forward-declare the function. kib: If you put SYSINIT right after the timerfd_init(), you can avoid forward-declare the function. | ||||||||||||||||||||||||||
tfd = malloc(sizeof(*tfd), M_TIMERFD, M_WAITOK | M_ZERO); | ||||||||||||||||||||||||||
tfd->tfd_clockid = (clockid_t)clockid; | ||||||||||||||||||||||||||
mtx_init(&tfd->tfd_lock, "timerfd", NULL, MTX_DEF); | ||||||||||||||||||||||||||
callout_init_mtx(&tfd->tfd_callout, &tfd->tfd_lock, 0); | ||||||||||||||||||||||||||
knlist_init_mtx(&tfd->tfd_sel.si_note, &tfd->tfd_lock); | ||||||||||||||||||||||||||
Done Inline Actions
kib: | ||||||||||||||||||||||||||
fflags = FREAD; | ||||||||||||||||||||||||||
if ((flags & TFD_NONBLOCK) != 0) | ||||||||||||||||||||||||||
fflags |= FNONBLOCK; | ||||||||||||||||||||||||||
finit(fp, fflags, DTYPE_TIMERFD, tfd, &timerfdops); | ||||||||||||||||||||||||||
Done Inline ActionsThis overrides previously set O_CLOEXEC bit kib: This overrides previously set O_CLOEXEC bit | ||||||||||||||||||||||||||
fdrop(fp, td); | ||||||||||||||||||||||||||
Done Inline ActionsWhy do you directly use O_ namespace there, instead of TFD_ alias? kib: Why do you directly use O_ namespace there, instead of TFD_ alias? | ||||||||||||||||||||||||||
td->td_retval[0] = fd; | ||||||||||||||||||||||||||
return (error); | ||||||||||||||||||||||||||
Done Inline ActionsIf there is not free_unr64(), then alloc_unr64() is just a very slow atomic_add_64(). It seems pipes have the same issue. kib: If there is not free_unr64(), then alloc_unr64() is just a very slow atomic_add_64(). It seems… | ||||||||||||||||||||||||||
Done Inline Actions
Yes, a very slow atomic_fetchadd_64(). I was curious about this as well. I figured it was probably best to keep the unr64 interface in timerfd just in case the underlying code changes in the future. jfree: > If there is not free_unr64(), then alloc_unr64() is just a very slow atomic_add_64(). It… | ||||||||||||||||||||||||||
Done Inline ActionsNo, it is not too slow to execute single fetchadd for timerfd instantiation. I was wrong in that I forgot that unr64 has nothing to do with real unr which tracks allocations. kib: No, it is not too slow to execute single fetchadd for timerfd instantiation. I was wrong in… | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
static int | ||||||||||||||||||||||||||
timerfd_close(struct file *fp, struct thread *td) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
struct timerfd *tfd; | ||||||||||||||||||||||||||
tfd = fp->f_data; | ||||||||||||||||||||||||||
if (fp->f_type != DTYPE_TIMERFD || tfd == NULL) | ||||||||||||||||||||||||||
return (EINVAL); | ||||||||||||||||||||||||||
timespecclear(&tfd->tfd_time.it_value); | ||||||||||||||||||||||||||
timespecclear(&tfd->tfd_time.it_interval); | ||||||||||||||||||||||||||
callout_drain(&tfd->tfd_callout); | ||||||||||||||||||||||||||
seldrain(&tfd->tfd_sel); | ||||||||||||||||||||||||||
knlist_destroy(&tfd->tfd_sel.si_note); | ||||||||||||||||||||||||||
fp->f_ops = &badfileops; | ||||||||||||||||||||||||||
mtx_destroy(&tfd->tfd_lock); | ||||||||||||||||||||||||||
free(tfd, M_TIMERFD); | ||||||||||||||||||||||||||
Done Inline ActionsI am curious why do you check for these conditions, there and in all other places. How is it possible for either of the check to be true? kib: I am curious why do you check for these conditions, there and in all other places. How is it… | ||||||||||||||||||||||||||
return (0); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
static int | ||||||||||||||||||||||||||
timerfd_read(struct file *fp, struct uio *uio, struct ucred *active_cred, | ||||||||||||||||||||||||||
int flags, struct thread *td) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
struct timerfd *tfd; | ||||||||||||||||||||||||||
timerfd_t count; | ||||||||||||||||||||||||||
int error; | ||||||||||||||||||||||||||
tfd = fp->f_data; | ||||||||||||||||||||||||||
if (fp->f_type != DTYPE_TIMERFD || tfd == NULL) | ||||||||||||||||||||||||||
return (EINVAL); | ||||||||||||||||||||||||||
if (uio->uio_resid < sizeof(timerfd_t)) | ||||||||||||||||||||||||||
return (EINVAL); | ||||||||||||||||||||||||||
error = 0; | ||||||||||||||||||||||||||
mtx_lock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
retry: | ||||||||||||||||||||||||||
if (tfd->tfd_canceled) { | ||||||||||||||||||||||||||
tfd->tfd_count = 0; | ||||||||||||||||||||||||||
mtx_unlock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
return (ECANCELED); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
if (tfd->tfd_count == 0) { | ||||||||||||||||||||||||||
if ((fp->f_flag & FNONBLOCK) != 0) { | ||||||||||||||||||||||||||
mtx_unlock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
return (EAGAIN); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
error = mtx_sleep(&tfd->tfd_count, &tfd->tfd_lock, PCATCH, "ltfdrd", 0); | ||||||||||||||||||||||||||
if (error == 0) | ||||||||||||||||||||||||||
goto retry; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
if (error == 0) { | ||||||||||||||||||||||||||
count = tfd->tfd_count; | ||||||||||||||||||||||||||
tfd->tfd_count = 0; | ||||||||||||||||||||||||||
mtx_unlock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
error = uiomove(&count, sizeof(timerfd_t), uio); | ||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||
mtx_unlock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
return (error); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
static int | ||||||||||||||||||||||||||
timerfd_poll(struct file *fp, int events, struct ucred *active_cred, | ||||||||||||||||||||||||||
struct thread *td) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
struct timerfd *tfd; | ||||||||||||||||||||||||||
int revents = 0; | ||||||||||||||||||||||||||
tfd = fp->f_data; | ||||||||||||||||||||||||||
if (fp->f_type != DTYPE_TIMERFD || tfd == NULL) | ||||||||||||||||||||||||||
return (POLLERR); | ||||||||||||||||||||||||||
mtx_lock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
if ((events & (POLLIN | POLLRDNORM)) && tfd->tfd_count > 0) | ||||||||||||||||||||||||||
revents |= events & (POLLIN | POLLRDNORM); | ||||||||||||||||||||||||||
if (revents == 0) | ||||||||||||||||||||||||||
selrecord(td, &tfd->tfd_sel); | ||||||||||||||||||||||||||
mtx_unlock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
Done Inline Actions
kib: | ||||||||||||||||||||||||||
Done Inline Actions
kib: | ||||||||||||||||||||||||||
return (revents); | ||||||||||||||||||||||||||
Done Inline Actions
kib: | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
static int | ||||||||||||||||||||||||||
timerfd_kqfilter(struct file *fp, struct knote *kn) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
struct timerfd *tfd; | ||||||||||||||||||||||||||
tfd = fp->f_data; | ||||||||||||||||||||||||||
if (fp->f_type != DTYPE_TIMERFD || tfd == NULL || | ||||||||||||||||||||||||||
kn->kn_filter != EVFILT_READ) | ||||||||||||||||||||||||||
return (EINVAL); | ||||||||||||||||||||||||||
kn->kn_fop = &timerfd_rfiltops; | ||||||||||||||||||||||||||
kn->kn_hook = tfd; | ||||||||||||||||||||||||||
Done Inline Actions
kib: | ||||||||||||||||||||||||||
knlist_add(&tfd->tfd_sel.si_note, kn, 0); | ||||||||||||||||||||||||||
return (0); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
Done Inline ActionsThe leave just this line alone, removing if() and else kib: The leave just this line alone, removing if() and else | ||||||||||||||||||||||||||
static void | ||||||||||||||||||||||||||
filt_timerfddetach(struct knote *kn) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
struct timerfd *tfd = kn->kn_hook; | ||||||||||||||||||||||||||
mtx_lock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
knlist_remove(&tfd->tfd_sel.si_note, kn, 1); | ||||||||||||||||||||||||||
mtx_unlock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
static int | ||||||||||||||||||||||||||
filt_timerfdread(struct knote *kn, long hint) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
struct timerfd *tfd = kn->kn_hook; | ||||||||||||||||||||||||||
return (tfd->tfd_count > 0); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
static int | ||||||||||||||||||||||||||
timerfd_ioctl(struct file *fp, u_long cmd, void *data, | ||||||||||||||||||||||||||
struct ucred *active_cred, struct thread *td) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
#ifdef COMPAT_FREEBSD32 | ||||||||||||||||||||||||||
Done Inline Actions
brooks: | ||||||||||||||||||||||||||
struct itimerspec its; | ||||||||||||||||||||||||||
struct itimerspec32 *itsp32; | ||||||||||||||||||||||||||
int error; | ||||||||||||||||||||||||||
#endif | ||||||||||||||||||||||||||
if (fp->f_data == NULL || fp->f_type != DTYPE_TIMERFD) | ||||||||||||||||||||||||||
return (EINVAL); | ||||||||||||||||||||||||||
Done Inline Actions
brooks: | ||||||||||||||||||||||||||
switch (cmd) { | ||||||||||||||||||||||||||
case TFD_GETTIME: | ||||||||||||||||||||||||||
return (timerfd_gettime_common(td, fp, (struct itimerspec *)data)); | ||||||||||||||||||||||||||
#ifdef COMPAT_FREEBSD32 | ||||||||||||||||||||||||||
case TFD_GETTIME32: | ||||||||||||||||||||||||||
error = timerfd_gettime_common(td, fp, &its); | ||||||||||||||||||||||||||
if (error != 0) | ||||||||||||||||||||||||||
Done Inline Actions
Also remove the other TFD_GETTIME32 case. The _Static_assert isn't strictly necessary. brooks: Also remove the other TFD_GETTIME32 case.
The _Static_assert isn't strictly necessary. | ||||||||||||||||||||||||||
Done Inline ActionsCould you explain why we CAN depend on the command differing for TFD_SETTIME? I understand that the 32/64 ioctl() variants have args of differing sizes, but wouldn't any machine just use the TFD_SETTIME case given that libc passes that in? I don't see the control flow saying "if 32-bit, then use the 32-bit variants". jfree: Could you explain why we CAN depend on the command differing for `TFD_SETTIME`? I understand… | ||||||||||||||||||||||||||
Done Inline Actions
TFD_SETTIME is defined with the size of struct timerfd_settime_args, but that varies with the compiled ABI. When a 32-bit program is compiled that size is for the version with 32-bit pointers which is equivalent to struct timerfd_settime_args32. brooks: > Could you explain why we CAN depend on the command differing for `TFD_SETTIME`? I understand… | ||||||||||||||||||||||||||
Done Inline Actions
Perhaps you already answered this and I am misinterpreting, but I was trying to figure out why 32-bit binaries choose the TFD_GETTIME32 ioctl case when TFD_GETTIME (non-32) is passed in via libc? jfree: > TFD_SETTIME is defined with the size of `struct timerfd_settime_args`, but that varies with… | ||||||||||||||||||||||||||
Done Inline ActionsThe ioctl command values are obtained by some combination of three attributes: read/write mask, the io command itself, and the length of the parameter' structure. The details are provided in sys/ioccom.h. Now consider two compilation environments for userspace: ILP32 (32bit) and LP64 (64bit). The mask and the io command are same, but generally the parameter' structure sizes are different for 32 vs 64 bit. This means that the final value of the ioctl command passed to kernel should be typically different between 32 vs 64bit user space. Ultimately, this allows the kernel to distinguish the bitness of the ioctl caller without looking at ABI (AKA sysentvec). But if it happens that the parameter's structure layout is same for 32 and 64 bit ABIs, the resulting ioctl commands would have exactly the same numeric values. Then kernel could not distinguish the mode of caller. If parameters do not contain pointers, it is fine to leave only one handler for the command. But if there are pointers, you need to properly reconstruct usermode addresses. This is where ABI checks are needed. kib: The ioctl command values are obtained by some combination of three attributes: read/write mask… | ||||||||||||||||||||||||||
Done Inline ActionsThank you for writing this. I understand now :) jfree: Thank you for writing this. I understand now :) | ||||||||||||||||||||||||||
return (error); | ||||||||||||||||||||||||||
itsp32 = (struct itimerspec32 *)data; | ||||||||||||||||||||||||||
CP(its, *itsp32, it_interval.tv_sec); | ||||||||||||||||||||||||||
CP(its, *itsp32, it_interval.tv_nsec); | ||||||||||||||||||||||||||
Done Inline ActionsHow could it be that TFD_GETTIME32 is not defined? kib: How could it be that TFD_GETTIME32 is not defined? | ||||||||||||||||||||||||||
Done Inline ActionsI think it should only be defined on amd64 because the size of the argument only differs between i386 and everything else. If we define it on non-amd64 we'll have a duplicate case value which is an error. brooks: I think it should only be defined on amd64 because the size of the argument only differs… | ||||||||||||||||||||||||||
CP(its, *itsp32, it_value.tv_sec); | ||||||||||||||||||||||||||
CP(its, *itsp32, it_value.tv_nsec); | ||||||||||||||||||||||||||
Done Inline ActionsI do not see much use for this assert, esp. because I believe this code is never compiled. kib: I do not see much use for this assert, esp. because I believe this code is never compiled. | ||||||||||||||||||||||||||
#endif | ||||||||||||||||||||||||||
case TFD_SETTIME: | ||||||||||||||||||||||||||
Done Inline ActionsBTW, is there a useful interpretation of the fstat(2) fields for timerfd? Does Linux return ENXIO for fstat(timerfd)? kib: BTW, is there a useful interpretation of the fstat(2) fields for timerfd? Does Linux return… | ||||||||||||||||||||||||||
Done Inline Actions
I did some testing and it turns out that timerfd fds have inodes attached to them in Linux. Their fstat() output looks something like this: ID: 14 INODE: 2084 MODE: 384 NLINK: 1 UID: 0 GID: 0 RDEV: 0 SIZE: 0 BLKSIZE: 4096 BLOCKS: 0 I attempted doing a VOP_STAT() in timerfd_stat(), but quickly realized that a vnode is never initialized in this implementation. As a result, fp->f_vnode is NULL. Not sure whether I should bother allocating a vnode to replicate the Linux behavior. jfree: > BTW, is there a useful interpretation of the fstat(2) fields for timerfd? Does Linux return… | ||||||||||||||||||||||||||
Done Inline ActionsThere is no need to allocate a vnode, your file type is not a vnode' anyway. Implement the fo_stat() method for the file. kib: There is no need to allocate a vnode, your file type is not a vnode' anyway. Implement the… | ||||||||||||||||||||||||||
return (timerfd_settime_common(td, fp, (struct itimerspec *)data)); | ||||||||||||||||||||||||||
#ifdef COMPAT_FREEBSD32 | ||||||||||||||||||||||||||
case TFD_SETTIME32: | ||||||||||||||||||||||||||
itsp32 = (struct itimerspec32 *)data; | ||||||||||||||||||||||||||
CP(*itsp32, its, it_interval.tv_sec); | ||||||||||||||||||||||||||
CP(*itsp32, its, it_interval.tv_nsec); | ||||||||||||||||||||||||||
CP(*itsp32, its, it_value.tv_sec); | ||||||||||||||||||||||||||
CP(*itsp32, its, it_value.tv_nsec); | ||||||||||||||||||||||||||
return (timerfd_settime_common(td, fp, &its)); | ||||||||||||||||||||||||||
#endif | ||||||||||||||||||||||||||
case TFD_SETFLAGS: | ||||||||||||||||||||||||||
return (timerfd_setflags(td, fp, *(int *)data)); | ||||||||||||||||||||||||||
case FIONBIO: | ||||||||||||||||||||||||||
case FIOASYNC: | ||||||||||||||||||||||||||
return (0); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
return (ENOTTY); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
Done Inline ActionsAre you sure that this is the right implementation for these FIO* iocontrols? FIOASYNC, if implemented, should change async mode, if FIONBIO reports that async is supported. Also, what about FIONREAD? kib: Are you sure that this is the right implementation for these FIO* iocontrols? FIOASYNC, if… | ||||||||||||||||||||||||||
Done Inline Actions
These are remnants from @dchagin's lkpi tfd implementation. I am not sure why they are here as the tfd Linux man page does not mention explicit support for them. I can't really see an easy way to implement these generic ioctls and don't think they're necessary. What're your thoughts? jfree: > Are you sure that this is the right implementation for these FIO* iocontrols? FIOASYNC, if… | ||||||||||||||||||||||||||
Done Inline ActionsI believe it is not hard to implement them. You just need to understand the semantic. But having the dummy placeholders returning success is wrong any way. kib: I believe it is not hard to implement them. You just need to understand the semantic.
But… | ||||||||||||||||||||||||||
static int | ||||||||||||||||||||||||||
timerfd_stat(struct file *fp, struct stat *sb, struct ucred *active_cred) | ||||||||||||||||||||||||||
Done Inline ActionsWhat if more than one thread do the ioctl in parallel? Isn't it racy? kib: What if more than one thread do the ioctl in parallel? Isn't it racy? | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
bzero(sb, sizeof(*sb)); | ||||||||||||||||||||||||||
sb->st_nlink = fp->f_count; | ||||||||||||||||||||||||||
Done Inline ActionsThe value should be returned only when read would not block, otherwise you should return 0. kib: The value should be returned only when read would not block, otherwise you should return 0. | ||||||||||||||||||||||||||
return (0); | ||||||||||||||||||||||||||
kibUnsubmitted Done Inline ActionsIMO it makes sense to generate unique 'inode id' for timerfd, I suspect some Linux code might depend on the feature. Look at the sys_pipe.c' for pipe_ino, for example of how this is done. kib: IMO it makes sense to generate unique 'inode id' for timerfd, I suspect some Linux code might… | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
static int | ||||||||||||||||||||||||||
timerfd_fill_kinfo(struct file *fp, struct kinfo_file *kif, struct filedesc *fdp) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
struct timerfd *tfd; | ||||||||||||||||||||||||||
Done Inline ActionsThis expression is never true. kib: This expression is never true. | ||||||||||||||||||||||||||
Done Inline Actions
Oops. Lapse in my logic late at night to blame. I meant != 0. jfree: > This expression is never true.
Oops. Lapse in my logic late at night to blame. I meant `!=… | ||||||||||||||||||||||||||
Done Inline ActionsYou should only return non-zero *data when read would not block. It has nothing to do with non-blocked mode. kib: You should only return non-zero *data when read would not block. It has nothing to do with non… | ||||||||||||||||||||||||||
tfd = fp->f_data; | ||||||||||||||||||||||||||
kif->kf_type = KF_TYPE_TIMERFD; | ||||||||||||||||||||||||||
mtx_lock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
kif->kf_un.kf_timerfd.kf_timerfd_clockid = tfd->tfd_clockid; | ||||||||||||||||||||||||||
kif->kf_un.kf_timerfd.kf_timerfd_flags = tfd->tfd_flags; | ||||||||||||||||||||||||||
kif->kf_un.kf_timerfd.kf_timerfd_addr = (uintptr_t)tfd; | ||||||||||||||||||||||||||
mtx_unlock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
return (0); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
static void | ||||||||||||||||||||||||||
timerfd_clocktime(struct timerfd *tfd, struct timespec *ts) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
if (tfd->tfd_clockid == CLOCK_REALTIME) | ||||||||||||||||||||||||||
getnanotime(ts); | ||||||||||||||||||||||||||
else /* CLOCK_MONOTONIC */ | ||||||||||||||||||||||||||
getnanouptime(ts); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
static void | ||||||||||||||||||||||||||
timerfd_curval(struct timerfd *tfd, struct itimerspec *ots) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
struct timespec cts; | ||||||||||||||||||||||||||
timerfd_clocktime(tfd, &cts); | ||||||||||||||||||||||||||
*ots = tfd->tfd_time; | ||||||||||||||||||||||||||
if (ots->it_value.tv_sec != 0 || ots->it_value.tv_nsec != 0) { | ||||||||||||||||||||||||||
timespecsub(&ots->it_value, &cts, &ots->it_value); | ||||||||||||||||||||||||||
if (ots->it_value.tv_sec < 0 || | ||||||||||||||||||||||||||
(ots->it_value.tv_sec == 0 && | ||||||||||||||||||||||||||
ots->it_value.tv_nsec == 0)) { | ||||||||||||||||||||||||||
ots->it_value.tv_sec = 0; | ||||||||||||||||||||||||||
ots->it_value.tv_nsec = 1; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
int | ||||||||||||||||||||||||||
timerfd_gettime_common(struct thread *td, struct file *fp, | ||||||||||||||||||||||||||
struct itimerspec *cts) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
struct timerfd *tfd; | ||||||||||||||||||||||||||
tfd = fp->f_data; | ||||||||||||||||||||||||||
if (tfd == NULL || fp->f_type != DTYPE_TIMERFD) | ||||||||||||||||||||||||||
return (EINVAL); | ||||||||||||||||||||||||||
mtx_lock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
timerfd_curval(tfd, cts); | ||||||||||||||||||||||||||
mtx_unlock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
return (0); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
int | ||||||||||||||||||||||||||
timerfd_settime_common(struct thread *td, struct file *fp, | ||||||||||||||||||||||||||
const struct itimerspec *nts) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
struct timerfd *tfd; | ||||||||||||||||||||||||||
struct timespec cts, exp; | ||||||||||||||||||||||||||
struct timeval tv; | ||||||||||||||||||||||||||
int flags; | ||||||||||||||||||||||||||
tfd = fp->f_data; | ||||||||||||||||||||||||||
if (tfd == NULL || fp->f_type != DTYPE_TIMERFD) | ||||||||||||||||||||||||||
return (EINVAL); | ||||||||||||||||||||||||||
flags = tfd->tfd_flags; | ||||||||||||||||||||||||||
mtx_lock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
bcopy(nts, &tfd->tfd_time, sizeof(*nts)); | ||||||||||||||||||||||||||
tfd->tfd_count = 0; | ||||||||||||||||||||||||||
if (timespecisset(&nts->it_value)) { | ||||||||||||||||||||||||||
timerfd_clocktime(tfd, &cts); | ||||||||||||||||||||||||||
exp = nts->it_value; | ||||||||||||||||||||||||||
if ((flags & TFD_TIMER_ABSTIME) == 0) { | ||||||||||||||||||||||||||
timespecadd(&tfd->tfd_time.it_value, &cts, | ||||||||||||||||||||||||||
&tfd->tfd_time.it_value); | ||||||||||||||||||||||||||
Done Inline ActionsThis is inherited from the old code, but using hz here means that timerfd timers have quite low precision (1ms or 10ms by default). I think it'd be better to use tvtosbt() and callout_reset_sbt() instead. markj: This is inherited from the old code, but using `hz` here means that timerfd timers have quite… | ||||||||||||||||||||||||||
Done Inline ActionsI did some testing and sbt appears to be quite a bit less precise compared to hz. I simply modified the call to callout_reset_sbt(&tfd->tfd_callout, tvtosbt(tv), 0, timerfd_expire, tfd, 0);. Perhaps this is because I didn't specify C_PREL() in flags? I am unsure what the default precision for sbt is. jfree: I did some testing and `sbt` appears to be quite a bit less precise compared to `hz`. I simply… | ||||||||||||||||||||||||||
Done Inline ActionsNevermind now... I just recompiled with callout_reset(&tfd->tfd_callout, tvtohz(&tv), timerfd_expire, tfd); and I am experiencing the same latency. I must've created a delay in the code elsewhere. jfree: Nevermind now... I just recompiled with `callout_reset(&tfd->tfd_callout, tvtohz(&tv)… | ||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||
timespecsub(&exp, &cts, &exp); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
TIMESPEC_TO_TIMEVAL(&tv, &exp); | ||||||||||||||||||||||||||
callout_reset_sbt(&tfd->tfd_callout, tvtosbt(tv), 0, | ||||||||||||||||||||||||||
timerfd_expire, tfd, 0); | ||||||||||||||||||||||||||
tfd->tfd_canceled = false; | ||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||
tfd->tfd_canceled = true; | ||||||||||||||||||||||||||
callout_stop(&tfd->tfd_callout); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
Done Inline ActionsWrong indent. kib: Wrong indent. | ||||||||||||||||||||||||||
mtx_unlock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
return (0); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
int | ||||||||||||||||||||||||||
timerfd_setflags(struct thread *td, struct file *fp, int flags) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
struct timerfd *tfd; | ||||||||||||||||||||||||||
tfd = fp->f_data; | ||||||||||||||||||||||||||
if (tfd == NULL || fp->f_type != DTYPE_TIMERFD) | ||||||||||||||||||||||||||
Done Inline Actionsmalloc M_WAITOK does not fail, so there is no point null checking.... mjg: malloc M_WAITOK does not fail, so there is no point null checking.... | ||||||||||||||||||||||||||
return (EINVAL); | ||||||||||||||||||||||||||
if ((flags & ~TFD_SETTIME_FLAGS) != 0) | ||||||||||||||||||||||||||
return (EINVAL); | ||||||||||||||||||||||||||
mtx_lock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
tfd->tfd_flags = flags; | ||||||||||||||||||||||||||
mtx_unlock(&tfd->tfd_lock); | ||||||||||||||||||||||||||
return (0); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
Done Inline Actionsthis *does* fail and leaks tfd. the only thing which can fail in this func is falloc. just do it first. apart from missing timerfd_head locking mentioned by kib, I have not looked at other stuff mjg: this *does* fail and leaks tfd.
the only thing which can fail in this func is falloc. just do… | ||||||||||||||||||||||||||
Done Inline Actions
Thanks for the corrections. I'll add these changes in a new patch. jfree: > the only thing which can fail in this func is falloc. just do it first.
Thanks for the… | ||||||||||||||||||||||||||
static void | ||||||||||||||||||||||||||
timerfd_expire(void *arg) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
struct timespec cts, ts; | ||||||||||||||||||||||||||
struct timeval tv; | ||||||||||||||||||||||||||
struct timerfd *tfd; | ||||||||||||||||||||||||||
Done Inline ActionsThe indent is wrong. kib: The indent is wrong. | ||||||||||||||||||||||||||
tfd = (struct timerfd *)arg; | ||||||||||||||||||||||||||
timerfd_clocktime(tfd, &cts); | ||||||||||||||||||||||||||
if (timespeccmp(&cts, &tfd->tfd_time.it_value, >=)) { | ||||||||||||||||||||||||||
if (timespecisset(&tfd->tfd_time.it_interval)) { | ||||||||||||||||||||||||||
timespecadd(&tfd->tfd_time.it_value, | ||||||||||||||||||||||||||
&tfd->tfd_time.it_interval, | ||||||||||||||||||||||||||
&tfd->tfd_time.it_value); | ||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||
/* single shot timer */ | ||||||||||||||||||||||||||
timespecclear(&tfd->tfd_time.it_value); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
if (timespecisset(&tfd->tfd_time.it_value)) { | ||||||||||||||||||||||||||
timespecsub(&tfd->tfd_time.it_value, &cts, &ts); | ||||||||||||||||||||||||||
TIMESPEC_TO_TIMEVAL(&tv, &ts); | ||||||||||||||||||||||||||
Done Inline ActionsPut {} around both cases in the if. They are multiline. kib: Put {} around both cases in the if. They are multiline. | ||||||||||||||||||||||||||
callout_reset_sbt(&tfd->tfd_callout, tvtosbt(tv), 0, | ||||||||||||||||||||||||||
Done Inline ActionsSince TFD_SETTIME is IOWR args is a kernel pointer to a copy of the struct copied in by the ioctl framework. The fact that copyin works is an accident of the implementation. Likewise the copyout is redundant. brooks: Since TFD_SETTIME is IOWR `args` is a kernel pointer to a copy of the struct copied in by the… | ||||||||||||||||||||||||||
Done Inline ActionsI was under the impression that ioctl() copied in args, but not the pointers inside of args: args->new_value and args->old_value which are both struct itimerspec *. Does ioctl() recursively copyin? jfree: I was under the impression that ioctl() copied in `args`, but not the pointers inside of args… | ||||||||||||||||||||||||||
Done Inline ActionsSorry, not fully awake when I wrote that. You are correct. I had a different model in my head. brooks: Sorry, not fully awake when I wrote that. You are correct. I had a different model in my head. | ||||||||||||||||||||||||||
Done Inline ActionsFWIW, copyin/out from/to kernel addresses cause EFAULT. Not doing so results in the huge security hole. kib: FWIW, copyin/out from/to kernel addresses cause EFAULT. Not doing so results in the huge… | ||||||||||||||||||||||||||
timerfd_expire, tfd, 0); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
tfd->tfd_count++; | ||||||||||||||||||||||||||
KNOTE_LOCKED(&tfd->tfd_sel.si_note, 0); | ||||||||||||||||||||||||||
selwakeup(&tfd->tfd_sel); | ||||||||||||||||||||||||||
wakeup(&tfd->tfd_count); | ||||||||||||||||||||||||||
} else if (timespecisset(&tfd->tfd_time.it_value)) { | ||||||||||||||||||||||||||
timespecsub(&tfd->tfd_time.it_value, &cts, &ts); | ||||||||||||||||||||||||||
TIMESPEC_TO_TIMEVAL(&tv, &ts); | ||||||||||||||||||||||||||
callout_reset_sbt(&tfd->tfd_callout, tvtosbt(tv), 0, | ||||||||||||||||||||||||||
timerfd_expire, tfd, 0); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
Done Inline Actions
brooks: | ||||||||||||||||||||||||||
Done Inline Actions
brooks: | ||||||||||||||||||||||||||
Done Inline Actions
struct itimerspec32 has 32-bits of padding on non-x86 platforms so needs to be zeroed or we'll leak that to userspace. Is the pold_value juggling worth doing? The cost of a few four dead stores to the stack doesn't seem worth avoiding in the args->old_value==0 case. brooks: `struct itimerspec32` has 32-bits of padding on non-x86 platforms so needs to be zeroed or… | ||||||||||||||||||||||||||
Done Inline Actions
We can skip the process of assigning the old time to (p)old_value if it is passed in as NULL. If not NULL, then timerfd_curval() will be called for every timerfd_settime_common(). I figured a single pointer stack allocation would be less resource intensive than calling curval involuntarily. jfree: > Is the pold_value juggling worth doing? The cost of a few four dead stores to the stack… | ||||||||||||||||||||||||||
Done Inline Actions
kib: | ||||||||||||||||||||||||||
Done Inline Actions
kib: |
should be removed, Roman was involved in the epoll part