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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <488E7D92.8020501@cn.fujitsu.com>
Date:	Tue, 29 Jul 2008 10:16:50 +0800
From:	Li Zefan <lizf@...fujitsu.com>
To:	Paul Jackson <pj@....com>
CC:	akpm@...ux-foundation.org, menage@...gle.com,
	seto.hidetoshi@...fujitsu.com, laijs@...fujitsu.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] cpuset: fix wrong calculation of relax domain level

Paul Jackson wrote:
> Seems ok to me:
> 
> Reviewed-by: Paul Jackson <pj@....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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/