http://defect.opensolaris.org/bz/show_bug.cgi?id=6446
Eric Saxe <eric.saxe at sun.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |ACCEPTED
--- Comment #1 from Eric Saxe <eric.saxe at sun.com> 2009-02-13 01:32:39 ---
(In reply to comment #0)
> usr/src/uts/common/disp/cmt.c
> * Comments mention optimizing for affinity....Do we do anything for that
> now?
Yes. CPU's sharing last level caches are more likely to steal from one another
(since nosteal_nsec is disabled in that case).
> - pg_cmt_hier_rank()
> * Change comment to "Ask the platform which hardware sharing relationship
> is more important to optimize"
Done.
> * 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
Done.
> - 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?
Comment changed to indicate it's being promoted above the parent.
> * 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?
That happens because the PG being promoted is added to it's parent's siblings
set, which is the same group as the grandparent's children.
> - 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?
The promotion code can move down the lineage once the higher up PGs are the the
correct position. It might be possible doing this from the bottom up, but going
top down allows it to be more efficient.
> - pg_cmt_hier_sort()
> * This code is a little hard to understand
> * Is "sz" the number of levels in the given PG CMT hierarchy?
It's the number of levels in the hierarchy. I've fixed up the comment.
Maybe give
> it a better name?
Done, it's now "size".
> - cmt_pad_{en,dis}able()
> * I don't understand this routine, so I guess that more comments woulbe
> good....
I've added some more comments around these routines.
> * How does promoting PGs enable or disable PAD?
Policy is only implemented between siblings. Where a PG is an only child of
another PG, the parent's PG (and it's poicy) dominates. In this situation,
these routines promote the PG who's policy should dominate.
> * Can these routines operate on anything other than an active power
> domain? If not, maybe the assertion should be different?
>
The routines are specifically for the power domains, and deal with setting
policy for those specifically. They haven't been tested in other circumstances
so I would want callers passing types other than power domains to trip the
assertion.
> 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..
I've added a large block comment.
> * What does "state changes associated with transient work/idle are
> considered to be mispredictions" mean?
Clarified in the comment.
> * What's with the XXX's in cpupm_domain_init()?
Fixed.
> * Need 2009 copyrights....Happy New Year! (;-)
Fixed.
> 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[]
Good catch. Thank you.
> 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?
Not yet (although I'm hopeful we will soon). I've cleaned up the comment.
> * "...between cpus..." => "...between CPUs..."
>
Fixed.
> - cmt_should_migrate()
> * Why is utilization a fixed width type? Shouldn't just be an int or
> long or something not fixed?
>d
Hmm. Well, it should certainly be unsigned (utilization is 0 or greater). I
think I selected fixed width just so it would be easier to grok the range...but
also because it's passed to various atomic_add_* routines where that's the arg.
> * What's up with the XXX?
Fixed.
> 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?
>
I think they were added primarily for debugging/observability. The poower
domains are affiliated with PGs, but they don't live within. I agree, a better
API than kstats would be nice for enumerating these. That's probably more
within the domain of the PM usability project.
> usr/src/uts/common/os/cpu_pm.c
>
> usr/src/uts/common/os/group.c
> * "...return it's..." => "...return its..."
Fixed.
> usr/src/uts/common/os/pg.c
> * Should probably explain what you mean by "thread switch" and "thread
> remain" in comments for callback routines
>
Done.
> usr/src/uts/common/os/pghw.c
> - "Initialize the physical portion of a physical PG" => "Initialize the
> physical portion of a hardware PG"?
Done.
> 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?
Answered above.
> * Should comment for cmt_balance() be "CMT dispatcher load balancing
> policy" not just "CMT dispatcher policy"?
It's also coalescence.
> * 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?
This code is now gone (yay).
> 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?
Fixed.
> 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?
Fixed.
> usr/src/uts/common/sys/pghw.h
> * Need comments for PGHW_POW_* and pghw_type_t flavors?
Yes, fixed.
> * Should pghw_handle field in pghw_t have a comment?
Fixed.
> * 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?
I like hw_rank()...it does rank the hardware. Other suggestions?
> 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?
Done.
> 134 /*
> 135 * Express preference for optimizing for sharing relationship
> 136 * hw1 vs hw2
> 137 */
>
> * "polices" => "policies"
Thanks. Fixed.
>
> usr/src/uts/sun4u/os/cmp.c
> * See comments for usr/src/uts/sun4v/os/cmp.c
Fixed.
> usr/src/uts/i86pc/os/mp_machdep.c
> * Why isn't there a cmp.c for x86/x64?
> XXX Need to finish reviewing
Those exist on SPARC only. These routines live in mp_machdep.c on x86.
--
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.
