Re: [PATCH 12/15] xen: trace tasklets

["Jan Beulich" <JBeulich@suse.com>]

>>> On 01.06.17 at 19:35, <dario.faggioli@citrix.com> wrote:
> --- a/xen/common/tasklet.c
> +++ b/xen/common/tasklet.c
> @@ -30,10 +30,87 @@ static DEFINE_PER_CPU(struct list_head, softirq_tasklet_list);
>  /* Protects all lists and tasklet structures. */
>  static DEFINE_SPINLOCK(tasklet_lock);
>  
> +#ifdef CONFIG_TRACE_TASKLETS
> +static inline void trace_enqueue(const struct tasklet *t)
> +{
> +    uint64_t addr;
> +
> +    if ( likely(!tb_init_done) )
> +        return;
> +
> +    addr = (uint64_t)t->func;
> +    __trace_var(TRC_XEN_TASKLET_ENQUEUE, 0, sizeof(addr), &addr);
> +}
> +static inline void trace_schedule(const struct tasklet *t)
> +{
> +    struct {
> +        uint64_t addr;
> +        int16_t sched_on, is_sirq;
> +    } d;
> +
> +    if ( likely(!tb_init_done) )
> +        return;
> +
> +    d.addr = (uint64_t)t->func;
> +    d.sched_on = t->scheduled_on;
> +    d.is_sirq = t->is_softirq;
> +    __trace_var(TRC_XEN_TASKLET_SCHEDULE, 1, sizeof(d), &d);
> +}
> +static inline void trace_work(const struct tasklet *t)
> +{
> +    uint64_t addr;
> +
> +    if ( likely(!tb_init_done) )
> +        return;
> +
> +    addr = (uint64_t)t->func;
> +    __trace_var(TRC_XEN_TASKLET_WORK, 1, sizeof(addr), &addr);
> +}
> +static inline void trace_kill(const struct tasklet *t)
> +{
> +    struct {
> +        uint64_t addr;
> +        int16_t sched_on, is_run;
> +    } d;
> +
> +    if ( likely(!tb_init_done) )
> +        return;
> +
> +    d.addr = (uint64_t)t->func;
> +    d.sched_on = t->scheduled_on;
> +    d.is_run = t->is_running;
> +    __trace_var(TRC_XEN_TASKLET_KILL, 0, sizeof(d), &d);
> +}
> +static inline void trace_init(const struct tasklet *t)
> +{
> +    struct {
> +        uint64_t addr;
> +        uint32_t is_sirq;
> +    } d;
> +
> +    if ( likely(!tb_init_done) )
> +        return;
> +
> +    d.addr = (uint64_t)t->func;
> +    d.is_sirq = t->is_softirq;
> +    __trace_var(TRC_XEN_TASKLET_INIT, 0, sizeof(d), &d);
> +}
> +#define trace_migrate()      TRACE_0D(TRC_XEN_TASKLET_MIGR);
> +#else
> +#define trace_enqueue(t)     do {} while ( 0 )
> +#define trace_schedule(t)    do {} while ( 0 )
> +#define trace_work(t)        do {} while ( 0 )
> +#define trace_kill(t)        do {} while ( 0 )
> +#define trace_migrate()      do {} while ( 0 )
> +#define trace_init(t)        do {} while ( 0 )
> +#endif /* TRACE_TASKLETS */

Seeing how such additions add up, I think I'd prefer if you put them
into header files instead of cluttering source files this way. You
could have one such header per traceable component.

> @@ -178,6 +258,11 @@ static void migrate_tasklets_from_cpu(unsigned int cpu, struct list_head *list)
>  
>      spin_lock_irqsave(&tasklet_lock, flags);
>  
> +    if ( list_empty(list) )
> +        goto out;
> +
> +    trace_migrate();
> +
>      while ( !list_empty(list) )

Two alternatives:

    if ( !list_empty(list) )
        trace_migrate();

(avoiding the goto) or

    if ( list_empty(list) )
        goto out;

    trace_migrate();

    do {
        ...
    } while ( !list_empty(list) );

(avoiding the redundant check).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel