Zhang, Yanmin | 20 Aug 09:24

Re: VolanoMark regression with 2.6.27-rc1


On Mon, 2008-08-18 at 10:51 +0530, Dhaval Giani wrote: 
> > > > > > So with kernel 2.6.27-rc1, the successful wakeup_affine is about
> > > > > > double of the one of 2.6.27-rc1
> > > > > > on domain 0, but about 10 times on domain 1. That means more tasks are
> > > > > > woken up on waker cpus.
> > > > > > 
> > > > > > Does that mean it doesn't follow cache-hot checking?
> > > > > 
> > > > > I'm a bit puzzled, but you're right - I too noticed that volanomark is
> > > > > _very_ sensitive to affine wakeups.
> > > > > 
> > > > > I'll try and find what changed in that code for GROUP=n.
> > > > 
> > > > hi Yanmin,
> > > > 
> > > > I was wondering if you could send me your config and what sysctls you
> > > > have set. I have not been able to reproduce the 2.6.26 -> 2.6.27-rc1
> > > > GROUP=n regression.
> > > Pls. see the attachment. As for sysctl, I just set /proc/sys/kernel/sched_compat_yield=1.
> > > 
> > > I am wondering if the load balance causes the regression when group=n. I manually delete
> > > all GROUP codes and do a diff against 26 and 27-rc1.
> > > 
> > 
> > You can disable load balancing by being in uniprocessor mode.
> > 
> 
> Hi,
> 
(Continue reading)

Peter Zijlstra | 20 Aug 09:41

Re: VolanoMark regression with 2.6.27-rc1

On Wed, 2008-08-20 at 15:24 +0800, Zhang, Yanmin wrote:
> On Mon, 2008-08-18 at 10:51 +0530, Dhaval Giani wrote: 
> > > > > > > So with kernel 2.6.27-rc1, the successful wakeup_affine is about
> > > > > > > double of the one of 2.6.27-rc1
> > > > > > > on domain 0, but about 10 times on domain 1. That means more tasks are
> > > > > > > woken up on waker cpus.
> > > > > > > 
> > > > > > > Does that mean it doesn't follow cache-hot checking?
> > > > > > 
> > > > > > I'm a bit puzzled, but you're right - I too noticed that volanomark is
> > > > > > _very_ sensitive to affine wakeups.
> > > > > > 
> > > > > > I'll try and find what changed in that code for GROUP=n.
> > > > > 
> > > > > hi Yanmin,
> > > > > 
> > > > > I was wondering if you could send me your config and what sysctls you
> > > > > have set. I have not been able to reproduce the 2.6.26 -> 2.6.27-rc1
> > > > > GROUP=n regression.
> > > > Pls. see the attachment. As for sysctl, I just set /proc/sys/kernel/sched_compat_yield=1.
> > > > 
> > > > I am wondering if the load balance causes the regression when group=n. I manually delete
> > > > all GROUP codes and do a diff against 26 and 27-rc1.
> > > > 
> > > 
> > > You can disable load balancing by being in uniprocessor mode.
> > > 
> > 
> > Hi,
> > 
(Continue reading)

Ingo Molnar | 20 Aug 12:51
Favicon

Re: VolanoMark regression with 2.6.27-rc1


* Peter Zijlstra <a.p.zijlstra <at> chello.nl> wrote:

> And I must admit to having overlooked the effect on wake_affine..
> 
> Chris, could you see the effect of this on smp group fairness?

applied your commit below to tip/sched/urgent, thanks.

	Ingo

-------------->
From 939387c3a6141ec6aefc7acd40f8b186781bb098 Mon Sep 17 00:00:00 2001
From: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
Date: Wed, 20 Aug 2008 12:44:55 +0200
Subject: [PATCH] sched: enable LB_BIAS by default

Yanmin reported a significant regression on his 16-core machine due to:

  commit 93b75217df39e6d75889cc6f8050343286aff4a5
  Author: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
  Date:   Fri Jun 27 13:41:33 2008 +0200

Flip back to the old behaviour.

Reported-by: "Zhang, Yanmin" <yanmin_zhang <at> linux.intel.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
Signed-off-by: Ingo Molnar <mingo <at> elte.hu>
---
 kernel/sched_features.h |    2 +-
(Continue reading)

Peter Zijlstra | 20 Aug 15:32

Re: VolanoMark regression with 2.6.27-rc1

On Wed, 2008-08-20 at 12:51 +0200, Ingo Molnar wrote:
> * Peter Zijlstra <a.p.zijlstra <at> chello.nl> wrote:
> 
> > And I must admit to having overlooked the effect on wake_affine..
> > 
> > Chris, could you see the effect of this on smp group fairness?

Just realized my brainfart..

---
Subject: sched: load-balance bias fixes
From: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
Date: Wed Aug 20 15:28:51 CEST 2008

Yanmin spotted a regression with my patch that introduces LB_BIAS:

  commit 93b75217df39e6d75889cc6f8050343286aff4a5
  Author: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
  Date:   Fri Jun 27 13:41:33 2008 +0200

And I just spotted the brainfart - I should have replaced min/max with avg
instead of removing it completely.

Signed-off-by: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
---
 include/linux/kernel.h |    6 ++++++
 kernel/sched.c         |   10 ++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

Index: linux-2.6/include/linux/kernel.h
(Continue reading)

Ingo Molnar | 20 Aug 15:47
Favicon

Re: VolanoMark regression with 2.6.27-rc1


* Peter Zijlstra <a.p.zijlstra <at> chello.nl> wrote:

> Just realized my brainfart..
> 
> ---
> Subject: sched: load-balance bias fixes
> From: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
> Date: Wed Aug 20 15:28:51 CEST 2008
> 
> Yanmin spotted a regression with my patch that introduces LB_BIAS:

ok, i've applied this one to tip/sched/urgent instead of the 
feature-disabling patchlet. Yanmin, could you please check whether this 
one does the trick?

	Ingo
Zhang, Yanmin | 21 Aug 04:25

Re: VolanoMark regression with 2.6.27-rc1


On Wed, 2008-08-20 at 15:47 +0200, Ingo Molnar wrote:
> * Peter Zijlstra <a.p.zijlstra <at> chello.nl> wrote:
> 
> > Just realized my brainfart..
> > 
> > ---
> > Subject: sched: load-balance bias fixes
> > From: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
> > Date: Wed Aug 20 15:28:51 CEST 2008
> > 
> > Yanmin spotted a regression with my patch that introduces LB_BIAS:
> 
> ok, i've applied this one to tip/sched/urgent instead of the 
> feature-disabling patchlet. Yanmin, could you please check whether this 
> one does the trick?
This new patch almost doesn't help volanoMark. Pls. use the patch
which sets LB_BIAS=1 by default.

-yanmin

Ingo Molnar | 21 Aug 08:16
Favicon

Re: VolanoMark regression with 2.6.27-rc1


* Zhang, Yanmin <yanmin_zhang <at> linux.intel.com> wrote:

> > ok, i've applied this one to tip/sched/urgent instead of the 
> > feature-disabling patchlet. Yanmin, could you please check whether this 
> > one does the trick?
>
> This new patch almost doesn't help volanoMark. Pls. use the patch 
> which sets LB_BIAS=1 by default.

ok. That also removes the kernel.h complications ;-)

	Ingo
Zhang, Yanmin | 21 Aug 08:48

Re: VolanoMark regression with 2.6.27-rc1


On Thu, 2008-08-21 at 08:16 +0200, Ingo Molnar wrote:
> * Zhang, Yanmin <yanmin_zhang <at> linux.intel.com> wrote:
> 
> > > ok, i've applied this one to tip/sched/urgent instead of the 
> > > feature-disabling patchlet. Yanmin, could you please check whether this 
> > > one does the trick?
> >
> > This new patch almost doesn't help volanoMark. Pls. use the patch 
> > which sets LB_BIAS=1 by default.
> 
> ok. That also removes the kernel.h complications ;-)
Sorry, I have new update.
Originally, I worked on 2.6.27-rc1. I just move to 2.6.27-rc3 and found
something defferent when CONFIG_GROUP_SCHED=n.

With 2.6.27-rc3, on my 8-core stoakley, all volanoMark regression disappears,
no matter if I enable LB_BIAS. On 16-core tigerton, the regression is still
there if I don't enable LB_BIAS and regression becomes 11% from 65%.

8-core stoakley:
2.6.26_nogroup			340669
2.6.27-rc1_nogroup		267237
2.6.27-rc1_nogroup+LB_BIAS=1	330693
2.6.27-rc3_nogroup		352193
2.6.27-rc3_nogroup+LB_BIAS=1	355872

16-core tigerton:
2.6.26_nogroup			539644
2.6.27-rc1_nogroup		309334
(Continue reading)

Zhang, Yanmin | 29 Aug 05:35

Re: VolanoMark regression with 2.6.27-rc1


On Thu, 2008-08-21 at 14:48 +0800, Zhang, Yanmin wrote:
> On Thu, 2008-08-21 at 08:16 +0200, Ingo Molnar wrote:
> > * Zhang, Yanmin <yanmin_zhang <at> linux.intel.com> wrote:
> > 
> > > > ok, i've applied this one to tip/sched/urgent instead of the 
> > > > feature-disabling patchlet. Yanmin, could you please check whether this 
> > > > one does the trick?
> > >
> > > This new patch almost doesn't help volanoMark. Pls. use the patch 
> > > which sets LB_BIAS=1 by default.
> > 
> > ok. That also removes the kernel.h complications ;-)
> Sorry, I have new update.
> Originally, I worked on 2.6.27-rc1. I just move to 2.6.27-rc3 and found
> something defferent when CONFIG_GROUP_SCHED=n.
> 
> With 2.6.27-rc3, on my 8-core stoakley, all volanoMark regression disappears,
> no matter if I enable LB_BIAS. On 16-core tigerton, the regression is still
> there if I don't enable LB_BIAS and regression becomes 11% from 65%.
I have new updates on this regression. I checked volanoMark web page and
found the client command line has option rooms and users. rooms means how many
chat room will be started. users means how many users are in 1 room. The default
rooms is 10 and users is 20, so every room has about 800 threads. As all threads of a
room just communicate within this room, so the rooms number is important.

All my previous volanoMark testing uses default rooms 10 and users 20. With wake_offine
in kernel, waker/sleeper will be moved to the same cpu gradually. However, if the
rooms is not multiple of cpu number, due to load balance, kernel will move threads from
one cpu to another cpu continually. If there are too many threads to weaken the cache-hot
(Continue reading)

Zhang, Yanmin | 29 Aug 05:38

Re: VolanoMark regression with 2.6.27-rc1


On Fri, 2008-08-29 at 11:35 +0800, Zhang, Yanmin wrote:
> On Thu, 2008-08-21 at 14:48 +0800, Zhang, Yanmin wrote:
> > On Thu, 2008-08-21 at 08:16 +0200, Ingo Molnar wrote:
> > > * Zhang, Yanmin <yanmin_zhang <at> linux.intel.com> wrote:
> > > 
> > > > > ok, i've applied this one to tip/sched/urgent instead of the 
> > > > > feature-disabling patchlet. Yanmin, could you please check whether this 
> > > > > one does the trick?
> > > >
> > > > This new patch almost doesn't help volanoMark. Pls. use the patch 
> > > > which sets LB_BIAS=1 by default.
> > > 
> > > ok. That also removes the kernel.h complications ;-)
> > Sorry, I have new update.
> > Originally, I worked on 2.6.27-rc1. I just move to 2.6.27-rc3 and found
> > something defferent when CONFIG_GROUP_SCHED=n.
> > 
> > With 2.6.27-rc3, on my 8-core stoakley, all volanoMark regression disappears,
> > no matter if I enable LB_BIAS. On 16-core tigerton, the regression is still
> > there if I don't enable LB_BIAS and regression becomes 11% from 65%.
> I have new updates on this regression. I checked volanoMark web page and
> found the client command line has option rooms and users. rooms means how many
> chat room will be started. users means how many users are in 1 room. The default
> rooms is 10 and users is 20, so every room has about 800 threads.
Sorry. every room has 80 threads.

>  As all threads of a
> room just communicate within this room, so the rooms number is important.
> 
(Continue reading)

adobriyan | 20 Aug 16:32

Re: VolanoMark regression with 2.6.27-rc1

On Wed, Aug 20, 2008 at 03:32:17PM +0200, Peter Zijlstra wrote:
> On Wed, 2008-08-20 at 12:51 +0200, Ingo Molnar wrote:
> > * Peter Zijlstra <a.p.zijlstra <at> chello.nl> wrote:
> > 
> > > And I must admit to having overlooked the effect on wake_affine..
> > > 
> > > Chris, could you see the effect of this on smp group fairness?
> 
> Just realized my brainfart..
> 
> ---
> Subject: sched: load-balance bias fixes
> From: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
> Date: Wed Aug 20 15:28:51 CEST 2008
> 
> Yanmin spotted a regression with my patch that introduces LB_BIAS:
> 
>   commit 93b75217df39e6d75889cc6f8050343286aff4a5
>   Author: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
>   Date:   Fri Jun 27 13:41:33 2008 +0200
> 
> And I just spotted the brainfart - I should have replaced min/max with avg
> instead of removing it completely.

> --- linux-2.6.orig/include/linux/kernel.h
> +++ linux-2.6/include/linux/kernel.h
> @@ -367,6 +367,12 @@ static inline char *pack_hex_byte(char *
>  	(void) (&_max1 == &_max2);		\
>  	_max1 > _max2 ? _max1 : _max2; })
>  
(Continue reading)

Peter Zijlstra | 20 Aug 16:33

Re: VolanoMark regression with 2.6.27-rc1

On Wed, 2008-08-20 at 18:32 +0400, adobriyan <at> gmail.com wrote:
> On Wed, Aug 20, 2008 at 03:32:17PM +0200, Peter Zijlstra wrote:
> > On Wed, 2008-08-20 at 12:51 +0200, Ingo Molnar wrote:
> > > * Peter Zijlstra <a.p.zijlstra <at> chello.nl> wrote:
> > > 
> > > > And I must admit to having overlooked the effect on wake_affine..
> > > > 
> > > > Chris, could you see the effect of this on smp group fairness?
> > 
> > Just realized my brainfart..
> > 
> > ---
> > Subject: sched: load-balance bias fixes
> > From: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
> > Date: Wed Aug 20 15:28:51 CEST 2008
> > 
> > Yanmin spotted a regression with my patch that introduces LB_BIAS:
> > 
> >   commit 93b75217df39e6d75889cc6f8050343286aff4a5
> >   Author: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
> >   Date:   Fri Jun 27 13:41:33 2008 +0200
> > 
> > And I just spotted the brainfart - I should have replaced min/max with avg
> > instead of removing it completely.
> 
> > --- linux-2.6.orig/include/linux/kernel.h
> > +++ linux-2.6/include/linux/kernel.h
> > @@ -367,6 +367,12 @@ static inline char *pack_hex_byte(char *
> >  	(void) (&_max1 == &_max2);		\
> >  	_max1 > _max2 ? _max1 : _max2; })
(Continue reading)

Nick Piggin | 20 Aug 17:10

Re: VolanoMark regression with 2.6.27-rc1

On Thursday 21 August 2008 00:33, Peter Zijlstra wrote:
> On Wed, 2008-08-20 at 18:32 +0400, adobriyan <at> gmail.com wrote:
> > On Wed, Aug 20, 2008 at 03:32:17PM +0200, Peter Zijlstra wrote:

> > > +#define avg(x, y) ({				\
> > > +	typeof(x) _avg1 = ((x)+1)/2;		\
> > > +	typeof(x) _avg2 = ((y)+1)/2;		\
> >
> > ITYM, typeof(y)
>
> you thought right, I did mean that :-)
>
> > > +	(void) (&_avg1 == &_avg2);		\
> > > +	_avg1 + _avg2; })

I don't think this implementation of avg should go in kernel.h?

It gives an average of 1 and 1 to be 2, 3 and 3 is 4, 1 and 3 is
3 etc.

Maybe it is reasonable for very high numbers that would overflow
if added first, but it doesn't seem reasonable for a generic
averaging function.
Peter Zijlstra | 20 Aug 17:15

Re: VolanoMark regression with 2.6.27-rc1

On Thu, 2008-08-21 at 01:10 +1000, Nick Piggin wrote:
> On Thursday 21 August 2008 00:33, Peter Zijlstra wrote:
> > On Wed, 2008-08-20 at 18:32 +0400, adobriyan <at> gmail.com wrote:
> > > On Wed, Aug 20, 2008 at 03:32:17PM +0200, Peter Zijlstra wrote:
> 
> > > > +#define avg(x, y) ({				\
> > > > +	typeof(x) _avg1 = ((x)+1)/2;		\
> > > > +	typeof(x) _avg2 = ((y)+1)/2;		\
> > >
> > > ITYM, typeof(y)
> >
> > you thought right, I did mean that :-)
> >
> > > > +	(void) (&_avg1 == &_avg2);		\
> > > > +	_avg1 + _avg2; })
> 
> I don't think this implementation of avg should go in kernel.h?
> 
> It gives an average of 1 and 1 to be 2, 3 and 3 is 4, 1 and 3 is
> 3 etc.
> 
> Maybe it is reasonable for very high numbers that would overflow
> if added first, but it doesn't seem reasonable for a generic
> averaging function.

I had it in sched.c, then moved it to kernel.h and back again, etc.. I'm
fine with wherever..

---
Subject: sched: load-balance bias fixes
(Continue reading)

Ray Lee | 20 Aug 18:29

Re: VolanoMark regression with 2.6.27-rc1

On Wed, Aug 20, 2008 at 8:10 AM, Nick Piggin <nickpiggin <at> yahoo.com.au> wrote:
> On Thursday 21 August 2008 00:33, Peter Zijlstra wrote:
>> On Wed, 2008-08-20 at 18:32 +0400, adobriyan <at> gmail.com wrote:
>> > On Wed, Aug 20, 2008 at 03:32:17PM +0200, Peter Zijlstra wrote:
>
>> > > +#define avg(x, y) ({                             \
>> > > + typeof(x) _avg1 = ((x)+1)/2;            \
>> > > + typeof(x) _avg2 = ((y)+1)/2;            \
>> >
>> > ITYM, typeof(y)
>>
>> you thought right, I did mean that :-)
>>
>> > > + (void) (&_avg1 == &_avg2);              \
>> > > + _avg1 + _avg2; })
>
> I don't think this implementation of avg should go in kernel.h?
>
> It gives an average of 1 and 1 to be 2, 3 and 3 is 4, 1 and 3 is
> 3 etc.
>
> Maybe it is reasonable for very high numbers that would overflow
> if added first, but it doesn't seem reasonable for a generic
> averaging function.

The usual way of averaging numbers that may be large is

#define avg(x, y) ({            \
       typeof(x) _x = (x);      \
       typeof(x) _y = (y);      \
(Continue reading)

Peter Zijlstra | 20 Aug 18:51

Re: VolanoMark regression with 2.6.27-rc1

On Wed, 2008-08-20 at 09:29 -0700, Ray Lee wrote:
> On Wed, Aug 20, 2008 at 8:10 AM, Nick Piggin <nickpiggin <at> yahoo.com.au> wrote:
> > On Thursday 21 August 2008 00:33, Peter Zijlstra wrote:
> >> On Wed, 2008-08-20 at 18:32 +0400, adobriyan <at> gmail.com wrote:
> >> > On Wed, Aug 20, 2008 at 03:32:17PM +0200, Peter Zijlstra wrote:
> >
> >> > > +#define avg(x, y) ({                             \
> >> > > + typeof(x) _avg1 = ((x)+1)/2;            \
> >> > > + typeof(x) _avg2 = ((y)+1)/2;            \
> >> >
> >> > ITYM, typeof(y)
> >>
> >> you thought right, I did mean that :-)
> >>
> >> > > + (void) (&_avg1 == &_avg2);              \
> >> > > + _avg1 + _avg2; })
> >
> > I don't think this implementation of avg should go in kernel.h?
> >
> > It gives an average of 1 and 1 to be 2, 3 and 3 is 4, 1 and 3 is
> > 3 etc.
> >
> > Maybe it is reasonable for very high numbers that would overflow
> > if added first, but it doesn't seem reasonable for a generic
> > averaging function.
> 
> The usual way of averaging numbers that may be large is
> 
> #define avg(x, y) ({            \
>        typeof(x) _x = (x);      \
(Continue reading)

Peter Zijlstra | 20 Aug 19:21

Re: VolanoMark regression with 2.6.27-rc1

Ok, so one last time (I hope!)..

Everybody happy with this?

---
Subject: sched: load-balance bias fixes
From: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
Date: Wed Aug 20 15:28:51 CEST 2008

Yanmin spotted a regression with my patch that introduces LB_BIAS:

  commit 93b75217df39e6d75889cc6f8050343286aff4a5
  Author: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
  Date:   Fri Jun 27 13:41:33 2008 +0200

And I just spotted the brainfart - I should have replaced min/max with avg
instead of removing it completely.

[ray-lk <at> madrabbit.org: better avg implementation]
Signed-off-by: Peter Zijlstra <a.p.zijlstra <at> chello.nl>
---
 include/linux/kernel.h |    6 ++++++
 kernel/sched.c         |   10 ++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

Index: linux-2.6/kernel/sched.c
===================================================================
--- linux-2.6.orig/kernel/sched.c
+++ linux-2.6/kernel/sched.c
@@ -2008,9 +2008,12 @@ static unsigned long source_load(int cpu
(Continue reading)

Nick Piggin | 20 Aug 19:55

Re: VolanoMark regression with 2.6.27-rc1

On Thursday 21 August 2008 03:21, Peter Zijlstra wrote:
> Ok, so one last time (I hope!)..
>
> Everybody happy with this?

> Index: linux-2.6/include/linux/kernel.h
> ===================================================================
> --- linux-2.6.orig/include/linux/kernel.h
> +++ linux-2.6/include/linux/kernel.h
> @@ -367,6 +367,12 @@ static inline char *pack_hex_byte(char *
>  	(void) (&_max1 == &_max2);		\
>  	_max1 > _max2 ? _max1 : _max2; })
>
> +#define avg(x, y) ({				\
> +	typeof(x) _avg1 = (x);			\
> +	typeof(y) _avg2 = (y);			\
> +	(void) (&_avg1 == &_avg2);		\
> +	_avg1 + (_avg2 - _avg1)/2; })

That's not going to work with unsigned types.
Ray Lee | 20 Aug 20:15

Re: VolanoMark regression with 2.6.27-rc1

On Wed, Aug 20, 2008 at 10:55 AM, Nick Piggin <nickpiggin <at> yahoo.com.au> wrote:
> On Thursday 21 August 2008 03:21, Peter Zijlstra wrote:
>> Ok, so one last time (I hope!)..
>>
>> Everybody happy with this?
>
>
>> Index: linux-2.6/include/linux/kernel.h
>> ===================================================================
>> --- linux-2.6.orig/include/linux/kernel.h
>> +++ linux-2.6/include/linux/kernel.h
>> @@ -367,6 +367,12 @@ static inline char *pack_hex_byte(char *
>>       (void) (&_max1 == &_max2);              \
>>       _max1 > _max2 ? _max1 : _max2; })
>>
>> +#define avg(x, y) ({                         \
>> +     typeof(x) _avg1 = (x);                  \
>> +     typeof(y) _avg2 = (y);                  \
>> +     (void) (&_avg1 == &_avg2);              \
>> +     _avg1 + (_avg2 - _avg1)/2; })
>
> That's not going to work with unsigned types.

Uhm, I think it works fine, even with unsigned, even where _avg2 is
smaller than _avg1. Underflow is a good thing here. And I mocked up a
little test harness and it gives the correct answers for a half dozen
sets of values I tossed at it

But maybe I'm forgetting an obscure unsigned or signed int type
widening rule, so, care to elaborate?
(Continue reading)

Peter Zijlstra | 20 Aug 22:30

Re: VolanoMark regression with 2.6.27-rc1

On Wed, 2008-08-20 at 11:15 -0700, Ray Lee wrote:
> On Wed, Aug 20, 2008 at 10:55 AM, Nick Piggin <nickpiggin <at> yahoo.com.au> wrote:
> > On Thursday 21 August 2008 03:21, Peter Zijlstra wrote:
> >> Ok, so one last time (I hope!)..
> >>
> >> Everybody happy with this?
> >
> >
> >> Index: linux-2.6/include/linux/kernel.h
> >> ===================================================================
> >> --- linux-2.6.orig/include/linux/kernel.h
> >> +++ linux-2.6/include/linux/kernel.h
> >> @@ -367,6 +367,12 @@ static inline char *pack_hex_byte(char *
> >>       (void) (&_max1 == &_max2);              \
> >>       _max1 > _max2 ? _max1 : _max2; })
> >>
> >> +#define avg(x, y) ({                         \
> >> +     typeof(x) _avg1 = (x);                  \
> >> +     typeof(y) _avg2 = (y);                  \
> >> +     (void) (&_avg1 == &_avg2);              \
> >> +     _avg1 + (_avg2 - _avg1)/2; })
> >
> > That's not going to work with unsigned types.
> 
> Uhm, I think it works fine, even with unsigned, even where _avg2 is
> smaller than _avg1. Underflow is a good thing here. And I mocked up a
> little test harness and it gives the correct answers for a half dozen
> sets of values I tossed at it
> 
> But maybe I'm forgetting an obscure unsigned or signed int type
(Continue reading)

Peter Zijlstra | 20 Aug 22:56
Favicon

Re: VolanoMark regression with 2.6.27-rc1

On Wed, 2008-08-20 at 22:30 +0200, Peter Zijlstra wrote:
> On Wed, 2008-08-20 at 11:15 -0700, Ray Lee wrote:
> > On Wed, Aug 20, 2008 at 10:55 AM, Nick Piggin <nickpiggin <at> yahoo.com.au> wrote:
> > > On Thursday 21 August 2008 03:21, Peter Zijlstra wrote:
> > >> Ok, so one last time (I hope!)..
> > >>
> > >> Everybody happy with this?
> > >
> > >
> > >> Index: linux-2.6/include/linux/kernel.h
> > >> ===================================================================
> > >> --- linux-2.6.orig/include/linux/kernel.h
> > >> +++ linux-2.6/include/linux/kernel.h
> > >> @@ -367,6 +367,12 @@ static inline char *pack_hex_byte(char *
> > >>       (void) (&_max1 == &_max2);              \
> > >>       _max1 > _max2 ? _max1 : _max2; })
> > >>
> > >> +#define avg(x, y) ({                         \
> > >> +     typeof(x) _avg1 = (x);                  \
> > >> +     typeof(y) _avg2 = (y);                  \
> > >> +     (void) (&_avg1 == &_avg2);              \
> > >> +     _avg1 + (_avg2 - _avg1)/2; })
> > >
> > > That's not going to work with unsigned types.
> > 
> > Uhm, I think it works fine, even with unsigned, even where _avg2 is
> > smaller than _avg1. Underflow is a good thing here. And I mocked up a
> > little test harness and it gives the correct answers for a half dozen
> > sets of values I tossed at it
> > 
(Continue reading)

Nick Piggin | 21 Aug 08:11

Re: VolanoMark regression with 2.6.27-rc1

On Thursday 21 August 2008 06:56, Peter Zijlstra wrote:
> On Wed, 2008-08-20 at 22:30 +0200, Peter Zijlstra wrote:

> > works for the above example, but when I make it long long, so as to
> > match the longest supported type, it goes boom again - for as of yet
> > unknown reasons.
>
> Ok, people pointed out I got my promotion rules mixed up, I casted the
> result of the division to signed, instead of ending up with a signed
> division.
>
> #define avg(x, y) ({                            \
>         typeof(x) _avg1 = (x);                  \
>         typeof(y) _avg2 = (y);                  \
>         (void) (&_avg1 == &_avg2);              \
>         (typeof(x))(_avg1 + ((long long)_avg2 - _avg1)/2); })
>
> seems to work.

Right, I guess that will work, but unfortunately the code gen on 32-bit
is a monstrosity. If you're going to cast to 64-bit anyway, we might as
well then just do the normal add rather than playing the game to avoid
overflow.

Secondly, this is operating on the fixed point scaled load numbers, so in
the case of the scheduler I wouldn't worry too much about rounding... also
in most integer operations, rounding down is less surprising than rounding
up like the last code did.

I still don't know whether it is appropriate to put it into kernel.h
(Continue reading)

Peter Zijlstra | 21 Aug 10:17
Favicon

Re: VolanoMark regression with 2.6.27-rc1

On Thu, 2008-08-21 at 16:11 +1000, Nick Piggin wrote:
> On Thursday 21 August 2008 06:56, Peter Zijlstra wrote:
> > On Wed, 2008-08-20 at 22:30 +0200, Peter Zijlstra wrote:
> 
> > > works for the above example, but when I make it long long, so as to
> > > match the longest supported type, it goes boom again - for as of yet
> > > unknown reasons.
> >
> > Ok, people pointed out I got my promotion rules mixed up, I casted the
> > result of the division to signed, instead of ending up with a signed
> > division.
> >
> > #define avg(x, y) ({                            \
> >         typeof(x) _avg1 = (x);                  \
> >         typeof(y) _avg2 = (y);                  \
> >         (void) (&_avg1 == &_avg2);              \
> >         (typeof(x))(_avg1 + ((long long)_avg2 - _avg1)/2); })
> >
> > seems to work.
> 
> Right, I guess that will work, but unfortunately the code gen on 32-bit
> is a monstrosity. If you're going to cast to 64-bit anyway, we might as
> well then just do the normal add rather than playing the game to avoid
> overflow.
> 
> Secondly, this is operating on the fixed point scaled load numbers, so in
> the case of the scheduler I wouldn't worry too much about rounding... also
> in most integer operations, rounding down is less surprising than rounding
> up like the last code did.
> 
(Continue reading)

Ingo Molnar | 21 Aug 08:15
Favicon

Re: VolanoMark regression with 2.6.27-rc1


* Peter Zijlstra <peterz <at> infradead.org> wrote:

> Ok, people pointed out I got my promotion rules mixed up, I casted the 
> result of the division to signed, instead of ending up with a signed 
> division.
> 
> #define avg(x, y) ({                            \
>         typeof(x) _avg1 = (x);                  \
>         typeof(y) _avg2 = (y);                  \
>         (void) (&_avg1 == &_avg2);              \
>         (typeof(x))(_avg1 + ((long long)_avg2 - _avg1)/2); })

ok, could you please just send a patch that is local to sched.c and then 
we can let this kernel.h change play out independently? There's too many 
iterations of this and it's better to decouple the two.

	Ingo
Ray Lee | 20 Aug 22:58

Re: VolanoMark regression with 2.6.27-rc1

On Wed, Aug 20, 2008 at 1:30 PM, Peter Zijlstra <a.p.zijlstra <at> chello.nl> wrote:
> Nick is right, try:
>
> int main(int argc, char **argv)
> {
>        unsigned int x = 7, y = 5;
>        printf("%d\n", avg(x,y));
>        return 0;
> }
>
> It fails because 5-7 = -2, which needs a signed division or sign
> extending right shift.
>
> we'd need something like:
>
> #define avg(x, y) ({                            \
>        typeof(x) _avg1 = (x);                  \
>        typeof(y) _avg2 = (y);                  \
>        (void) (&_avg1 == &_avg2);              \
>        _avg1 + (signed typeof(x))(_avg2 - _avg1)/2; })
>
> except that typeof() doesn't work that way.
>
> #define avg(x, y) ({                            \
>        typeof(x) _avg1 = (x);                  \
>        typeof(y) _avg2 = (y);                  \
>        (void) (&_avg1 == &_avg2);              \
>        _avg1 + (long)(_avg2 - _avg1)/2; })
>
> works for the above example, but when I make it long long, so as to
(Continue reading)

Peter Zijlstra | 20 Aug 23:04

Re: VolanoMark regression with 2.6.27-rc1

On Wed, 2008-08-20 at 13:58 -0700, Ray Lee wrote:
> On Wed, Aug 20, 2008 at 1:30 PM, Peter Zijlstra <a.p.zijlstra <at> chello.nl> wrote:
> > Nick is right, try:
> >
> > int main(int argc, char **argv)
> > {
> >        unsigned int x = 7, y = 5;
> >        printf("%d\n", avg(x,y));
> >        return 0;
> > }
> >
> > It fails because 5-7 = -2, which needs a signed division or sign
> > extending right shift.
> >
> > we'd need something like:
> >
> > #define avg(x, y) ({                            \
> >        typeof(x) _avg1 = (x);                  \
> >        typeof(y) _avg2 = (y);                  \
> >        (void) (&_avg1 == &_avg2);              \
> >        _avg1 + (signed typeof(x))(_avg2 - _avg1)/2; })
> >
> > except that typeof() doesn't work that way.
> >
> > #define avg(x, y) ({                            \
> >        typeof(x) _avg1 = (x);                  \
> >        typeof(y) _avg2 = (y);                  \
> >        (void) (&_avg1 == &_avg2);              \
> >        _avg1 + (long)(_avg2 - _avg1)/2; })
> >
(Continue reading)

Ingo Molnar | 21 Aug 08:12
Favicon

Re: VolanoMark regression with 2.6.27-rc1


* Peter Zijlstra <a.p.zijlstra <at> chello.nl> wrote:

> Ok, so one last time (I hope!)..
> 
> Everybody happy with this?

looks good to me. What is missing is Yanmin's confirmation that the 
patch indeed solves/improves the regression :-)

	Ingo

Gmane