Re: [Xen-devel] [PATCH 0/4] enhance lock debugging

[Juergen Gross <jgross@xxxxxxxx>]

On 08.08.19 10:33, Andrew Cooper wrote:
> On 08/08/2019 05:50, Juergen Gross wrote:
> > On 07.08.19 20:11, Andrew Cooper wrote:
> > > <snip>
> > > Its not exactly the easiest to dump to follow.
> > >
> > > First of all - I don't see why the hold/block time are printed like
> > > that.  It
> > > might be a holdover from the 32bit build, pre PRId64 support.  They
> > > should
> > > probably use PRI_stime anyway.
> > Fine with me.
> >
> > > The domid rendering is unfortunate.  Ideally we'd use %pd but that would
> > > involve rearranging the logic to get a struct domain* in hand.
> > > Seeing as
> > > you're the last person to modify this code, how hard would that be to
> > > do?
> > It would completely break the struct type agnostic design.
> Ok.  As an alternatively, how about %pdr which takes a raw domid?  It
> would be a trivial adjustment in the vsnprintf code, and could plausibly
> be useful elsewhere where we have a domid and not a domain pointer.
Lock profiling has no knowledge that it is working on a struct domain.
It is just working on a lock in a struct and an index to differentiate
the struct instance. Using the domid as the index is just for making it
more easy to understand the printout.

I wouldn't want e.g. a per-event lock to be named "IDLE" just because
it happens to be index 32767.

> > I have a debug patch adding credit2 run-queue lock. It requires to just
> > add 5 lines of code (and this includes moving the spinlock_init() out of
> > an irq-off section). It will produce:
> >
> > (XEN) credit2-runq 0 lock: addr=ffff830413351010, lockval=de00ddf, cpu=6
> > (XEN)   lock: 896287(00000000:00FAA6B2), block:  819(00000000:00009C80)
> What is the 0 here?
The runq number.

> > > We have several global locks called lock:
> > >
> > >     (XEN) Global lock: addr=ffff82d0808181e0, lockval=10001, cpu=4095
> > >     (XEN)   lock:           1(00000000:01322165), block:
> > > 0(00000000:00000000)
> > >     (XEN) Global lock: addr=ffff82d080817cc0, lockval=100010, cpu=4095
> > >     (XEN)   lock:           0(00000000:00000000), block:
> > > 0(00000000:00000000)
> > >     (XEN) Global lock: addr=ffff82d080817800, lockval=0000, cpu=4095
> > >     (XEN)   lock:           0(00000000:00000000), block:
> > > 0(00000000:00000000)
> > >     (XEN) Global lock: addr=ffff82d080817780, lockval=0000, cpu=4095
> > >     (XEN)   lock:           0(00000000:00000000), block:
> > > 0(00000000:00000000)
> > >     (XEN) Global lock: addr=ffff82d080817510, lockval=0000, cpu=4095
> > >     (XEN)   lock:           0(00000000:00000000), block:
> > > 0(00000000:00000000)
> > >
> > > The second one in particular has corrupt data, seeing has it has been
> > > taken
> > > and released several times without lock_cnt increasing.
> > The lock might have been taken/released before lock profiling has been
> > initialized.
> What is there to initialise?  It all looks statically set up.
lock->profile is set only in lock_prof_init().

> > > For sanity sake, we should enforce unique naming of any lock
> > > registered for
> > > profiling.
> > This would be every lock inited via DEFINE_SPINLOCK(). I can do a
> > followup patch for that purpose.
> I was wondering how to do this.  One option which comes to mind is to
> put an non-static object into .discard.lock_profile or something, so the
> linker will object to repeated symbol names and then throw all of them away.
I could just drop the "static" in the _LOCK_PROFILE_PTR() macro.
At the same time I should move the ".lockprofile.data" section in the
linker scripts to the init part maybe.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel