Messages in this thread | | | From | Mike Frysinger <> | Date | Wed, 24 Nov 2010 15:57:13 -0500 | Subject | Re: [PATCH] hrtimer: make epoll_wait() use the hrtimer range feature |
| |
On Wed, Nov 24, 2010 at 09:52, Shawn Bohrer wrote: > On Wed, Nov 24, 2010 at 03:33:02AM -0500, Mike Frysinger wrote: >> On Sun, Aug 8, 2010 at 18:45, Shawn Bohrer wrote: >> > @@ -1116,18 +1113,22 @@ static int ep_send_events(struct eventpoll *ep, >> > static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events, >> > int maxevents, long timeout) >> > { >> > - int res, eavail; >> > + int res, eavail, timed_out = 0; >> > unsigned long flags; >> > - long jtimeout; >> > + long slack; >> > wait_queue_t wait; >> > - >> > - /* >> > - * Calculate the timeout by checking for the "infinite" value (-1) >> > - * and the overflow condition. The passed timeout is in milliseconds, >> > - * that why (t * HZ) / 1000. >> > - */ >> > - jtimeout = (timeout < 0 || timeout >= EP_MAX_MSTIMEO) ? >> > - MAX_SCHEDULE_TIMEOUT : (timeout * HZ + 999) / 1000; >> > + struct timespec end_time; >> > + ktime_t expires, *to = NULL; >> > + >> > + if (timeout > 0) { >> > + ktime_get_ts(&end_time); >> > + timespec_add_ns(&end_time, (u64)timeout * NSEC_PER_MSEC); >> > + slack = estimate_accuracy(&end_time); >> > + to = &expires; >> > + *to = timespec_to_ktime(end_time); >> > + } else if (timeout == 0) { >> > + timed_out = 1; >> > + } >> > >> > retry: >> > spin_lock_irqsave(&ep->lock, flags); >> > @@ -1149,7 +1150,7 @@ retry: >> > * to TASK_INTERRUPTIBLE before doing the checks. >> > */ >> > set_current_state(TASK_INTERRUPTIBLE); >> > - if (!list_empty(&ep->rdllist) || !jtimeout) >> > + if (!list_empty(&ep->rdllist) || timed_out) >> > break; >> > if (signal_pending(current)) { >> > res = -EINTR; >> > @@ -1157,7 +1158,9 @@ retry: >> > } >> > >> > spin_unlock_irqrestore(&ep->lock, flags); >> > - jtimeout = schedule_timeout(jtimeout); >> > + if (!schedule_hrtimeout_range(to, slack, HRTIMER_MODE_ABS)) >> > + timed_out = 1; >> > + >> > spin_lock_irqsave(&ep->lock, flags); >> > } >> > __remove_wait_queue(&ep->wq, &wait); >> >> this code introduces a warning: >> fs/eventpoll.c: In function ‘ep_poll’: >> fs/eventpoll.c:1119: warning: ‘slack’ may be used uninitialized in this function >> >> looks to me like you arent properly handling negative timeouts. >> certainly epoll_wait() passes the timeout value straight from >> userspace to ep_poll() without any error checking, so if userspace >> passes a negative timeout value, it looks like "slack" will be used >> uninitialized. > > If a negative timeout is passed in then 'to' remains NULL. When 'to > is NULL schedule_hrtimeout_range() has an infinite timeout and the > 'slack' parameter is never used. So technically everything should be > fine here.
ok, but that depends on an external function never changing behavior and makes changing the API pretty hard since all callers must be closely analyzed
> Of course it would be safest and best to simply initialize slack to 0. > I can send a patch this evening with the fix.
thanks ! -mike -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |