Re: [perfmon2] [PATCH] perf_events: improve x86 event scheduling

[Peter Z. <pe...@in...>]

On Wed, 2010-01-13 at 10:54 +0100, Stephane Eranian wrote:

> > One concern I do have is the loss of error checking on
> > event_sched_in()'s event->pmu->enable(), that could be another
> > 'hardware' PMU like breakpoints, in which case it could fail.
> >
> Well, x86_pmu_enable() does return an error code, so it is up
> to the upper layer to handle the error gracefully and in particular
> in perf_ctx_adjust_freq().

> +static void event_sched_in(struct perf_event *event, int cpu)
> +{
> +       event->state = PERF_EVENT_STATE_ACTIVE;
> +       event->oncpu = cpu;
> +       event->tstamp_running += event->ctx->time - event->tstamp_stopped;
> +       if (is_software_event(event))
> +               event->pmu->enable(event);
> +}
> +
> +/*
> + * Called to enable a whole group of events.
> + * Returns 1 if the group was enabled, or -EAGAIN if it could not be.
> + * Assumes the caller has disabled interrupts and has
> + * frozen the PMU with hw_perf_save_disable.
> + *
> + * called with PMU disabled. If successful and return value 1,
> + * then guaranteed to call perf_enable() and hw_perf_enable()
> + */
> +int hw_perf_group_sched_in(struct perf_event *leader,
> +              struct perf_cpu_context *cpuctx,
> +              struct perf_event_context *ctx, int cpu)
> +{
> +       struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
> +       struct perf_event *sub;
> +       int assign[X86_PMC_IDX_MAX];
> +       int n, n0, ret;
> +
> +       n0 = cpuc->n_events;
> +
> +       n = collect_events(cpuc, leader, true);
> +       if (n < 0)
> +               return n;
> +
> +       ret = x86_schedule_events(cpuc, n, assign);
> +       if (ret)
> +               return ret;
> +
> +       /*
> +        * copy new assignment, now we know it is possible
> +        * will be used by hw_perf_enable()
> +        */
> +       memcpy(cpuc->assign, assign, n*sizeof(int));
> +
> +       cpuc->n_events = n;
> +       cpuc->n_added  = n - n0;
> +
> +       n = 1;
> +       event_sched_in(leader, cpu);
> +       list_for_each_entry(sub, &leader->sibling_list, group_entry) {
> +               if (sub->state != PERF_EVENT_STATE_OFF) {
> +                       event_sched_in(sub, cpu);
> +                       ++n;
> +               }
> +       }
> +       ctx->nr_active += n;
> +
> +       /*
> +        * 1 means successful and events are active
> +        * This is not quite true because we defer
> +        * actual activation until hw_perf_enable() but
> +        * this way we* ensure caller won't try to enable
> +        * individual events
> +        */
> +       return 1;
> +}

That most certainly looses error codes for all !is_x86_event() events in
the group.

So you either need to deal with that event_sched_in() failing, or
guarantee that it always succeeds (forcing software events only for
example).