lkml.org 
[lkml]   [2011]   [Jan]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH resend] fs/eventpoll.c: fix compilation warning
    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


    \
     
     \ /
      Last update: 2011-01-15 17:23    [W:0.026 / U:14.828 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site