[PATCH 3/3] cpuset: fix wrong calculation of relax domain level

[PATCH 3/3] cpuset: fix wrong calculation of relax domain level

[Li Zefan]

When multiple cpusets are overlapping in their 'cpus' and hence they
form a single sched domain, the largest sched_relax_domain_level among
those should be used. But when top_cpuset's sched_load_balance is
set, its sched_relax_domain_level is used regardless other sub-cpusets'.

This patch fixes it by walking the cpuset hierarchy to find the largest
sched_relax_domain_level.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/cpuset.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index d274a94..b1f1fa1 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -614,7 +614,7 @@ void rebuild_sched_domains(void)
 		dattr = kmalloc(sizeof(struct sched_domain_attr), GFP_KERNEL);
 		if (dattr) {
 			*dattr = SD_ATTR_INIT;
-			update_domain_attr(dattr, &top_cpuset);
+			update_domain_attr_tree(dattr, &top_cpuset);
 		}
 		*doms = top_cpuset.cpus_allowed;
 		goto rebuild;
-- 
1.5.4.rc3

Re: [PATCH 3/3] cpuset: fix wrong calculation of relax domain level

[Paul Jackson]

Seems ok to me:

Reviewed-by: Paul Jackson <pj@sgi.com>


Li Zefan wrote:
> -			update_domain_attr(dattr, &top_cpuset);
> +			update_domain_attr_tree(dattr, &top_cpuset);

Does this change mean that there is now only -one- place that calls
"update_domain_attr()", that being "update_domain_attr_tree()" ?

If so, then perhaps:
 1) "update_domain_attr()" could be removed as a separate routine,
    with its code folded into "update_domain_attr_tree()".
 2) a proper opening comment could be provided "update_domain_attr()",
    stating what it does, and its locking needs.

The above, if it makes sense, would be an additional PATCH, on top
of your present patches, further refining them.


Separate topic ...

It bothers me a little that there is a generic 'attributes' and related
*_attr() and dattr variable names, all speaking of some set of multiple
generic attributes, such as in:

        struct sched_domain_attr *dattr;  /* attributes for custom domains */

even though, when all is said and done, there is only one attribute,
the relax_domain_level.  The generic, content-free word 'attributes'
just obfuscates the single specific value, relax_domain_level, being
managed here.

... However, I'm too lazy to propose a patch to mess with this.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.940.382.4214

Re: [PATCH 3/3] cpuset: fix wrong calculation of relax domain level

[Li Zefan]

Paul Jackson wrote:
> Seems ok to me:
> 
> Reviewed-by: Paul Jackson <pj@sgi.com>
> 
> 
> Li Zefan wrote:
>> -			update_domain_attr(dattr, &top_cpuset);
>> +			update_domain_attr_tree(dattr, &top_cpuset);
> 
> Does this change mean that there is now only -one- place that calls
> "update_domain_attr()", that being "update_domain_attr_tree()" ?
> 
> If so, then perhaps:
>  1) "update_domain_attr()" could be removed as a separate routine,
>     with its code folded into "update_domain_attr_tree()".

It will be folded into update_domain_attr_tree() by gcc.

>  2) a proper opening comment could be provided "update_domain_attr()",
>     stating what it does, and its locking needs.
> 

I think update_domain_attr_tree() rather than update_domain_attr() needs
a comment to state what is does, but as it is a helper function for
rebuild_sched_domains(), I don't think we need to state its locking needs.

> The above, if it makes sense, would be an additional PATCH, on top
> of your present patches, further refining them.
> 
> 
> Separate topic ...
> 
> It bothers me a little that there is a generic 'attributes' and related
> *_attr() and dattr variable names, all speaking of some set of multiple
> generic attributes, such as in:
> 
>         struct sched_domain_attr *dattr;  /* attributes for custom domains */
> 
> even though, when all is said and done, there is only one attribute,
> the relax_domain_level.  The generic, content-free word 'attributes'
> just obfuscates the single specific value, relax_domain_level, being
> managed here.
> 
> ... However, I'm too lazy to propose a patch to mess with this.
> 

But it doesn't bother me. ;)

IMO it's not good to mess things up by sending a patch to just rename all
the sched_domain_attr to relax_domain_level without doing anything other
useful work.

Re: [PATCH 3/3] cpuset: fix wrong calculation of relax domain level

[Paul Jackson]

Li Zefan, replying to pj:
> > ... However, I'm too lazy to propose a patch to mess with this.
> > 
> 
> But it doesn't bother me. ;)
> 
> IMO it's not good to mess things up by sending a patch to just rename all

Agreed - guess that 'attr' stuff will remain as it be for now.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.940.382.4214

Re: [PATCH 3/3] cpuset: fix wrong calculation of relax domain level

[Paul Jackson]

Li Zefan wrote:
> > If so, then perhaps:
> >  1) "update_domain_attr()" could be removed as a separate routine,
> >     with its code folded into "update_domain_attr_tree()".
> 
> It will be folded into update_domain_attr_tree() by gcc.

My (mild) preference for folding "update_domain_attr()" into
"update_domain_attr_tree()" was not to save runtime CPU cycles.
This is not a hot code path.  It was to improve code readability.

Do what you think is best here ... I'm easy.

> I think update_domain_attr_tree() rather than update_domain_attr() needs
> a comment to state what is does, but as it is a helper function for
> rebuild_sched_domains(), I don't think we need to state its locking needs.

Then perhaps one could include in the comment for
"update_domain_attr_tree()" that it is a helper function
for rebuild_sched_domains(), where its locking is described.

--
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.940.382.4214