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
