On 03.09.19 16:46, Jan Beulich wrote:
On 29.08.2019 12:18, Juergen Gross wrote:
Today adding locks located in a struct to lock profiling requires a
unique type index for each structure. This makes it hard to add any
new structure as the related sysctl interface needs to be changed, too.
Instead of using an index the already existing struct name specified
in lock_profile_register_struct() can be used as an identifier instead.
Modify the sysctl interface to use the struct name instead of the type
index and adapt the related coding accordingly. Instead of an array of
struct anchors for lock profiling we now use a linked list for that
purpose. Use the special idx value -1 for cases where the idx isn't
relevant (global locks) and shouldn't be printed.
Just to make explicit what was implied by my v1 reply: I can see why
you want to do this, but I'm not really a friend of string literals
in the hypercall interface, when binary values can also do. So to
me this looks to be a move in the wrong direction. Therefore, while
I'm fine reviewing such a change, I'm not very likely to eventually
ack it.
I'll write two example patches for adding support of lock profiling in a
new structure, one with patch 4 of this series applied and one for the
interface without that patch. This should make clear why I'm really in
favor of this patch.
@@ -465,31 +466,70 @@ int spinlock_profile_control(struct
xen_sysctl_lockprof_op *pc)
return rc;
}
-void _lock_profile_register_struct(
- int32_t type, struct lock_profile_qhead *qhead, int32_t idx, char *name)
+static struct lock_profile_anc *find_prof_anc(const char *name)
{
- qhead->idx = idx;
+ struct lock_profile_anc *anc;
+
+ for ( anc = lock_profile_ancs; anc; anc = anc->next )
+ if ( !strcmp(anc->name, name) )
+ return anc;
+ return NULL;
+}
Blank line before main return statement please.
Yes.
+void _lock_profile_register_struct(struct lock_profile_qhead *qhead,
+ int32_t idx, const char *name)
+{
+ struct lock_profile_anc *anc;
+
spin_lock(&lock_profile_lock);
- qhead->head_q = lock_profile_ancs[type].head_q;
- lock_profile_ancs[type].head_q = qhead;
- lock_profile_ancs[type].name = name;
+
+ anc = find_prof_anc(name);
+ if ( !anc )
+ {
+ anc = xzalloc(struct lock_profile_anc);
Hmm, another allocation with a lock held. We try to avoid such as
much as possible, and it doesn't look overly difficult to avoid it
here.
I'll modify it.
+ if ( !anc )
+ goto out;
+ anc->name = name;
+ anc->next = lock_profile_ancs;
+ lock_profile_ancs = anc;
+ }
+
+ qhead->idx = idx;
+ qhead->head_q = anc->head_q;
+ anc->head_q = qhead;
+
+ out:
spin_unlock(&lock_profile_lock);
}
-void _lock_profile_deregister_struct(
- int32_t type, struct lock_profile_qhead *qhead)
+void _lock_profile_deregister_struct(struct lock_profile_qhead *qhead,
+ const char *name)
{
+ struct lock_profile_anc *anc;
struct lock_profile_qhead **q;
+ struct lock_profile *elem;
spin_lock(&lock_profile_lock);
- for ( q = &lock_profile_ancs[type].head_q; *q; q = &(*q)->head_q )
+
+ anc = find_prof_anc(name);
+ if ( anc )
{
- if ( *q == qhead )
+ for ( q = &anc->head_q; *q; q = &(*q)->head_q )
{
- *q = qhead->head_q;
- break;
+ if ( *q == qhead )
+ {
+ *q = qhead->head_q;
+ while ( qhead->elem_q )
+ {
+ elem = qhead->elem_q;
+ qhead->elem_q = elem->next;
+ xfree(elem);
Which assumes the global list would never get handed here. Probably
fine.
I can add an ASSERT() to make this very clear.
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -50,19 +50,24 @@ union lock_debug { };
with ptr being the main structure pointer and lock the spinlock field
+ It should be noted that this will need to allocate memory, so interrupts
+ must be enabled.
+
- each structure has to be added to profiling with
- lock_profile_register_struct(type, ptr, idx, print);
+ lock_profile_register_struct(ptr, idx, print);
with:
- type: something like LOCKPROF_TYPE_PERDOM
ptr: pointer to the structure
idx: index of that structure, e.g. domid
print: descriptive string like "domain"
+ It should be noted that this will might need to allocate memory, so
Nit: "will" or "might", but not both.
Indeed.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
