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@xxxxxxx>
> 
> 
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/