[PATCH] sched: Enable arch-specific asym packing option in sched domain

[PATCH] sched: Enable arch-specific asym packing option in sched domain

[Diwakar Tundlam]

Add arch specific (weak) routine to set (or not set) the ASYM_PACKING
sched domain flag. This change itself does nothing, but allows archs
that require asym-packing option to set it by implementing the arch
specific routine to turn it on.
    
The weak symbol is already defined in sched_fair.c but was mis-spelled
in the header file. Fixed spelling error in the weak symbol definition.
    
Change-Id: Ibdf38e0a40f76ee3c1829f08b5feedcf900a1b89
Signed-off-by: Diwakar Tundlam <dtundlam@nvidia.com>
---
 include/linux/sched.h    |    2 +-
 include/linux/topology.h |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index c569719..cf793f0 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -903,7 +903,7 @@ static inline int sd_balance_for_package_power(void)
        return SD_PREFER_SIBLING;
 }
 
-extern int __weak arch_sd_sibiling_asym_packing(void);
+extern int __weak arch_sd_sibling_asym_packing(void);
 
 /*
  * Optimise SD flags for power savings:
diff --git a/include/linux/topology.h b/include/linux/topology.h
index b480403..eb09cd4 100644
--- a/include/linux/topology.h
+++ b/include/linux/topology.h
@@ -168,6 +168,7 @@ int arch_update_cpu_topology(void);
                                | 0*SD_SHARE_CPUPOWER                   \
                                | 0*SD_SHARE_PKG_RESOURCES              \
                                | 0*SD_SERIALIZE                        \
+                               | arch_sd_sibling_asym_packing()        \
                                | sd_balance_for_package_power()        \
                                | sd_power_saving_flags()               \
                                ,                                       \

Re: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Peter Zijlstra]

On Fri, 2012-05-04 at 13:28 -0700, Diwakar Tundlam wrote:
> Add arch specific (weak) routine to set (or not set) the ASYM_PACKING
> sched domain flag. This change itself does nothing, but allows archs
> that require asym-packing option to set it by implementing the arch
> specific routine to turn it on.
>     
> The weak symbol is already defined in sched_fair.c but was mis-spelled
> in the header file. Fixed spelling error in the weak symbol definition.
>     
> Change-Id: Ibdf38e0a40f76ee3c1829f08b5feedcf900a1b89

This does not go in a patch..

> Signed-off-by: Diwakar Tundlam <dtundlam@nvidia.com>
> ---
>  include/linux/sched.h    |    2 +-
>  include/linux/topology.h |    1 +
>  2 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index c569719..cf793f0 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -903,7 +903,7 @@ static inline int sd_balance_for_package_power(void)
>         return SD_PREFER_SIBLING;
>  }
>  
> -extern int __weak arch_sd_sibiling_asym_packing(void);
> +extern int __weak arch_sd_sibling_asym_packing(void);
>  
>  /*
>   * Optimise SD flags for power savings:
> diff --git a/include/linux/topology.h b/include/linux/topology.h
> index b480403..eb09cd4 100644
> --- a/include/linux/topology.h
> +++ b/include/linux/topology.h
> @@ -168,6 +168,7 @@ int arch_update_cpu_topology(void);
>                                 | 0*SD_SHARE_CPUPOWER                   \
>                                 | 0*SD_SHARE_PKG_RESOURCES              \
>                                 | 0*SD_SERIALIZE                        \
> +                               | arch_sd_sibling_asym_packing()        \
>                                 | sd_balance_for_package_power()        \
>                                 | sd_power_saving_flags()               \
>                                 ,                                       \


I think you just wrecked Power7 here..

(also your patch seems white-space challenged)

RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Diwakar Tundlam]

Add arch specific (weak) routine to set (or not set) the ASYM_PACKING
sched domain flag. This change itself does nothing, but allows archs
that require asym-packing option to set it by implementing the arch
specific routine to turn it on.

The weak symbol is already defined in sched/core.c but was mis-spelled
in the header file. Fixed spelling error in the weak symbol definition.

Signed-off-by: Diwakar Tundlam <dtundlam@nvidia.com>

diff --git a/include/linux/sched.h b/include/linux/sched.h
index c569719..cf793f0 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -903,7 +903,7 @@ static inline int sd_balance_for_package_power(void)
 	return SD_PREFER_SIBLING;
 }
 
-extern int __weak arch_sd_sibiling_asym_packing(void);
+extern int __weak arch_sd_sibling_asym_packing(void);
 
 /*
  * Optimise SD flags for power savings:
diff --git a/include/linux/topology.h b/include/linux/topology.h
index b480403..eb09cd4 100644
--- a/include/linux/topology.h
+++ b/include/linux/topology.h
@@ -168,6 +168,7 @@ int arch_update_cpu_topology(void);
 				| 0*SD_SHARE_CPUPOWER			\
 				| 0*SD_SHARE_PKG_RESOURCES		\
 				| 0*SD_SERIALIZE			\
+				| arch_sd_sibling_asym_packing()	\
 				| sd_balance_for_package_power()	\
 				| sd_power_saving_flags()		\
 				,					\

RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Peter Zijlstra]

On Fri, 2012-05-04 at 15:06 -0700, Diwakar Tundlam wrote:
> +                               | arch_sd_sibling_asym_packing()        \
>                                 | sd_balance_for_package_power()        \
>                                 | sd_power_saving_flags()               \
>                                 ,                                       \ 

A repost doesn't make it right to add sibling (SMT) properties to a
package/socket and will still upset Power7.

RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Diwakar Tundlam]

>> A repost doesn't make it right to add sibling (SMT) properties to a package/socket and will still upset Power7.

Agreed. I only fixed the whitespace and commitId.

arch_sd_sibling_asym_packing() is already present under ifdef CONFIG_SMT.
I didn't touch that. I only added it to SD_CPU_INIT for all cpu's.
I assumed Power7 shouldn't use SD_CPU_INIT.

I don't understand Power7 arch to comment on impact.
Michael Neuling should review this carefully and advise.

Maybe I should define a separate weak symbol, say arch_sd_bias_to_lower_num_cpu()?
Then Power7 can define arch_sd_sibling_asym_packing() to be '1' and it will not
break all-cpu init.

We need this for Tegra's slight asymmetry with core0.

Thanks,
--Diwakar.

-----Original Message-----
From: Peter Zijlstra [mailto:a.p.zijlstra@chello.nl] 
Sent: Friday, May 04, 2012 3:10 PM
To: Diwakar Tundlam
Cc: 'Ingo Molnar'; 'Andrew Morton'; 'Christoph Lameter'; 'Michael Neuling'; 'Stephen Rothwell'; 'Benjamin Herrenschmidt'; 'David Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
Subject: RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain

On Fri, 2012-05-04 at 15:06 -0700, Diwakar Tundlam wrote:
> +                               | arch_sd_sibling_asym_packing()        \
>                                 | sd_balance_for_package_power()        \
>                                 | sd_power_saving_flags()               \
>                                 ,                                       \ 

A repost doesn't make it right to add sibling (SMT) properties to a package/socket and will still upset Power7.

RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Peter Zijlstra]

On Fri, 2012-05-04 at 15:18 -0700, Diwakar Tundlam wrote:
> 
> arch_sd_sibling_asym_packing() is already present under ifdef CONFIG_SMT.
> I didn't touch that. I only added it to SD_CPU_INIT for all cpu's.
> I assumed Power7 shouldn't use SD_CPU_INIT.

All archs use SD_CPU_INIT, its the default topology level for a
package/socket. So now you've made Power7 prefer lower numbered sockets
over higher numbered sockets.. not fatal, but not really nice either.

[ power7 is the only one that implements arch_sd_sibling_asym_packing ]

> Maybe I should define a separate weak symbol, say arch_sd_bias_to_lower_num_cpu()?
> Then Power7 can define arch_sd_sibling_asym_packing() to be '1' and it will not
> break all-cpu init.

A slightly saner name would be:

  arch_sd_package_asym_packing()

FWIW, I suspect you're wanting to use this for some ARM chip (nvidia
doesn't do much else -- aside from this graphics stuff) so that if
there's hardly anything it runs on cpu0. Now, last time I checked, these
ARM things had only 1 package, so I still don't see the point :-)

I suspect you want to modify SD_MC_INIT() with something like:

  arch_sd_mc_asym_packing()

Or is this the big-little thing and you're representing them as separate
packages?

See how a little extra information avoids me having to endlessly second
guess wtf you're actually wanting to do?

RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Diwakar Tundlam]

Thanks for clarifying the Power7. I see the point - package vs cpu's within a package.

Yes, this is for Nvidia's ARM Quad-core Tegra CPU. It is a single package, organized flatly.
To save power, we want cores to be loaded up in order from cpu0, 1, etc.
The ASYM_PACKING flag seems to do exactly what we need if set in the domain flag.

Thanks,
--Diwakar.

-----Original Message-----
From: Peter Zijlstra [mailto:a.p.zijlstra@chello.nl] 
Sent: Friday, May 04, 2012 3:30 PM
To: Diwakar Tundlam
Cc: 'Ingo Molnar'; 'Andrew Morton'; 'Christoph Lameter'; 'Michael Neuling'; 'Stephen Rothwell'; 'Benjamin Herrenschmidt'; 'David Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
Subject: RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain

On Fri, 2012-05-04 at 15:18 -0700, Diwakar Tundlam wrote:
> 
> arch_sd_sibling_asym_packing() is already present under ifdef CONFIG_SMT.
> I didn't touch that. I only added it to SD_CPU_INIT for all cpu's.
> I assumed Power7 shouldn't use SD_CPU_INIT.

All archs use SD_CPU_INIT, its the default topology level for a package/socket. So now you've made Power7 prefer lower numbered sockets over higher numbered sockets.. not fatal, but not really nice either.

[ power7 is the only one that implements arch_sd_sibling_asym_packing ]

> Maybe I should define a separate weak symbol, say arch_sd_bias_to_lower_num_cpu()?
> Then Power7 can define arch_sd_sibling_asym_packing() to be '1' and it 
> will not break all-cpu init.

A slightly saner name would be:

  arch_sd_package_asym_packing()

FWIW, I suspect you're wanting to use this for some ARM chip (nvidia doesn't do much else -- aside from this graphics stuff) so that if there's hardly anything it runs on cpu0. Now, last time I checked, these ARM things had only 1 package, so I still don't see the point :-)

I suspect you want to modify SD_MC_INIT() with something like:

  arch_sd_mc_asym_packing()

Or is this the big-little thing and you're representing them as separate packages?

See how a little extra information avoids me having to endlessly second guess wtf you're actually wanting to do?

Re: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Michael Neuling]

> Thanks for clarifying the Power7. I see the point - package vs cpu's within a package.
> 
> Yes, this is for Nvidia's ARM Quad-core Tegra CPU. It is a single
> package, organized flatly.  To save power, we want cores to be loaded
> up in order from cpu0, 1, etc.  The ASYM_PACKING flag seems to do
> exactly what we need if set in the domain flag.

Arrh, so it's not a performance issue but power savings.. that would
this asymmetric packing a lot more appropriate to use.

Mikey

> 
> Thanks,
> --Diwakar.
> 
> -----Original Message-----
> From: Peter Zijlstra [mailto:a.p.zijlstra@chello.nl] 
> Sent: Friday, May 04, 2012 3:30 PM
> To: Diwakar Tundlam
> Cc: 'Ingo Molnar'; 'Andrew Morton'; 'Christoph Lameter'; 'Michael Neuling'; 'Stephen Rothwell'; 'Benjamin Herrenschmidt'; 'David Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
> Subject: RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain
> 
> On Fri, 2012-05-04 at 15:18 -0700, Diwakar Tundlam wrote:
> > 
> > arch_sd_sibling_asym_packing() is already present under ifdef CONFIG_SMT.
> > I didn't touch that. I only added it to SD_CPU_INIT for all cpu's.
> > I assumed Power7 shouldn't use SD_CPU_INIT.
> 
> All archs use SD_CPU_INIT, its the default topology level for a package/socket. So now you've made Power7 prefer lower numbered sockets over higher numbered sockets.. not fatal, but not really nice either.
> 
> [ power7 is the only one that implements arch_sd_sibling_asym_packing ]
> 
> > Maybe I should define a separate weak symbol, say arch_sd_bias_to_lower_num_cpu()?
> > Then Power7 can define arch_sd_sibling_asym_packing() to be '1' and it 
> > will not break all-cpu init.
> 
> A slightly saner name would be:
> 
>   arch_sd_package_asym_packing()
> 
> FWIW, I suspect you're wanting to use this for some ARM chip (nvidia doesn't do much else -- aside from this graphics stuff) so that if there's hardly anything it runs on cpu0. Now, last time I checked, these ARM things had only 1 package, so I still don't see the point :-)
> 
> I suspect you want to modify SD_MC_INIT() with something like:
> 
>   arch_sd_mc_asym_packing()
> 
> Or is this the big-little thing and you're representing them as separate packages?
> 
> See how a little extra information avoids me having to endlessly second guess wtf you're actually wanting to do?
> 

RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Diwakar Tundlam]

>> Arrh, so it's not a performance issue but power savings.. that would this asymmetric packing a lot more appropriate to use.

Yes, it is for power saving..

Could you suggest a way t use this flag without affecting Power7, then?
I was thinking to define a different weak arch_sd_package_asym_packing() in SD_CPU_INIT.
And the flag is set only for arch/arm/mach-tegra/tegra3.

Thanks,
--Diwakar.

-----Original Message-----
From: Michael Neuling [mailto:mikey@neuling.org] 
Sent: Friday, May 04, 2012 4:21 PM
To: Diwakar Tundlam
Cc: 'Peter Zijlstra'; 'Ingo Molnar'; 'Andrew Morton'; 'Christoph Lameter'; 'Stephen Rothwell'; 'Benjamin Herrenschmidt'; 'David Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
Subject: Re: [PATCH] sched: Enable arch-specific asym packing option in sched domain

> Thanks for clarifying the Power7. I see the point - package vs cpu's within a package.
> 
> Yes, this is for Nvidia's ARM Quad-core Tegra CPU. It is a single 
> package, organized flatly.  To save power, we want cores to be loaded 
> up in order from cpu0, 1, etc.  The ASYM_PACKING flag seems to do 
> exactly what we need if set in the domain flag.

Arrh, so it's not a performance issue but power savings.. that would this asymmetric packing a lot more appropriate to use.

Mikey

> 
> Thanks,
> --Diwakar.
> 
> -----Original Message-----
> From: Peter Zijlstra [mailto:a.p.zijlstra@chello.nl]
> Sent: Friday, May 04, 2012 3:30 PM
> To: Diwakar Tundlam
> Cc: 'Ingo Molnar'; 'Andrew Morton'; 'Christoph Lameter'; 'Michael 
> Neuling'; 'Stephen Rothwell'; 'Benjamin Herrenschmidt'; 'David 
> Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
> Subject: RE: [PATCH] sched: Enable arch-specific asym packing option 
> in sched domain
> 
> On Fri, 2012-05-04 at 15:18 -0700, Diwakar Tundlam wrote:
> > 
> > arch_sd_sibling_asym_packing() is already present under ifdef CONFIG_SMT.
> > I didn't touch that. I only added it to SD_CPU_INIT for all cpu's.
> > I assumed Power7 shouldn't use SD_CPU_INIT.
> 
> All archs use SD_CPU_INIT, its the default topology level for a package/socket. So now you've made Power7 prefer lower numbered sockets over higher numbered sockets.. not fatal, but not really nice either.
> 
> [ power7 is the only one that implements arch_sd_sibling_asym_packing 
> ]
> 
> > Maybe I should define a separate weak symbol, say arch_sd_bias_to_lower_num_cpu()?
> > Then Power7 can define arch_sd_sibling_asym_packing() to be '1' and 
> > it will not break all-cpu init.
> 
> A slightly saner name would be:
> 
>   arch_sd_package_asym_packing()
> 
> FWIW, I suspect you're wanting to use this for some ARM chip (nvidia 
> doesn't do much else -- aside from this graphics stuff) so that if 
> there's hardly anything it runs on cpu0. Now, last time I checked, 
> these ARM things had only 1 package, so I still don't see the point 
> :-)
> 
> I suspect you want to modify SD_MC_INIT() with something like:
> 
>   arch_sd_mc_asym_packing()
> 
> Or is this the big-little thing and you're representing them as separate packages?
> 
> See how a little extra information avoids me having to endlessly second guess wtf you're actually wanting to do?
> 

Re: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Michael Neuling]

Diwakar Tundlam <dtundlam@nvidia.com> wrote:

> >> Arrh, so it's not a performance issue but power savings.. that would this asymmetric packing a lot more appropriate to use.
> 
> Yes, it is for power saving..
> 
> Could you suggest a way t use this flag without affecting Power7, then?
> I was thinking to define a different weak arch_sd_package_asym_packing() in SD_CPU_INIT.
> And the flag is set only for arch/arm/mach-tegra/tegra3.

That would work but Peter knows much better than me.  

Mikey

> 
> Thanks,
> --Diwakar.
> 
> -----Original Message-----
> From: Michael Neuling [mailto:mikey@neuling.org] 
> Sent: Friday, May 04, 2012 4:21 PM
> To: Diwakar Tundlam
> Cc: 'Peter Zijlstra'; 'Ingo Molnar'; 'Andrew Morton'; 'Christoph Lameter'; 'Stephen Rothwell'; 'Benjamin Herrenschmidt'; 'David Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
> Subject: Re: [PATCH] sched: Enable arch-specific asym packing option in sched domain
> 
> > Thanks for clarifying the Power7. I see the point - package vs cpu's within a package.
> > 
> > Yes, this is for Nvidia's ARM Quad-core Tegra CPU. It is a single 
> > package, organized flatly.  To save power, we want cores to be loaded 
> > up in order from cpu0, 1, etc.  The ASYM_PACKING flag seems to do 
> > exactly what we need if set in the domain flag.
> 
> Arrh, so it's not a performance issue but power savings.. that would this asymmetric packing a lot more appropriate to use.
> 
> Mikey
> 
> > 
> > Thanks,
> > --Diwakar.
> > 
> > -----Original Message-----
> > From: Peter Zijlstra [mailto:a.p.zijlstra@chello.nl]
> > Sent: Friday, May 04, 2012 3:30 PM
> > To: Diwakar Tundlam
> > Cc: 'Ingo Molnar'; 'Andrew Morton'; 'Christoph Lameter'; 'Michael 
> > Neuling'; 'Stephen Rothwell'; 'Benjamin Herrenschmidt'; 'David 
> > Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
> > Subject: RE: [PATCH] sched: Enable arch-specific asym packing option 
> > in sched domain
> > 
> > On Fri, 2012-05-04 at 15:18 -0700, Diwakar Tundlam wrote:
> > > 
> > > arch_sd_sibling_asym_packing() is already present under ifdef CONFIG_SMT.
> > > I didn't touch that. I only added it to SD_CPU_INIT for all cpu's.
> > > I assumed Power7 shouldn't use SD_CPU_INIT.
> > 
> > All archs use SD_CPU_INIT, its the default topology level for a package/socket. So now you've made Power7 prefer lower numbered sockets over higher numbered sockets.. not fatal, but not really nice either.
> > 
> > [ power7 is the only one that implements arch_sd_sibling_asym_packing 
> > ]
> > 
> > > Maybe I should define a separate weak symbol, say arch_sd_bias_to_lower_num_cpu()?
> > > Then Power7 can define arch_sd_sibling_asym_packing() to be '1' and 
> > > it will not break all-cpu init.
> > 
> > A slightly saner name would be:
> > 
> >   arch_sd_package_asym_packing()
> > 
> > FWIW, I suspect you're wanting to use this for some ARM chip (nvidia 
> > doesn't do much else -- aside from this graphics stuff) so that if 
> > there's hardly anything it runs on cpu0. Now, last time I checked, 
> > these ARM things had only 1 package, so I still don't see the point 
> > :-)
> > 
> > I suspect you want to modify SD_MC_INIT() with something like:
> > 
> >   arch_sd_mc_asym_packing()
> > 
> > Or is this the big-little thing and you're representing them as separate packages?
> > 
> > See how a little extra information avoids me having to endlessly second guess wtf you're actually wanting to do?
> > 
> 

Re: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Michael Wang]

On 05/05/2012 07:25 AM, Diwakar Tundlam wrote:

>>> Arrh, so it's not a performance issue but power savings.. that would this asymmetric packing a lot more appropriate to use.
> 
> Yes, it is for power saving..
> 
> Could you suggest a way t use this flag without affecting Power7, then?
> I was thinking to define a different weak arch_sd_package_asym_packing() in SD_CPU_INIT.
> And the flag is set only for arch/arm/mach-tegra/tegra3.
> 


SD_ASYM_PACKING will help to change the idle processor from higher id to
lower, so if a lower id cpu thread is going to idle, we will move all
the tasks of it's higher id sibling to him.

I don't know how can this way cause power saving? Do you mean on this
tegra3 arch, higher id smt will use more power than lower one?

And could the SD_POWERSAVINGS_BALANCE do some help in your case?

Regards,
Michael Wang

> Thanks,
> --Diwakar.
> 
> -----Original Message-----
> From: Michael Neuling [mailto:mikey@neuling.org] 
> Sent: Friday, May 04, 2012 4:21 PM
> To: Diwakar Tundlam
> Cc: 'Peter Zijlstra'; 'Ingo Molnar'; 'Andrew Morton'; 'Christoph Lameter'; 'Stephen Rothwell'; 'Benjamin Herrenschmidt'; 'David Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
> Subject: Re: [PATCH] sched: Enable arch-specific asym packing option in sched domain
> 
>> Thanks for clarifying the Power7. I see the point - package vs cpu's within a package.
>>
>> Yes, this is for Nvidia's ARM Quad-core Tegra CPU. It is a single 
>> package, organized flatly.  To save power, we want cores to be loaded 
>> up in order from cpu0, 1, etc.  The ASYM_PACKING flag seems to do 
>> exactly what we need if set in the domain flag.
> 
> Arrh, so it's not a performance issue but power savings.. that would this asymmetric packing a lot more appropriate to use.
> 
> Mikey
> 
>>
>> Thanks,
>> --Diwakar.
>>
>> -----Original Message-----
>> From: Peter Zijlstra [mailto:a.p.zijlstra@chello.nl]
>> Sent: Friday, May 04, 2012 3:30 PM
>> To: Diwakar Tundlam
>> Cc: 'Ingo Molnar'; 'Andrew Morton'; 'Christoph Lameter'; 'Michael 
>> Neuling'; 'Stephen Rothwell'; 'Benjamin Herrenschmidt'; 'David 
>> Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
>> Subject: RE: [PATCH] sched: Enable arch-specific asym packing option 
>> in sched domain
>>
>> On Fri, 2012-05-04 at 15:18 -0700, Diwakar Tundlam wrote:
>>>
>>> arch_sd_sibling_asym_packing() is already present under ifdef CONFIG_SMT.
>>> I didn't touch that. I only added it to SD_CPU_INIT for all cpu's.
>>> I assumed Power7 shouldn't use SD_CPU_INIT.
>>
>> All archs use SD_CPU_INIT, its the default topology level for a package/socket. So now you've made Power7 prefer lower numbered sockets over higher numbered sockets.. not fatal, but not really nice either.
>>
>> [ power7 is the only one that implements arch_sd_sibling_asym_packing 
>> ]
>>
>>> Maybe I should define a separate weak symbol, say arch_sd_bias_to_lower_num_cpu()?
>>> Then Power7 can define arch_sd_sibling_asym_packing() to be '1' and 
>>> it will not break all-cpu init.
>>
>> A slightly saner name would be:
>>
>>   arch_sd_package_asym_packing()
>>
>> FWIW, I suspect you're wanting to use this for some ARM chip (nvidia 
>> doesn't do much else -- aside from this graphics stuff) so that if 
>> there's hardly anything it runs on cpu0. Now, last time I checked, 
>> these ARM things had only 1 package, so I still don't see the point 
>> :-)
>>
>> I suspect you want to modify SD_MC_INIT() with something like:
>>
>>   arch_sd_mc_asym_packing()
>>
>> Or is this the big-little thing and you're representing them as separate packages?
>>
>> See how a little extra information avoids me having to endlessly second guess wtf you're actually wanting to do?
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Suresh Siddha]

On Fri, 2012-05-04 at 16:18 -0700, Diwakar Tundlam wrote:
> Yes, this is for Nvidia's ARM Quad-core Tegra CPU. It is a single package, organized flatly.
> To save power, we want cores to be loaded up in order from cpu0, 1, etc.
> The ASYM_PACKING flag seems to do exactly what we need if set in the domain flag.

Diwakar, But ASYM_PACKING will lead to more task migrations and also it
relies on the periodic load balancing to correct it.

Perhaps we should expose this info more natively and prefer the lower
siblings during task wakeup etc. And perhaps be more aggressive in
moving the load to lower siblings etc.

Can you elaborate more about the power-saving scenario and what causes
the power-savings etc to better understand the problem.

thanks,
suresh

RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Peter Zijlstra]

On Fri, 2012-05-04 at 16:18 -0700, Diwakar Tundlam wrote:
> 
> Yes, this is for Nvidia's ARM Quad-core Tegra CPU. It is a single package, organized flatly.

Then you don't want to poke at SD_CPU_INIT since thats across packages.

RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Diwakar Tundlam]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 909 bytes --]

>> >> Yes, this is for Nvidia's ARM Quad-core Tegra CPU. It is a single package, organized flatly.
>> Then you don't want to poke at SD_CPU_INIT since thats across packages.
Yes, I see that now.

I will rework the patch with guidance from other experts.

Thanks,
--Diwakar.

-----Original Message-----
From: Peter Zijlstra [mailto:a.p.zijlstra@chello.nl] 
Sent: Saturday, May 05, 2012 1:35 PM
To: Diwakar Tundlam
Cc: 'Ingo Molnar'; 'Andrew Morton'; 'Christoph Lameter'; 'Michael Neuling'; 'Stephen Rothwell'; 'Benjamin Herrenschmidt'; 'David Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
Subject: RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain

On Fri, 2012-05-04 at 16:18 -0700, Diwakar Tundlam wrote:

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Peter Zijlstra]

On Mon, 2012-05-07 at 10:25 -0700, Diwakar Tundlam wrote:
> I will rework the patch with guidance from other experts.

But please also expand on the actual hardware and its needs. I still
don't understand why you want the CPU domain and not the MC domain. So
explain the physical topology, the way the arm architecture code maps
this to linux topology and how asym_packing helps conserve power.

Re: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Michael Neuling]

Diwakar Tundlam <dtundlam@nvidia.com> wrote:

> >> A repost doesn't make it right to add sibling (SMT) properties to a package/socket and will still upset Power7.
> 
> Agreed. I only fixed the whitespace and commitId.
> 
> arch_sd_sibling_asym_packing() is already present under ifdef CONFIG_SMT.
> I didn't touch that. I only added it to SD_CPU_INIT for all cpu's.
> I assumed Power7 shouldn't use SD_CPU_INIT.

You are using sibling at the cpu level.  POWER7 sets this at the sibling
level and we don't really want it at the cpu level.

> I don't understand Power7 arch to comment on impact.
> Michael Neuling should review this carefully and advise.

Ask Peter said, it's broken for POWER7.  We don't want to set this at
the cpu level.

> Maybe I should define a separate weak symbol, say
> arch_sd_bias_to_lower_num_cpu()?  Then Power7 can define
> arch_sd_sibling_asym_packing() to be '1' and it will not break all-cpu
> init.

This sounds better but I'd follow the old name call it
arch_sd_cpu_asym_packing()

Mikey

Re: [PATCH] sched: Enable arch-specific asym packing option in sched domain

[Michael Neuling]

> We need this for Tegra's slight asymmetry with core0.

This would seem to be to be a different problem.  I don't think the
packing mechanism is what you want.

Shouldn't you be increasing the CPU power of this core, so that all
tasks get a fair go on this core?  Otherwise you're breaking the
fundamental concept of a completely fair scheduler.

Using this packing mechanism you won't get this.  Any task that lands on
core 0 will get an unfair amount of computation power, compared to tasks
that landed on core 1.

Mikey