http://defect.opensolaris.org/bz/show_bug.cgi?id=6219
Eric Saxe <eric.saxe at sun.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|ACCEPTED |FIXINPROGRESS
--- Comment #3 from Eric Saxe <eric.saxe at sun.com> 2009-02-16 15:02:05 ---
(In reply to comment #0)
> pad code review
> ===============
>
> Author: Alexander Kolbasov <akolb at eng.sun.com>
> Date: 2009/01/23
> URL: http://cr.opensolaris.org/~esaxe/pad-cr/
>
>
> 1 usr/src/cmd/mdb/common/modules/genunix/pg.c
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> 1.1 Future RFEs
>
> Are there some extra things that can be done in MDB to simplify
> debugging for PAD? (future RFEs).
We've found the group walkers tremendously useful. Perhaps something that can
walk and display the PG hierarchy would be useful as well, since we do a lot of
structure surfing looking at parents, sibling sets, etc.
> 2 usr/src/uts/common/sys/pg.h
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> 2.1
> ====
>
> 59 typedef struct pg_cb_ops {
> 60 void (*thread_swtch)(struct pg *, struct cpu *,
> kthread_t *,
> 61 kthread_t *);
> 62 void (*thread_remain)(struct pg *, struct cpu *,
> 63 kthread_t *);
> 64 } pg_cb_ops_t;
> 65
> 66 /*
> 67 * Processor group structure
> 68 */
> 69 typedef struct pg {
> 70 pgid_t pg_id; /* seq id */
> 71 pg_relation_t pg_relation; /* grouping relationship
> */
> 72 struct pg_class *pg_class; /* pg class */
> 73 struct group pg_cpus; /* group of CPUs */
> 74 pg_cb_ops_t pg_cb; /* pg events ops vector
> */
> 75 } pg_t;
>
> Why pg_cb_ops are part of pg and not pg_cmt? Are these relevant for all kinds
> of
> PGs?
>
Conceivably, yes. The callbacks are CMT specific, but other classes (outside of
CMT) down the road will want to register their own.
> 2.2
> ====
>
> 144 #define PG_NUM_CPUS(pgrp) \
> 145 (GROUP_SIZE(&((pg_t *)pgrp)->pg_cpus))
>
> Doing the (pg_t *) cast in aa macro is dangerous - it is easy to pass some
> unrelated pointer to it and get away with it. Can the cast be done by a caller
> where it is easier to verify that the pointer is the right one?
Yes, done.
> 2.3
> ====
>
> 182 * PG Event callbacks
> 183 */
> 184 void pg_ev_thread_swtch(cpu_t *, kthread_t *, kthread_t *);
> 185 void pg_ev_thread_remain(cpu_t *, kthread_t *);
>
> What is the relationship of these callbacks with pg cb callbacks with similar
> names?
These are PG event interfaces that implement a walking of the CPU's PG
hierarchy
and invoke each PG's event callback. These interfaces are invoked by the
dispatcher.
> 3 src/uts/common/sys/cpuvar.h
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> - No comments
>
> 4 usr/src/uts/common/sys/cyclic.h
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> - There are no actual changes in this file
>
> 5 usr/src/uts/common/sys/pghw.h
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> 5.1
> ====
>
> 60 * Returns true if the hardware is a type of power management domain
>
> This comment is rather mysterious, but the code below is clear.
>
> 62 #define PGHW_PM_DOMAIN(hw) \
> 63 (hw == PGHW_POW_ACTIVE || hw == PGHW_POW_IDLE)
>
> with the exception that it is a boolean check, so may be it should be
>
> PGHW_IS_PM_DOMAIN(hw) ?
Yes, done. Thanks.
> 5.2
> ====
>
> 89 kstat_t *pghw_kstat; /* physical kstats exported */
>
> Do you think that kstats belong to hardware PG or to cmt pg layer (or may be
> both?)
Looking at where we want to go post PAD, I think the pghw layer is
right...especially since we'll be moving most of the hierarchy stuff, (and the
capacity utilization) to that layer as well.
> 6 usr/src/uts/common/os/pg.c
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> 6.1
> ====
>
> 154 #define PG_POLICY_NAME(pg) \
> 155 ((pg)->pg_class->pgc_ops->policy_name ?
>
> \
> 156 (pg)->pg_class->pgc_ops->policy_name(pg) : 0) \
>
> This should be NULL, instead of 0.
Yes, Fixed. Thanks.
> 6.2
> ====
>
> Why events callback vercot is bzeroed instead of initializing to null ops or
> the default?
>
>
> 398 /*
> 399 * Initialize the events ops vector
> 400 */
> 401 bcopy(&pg_cb_ops_default, &pg->pg_cb, sizeof (struct pg_cb_ops));
> 402
Hmm. It's not bzeroed, it is initialized to the default set of ops using bcopy.
Maybe I'm misunderstanding what you mean...
> 7 usr/src/uts/common/os/pghw.c
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> 7.1
> ====
>
> Who now calls pghw_type_string?
>
> 419 kstat_named_setstr(&pgsp->pg_policy, pg_policy_name((pg_t *)pg));
It's called by pghw_kstat_update() to retrieve the string for the hardware
type.
> 7.2
> ====
>
> I am wondering whether it is more useful to put kstats to cmt layer rather
> than
> hw layer. This will allow them to show information from cmt, hw and group
> level
> at once.
I agree it will be good to get some of the goodies from the CMT layer out in
the kstats. I think Rafael's work will help here, since he's bringing those
elements up to the hw layer...
(To be continued)
--
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.
