cpuset: fix wrong calculation of relax domain level
PATCH 3/3 cpuset: fix wrong calculation of relax domain level
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.
--- kernel/cpuset.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/cpuset.c +1 -1
@@ -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;
Signature · 1 line
1.5.4.rc3
Seems ok to me:
kernel/cpuset.c void rebuild_sched_domains(void) · line 617
Quoted 2 lines · Li Zefan
kernel/cpuset.c +1
-1-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.
Signature · 3 lines
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.940.382.4214Quoted 12 lines · Paul Jackson
Seems ok to me:
Reviewed-by Paul Jackson <[email protected]>kernel/cpuset.c void rebuild_sched_domains(void) · line 617
Quoted 2 lines · Li Zefan
kernel/cpuset.c +1
-1-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()" ?
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 10 lines
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:
Quoted 9 lines
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.
Li Zefan, replying to pj:
Quoted 6 lines
Quoted 2 lines
... 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.
Signature · 3 lines
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.940.382.4214Quoted 5 lines · Li Zefan
Quoted 3 lines
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.
Quoted 3 lines
I think
update_domain_attr_tree()rather thanupdate_domain_attr()needs a comment to state what is does, but as it is a helper function forrebuild_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 <[email protected]> 1.940.382.4214