http://defect.opensolaris.org/bz/show_bug.cgi?id=6446
Summary: Implement code review feedback for Jonathan Chew
Classification: Development
Product: power-mgmt
Version: unspecified
Platform: Other
OS/Version: Solaris
Status: NEW
Severity: minor
Priority: P3
Component: PAD
AssignedTo: eric.saxe at sun.com
ReportedBy: eric.saxe at sun.com
CC: tesla-dev at opensolaris.org
usr/src/uts/common/disp/cmt.c
* Comments mention optimizing for affinity....Do we do anything for that
now?
- pg_cmt_hier_rank()
* Change comment to "Ask the platform which hardware sharing relationship
is more important to optimize"
* Comment about a "power domain is only important if CPUPM is enabled"
should be moved up higher or put in both the then and else clauses
- cmt_hier_promote()
* Comment says that this promotes given PG up one level....Does this
promote it up two levels since it appears to promote it above its parent?
Maybe it should just say that it promotes the given PG above its parent to be
clear and not talk about levels?
* Doesn't the grandparent's children need to change to include the given
PG or did I miss that in the parents, children, and sibling shuffling?
- pg_cmt_cpu_init()
* Why does the code to promote PGs need to start at the top of the PG
hierarchy after a PG is promoted?
- pg_cmt_hier_sort()
* This code is a little hard to understand
* Is "sz" the number of levels in the given PG CMT hierarchy? Maybe give
it a better name?
- cmt_pad_{en,dis}able()
* I don't understand this routine, so I guess that more comments woulbe
good....
* How does promoting PGs enable or disable PAD?
* Can these routines operate on anything other than an active power
domain? If not, maybe the assertion should be different?
usr/src/uts/common/os/cpu_pm.c
* I think that there probably needs to be a big comment at the beginning
that gives the theory of operation for this module, defines all the terminology
used, and explains the policies, design, implementation, purpose, etc..
* What does "state changes associated with transient work/idle are
considered to be mispredictions" mean?
* What's with the XXX's in cpupm_domain_init()?
* Need 2009 copyrights....Happy New Year! (;-)
usr/src/cmd/mdb/common/modules/genunix/pg.c
usr/src/uts/common/conf/param.c
* Need to remove extern declaration of pg_*init() since they are no
longer in init_tbl[]
usr/src/uts/common/disp/cmt.c
usr/src/uts/common/disp/cmt_policy.
* Comment at top seems to imply that we implement "optimized affinity"
CMT dispatching policy using PGs. I thought that we are just load balancing
using PGs and not using them to schedule threads for affinity yet. Have you
implemented affinity optimizations using PGs with your changes?
* "...between cpus..." => "...between CPUs..."
- cmt_should_migrate()
* Why is utilization a fixed width type? Shouldn't just be an int or
long or something not fixed?
* What's up with the XXX?
usr/src/uts/common/disp/disp.c
usr/src/uts/common/os/cpu.c
- Added kstats for current C-state and max no. of C-states
* Why are these being exposed? Should there be an API for
getting/setting these? If power domains live in PGs, should this be
characteristics or properties of the power domain PGs?
usr/src/uts/common/os/cpu_pm.c
usr/src/uts/common/os/group.c
* "...return it's..." => "...return its..."
usr/src/uts/common/os/pg.c
* Should probably explain what you mean by "thread switch" and "thread
remain" in comments for callback routines
usr/src/uts/common/os/pghw.c
- "Initialize the physical portion of a physical PG" => "Initialize the
physical portion of a hardware PG"?
usr/src/uts/common/sys/cmt.h
* Why is type of cmt_utilization field in pg_cmt_t a uint32_t? Does
this really need to be a fixed width type?
* Should comment for cmt_balance() be "CMT dispatcher load balancing
policy" not just "CMT dispatcher policy"?
* Should CMT policies be prefixed with "CMT_POLICY" (eg.
CMT_POLICY_{NONE,BALANCE,etc.}?
usr/src/uts/common/sys/cpuvar.h
* Why have CPU_DISP_CMT_TRANS as name for dispatcher flag signifying
when transient thread running on CPU? It doesn't seem like it has much to do
with CMT. Maybe CPU_DISP_TRANSIENT or CPU_DISP_TRANSIENT_THREAD would be
better and
more clear?
usr/src/uts/common/sys/group.h
* Does group_find() need a more descriptive name or comment? What do
you give it to find? Does it return the index? Can the name or comment say?
usr/src/uts/common/sys/pg.h
* Should pg_cb_ops_t have some comments?
* Should policy_name() field in struct pg_ops have a more descriptive
name or comment?
usr/src/uts/common/sys/pghw.h
* Need comments for PGHW_POW_* and pghw_type_t flavors?
* Should pghw_handle field in pghw_t have a comment?
* Should pg_plat_hw_rank() have a name that describes what it does
better or is "rank" as good as it gets without the name being a full sentence?
usr/src/uts/sun4v/os/cmp.c
* I don't understand the following comment....What do you mean by
"preference for optimizing for sharing relationship"? Optimizing what for the
sharing relationship? Maybe this comment should be made more clear?
134 /*
135 * Express preference for optimizing for sharing relationship
136 * hw1 vs hw2
137 */
* "polices" => "policies"
usr/src/uts/sun4u/os/cmp.c
* See comments for usr/src/uts/sun4v/os/cmp.c
usr/src/uts/i86pc/os/mp_machdep.c
* Why isn't there a cmp.c for x86/x64?
XXX Need to finish reviewing
--
Configure bugmail: http://defect.opensolaris.org/bz/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
