diff mbox series

[v3,4/5] xen: modify lock profiling interface

Message ID 20190829101846.21369-5-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series enhance lock debugging | expand

Commit Message

Jürgen Groß Aug. 29, 2019, 10:18 a.m. UTC
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.

Increment the sysctl.h interface version as an interface is being
modified.

Add the missing setting of profiling time to xc_lockprof_query().

Add freeing element data when deregistering a structure.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- add comment about memory allocation needs (Jan Beulich)
- check success of memory allocation (Jan Beulich)
---
 tools/libxc/xc_misc.c       |  1 +
 tools/misc/xenlockprof.c    | 17 ++------
 xen/common/domain.c         |  4 +-
 xen/common/spinlock.c       | 95 ++++++++++++++++++++++++++++++++-------------
 xen/include/public/sysctl.h | 11 ++----
 xen/include/xen/spinlock.h  | 29 ++++++++------
 6 files changed, 95 insertions(+), 62 deletions(-)

Comments

Jan Beulich Sept. 3, 2019, 2:46 p.m. UTC | #1
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.

> @@ -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.

> +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.

> +        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.

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

Jan
Jürgen Groß Sept. 4, 2019, 8:30 a.m. UTC | #2
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
Jan Beulich Sept. 4, 2019, 8:57 a.m. UTC | #3
On 04.09.2019 10:30, Juergen Gross wrote:
> 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.

Well, I can easily see how one will be quite a bit smaller than the
other, but patch size / intrusiveness is not the only aspect to
consider. IOW I'm afraid the difference won't change my opinion on
string literals in hypercall interfaces. But recall, you don't
depend on me acking this patch of yours, there are enough other
people who can if they are happy with such a model.

Jan
diff mbox series

Patch

diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 8e60b6e9f0..22708f1b1f 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -533,6 +533,7 @@  int xc_lockprof_query(xc_interface *xch,
     rc = do_sysctl(xch, &sysctl);
 
     *n_elems = sysctl.u.lockprof_op.nr_elem;
+    *time = sysctl.u.lockprof_op.time;
 
     return rc;
 }
diff --git a/tools/misc/xenlockprof.c b/tools/misc/xenlockprof.c
index 11f43a35e3..c6aa3fe841 100644
--- a/tools/misc/xenlockprof.c
+++ b/tools/misc/xenlockprof.c
@@ -88,19 +88,10 @@  int main(int argc, char *argv[])
     sb = 0;
     for ( j = 0; j < i; j++ )
     {
-        switch ( data[j].type )
-        {
-        case LOCKPROF_TYPE_GLOBAL:
-            sprintf(name, "global lock %s", data[j].name);
-            break;
-        case LOCKPROF_TYPE_PERDOM:
-            sprintf(name, "domain %d lock %s", data[j].idx, data[j].name);
-            break;
-        default:
-            sprintf(name, "unknown type(%d) %d lock %s", data[j].type,
-                    data[j].idx, data[j].name);
-            break;
-        }
+        if ( data[j].idx == LOCKPROF_IDX_NONE )
+            sprintf(name, "%s %s", data[j].type, data[j].name);
+        else
+            sprintf(name, "%s %d %s", data[j].type, data[j].idx, data[j].name);
         l = (double)(data[j].lock_time) / 1E+09;
         b = (double)(data[j].block_time) / 1E+09;
         sl += l;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 1f7a449f14..84749a28c8 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -290,7 +290,7 @@  static void _domain_destroy(struct domain *d)
 
     xsm_free_security_domain(d);
 
-    lock_profile_deregister_struct(LOCKPROF_TYPE_PERDOM, d);
+    lock_profile_deregister_struct(d, "Domain");
 
     free_domain_struct(d);
 }
@@ -369,7 +369,7 @@  struct domain *domain_create(domid_t domid,
         d->max_vcpus = config->max_vcpus;
     }
 
-    lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid, "Domain");
+    lock_profile_register_struct(d, domid, "Domain");
 
     if ( (err = xsm_alloc_security_domain(d)) != 0 )
         goto fail;
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index c4f706c627..cbde896ae6 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -330,42 +330,43 @@  void _spin_unlock_recursive(spinlock_t *lock)
 #ifdef CONFIG_DEBUG_LOCK_PROFILE
 
 struct lock_profile_anc {
+    struct lock_profile_anc   *next;     /* next type */
     struct lock_profile_qhead *head_q;   /* first head of this type */
-    char                      *name;     /* descriptive string for print */
+    const char                *name;     /* descriptive string for print */
 };
 
 typedef void lock_profile_subfunc(
-    struct lock_profile *, int32_t, int32_t, void *);
+    struct lock_profile *, const char *, int32_t, void *);
 
 extern struct lock_profile *__lock_profile_start;
 extern struct lock_profile *__lock_profile_end;
 
 static s_time_t lock_profile_start;
-static struct lock_profile_anc lock_profile_ancs[LOCKPROF_TYPE_N];
+static struct lock_profile_anc *lock_profile_ancs;
 static struct lock_profile_qhead lock_profile_glb_q;
 static spinlock_t lock_profile_lock = SPIN_LOCK_UNLOCKED;
 
 static void spinlock_profile_iterate(lock_profile_subfunc *sub, void *par)
 {
-    int i;
+    struct lock_profile_anc *anc;
     struct lock_profile_qhead *hq;
     struct lock_profile *eq;
 
     spin_lock(&lock_profile_lock);
-    for ( i = 0; i < LOCKPROF_TYPE_N; i++ )
-        for ( hq = lock_profile_ancs[i].head_q; hq; hq = hq->head_q )
+    for ( anc = lock_profile_ancs; anc; anc = anc->next )
+        for ( hq = anc->head_q; hq; hq = hq->head_q )
             for ( eq = hq->elem_q; eq; eq = eq->next )
-                sub(eq, i, hq->idx, par);
+                sub(eq, anc->name, hq->idx, par);
     spin_unlock(&lock_profile_lock);
 }
 
 static void spinlock_profile_print_elem(struct lock_profile *data,
-    int32_t type, int32_t idx, void *par)
+    const char *type, int32_t idx, void *par)
 {
     struct spinlock *lock = data->lock;
 
-    printk("%s ", lock_profile_ancs[type].name);
-    if ( type != LOCKPROF_TYPE_GLOBAL )
+    printk("%s ", type);
+    if ( idx != LOCKPROF_IDX_NONE )
         printk("%d ", idx);
     printk("%s: addr=%p, lockval=%08x, ", data->name, lock,
            lock->tickets.head_tail);
@@ -389,7 +390,7 @@  void spinlock_profile_printall(unsigned char key)
 }
 
 static void spinlock_profile_reset_elem(struct lock_profile *data,
-    int32_t type, int32_t idx, void *par)
+    const char *type, int32_t idx, void *par)
 {
     data->lock_cnt = 0;
     data->block_cnt = 0;
@@ -413,7 +414,7 @@  typedef struct {
 } spinlock_profile_ucopy_t;
 
 static void spinlock_profile_ucopy_elem(struct lock_profile *data,
-    int32_t type, int32_t idx, void *par)
+    const char *type, int32_t idx, void *par)
 {
     spinlock_profile_ucopy_t *p = par;
     struct xen_sysctl_lockprof_data elem;
@@ -424,7 +425,7 @@  static void spinlock_profile_ucopy_elem(struct lock_profile *data,
     if ( p->pc->nr_elem < p->pc->max_elem )
     {
         safe_strcpy(elem.name, data->name);
-        elem.type = type;
+        safe_strcpy(elem.type, type);
         elem.idx = idx;
         elem.lock_cnt = data->lock_cnt;
         elem.block_cnt = data->block_cnt;
@@ -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;
+}
+
+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);
+        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);
+                }
+                break;
+            }
         }
     }
+
     spin_unlock(&lock_profile_lock);
 }
 
@@ -504,9 +544,8 @@  static int __init lock_prof_init(void)
         (*q)->lock->profile = *q;
     }
 
-    _lock_profile_register_struct(
-        LOCKPROF_TYPE_GLOBAL, &lock_profile_glb_q,
-        0, "Global lock");
+    _lock_profile_register_struct(&lock_profile_glb_q, LOCKPROF_IDX_NONE,
+                                  "Global");
 
     return 0;
 }
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 91c48dcae0..fafce64f02 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -35,7 +35,7 @@ 
 #include "domctl.h"
 #include "physdev.h"
 
-#define XEN_SYSCTL_INTERFACE_VERSION 0x00000012
+#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013
 
 /*
  * Read console content from Xen buffer ring.
@@ -433,14 +433,11 @@  struct xen_sysctl_page_offline_op {
 /* Sub-operations: */
 #define XEN_SYSCTL_LOCKPROF_reset 1   /* Reset all profile data to zero. */
 #define XEN_SYSCTL_LOCKPROF_query 2   /* Get lock profile information. */
-/* Record-type: */
-#define LOCKPROF_TYPE_GLOBAL      0   /* global lock, idx meaningless */
-#define LOCKPROF_TYPE_PERDOM      1   /* per-domain lock, idx is domid */
-#define LOCKPROF_TYPE_N           2   /* number of types */
 struct xen_sysctl_lockprof_data {
-    char     name[40];     /* lock name (may include up to 2 %d specifiers) */
-    int32_t  type;         /* LOCKPROF_TYPE_??? */
+    char     name[40];     /* lock name */
+    char     type[20];     /* e.g. "domain" */
     int32_t  idx;          /* index (e.g. domain id) */
+#define LOCKPROF_IDX_NONE  -1
     uint64_aligned_t lock_cnt;     /* # of locking succeeded */
     uint64_aligned_t block_cnt;    /* # of wait for lock */
     uint64_aligned_t lock_time;    /* nsecs lock held */
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index a7c1967ec7..decaddd9fc 100644
--- 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
+      interrupts must be enabled.
+
     - removing of a structure is done via
 
-      lock_profile_deregister_struct(type, ptr);
+      lock_profile_deregister_struct(ptr, print);
 */
 
 struct spinlock;
@@ -108,14 +113,14 @@  struct lock_profile_qhead {
         (s)->profile_head.elem_q = prof;                                      \
     } while(0)
 
-void _lock_profile_register_struct(
-    int32_t, struct lock_profile_qhead *, int32_t, char *);
-void _lock_profile_deregister_struct(int32_t, struct lock_profile_qhead *);
+void _lock_profile_register_struct(struct lock_profile_qhead *, int32_t,
+                                   const char *);
+void _lock_profile_deregister_struct(struct lock_profile_qhead *, const char *);
 
-#define lock_profile_register_struct(type, ptr, idx, print)                   \
-    _lock_profile_register_struct(type, &((ptr)->profile_head), idx, print)
-#define lock_profile_deregister_struct(type, ptr)                             \
-    _lock_profile_deregister_struct(type, &((ptr)->profile_head))
+#define lock_profile_register_struct(ptr, idx, print)                         \
+    _lock_profile_register_struct(&((ptr)->profile_head), idx, print)
+#define lock_profile_deregister_struct(ptr, print)                            \
+    _lock_profile_deregister_struct(&((ptr)->profile_head), print)
 
 extern int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc);
 extern void spinlock_profile_printall(unsigned char key);
@@ -129,8 +134,8 @@  struct lock_profile_qhead { };
 #define DEFINE_SPINLOCK(l) spinlock_t l = SPIN_LOCK_UNLOCKED
 
 #define spin_lock_init_prof(s, l) spin_lock_init(&((s)->l))
-#define lock_profile_register_struct(type, ptr, idx, print)
-#define lock_profile_deregister_struct(type, ptr)
+#define lock_profile_register_struct(ptr, idx, print)
+#define lock_profile_deregister_struct(ptr, print)
 #define spinlock_profile_printall(key)
 
 #endif