All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH resend] fs/eventpoll.c: fix compilation warning
@ 2011-01-14 11:52 Viresh Kumar
  2011-01-14 13:07 ` Jack Stone
  0 siblings, 1 reply; 15+ messages in thread
From: Viresh Kumar @ 2011-01-14 11:52 UTC (permalink / raw)
  To: linux-kernel, linux-fsdevel, jwjstone; +Cc: 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH resend] fs/eventpoll.c: fix compilation warning
  2011-01-14 11:52 [PATCH resend] fs/eventpoll.c: fix compilation warning Viresh Kumar
@ 2011-01-14 13:07 ` Jack Stone
  2011-01-14 14:48     ` Shawn Bohrer
  0 siblings, 1 reply; 15+ messages in thread
From: Jack Stone @ 2011-01-14 13:07 UTC (permalink / raw)
  To: Viresh Kumar; +Cc: linux-kernel, linux-fsdevel, viro, shawn.bohrer

[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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH resend] fs/eventpoll.c: fix compilation warning
  2011-01-14 13:07 ` Jack Stone
@ 2011-01-14 14:48     ` Shawn Bohrer
  0 siblings, 0 replies; 15+ messages in thread
From: Shawn Bohrer @ 2011-01-14 14:48 UTC (permalink / raw)
  To: Jack Stone, Andrew Morton; +Cc: Viresh Kumar, linux-kernel, linux-fsdevel, viro

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH resend] fs/eventpoll.c: fix compilation warning
@ 2011-01-14 14:48     ` Shawn Bohrer
  0 siblings, 0 replies; 15+ messages in thread
From: Shawn Bohrer @ 2011-01-14 14:48 UTC (permalink / raw)
  To: Jack Stone, Andrew Morton; +Cc: Viresh Kumar, linux-kernel, linux-fsdevel, viro

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH resend] fs/eventpoll.c: fix compilation warning
  2011-01-14 14:48     ` Shawn Bohrer
  (?)
@ 2011-01-14 15:21     ` Davide Libenzi
  -1 siblings, 0 replies; 15+ messages in thread
From: Davide Libenzi @ 2011-01-14 15:21 UTC (permalink / raw)
  To: Shawn Bohrer
  Cc: Jack Stone, Andrew Morton, Viresh Kumar,
	Linux Kernel Mailing List, linux-fsdevel, viro

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH resend] fs/eventpoll.c: fix compilation warning
  2011-01-14 14:48     ` Shawn Bohrer
  (?)
  (?)
@ 2011-01-15  0:05     ` Andrew Morton
  2011-01-15 11:10       ` Jack Stone
  2011-01-15 16:20       ` Shawn Bohrer
  -1 siblings, 2 replies; 15+ messages in thread
From: Andrew Morton @ 2011-01-15  0:05 UTC (permalink / raw)
  To: Shawn Bohrer; +Cc: Jack Stone, Viresh Kumar, linux-kernel, linux-fsdevel, viro

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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH resend] fs/eventpoll.c: fix compilation warning
  2011-01-15  0:05     ` Andrew Morton
@ 2011-01-15 11:10       ` Jack Stone
  2011-01-15 16:20       ` Shawn Bohrer
  1 sibling, 0 replies; 15+ messages in thread
From: Jack Stone @ 2011-01-15 11:10 UTC (permalink / raw)
  To: Andrew Morton, Shawn Bohrer
  Cc: Viresh Kumar, linux-kernel, linux-fsdevel, viro

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH resend] fs/eventpoll.c: fix compilation warning
  2011-01-15  0:05     ` Andrew Morton
  2011-01-15 11:10       ` Jack Stone
@ 2011-01-15 16:20       ` Shawn Bohrer
  2011-01-15 17:00         ` [PATCH 1/3] epoll: initialize slack for negative timeout values Shawn Bohrer
                           ` (2 more replies)
  1 sibling, 3 replies; 15+ messages in thread
From: Shawn Bohrer @ 2011-01-15 16:20 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jack Stone, Viresh Kumar, linux-kernel, linux-fsdevel, viro,
	Davide Libenzi

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/3] epoll: initialize slack for negative timeout values
  2011-01-15 16:20       ` Shawn Bohrer
@ 2011-01-15 17:00         ` Shawn Bohrer
  2011-01-15 19:06           ` Davide Libenzi
  2011-03-18  7:38             ` Mike Frysinger
  2011-01-15 17:00         ` [PATCH 2/3] epoll: short circuit the timeout==0 case Shawn Bohrer
  2011-01-15 17:00         ` [PATCH 3/3] epoll: remove unnecessary test of ep->ovflist for available events Shawn Bohrer
  2 siblings, 2 replies; 15+ messages in thread
From: Shawn Bohrer @ 2011-01-15 17:00 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jack Stone, Viresh Kumar, linux-kernel, linux-fsdevel, viro,
	Davide Libenzi, 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/3] epoll: short circuit the timeout==0 case
  2011-01-15 16:20       ` Shawn Bohrer
  2011-01-15 17:00         ` [PATCH 1/3] epoll: initialize slack for negative timeout values Shawn Bohrer
@ 2011-01-15 17:00         ` Shawn Bohrer
  2011-01-15 17:00         ` [PATCH 3/3] epoll: remove unnecessary test of ep->ovflist for available events Shawn Bohrer
  2 siblings, 0 replies; 15+ messages in thread
From: Shawn Bohrer @ 2011-01-15 17:00 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jack Stone, Viresh Kumar, linux-kernel, linux-fsdevel, viro,
	Davide Libenzi, 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/3] epoll: remove unnecessary test of ep->ovflist for available events
  2011-01-15 16:20       ` Shawn Bohrer
  2011-01-15 17:00         ` [PATCH 1/3] epoll: initialize slack for negative timeout values Shawn Bohrer
  2011-01-15 17:00         ` [PATCH 2/3] epoll: short circuit the timeout==0 case Shawn Bohrer
@ 2011-01-15 17:00         ` Shawn Bohrer
  2011-01-15 19:03           ` Davide Libenzi
  2 siblings, 1 reply; 15+ messages in thread
From: Shawn Bohrer @ 2011-01-15 17:00 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jack Stone, Viresh Kumar, linux-kernel, linux-fsdevel, viro,
	Davide Libenzi, 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] epoll: remove unnecessary test of ep->ovflist for available events
  2011-01-15 17:00         ` [PATCH 3/3] epoll: remove unnecessary test of ep->ovflist for available events Shawn Bohrer
@ 2011-01-15 19:03           ` Davide Libenzi
  0 siblings, 0 replies; 15+ messages in thread
From: Davide Libenzi @ 2011-01-15 19:03 UTC (permalink / raw)
  To: Shawn Bohrer
  Cc: Andrew Morton, Jack Stone, Viresh Kumar,
	Linux Kernel Mailing List, linux-fsdevel, viro

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] epoll: initialize slack for negative timeout values
  2011-01-15 17:00         ` [PATCH 1/3] epoll: initialize slack for negative timeout values Shawn Bohrer
@ 2011-01-15 19:06           ` Davide Libenzi
  2011-03-18  7:38             ` Mike Frysinger
  1 sibling, 0 replies; 15+ messages in thread
From: Davide Libenzi @ 2011-01-15 19:06 UTC (permalink / raw)
  To: Shawn Bohrer
  Cc: Andrew Morton, Jack Stone, Viresh Kumar,
	Linux Kernel Mailing List, linux-fsdevel, viro

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] epoll: initialize slack for negative timeout values
  2011-01-15 17:00         ` [PATCH 1/3] epoll: initialize slack for negative timeout values Shawn Bohrer
@ 2011-03-18  7:38             ` Mike Frysinger
  2011-03-18  7:38             ` Mike Frysinger
  1 sibling, 0 replies; 15+ messages in thread
From: Mike Frysinger @ 2011-03-18  7:38 UTC (permalink / raw)
  To: Shawn Bohrer
  Cc: Andrew Morton, Jack Stone, Viresh Kumar, linux-kernel,
	linux-fsdevel, viro, Davide Libenzi

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] epoll: initialize slack for negative timeout values
@ 2011-03-18  7:38             ` Mike Frysinger
  0 siblings, 0 replies; 15+ messages in thread
From: Mike Frysinger @ 2011-03-18  7:38 UTC (permalink / raw)
  To: Shawn Bohrer
  Cc: Andrew Morton, Jack Stone, Viresh Kumar, linux-kernel,
	linux-fsdevel, viro, Davide Libenzi

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2011-03-18  7:38 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-14 11:52 [PATCH resend] fs/eventpoll.c: fix compilation warning Viresh Kumar
2011-01-14 13:07 ` Jack Stone
2011-01-14 14:48   ` Shawn Bohrer
2011-01-14 14:48     ` Shawn Bohrer
2011-01-14 15:21     ` Davide Libenzi
2011-01-15  0:05     ` Andrew Morton
2011-01-15 11:10       ` Jack Stone
2011-01-15 16:20       ` Shawn Bohrer
2011-01-15 17:00         ` [PATCH 1/3] epoll: initialize slack for negative timeout values Shawn Bohrer
2011-01-15 19:06           ` Davide Libenzi
2011-03-18  7:38           ` Mike Frysinger
2011-03-18  7:38             ` Mike Frysinger
2011-01-15 17:00         ` [PATCH 2/3] epoll: short circuit the timeout==0 case Shawn Bohrer
2011-01-15 17:00         ` [PATCH 3/3] epoll: remove unnecessary test of ep->ovflist for available events Shawn Bohrer
2011-01-15 19:03           ` Davide Libenzi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.