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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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