[mdb-discuss] code review request for 7355 mdb(1) reads lbolt through mdb_vread()
On Thu, Sep 10, 2009 at 11:42:22AM -0700, Rafael Vanoni wrote: > webrev @ http://cr.opensolaris.org/~rafaelv/mdb-lbolt/ > > > >Why not move the panic_lbolt() read into mdb_get_lbolt() rather than > >duplicate it? The existing modules that don't respect panic_lbolt are > >essentially wrong anyway, I think, and should be harmonised. > > Ok, that seems reasonable. New webrev @ > http://cr.opensolaris.org/~rafaelv/mdb-lbolt2/ Looks good regards john
[mdb-discuss] code review request for 7355 mdb(1) reads lbolt through mdb_vread()
On Wed, Sep 09, 2009 at 11:01:35PM -0700, Rafael Vanoni wrote: > >>webrev @ http://cr.opensolaris.org/~rafaelv/mdb-lbolt/ Why not move the panic_lbolt() read into mdb_get_lbolt() rather than duplicate it? The existing modules that don't respect panic_lbolt are essentially wrong anyway, I think, and should be harmonised. regards john
[mdb-discuss] code review request for 7355 mdb(1) reads lbolt through mdb_vread()
John Levon wrote: > On Wed, Sep 09, 2009 at 11:01:35PM -0700, Rafael Vanoni wrote: > webrev @ http://cr.opensolaris.org/~rafaelv/mdb-lbolt/ > > Why not move the panic_lbolt() read into mdb_get_lbolt() rather than > duplicate it? The existing modules that don't respect panic_lbolt are > essentially wrong anyway, I think, and should be harmonised. Ok, that seems reasonable. New webrev @ http://cr.opensolaris.org/~rafaelv/mdb-lbolt2/ Thanks, Rafael
[mdb-discuss] code review request for 7355 mdb(1) reads lbolt through mdb_vread()
On Wed, Sep 09, 2009 at 03:11:49PM -0700, Rafael Vanoni wrote:
> webrev @ http://cr.opensolaris.org/~rafaelv/mdb-lbolt/
mdb_ks.c:1598
Isn't this wrong for mdb examining crash dumps - it gives the live
kernel's hrtime, not the dump's? Surely you should be looking for
lbolt_debug_ts in the post-mortem case too. Not sure about a live
dump...
genunix.c:
3441 3440 cid.cid_lbolt = (uintptr_t)sym.st_value;
If we find panic_lbolt, we'll save a pointer to it.
3448 3447 if (lbolt == 0) {
Or if it's zero (no panic), we'll get a pointer to 'lbolt':
3449 -if (mdb_lookup_by_name("lbolt", &sym) == -1) {
3450 -mdb_warn("failed to find lbolt");
3453 -cid.cid_lbolt = (uintptr_t)sym.st_value;
Then, later, we'll use this pointer value:
3213 -if (mdb_vread(&lbolt, sizeof (lbolt),
cid->cid_lbolt) == -1) {
3214 -mdb_warn("failed to read lbolt at %p",
cid->cid_lbolt);
But you made this be:
3213 +if ((lbolt = (clock_t)mdb_get_lbolt()) != -1)
3214 +mdb_printf("t-%-4d ", lbolt -
cpu->cpu_last_swtch);
3215 +else
which will ignore panic_lbolt altogether. I think you need the
mdb_get_lbolt() fix I mention above, and drop cid_lbolt altogether.
At that point, a short comment on the meaning of mdb_get_lbolt() would
be nice.
genunix.c:4348
Please add a help function.
kobj_kdi.c:
The point of the KDI is to isolate KMDB from the kernel - please don't
muddy the interface further. Your calls should be KDI (kernel) side.
There's a (plat) system_claim() call that's the right place. (You'd have
to make the SPARC platform modules call a sparc_system_claim/release(),
which is a slight pain, but much the better way).
regards
john
[mdb-discuss] code review request for 7355 mdb(1) reads lbolt through mdb_vread()
John Levon wrote:
> On Wed, Sep 09, 2009 at 03:11:49PM -0700, Rafael Vanoni wrote:
>
>> webrev @ http://cr.opensolaris.org/~rafaelv/mdb-lbolt/
>
> mdb_ks.c:1598
>
> Isn't this wrong for mdb examining crash dumps - it gives the live
> kernel's hrtime, not the dump's? Surely you should be looking for
> lbolt_debug_ts in the post-mortem case too. Not sure about a live
> dump...
Ok.
> genunix.c:
>
> 3441 3440 cid.cid_lbolt = (uintptr_t)sym.st_value;
>
> If we find panic_lbolt, we'll save a pointer to it.
>
> 3448 3447 if (lbolt == 0) {
>
> Or if it's zero (no panic), we'll get a pointer to 'lbolt':
>
> 3449 -if (mdb_lookup_by_name("lbolt", &sym) == -1) {
> 3450 -mdb_warn("failed to find lbolt");
> 3453 -cid.cid_lbolt = (uintptr_t)sym.st_value;
>
> Then, later, we'll use this pointer value:
>
> 3213 -if (mdb_vread(&lbolt, sizeof (lbolt),
> cid->cid_lbolt) == -1) {
> 3214 -mdb_warn("failed to read lbolt at %p",
> cid->cid_lbolt);
>
> But you made this be:
>
> 3213 +if ((lbolt = (clock_t)mdb_get_lbolt()) != -1)
> 3214 +mdb_printf("t-%-4d ", lbolt -
> cpu->cpu_last_swtch);
> 3215 +else
>
> which will ignore panic_lbolt altogether. I think you need the
> mdb_get_lbolt() fix I mention above, and drop cid_lbolt altogether.
True, fixed.
> At that point, a short comment on the meaning of mdb_get_lbolt() would
> be nice.
Ok
> genunix.c:4348
>
> Please add a help function.
Ok
> kobj_kdi.c:
>
> The point of the KDI is to isolate KMDB from the kernel - please don't
> muddy the interface further. Your calls should be KDI (kernel) side.
> There's a (plat) system_claim() call that's the right place. (You'd have
> to make the SPARC platform modules call a sparc_system_claim/release(),
> which is a slight pain, but much the better way).
Right, I mistook kobj_kdi_system_claim() and kobj_kdi_system_release()
for common code. Fixed.
Just updated the webrev, please let me know what you think.
Thanks,
Rafael
[mdb-discuss] code review request for 7355 mdb(1) reads lbolt through mdb_vread()
Hi folks Could someone please review my changes to (k)mdb for the tickless/lbolt project? Here's a summary of the changes: - added two new routines to mdb_ks.c, mdb_gethrtime() and mdb_get_lbolt() - moved former mdb consumers of the lbolt variable to mdb_get_lbolt() - created a new dcmd, ::time, to retrieve system time when dropped into (k)mdb - calculating time spent in kmdb off of kdi_system_claim and kdi_system_release. webrev @ http://cr.opensolaris.org/~rafaelv/mdb-lbolt/ We're targeting snv_125, so please send comments by next Tuesday (9.15). I have test systems running these bits if anyone's interested. Thanks, Rafael
