On Fri, Jan 14, 2011 at 04:05:08PM -0800, Andrew Morton wrote:
> I've looked at this warning several times - the code is non-buggy and
> it's a bit sad to add extra instructions unnecessarily. It would be
> better to make this warning go away by cleaning up or restructuring the
> code.
I agree there really isn't a bug here and thus we don't _need_ to
initialize 'slack', but that depends on the current implementation of
schedule_hrtimeout_range() not using 'slack' when 'to' is NULL. I
can't imagine that changing anytime soon, but that does seem like it
may be a bad assumption.
Furthermore, I've looked at the code pretty hard and I don't see a way
to simply restructure and make the warning go away.
> And the code _is_ pretty stupid. If timed_out is set to 1 then the
> function does a great pile of useless junk. I had a quick tinkle, made
> things worse and gave up:
Ah, I think you may have misunderstood. The warning that 'slack' may
be used uninitialized occurs when a negative timeout is provided, not
when timeout==0.
> --- a/fs/eventpoll.c~a
> +++ a/fs/eventpoll.c
> @@ -1124,16 +1124,20 @@ static int ep_poll(struct eventpoll *ep,
> 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 = select_estimate_accuracy(&end_time);
> - to = &expires;
> - *to = timespec_to_ktime(end_time);
> - } else if (timeout == 0) {
> - timed_out = 1;
> + if (timeout == 0) {
> + /*
> + * explanation of what timeout==0 means goes here
> + */
> + spin_lock_irqsave(&ep->lock, flags);
> + goto skip;
> }
>
> + ktime_get_ts(&end_time);
> + timespec_add_ns(&end_time, (u64)timeout * NSEC_PER_MSEC);
> + slack = select_estimate_accuracy(&end_time);
> + to = &expires;
> + *to = timespec_to_ktime(end_time);
> +
> retry:
> spin_lock_irqsave(&ep->lock, flags);
>
> @@ -1149,9 +1153,10 @@ retry:
>
> for (;;) {
> /*
> - * We don't want to sleep if the ep_poll_callback() sends us
> - * a wakeup in between. That's why we set the task state
> - * to TASK_INTERRUPTIBLE before doing the checks.
> + * We don't want to sleep if the ep_poll_callback()
> + * sends us a wakeup in between. That's why we set the
> + * task state to TASK_INTERRUPTIBLE before doing the
> + * checks.
> */
> set_current_state(TASK_INTERRUPTIBLE);
> if (!list_empty(&ep->rdllist) || timed_out)
> @@ -1171,6 +1176,7 @@ retry:
>
> set_current_state(TASK_RUNNING);
> }
> +skip:
> /* Is it worth to try to dig for events ? */
> eavail = !list_empty(&ep->rdllist) || ep->ovflist != EP_UNACTIVE_PTR;
>
> _
>
>
> but you get the idea ;)
>
> I think the attempt to munge the "timeout==0" spacial case into the
> main body of the polling loop was a mistake, and that the code would be
> better/cleaner if that special case was handled quite separately.
I agree that the timeout==0 case could be optimized here. I've got a
patch set that I'm currently testing to do just that. I'll send it
out shortly.
--
Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
