[PATCH 0/2] lockprof: eliminate a minor bug and a quirk

Jan Beulich posted 2 patches 2 weeks ago
Only 0 patches received!

[PATCH 0/2] lockprof: eliminate a minor bug and a quirk

Posted by Jan Beulich 2 weeks ago
1: don't leave locks uninitialized upon allocation failure
2: don't pass name into registration function

Jan

[PATCH 1/2] lockprof: don't leave locks uninitialized upon allocation failure

Posted by Jan Beulich 2 weeks ago
Even if a specific struct lock_profile instance can't be allocated, the
lock itself should still be functional. As this isn't a production use
feature, also log a message in the event that the profiling struct can't
be allocated.

Fixes: d98feda5c756 ("Make lock profiling usable again")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -103,10 +103,16 @@ struct lock_profile_qhead {
     do {                                                                      \
         struct lock_profile *prof;                                            \
         prof = xzalloc(struct lock_profile);                                  \
-        if (!prof) break;                                                     \
+        (s)->l = (spinlock_t)_SPIN_LOCK_UNLOCKED(prof);                       \
+        if ( !prof )                                                          \
+        {                                                                     \
+            printk(XENLOG_WARNING                                             \
+                   "lock profiling unavailable for %p(%d)'s " #l "\n",        \
+                   s, (s)->profile_head.idx);                                 \
+            break;                                                            \
+        }                                                                     \
         prof->name = #l;                                                      \
         prof->lock = &(s)->l;                                                 \
-        (s)->l = (spinlock_t)_SPIN_LOCK_UNLOCKED(prof);                       \
         prof->next = (s)->profile_head.elem_q;                                \
         (s)->profile_head.elem_q = prof;                                      \
     } while(0)


Re: [PATCH 1/2] lockprof: don't leave locks uninitialized upon allocation failure

Posted by Andrew Cooper 2 weeks ago
On 23/07/2020 11:51, Jan Beulich wrote:
> Even if a specific struct lock_profile instance can't be allocated, the
> lock itself should still be functional. As this isn't a production use
> feature, also log a message in the event that the profiling struct can't
> be allocated.
>
> Fixes: d98feda5c756 ("Make lock profiling usable again")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -103,10 +103,16 @@ struct lock_profile_qhead {
>      do {                                                                      \
>          struct lock_profile *prof;                                            \
>          prof = xzalloc(struct lock_profile);                                  \
> -        if (!prof) break;                                                     \
> +        (s)->l = (spinlock_t)_SPIN_LOCK_UNLOCKED(prof);                       \
> +        if ( !prof )                                                          \
> +        {                                                                     \
> +            printk(XENLOG_WARNING                                             \
> +                   "lock profiling unavailable for %p(%d)'s " #l "\n",        \
> +                   s, (s)->profile_head.idx);                                 \

You'll end up with far less .rodata if you use %s and pass #l in as a
parameter.

Either way, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> +            break;                                                            \
> +        }                                                                     \
>          prof->name = #l;                                                      \
>          prof->lock = &(s)->l;                                                 \
> -        (s)->l = (spinlock_t)_SPIN_LOCK_UNLOCKED(prof);                       \
>          prof->next = (s)->profile_head.elem_q;                                \
>          (s)->profile_head.elem_q = prof;                                      \
>      } while(0)
>


Re: [PATCH 1/2] lockprof: don't leave locks uninitialized upon allocation failure

Posted by Jan Beulich 2 weeks ago
On 23.07.2020 13:23, Andrew Cooper wrote:
> On 23/07/2020 11:51, Jan Beulich wrote:
>> Even if a specific struct lock_profile instance can't be allocated, the
>> lock itself should still be functional. As this isn't a production use
>> feature, also log a message in the event that the profiling struct can't
>> be allocated.
>>
>> Fixes: d98feda5c756 ("Make lock profiling usable again")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/include/xen/spinlock.h
>> +++ b/xen/include/xen/spinlock.h
>> @@ -103,10 +103,16 @@ struct lock_profile_qhead {
>>      do {                                                                      \
>>          struct lock_profile *prof;                                            \
>>          prof = xzalloc(struct lock_profile);                                  \
>> -        if (!prof) break;                                                     \
>> +        (s)->l = (spinlock_t)_SPIN_LOCK_UNLOCKED(prof);                       \
>> +        if ( !prof )                                                          \
>> +        {                                                                     \
>> +            printk(XENLOG_WARNING                                             \
>> +                   "lock profiling unavailable for %p(%d)'s " #l "\n",        \
>> +                   s, (s)->profile_head.idx);                                 \
> 
> You'll end up with far less .rodata if you use %s and pass #l in as a
> parameter.

Well, "far less" perhaps goes a little far, as we currently have
just three use sites, but I see your point and hence will switch.

> Either way, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks, Jan

[PATCH 2/2] lockprof: don't pass name into registration function

Posted by Jan Beulich 2 weeks ago
The type uniquely identifies the associated name, hence the name fields
can be statically initialized.

Also constify not just the involved struct field, but also struct
lock_profile's. Rather than specifying lock_profile_ancs[]' dimension at
definition time, add a suitable build time check, such that at least
missing tail additions to the initializer can be spotted easily.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -392,7 +392,7 @@ struct domain *domain_create(domid_t dom
         d->max_vcpus = config->max_vcpus;
     }
 
-    lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid, "Domain");
+    lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid);
 
     if ( (err = xsm_alloc_security_domain(d)) != 0 )
         goto fail;
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -338,7 +338,7 @@ void _spin_unlock_recursive(spinlock_t *
 
 struct lock_profile_anc {
     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(
@@ -348,7 +348,10 @@ extern struct lock_profile *__lock_profi
 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[] = {
+    [LOCKPROF_TYPE_GLOBAL] = { .name = "Global" },
+    [LOCKPROF_TYPE_PERDOM] = { .name = "Domain" },
+};
 static struct lock_profile_qhead lock_profile_glb_q;
 static spinlock_t lock_profile_lock = SPIN_LOCK_UNLOCKED;
 
@@ -473,13 +476,12 @@ int spinlock_profile_control(struct xen_
 }
 
 void _lock_profile_register_struct(
-    int32_t type, struct lock_profile_qhead *qhead, int32_t idx, char *name)
+    int32_t type, struct lock_profile_qhead *qhead, int32_t idx)
 {
     qhead->idx = idx;
     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;
     spin_unlock(&lock_profile_lock);
 }
 
@@ -504,6 +506,8 @@ static int __init lock_prof_init(void)
 {
     struct lock_profile **q;
 
+    BUILD_BUG_ON(ARRAY_SIZE(lock_profile_ancs) != LOCKPROF_TYPE_N);
+
     for ( q = &__lock_profile_start; q < &__lock_profile_end; q++ )
     {
         (*q)->next = lock_profile_glb_q.elem_q;
@@ -511,9 +515,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(LOCKPROF_TYPE_GLOBAL,
+                                  &lock_profile_glb_q, 0);
 
     return 0;
 }
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -72,7 +72,7 @@ struct spinlock;
 
 struct lock_profile {
     struct lock_profile *next;       /* forward link */
-    char                *name;       /* lock name */
+    const char          *name;       /* lock name */
     struct spinlock     *lock;       /* the lock itself */
     u64                 lock_cnt;    /* # of complete locking ops */
     u64                 block_cnt;   /* # of complete wait for lock */
@@ -118,11 +118,11 @@ struct lock_profile_qhead {
     } while(0)
 
 void _lock_profile_register_struct(
-    int32_t, struct lock_profile_qhead *, int32_t, char *);
+    int32_t, struct lock_profile_qhead *, int32_t);
 void _lock_profile_deregister_struct(int32_t, struct lock_profile_qhead *);
 
-#define lock_profile_register_struct(type, ptr, idx, print)                   \
-    _lock_profile_register_struct(type, &((ptr)->profile_head), idx, print)
+#define lock_profile_register_struct(type, ptr, idx)                          \
+    _lock_profile_register_struct(type, &((ptr)->profile_head), idx)
 #define lock_profile_deregister_struct(type, ptr)                             \
     _lock_profile_deregister_struct(type, &((ptr)->profile_head))
 
@@ -138,7 +138,7 @@ 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_register_struct(type, ptr, idx)
 #define lock_profile_deregister_struct(type, ptr)
 #define spinlock_profile_printall(key)
 


Re: [PATCH 2/2] lockprof: don't pass name into registration function

Posted by Andrew Cooper 2 weeks ago
On 23/07/2020 11:52, Jan Beulich wrote:
> The type uniquely identifies the associated name, hence the name fields
> can be statically initialized.
>
> Also constify not just the involved struct field, but also struct
> lock_profile's. Rather than specifying lock_profile_ancs[]' dimension at
> definition time, add a suitable build time check, such that at least
> missing tail additions to the initializer can be spotted easily.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>