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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/08/2019 10:36, Juergen Gross wrote:
> 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.

Ok, but clearly there is something which distinguishes domain indices
from other indices?

>
>>
>>>
>>>> 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().

Ah - so it is.  I wonder if this can be done at compile time?  Its just
a backreference for the forward reference which is done at.

(Wow this code is complicated to follow).  I think it can be done with a
forward declaration of static struct lock_profile
__lock_profile_data_##l and passing something other than NULL into
_SPIN_LOCK_UNLOCKED(), but that will break the ability to do static
DEFINE_SPINLOCK().

Probably not worth messing with this now, but perhaps something to think
over.

~Andrew

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