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

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

[Viresh Kumar]

This patch fixes following compilation warning
fs/eventpoll.c:1119: warning: 'slack' may be used uninitialized in this function

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 fs/eventpoll.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 8cf0724..89b5e98 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1116,7 +1116,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 {
 	int res, eavail, timed_out = 0;
 	unsigned long flags;
-	long slack;
+	long uninitialized_var(slack);
 	wait_queue_t wait;
 	struct timespec end_time;
 	ktime_t expires, *to = NULL;
-- 
1.7.2.2

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

[Jack Stone]

[cc Al Viro and Shawn Bohrer]
On 14/01/2011 11:52, Viresh Kumar wrote:
> This patch fixes following compilation warning
> fs/eventpoll.c:1119: warning: 'slack' may be used uninitialized in this function
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>  fs/eventpoll.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index 8cf0724..89b5e98 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -1116,7 +1116,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
>  {
>  	int res, eavail, timed_out = 0;
>  	unsigned long flags;
> -	long slack;
> +	long uninitialized_var(slack);
>  	wait_queue_t wait;
>  	struct timespec end_time;
>  	ktime_t expires, *to = NULL;

Hi Viresh,

This is certainly the correct thing to do if timeout cannot be negative.

Al, Shawn

Can timeout be negative, and if so what does it mean?

Thanks,

Jack

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

[Shawn Bohrer]

On Fri, Jan 14, 2011 at 7:07 AM, Jack Stone <jwjstone@fastmail.fm> wrote:
> [cc Al Viro and Shawn Bohrer]
> On 14/01/2011 11:52, Viresh Kumar wrote:
>> This patch fixes following compilation warning
>> fs/eventpoll.c:1119: warning: 'slack' may be used uninitialized in this function
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
>> ---
>>  fs/eventpoll.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
>> index 8cf0724..89b5e98 100644
>> --- a/fs/eventpoll.c
>> +++ b/fs/eventpoll.c
>> @@ -1116,7 +1116,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
>>  {
>>       int res, eavail, timed_out = 0;
>>       unsigned long flags;
>> -     long slack;
>> +     long uninitialized_var(slack);
>>       wait_queue_t wait;
>>       struct timespec end_time;
>>       ktime_t expires, *to = NULL;
>
> Hi Viresh,
>
> This is certainly the correct thing to do if timeout cannot be negative.
>
> Al, Shawn
>
> Can timeout be negative, and if so what does it mean?

Yes timeout can be negative.  When timeout is negative it signifies an
infinite timeout.  Therefore I think the correct fix is to initialize
slack to 0.  I actually sent a patch to fix this back in November, but
it looks like it was never applied.

https://lkml.org/lkml/2010/11/27/143

Andrew, can you apply this patch?  Let me know if I need to resend.

--
Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Davide Libenzi]

On Fri, 14 Jan 2011, Shawn Bohrer wrote:

> Yes timeout can be negative.  When timeout is negative it signifies an
> infinite timeout.  Therefore I think the correct fix is to initialize
> slack to 0.  I actually sent a patch to fix this back in November, but
> it looks like it was never applied.
> 
> https://lkml.org/lkml/2010/11/27/143
> 
> Andrew, can you apply this patch?  Let me know if I need to resend.

Yes, please.


- Davide

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

[Andrew Morton]

On Fri, 14 Jan 2011 08:48:11 -0600
Shawn Bohrer <shawn.bohrer@gmail.com> wrote:

> On Fri, Jan 14, 2011 at 7:07 AM, Jack Stone <jwjstone@fastmail.fm> wrote:
> > [cc Al Viro and Shawn Bohrer]
> > On 14/01/2011 11:52, Viresh Kumar wrote:
> >> This patch fixes following compilation warning
> >> fs/eventpoll.c:1119: warning: 'slack' may be used uninitialized in this function
> >>
> >> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> >> ---
> >>  fs/eventpoll.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> >> index 8cf0724..89b5e98 100644
> >> --- a/fs/eventpoll.c
> >> +++ b/fs/eventpoll.c
> >> @@ -1116,7 +1116,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event  user *events,
> >>  {
> >>       int res, eavail, timed_out = 0;
> >>       unsigned long flags;
> >> -     long slack;
> >> +     long uninitialized_var(slack);
> >>       wait_queue_t wait;
> >>       struct timespec end_time;
> >>       ktime_t expires, *to = NULL;
> >
> > Hi Viresh,
> >
> > This is certainly the correct thing to do if timeout cannot be negative.
> >
> > Al, Shawn
> >
> > Can timeout be negative, and if so what does it mean?
> 
> Yes timeout can be negative.  When timeout is negative it signifies an
> infinite timeout.  Therefore I think the correct fix is to initialize
> slack to 0.  I actually sent a patch to fix this back in November, but
> it looks like it was never applied.
> 
> https://lkml.org/lkml/2010/11/27/143
> 
> Andrew, can you apply this patch?  Let me know if I need to resend.

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.  

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:

--- 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.

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

[Jack Stone]

"Andrew Morton" <akpm@linux-foundation.org> 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.  
>
>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:

[snip]

>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 can have a go at a patch later on today if you wish.

Thanks,

Jack


-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

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

[Shawn Bohrer]

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

[PATCH 1/3] epoll: initialize slack for negative timeout values

[Shawn Bohrer]

When a negative timeout value is passed to epoll the 'slack' variable is
currently unitialized:

fs/eventpoll.c: In function ‘ep_poll’:
fs/eventpoll.c:1119: warning: ‘slack’ may be used uninitialized in this function

In this case a NULL pointer is passed to schedule_hrtimeout_range()
specifying an infinite timeout.  The current implementation of
schedule_hrtimeout_range() does not use slack in this case, but we
should still initialize slack to 0 in case future implementations use it.

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
Acked-by: Davide Libenzi <davidel@xmailserver.org>
---
 fs/eventpoll.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 8cf0724..c24a032 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1116,7 +1116,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 {
 	int res, eavail, timed_out = 0;
 	unsigned long flags;
-	long slack;
+	long slack = 0;
 	wait_queue_t wait;
 	struct timespec end_time;
 	ktime_t expires, *to = NULL;
-- 
1.7.3.4

Re: [PATCH 1/3] epoll: initialize slack for negative timeout values

[Davide Libenzi]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1309 bytes --]

On Sat, 15 Jan 2011, Shawn Bohrer wrote:

> When a negative timeout value is passed to epoll the 'slack' variable is
> currently unitialized:
> 
> fs/eventpoll.c: In function ‘ep_poll’:
> fs/eventpoll.c:1119: warning: ‘slack’ may be used uninitialized in this function
> 
> In this case a NULL pointer is passed to schedule_hrtimeout_range()
> specifying an infinite timeout.  The current implementation of
> schedule_hrtimeout_range() does not use slack in this case, but we
> should still initialize slack to 0 in case future implementations use it.
> 
> Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
> Acked-by: Davide Libenzi <davidel@xmailserver.org>
> ---
>  fs/eventpoll.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index 8cf0724..c24a032 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -1116,7 +1116,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
>  {
>  	int res, eavail, timed_out = 0;
>  	unsigned long flags;
> -	long slack;
> +	long slack = 0;
>  	wait_queue_t wait;
>  	struct timespec end_time;
>  	ktime_t expires, *to = NULL;

Re-ACK.
But, I am consolidating 1 and (a rewritten) 2, into a single one, since 
they would otherwise conflict with another patch.



- Davide

Re: [PATCH 1/3] epoll: initialize slack for negative timeout values

[Mike Frysinger]

On Sat, Jan 15, 2011 at 12:00, Shawn Bohrer wrote:
> When a negative timeout value is passed to epoll the 'slack' variable is
> currently unitialized:
>
> fs/eventpoll.c: In function ‘ep_poll’:
> fs/eventpoll.c:1119: warning: ‘slack’ may be used uninitialized in this function

still in 2.6.38 ? :/
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] epoll: short circuit the timeout==0 case

[Shawn Bohrer]

If a timeout == 0 is specified we will return immediately even if there
are no events so there is no need to enter the polling loop.

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 fs/eventpoll.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index c24a032..57a77f5 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1121,6 +1121,10 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 	struct timespec end_time;
 	ktime_t expires, *to = NULL;
 
+	/*
+	 * A negative timeout means wait indefinitely and leaves 'to' NULL for
+	 * an infinite timeout.
+	 */
 	if (timeout > 0) {
 		ktime_get_ts(&end_time);
 		timespec_add_ns(&end_time, (u64)timeout * NSEC_PER_MSEC);
@@ -1128,7 +1132,12 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 		to = &expires;
 		*to = timespec_to_ktime(end_time);
 	} else if (timeout == 0) {
+		/*
+		 * Return immediately even if no events are available.
+		 */
 		timed_out = 1;
+		spin_lock_irqsave(&ep->lock, flags);
+		goto skip;
 	}
 
 retry:
@@ -1146,9 +1155,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)
@@ -1168,6 +1178,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;
 
-- 
1.7.3.4

[PATCH 3/3] epoll: remove unnecessary test of ep->ovflist for available events

[Shawn Bohrer]

The additional test for ep->ovflist != EP_UNACTIVE_PTR to signify
available events was added in 5071f97ec6d74f006072de0ce89b67c8792fe5a1
but doesn't appear to do anything.  Either this is a bug or the check
isn't needed.

If the ep->ovflist is not EP_UNACTIVE_PTR then ep_send_events() calls
ep_scan_ready_list() which sets ep->ovflist = NULL thus loosing any
events which may have been stored there.

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 fs/eventpoll.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 57a77f5..afeb78c 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1180,7 +1180,7 @@ retry:
 	}
 skip:
 	/* Is it worth to try to dig for events ? */
-	eavail = !list_empty(&ep->rdllist) || ep->ovflist != EP_UNACTIVE_PTR;
+	eavail = !list_empty(&ep->rdllist);
 
 	spin_unlock_irqrestore(&ep->lock, flags);
 
-- 
1.7.3.4

Re: [PATCH 3/3] epoll: remove unnecessary test of ep->ovflist for available events

[Davide Libenzi]

On Sat, 15 Jan 2011, Shawn Bohrer wrote:

> The additional test for ep->ovflist != EP_UNACTIVE_PTR to signify
> available events was added in 5071f97ec6d74f006072de0ce89b67c8792fe5a1
> but doesn't appear to do anything.  Either this is a bug or the check
> isn't needed.
> 
> If the ep->ovflist is not EP_UNACTIVE_PTR then ep_send_events() calls
> ep_scan_ready_list() which sets ep->ovflist = NULL thus loosing any
> events which may have been stored there.
> 
> Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>

NACK. Not only NACK, but hell NACK.
The epoll_wait() might hit right after the delivery of the current events 
ended in/right-after:

	error = (*sproc)(ep, &txlist, priv);

So, if there are overflowed events, a following ep_send_events() can go 
fetch them, because ep_scan_ready_list() will go drop them back in the 
ready list (right after the line above).
Events in the overflow list are no different from the ones in the ready 
list, and removing such test will make you, either return with no events 
when they are really there, or take another unnecessary spin lock/unlock 
trip.
On the contrary, a missed optimization is applying the same rule even 
above, instead of the bare list_empty(). Will send a patch to Andrew.
And no, it is not a bug, because ep_scan_ready_list() is protected by a 
mutex.
	


- Davide