Re: [PATCH resend] fs/eventpoll.c: fix compilation warning

[Shawn Bohrer <shawn.bohrer@gmail.com>]

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