Thomas Gleixner wrote: > On Wed, 2007-05-02 at 02:22 -0300, Davi Arnaut wrote: >> plain text document attachment (pollfs-timer.patch) >> Per file descriptor high-resolution timers. A classic unix file interface for >> the POSIX timer_(create|settime|gettime|delete) family of functions. >> >> Signed-off-by: Davi E. M. Arnaut <[EMAIL PROTECTED]> > > Nacked-by-me. > > Aside of the fact, that it is a bad clone of the timerfd code, it is > simply broken and untested.
I've made it by the same time of timerfd, I even sent it to Davide and the list. "Clone" is a bit of overstatment, timerfd is not bugged as this :) >> + >> +struct hrtimerspec { >> + int flags; >> + clockid_t clock; >> + struct itimerspec expr; >> +}; > > How exactly knows userspace what a struct hrtimerspec is ? Is the c file > exported as a header ? Will move then all to another header later. >> +static ssize_t read(struct pfs_timer *evs, struct itimerspec __user *uspec) >> +{ >> + int ret = -EAGAIN; >> + ktime_t remaining = {}; >> + unsigned long overruns = 0; >> + struct itimerspec spec = {}; >> + struct hrtimer *timer = &evs->timer; >> + >> + spin_lock_irq(&evs->lock); >> + >> + if (!evs->overruns) >> + goto out_unlock; >> + >> + if (hrtimer_active(timer)) >> + remaining = hrtimer_get_remaining(timer); >> + else if (evs->interval.tv64 > 0) >> + overruns = hrtimer_forward(timer, hrtimer_cb_get_time(timer), >> + evs->interval); > > Where is the logic here ? Return the remaing time for timer firing, or rearm the timer. And its pretty broken because of the first if and I forgot to reset overruns. > If no overrun, return remaining time = 0 > > If active, return the real remaining time. This path is never hit, as > the timer is nowhere restarted. > > If not active, return remanining time = 0. How does the caller know how > many events are missed ? > >> + ret = -EOVERFLOW; >> + if (overruns > (ULONG_MAX - evs->overruns)) >> + goto out_unlock; >> + else >> + evs->overruns += overruns; > > Interesting feature. evs->overruns is adding up forever and then limited > to ULONG_MAX See third comment! >> +static enum hrtimer_restart timer_fn(struct hrtimer *timer) >> +{ >> + struct pfs_timer *evs = container_of(timer, struct pfs_timer, timer); >> + unsigned long flags; >> + >> + spin_lock_irqsave(&evs->lock, flags); >> + /* timer tick, interval has elapsed */ >> + if (!evs->overruns++) >> + wake_up_all(&evs->wait); > > Cool. Waiters, which came after the first event are stuck. Simply > because there is no second event. See third comment! >> +static ssize_t write(struct pfs_timer *evs, >> + const struct hrtimerspec __user *uspec) >> +{ >> + struct hrtimerspec spec; > > See first comment ! > >> + if (copy_from_user(&spec, uspec, sizeof(spec))) >> + return -EFAULT; >> + >> + if (spec_invalid(&spec)) >> + return -EINVAL; >> + >> + rearm_timer(evs, &spec); >> + >> + return 0; >> +} >> + >> +static int poll(struct pfs_timer *evs) >> +{ >> + int ret; >> + >> + ret = evs->overruns ? POLLIN : 0; >> + >> + return ret; >> +} > > Creative lockless programming style with 4 lines overhead and a > guaranteed return POLLIN after the first timer event. This is really > cute as it covers the missing timer restart and guarantees 100% CPU load > for ever. Hmm, maybe it's correct: polling should loop for ever, > shouldn't it ? See third comment! -- It will remain lockless, as reading and setting this data type is guaranteed to happen atomically. >> +static const struct pfs_operations timer_ops = { >> + .read = PFS_READ(read, struct pfs_timer, struct itimerspec), >> + .write = PFS_WRITE(write, struct pfs_timer, struct hrtimerspec), >> + .poll = PFS_POLL(poll, struct pfs_timer), >> + .release = PFS_RELEASE(release, struct pfs_timer), >> + .rsize = sizeof(struct itimerspec), >> + .wsize = sizeof(struct hrtimerspec), > > See first comment ! This has nothing to do with user space, or you got lost in comments references. -- Davi Arnaut - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/