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

[Juergen Gross <jgross@xxxxxxxx>]

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;
> > +
> >        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);
> > +        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
> way.
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.


Juergen

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