Re: [PATCH 1/2] perf, x86: Implement event scheduler helper functions

Peter Zijlstra
2011-09-14T14:43:41Z

On Sat, 2011-09-10 at 16:49 +0200, Robert Richter wrote:
> This patch introduces x86 perf scheduler code helper functions. We
> need this to later add more complex functionality to support
> overlapping counter constraints (next patch).
>
> The algorithm is modified so that the range of weight values is now
> generated from the constraints. There shouldn't be other functional
> changes.
>
> With the helper functions the scheduler is controlled. There are
> functions to initialize, traverse the event list, find unused counters
> etc. The scheduler keeps its own state.
>
> Cc: Stephane Eranian <eranian@google.com>
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
> arch/x86/kernel/cpu/perf_event.c | 158 +++++++++++++++++++++++++-------------
> 1 files changed, 105 insertions(+), 53 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index 594d425..44ec767 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -790,18 +790,118 @@ static inline int is_x86_event(struct perf_event *event)
> return event->pmu == &pmu;
> }
>
> +struct sched_state {
> + int weight;
> + int event;
> + int counter;
> + int unassigned;
> + unsigned long used[BITS_TO_LONGS(X86_PMC_IDX_MAX)];
> +};

Maybe add a few comments here? Took me a while to figure out unassigned
is the number of unassigned events.

> +static struct sched_state *perf_sched_find_counter(struct perf_sched *sched)
> +{
> + struct event_constraint *c;
> + int idx;
> +
> + if (!sched->state.unassigned)
> + return NULL;

So bail when we're done and there's nothing left to assign.

> + c = sched->constraints[sched->state.event];
> +
> + idx = sched->state.counter;

Which is typically 0, but this could be a restart, at which point we
continue looking where we left off.

> + /* for each bit in idxmsk starting from idx */
> + while (idx < X86_PMC_IDX_MAX) {
> + idx = find_next_bit(c->idxmsk, X86_PMC_IDX_MAX, idx);
> + if (idx == X86_PMC_IDX_MAX)
> + break;
> + if (!__test_and_set_bit(idx, sched->state.used))
> + break;
> + idx++;
> + }

#define for_each_set_bit_continue(bit, addr, size) \
for( ; (bit) < (size); \
(bit) = find_next_bit((addr), (size), (bit) + 1))

for_each_set_bit_continue(idx, c->idxmask, X86_PMC_IDX_MAX) {
if (!__test_and_set_bit(idx, sched->state.used))
break;
}

> + sched->state.counter = idx;
> +
> + if (idx >= X86_PMC_IDX_MAX)
> + return NULL;

OK, so its important to assign idx to counter even if we're too big,
because of the restart, right? That wants a comment.

> +
> + return &sched->state;
> +}
> +
> +static int perf_sched_next_event(struct perf_sched *sched)
> +{
> + struct event_constraint *c;
> +
> + if (!sched->state.unassigned || !--sched->state.unassigned)
> + return 0;

Shouldn't we avoid getting here if there's nothing to do? I get
the !--unassigned case, but am a bit puzzled by the !unassigned case.

> + do {
> + /* next event */
> + sched->state.event++;
> + if (sched->state.event >= sched->max_events) {
> + /* next weight */
> + sched->state.event = 0;
> + sched->state.weight++;
> + if (sched->state.weight > sched->max_weight)
> + return 0;
> + }
> + c = sched->constraints[sched->state.event];
> + } while (c->weight != sched->state.weight);
> +
> + sched->state.counter = 0; /* start with first counter */
> +
> + return 1;
> +}

fair enough..

Looks ok otherwise, just a tad hard to grok in one go.. a few comments
could go a long way. I'm sure I'll have forgotten how it works in a few
weeks.