Re: [PATCH] os: Turn off the scheduler alarm after an arbitrary length of time

2010-08-05 Thread Adam Jackson
On Wed, 2010-08-04 at 18:18 +0300, Vignatti Tiago (Nokia-MS/Helsinki)
wrote:
 On Mon, Aug 02, 2010 at 07:34:57PM +0200, ext Adam Jackson wrote:
  On Mon, 2010-08-02 at 17:40 +0300, Tiago Vignatti wrote:
   just wondering what made you think 10 is a good number?
  
  I'd be lying if I said I thought very hard about it.  The way the
  defaults work out, the maximum timeslice is 200ms, which is 10 times as
  long as the scheduler tick.  Another 10x is 2 seconds, which is:
  
  - well into wtf why is it stuck territory
  - a bit before anyone would have thought to attach strace
  - long enough that no amount of scheduler punishment will improve the
user experience
  
  But it's not something that's interesting to be precise about.
 
 I understand your intention to shut up SIGALRM in strace when a client is
 doing a busy rendering. But I'm afraid adding this feature will impact in the
 overall scheduling - not that we're doing great on this area, but would
 require more study to see if this prioritization of a busy client doesn't
 negatively affects other clients... it's just hard to validate your changes.

By the time we turn the alarm off (in this patch), we've already
accumulated several points of penalty for the busy client.  It'll be
punished, just not _infinitely_ punished like before.

 BTW, I didn't tried your patch, but are you sure this works as expected? If I
 understood correctly, this only works for single large requests and for other
 cases SmartScheduleStartTimer is going to be called always after another
 request wakes the server, re-starting the SIGALRM madness. So if there's
 always chunks of large requests you're going to see SIGALRM handler anyway.

Sure.  I'm not trying to solve that.  I'm trying to solve a single stuck
request being misinterpreted as SIGALRM party instead of DRM command
stuck forever.

- ajax


signature.asc
Description: This is a digitally signed message part
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] os: Turn off the scheduler alarm after an arbitrary length of time

2010-08-04 Thread Vignatti Tiago (Nokia-MS/Helsinki)
On Mon, Aug 02, 2010 at 07:34:57PM +0200, ext Adam Jackson wrote:
 On Mon, 2010-08-02 at 17:40 +0300, Tiago Vignatti wrote:
  just wondering what made you think 10 is a good number?
 
 I'd be lying if I said I thought very hard about it.  The way the
 defaults work out, the maximum timeslice is 200ms, which is 10 times as
 long as the scheduler tick.  Another 10x is 2 seconds, which is:
 
 - well into wtf why is it stuck territory
 - a bit before anyone would have thought to attach strace
 - long enough that no amount of scheduler punishment will improve the
   user experience
 
 But it's not something that's interesting to be precise about.

I understand your intention to shut up SIGALRM in strace when a client is
doing a busy rendering. But I'm afraid adding this feature will impact in the
overall scheduling - not that we're doing great on this area, but would
require more study to see if this prioritization of a busy client doesn't
negatively affects other clients... it's just hard to validate your changes.

BTW, I didn't tried your patch, but are you sure this works as expected? If I
understood correctly, this only works for single large requests and for other
cases SmartScheduleStartTimer is going to be called always after another
request wakes the server, re-starting the SIGALRM madness. So if there's
always chunks of large requests you're going to see SIGALRM handler anyway.


Cheers,
 Tiago
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel


Re: [PATCH] os: Turn off the scheduler alarm after an arbitrary length of time

2010-08-02 Thread Tiago Vignatti
On Thu, Jul 29, 2010 at 11:41:10PM +0200, ext Adam Jackson wrote:
 If we're stuck in some difficult bit of processing, there's no point in
 punishing the client too strongly.  Worse, the continuous stream of
 SIGALRM shows up in strace and thus people report infinite stream of
 SIGALRM rather than whatever the bug actually is.
 
 So if we're more than 10x past the maximum timeslice, just turn the
 signal off.

just wondering what made you think 10 is a good number?

 Tiago
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel


Re: [PATCH] os: Turn off the scheduler alarm after an arbitrary length of time

2010-08-02 Thread Adam Jackson
On Mon, 2010-08-02 at 17:40 +0300, Tiago Vignatti wrote:
 On Thu, Jul 29, 2010 at 11:41:10PM +0200, ext Adam Jackson wrote:
  If we're stuck in some difficult bit of processing, there's no point in
  punishing the client too strongly.  Worse, the continuous stream of
  SIGALRM shows up in strace and thus people report infinite stream of
  SIGALRM rather than whatever the bug actually is.
  
  So if we're more than 10x past the maximum timeslice, just turn the
  signal off.
 
 just wondering what made you think 10 is a good number?

I'd be lying if I said I thought very hard about it.  The way the
defaults work out, the maximum timeslice is 200ms, which is 10 times as
long as the scheduler tick.  Another 10x is 2 seconds, which is:

- well into wtf why is it stuck territory
- a bit before anyone would have thought to attach strace
- long enough that no amount of scheduler punishment will improve the
  user experience

But it's not something that's interesting to be precise about.

- ajax


signature.asc
Description: This is a digitally signed message part
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] os: Turn off the scheduler alarm after an arbitrary length of time

2010-07-30 Thread Pat Kane
On Thu, Jul 29, 2010 at 4:41 PM, Adam Jackson a...@redhat.com wrote:
  ...  the continuous stream of
  SIGALRM shows up in strace and thus people report infinite stream of
  SIGALRM rather than whatever the bug actually is.

I really hate those SIGALRMs.

Reviewed-by: Patrick E. Kane pekan...@gmail.com

Pat
---

On Thu, Jul 29, 2010 at 4:41 PM, Adam Jackson a...@redhat.com wrote:
 If we're stuck in some difficult bit of processing, there's no point in
 punishing the client too strongly.  Worse, the continuous stream of
 SIGALRM shows up in strace and thus people report infinite stream of
 SIGALRM rather than whatever the bug actually is.

 So if we're more than 10x past the maximum timeslice, just turn the
 signal off.

 Signed-off-by: Adam Jackson a...@redhat.com
 ---
  os/utils.c |    3 +++
  1 files changed, 3 insertions(+), 0 deletions(-)

 diff --git a/os/utils.c b/os/utils.c
 index 51455cc..a3be61d 100644
 --- a/os/utils.c
 +++ b/os/utils.c
 @@ -1163,6 +1163,9 @@ static void
  SmartScheduleTimer (int sig)
  {
     SmartScheduleTime += SmartScheduleInterval;
 +
 +    if (SmartScheduleTime = 10 * SmartScheduleMaxSlice)
 +       SmartScheduleStopTimer();
  }
  #endif

 --
 1.7.2

 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel


[PATCH] os: Turn off the scheduler alarm after an arbitrary length of time

2010-07-29 Thread Adam Jackson
If we're stuck in some difficult bit of processing, there's no point in
punishing the client too strongly.  Worse, the continuous stream of
SIGALRM shows up in strace and thus people report infinite stream of
SIGALRM rather than whatever the bug actually is.

So if we're more than 10x past the maximum timeslice, just turn the
signal off.

Signed-off-by: Adam Jackson a...@redhat.com
---
 os/utils.c |3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/os/utils.c b/os/utils.c
index 51455cc..a3be61d 100644
--- a/os/utils.c
+++ b/os/utils.c
@@ -1163,6 +1163,9 @@ static void
 SmartScheduleTimer (int sig)
 {
 SmartScheduleTime += SmartScheduleInterval;
+
+if (SmartScheduleTime = 10 * SmartScheduleMaxSlice)
+   SmartScheduleStopTimer();
 }
 #endif
 
-- 
1.7.2

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel