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

[Juergen Gross <jgross@xxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel