[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 4/4] xen: modify lock profiling interface

On 08.08.19 09:32, Jan Beulich wrote:
On 07.08.2019 16:31, Juergen Gross wrote:
@@ -463,31 +464,67 @@ 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;
-    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);
+        anc->name = name;
+        anc->next = lock_profile_ancs;
+        lock_profile_ancs = anc;
+    }

This is an imo fundamental weakness of the new model: You now
require a dynamic memory allocation (of which you don't even
check that it was successful). Right now the path above will
only be taken at boot time, but that's not stated anywhere as
a restriction (afaics), and hence doesn't need to remain this

Yes, allocation success must be checked, of course.

Adding a memory allocation in the path should be no real problem,
as normally registering a struct is accompanied by registering
the locks in that struct, which is already doing some memory
allocation. I should mention that in the comment section for the
usage of lock profiling.

Furthermore "name" now serves two purposes when previously it
served just one. This is best noticeable with the function's
use in domain_create(): Previously that set up the class
"per-domain" with an initial instance "struct domain". This
did provide for someone later going and also registering
another per-domain structure (e.g. struct p2m_domain) as
another "per-domain" class instance. Then again maybe I'm not
correctly understanding the original intentions ...

While the printout might be not as clear as desired, it will still
be correct. In both cases the "print" value will be used. The now
omitted "type" parameter was only used to decide whether to print
the index and for knowing what to print in the xenlockprof tool.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.