Re: [PATCH] perf_events: proposed fix for broken intr throttling (v2)

[Stephane Eranian]

On Sat, Jan 7, 2012 at 6:03 PM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> On Fri, 2012-01-06 at 19:19 +0100, Stephane Eranian wrote:
>> The throttling mechanism REQUIRES that the hwc->interrupts counter be reset
>> at EACH timer tick.
>
> Just to be contrary.. :-)
>
I'll look into that one. But at first glance, it does not address the issue
of adjusting the period at each timer tick. Only multiplexing (rotation) should
happen at the multiplier rate.

> ---
> Âinclude/linux/perf_event.h | Â Â1 +
> Âkernel/events/core.c    |  76 +++++++++++++++++++++++++++++++-------------
> Â2 files changed, 55 insertions(+), 22 deletions(-)
>
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 0b91db2..29dedf4 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -589,6 +589,7 @@ struct hw_perf_event {
> Â Â Â Âu64 Â Â Â Â Â Â Â Â Â Â Â Â Â Â sample_period;
> Â Â Â Âu64 Â Â Â Â Â Â Â Â Â Â Â Â Â Â last_period;
>    Âlocal64_t            period_left;
> + Â Â Â u64 Â Â Â Â Â Â Â Â Â Â Â Â Â Â interrupts_seq;
> Â Â Â Âu64 Â Â Â Â Â Â Â Â Â Â Â Â Â Â interrupts;
>
> Â Â Â Âu64 Â Â Â Â Â Â Â Â Â Â Â Â Â Â freq_time_stamp;
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 91fb68a..693e1ce 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2325,11 +2325,14 @@ static void perf_adjust_period(struct perf_event *event, u64 nsec, u64 count)
> Â Â Â Â}
> Â}
>
> +static DEFINE_PER_CPU(int, perf_throttled_count);
> +static DEFINE_PER_CPU(u64, perf_throttled_seq);
> +
> Âstatic void perf_ctx_adjust_freq(struct perf_event_context *ctx, u64 period)
> Â{
> Â Â Â Âstruct perf_event *event;
> Â Â Â Âstruct hw_perf_event *hwc;
> - Â Â Â u64 interrupts, now;
> + Â Â Â u64 now;
> Â Â Â Âs64 delta;
>
> Â Â Â Âif (!ctx->nr_freq)
> @@ -2342,22 +2345,11 @@ static void perf_ctx_adjust_freq(struct perf_event_context *ctx, u64 period)
> Â Â Â Â Â Â Â Âif (!event_filter_match(event))
> Â Â Â Â Â Â Â Â Â Â Â Âcontinue;
>
> - Â Â Â Â Â Â Â hwc = &event->hw;
> -
> - Â Â Â Â Â Â Â interrupts = hwc->interrupts;
> - Â Â Â Â Â Â Â hwc->interrupts = 0;
> -
> - Â Â Â Â Â Â Â /*
> - Â Â Â Â Â Â Â Â* unthrottle events on the tick
> - Â Â Â Â Â Â Â Â*/
> - Â Â Â Â Â Â Â if (interrupts == MAX_INTERRUPTS) {
> - Â Â Â Â Â Â Â Â Â Â Â perf_log_throttle(event, 1);
> - Â Â Â Â Â Â Â Â Â Â Â event->pmu->start(event, 0);
> - Â Â Â Â Â Â Â }
> -
> Â Â Â Â Â Â Â Âif (!event->attr.freq || !event->attr.sample_freq)
> Â Â Â Â Â Â Â Â Â Â Â Âcontinue;
>
> + Â Â Â Â Â Â Â hwc = &event->hw;
> +
> Â Â Â Â Â Â Â Âevent->pmu->read(event);
> Â Â Â Â Â Â Â Ânow = local64_read(&event->count);
> Â Â Â Â Â Â Â Âdelta = now - hwc->freq_count_stamp;
> @@ -2441,14 +2433,45 @@ static void perf_rotate_context(struct perf_cpu_context *cpuctx)
> Â Â Â Â Â Â Â Âlist_del_init(&cpuctx->rotation_list);
> Â}
>
> +static void perf_unthrottle_context(struct perf_event_context *ctx)
> +{
> + Â Â Â raw_spin_lock(&ctx->lock);
> + Â Â Â list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
> + Â Â Â Â Â Â Â if (event->state != PERF_EVENT_STATE_ACTIVE)
> + Â Â Â Â Â Â Â Â Â Â Â continue;
> +
> + Â Â Â Â Â Â Â if (!event_filter_match(event))
> + Â Â Â Â Â Â Â Â Â Â Â continue;
> +
> + Â Â Â Â Â Â Â if (event->hw.interrupts == MAX_INTERRUPTS) {
> + Â Â Â Â Â Â Â Â Â Â Â perf_log_throttle(event, 1);
> + Â Â Â Â Â Â Â Â Â Â Â event->pmu->start(event, 0);
> + Â Â Â Â Â Â Â }
> + Â Â Â }
> + Â Â Â raw_spin_unlock(&ctx->lock);
> +}
> +
> Âvoid perf_event_task_tick(void)
> Â{
> Â Â Â Âstruct list_head *head = &__get_cpu_var(rotation_list);
> Â Â Â Âstruct perf_cpu_context *cpuctx, *tmp;
> + Â Â Â int throttle;
>
> Â Â Â ÂWARN_ON(!irqs_disabled());
>
> + Â Â Â __this_cpu_inc(perf_throttled_seq);
> + Â Â Â throttled = __this_cpu_xchg(perf_throttled_count, 0);
> +
> Â Â Â Âlist_for_each_entry_safe(cpuctx, tmp, head, rotation_list) {
> + Â Â Â Â Â Â Â if (throttled) {
> + Â Â Â Â Â Â Â Â Â Â Â struct perf_event_context *ctx;
> +
> + Â Â Â Â Â Â Â Â Â Â Â perf_unthrottle_context(&cpuctx->ctx);
> + Â Â Â Â Â Â Â Â Â Â Â ctx = cpuctx->task_ctx;
> + Â Â Â Â Â Â Â Â Â Â Â if (ctx)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â perf_unthrottle_context(ctx);
> + Â Â Â Â Â Â Â }
> +
> Â Â Â Â Â Â Â Âif (cpuctx->jiffies_interval == 1 ||
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â!(jiffies % cpuctx->jiffies_interval))
> Â Â Â Â Â Â Â Â Â Â Â Âperf_rotate_context(cpuctx);
> @@ -4514,6 +4537,7 @@ static int __perf_event_overflow(struct perf_event *event,
> Â{
> Â Â Â Âint events = atomic_read(&event->event_limit);
> Â Â Â Âstruct hw_perf_event *hwc = &event->hw;
> + Â Â Â u64 seq;
> Â Â Â Âint ret = 0;
>
> Â Â Â Â/*
> @@ -4523,14 +4547,22 @@ static int __perf_event_overflow(struct perf_event *event,
> Â Â Â Âif (unlikely(!is_sampling_event(event)))
> Â Â Â Â Â Â Â Âreturn 0;
>
> - Â Â Â if (unlikely(hwc->interrupts >= max_samples_per_tick)) {
> - Â Â Â Â Â Â Â if (throttle) {
> - Â Â Â Â Â Â Â Â Â Â Â hwc->interrupts = MAX_INTERRUPTS;
> - Â Â Â Â Â Â Â Â Â Â Â perf_log_throttle(event, 0);
> - Â Â Â Â Â Â Â Â Â Â Â ret = 1;
> - Â Â Â Â Â Â Â }
> - Â Â Â } else
> - Â Â Â Â Â Â Â hwc->interrupts++;
> + Â Â Â seq = ACCESS_ONCE(__this_cpu_read(perf_throttled_seq));
> +
> + Â Â Â if (seq != hwc->interrupts_seq) {
> + Â Â Â Â Â Â Â hwc->interrupts_seq = seq;
> + Â Â Â Â Â Â Â hwc->interrupts = 1;
> + Â Â Â } else {
> + Â Â Â Â Â Â Â if (unlikely(hwc->interrupts >= max_samples_per_tick)) {
> + Â Â Â Â Â Â Â Â Â Â Â if (throttle) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â __this_cpu_inc(perf_throttled_count);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â hwc->interrupts = MAX_INTERRUPTS;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â perf_log_throttle(event, 0);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ret = 1;
> + Â Â Â Â Â Â Â Â Â Â Â }
> + Â Â Â Â Â Â Â } else
> + Â Â Â Â Â Â Â Â Â Â Â hwc->interrupts++;
> + Â Â Â }
>
> Â Â Â Âif (event->attr.freq) {
> Â Â Â Â Â Â Â Âu64 now = perf_clock();
>
èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—