Re: [PATCH] perf cgroups: Don't rotate events for cgroups unnecessarily

[Ian Rogers <irogers@google.com>]

On Fri, Jun 21, 2019 at 1:24 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Sat, Jun 01, 2019 at 01:27:22AM -0700, Ian Rogers wrote:
> > @@ -3325,6 +3331,15 @@ static int flexible_sched_in(struct perf_event *event, void *data)
> >                       sid->can_add_hw = 0;
> >       }
> >
> > +     /*
> > +      * If the group wasn't scheduled then set that multiplexing is necessary
> > +      * for the context. Note, this won't be set if the event wasn't
> > +      * scheduled due to event_filter_match failing due to the earlier
> > +      * return.
> > +      */
> > +     if (event->state == PERF_EVENT_STATE_INACTIVE)
> > +             sid->ctx->rotate_necessary = 1;
> > +
> >       return 0;
> >  }
>
> That looked odd; which had me look harder at this function which
> resulted in the below. Should we not terminate the context interation
> the moment one flexible thingy fails to schedule?

If we knew all the events were hardware events then this would be
true, as there may be software events that always schedule then the
continued iteration is necessary.

Thanks,
Ian

> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2314,12 +2314,8 @@ group_sched_in(struct perf_event *group_
>                 return 0;
>
>         pmu->start_txn(pmu, PERF_PMU_TXN_ADD);
> -
> -       if (event_sched_in(group_event, cpuctx, ctx)) {
> -               pmu->cancel_txn(pmu);
> -               perf_mux_hrtimer_restart(cpuctx);
> -               return -EAGAIN;
> -       }
> +       if (event_sched_in(group_event, cpuctx, ctx))
> +               goto cancel;
>
>         /*
>          * Schedule in siblings as one group (if any):
> @@ -2348,10 +2344,9 @@ group_sched_in(struct perf_event *group_
>         }
>         event_sched_out(group_event, cpuctx, ctx);
>
> +cancel:
>         pmu->cancel_txn(pmu);
> -
>         perf_mux_hrtimer_restart(cpuctx);
> -
>         return -EAGAIN;
>  }
>
> @@ -3317,6 +3312,7 @@ static int pinned_sched_in(struct perf_e
>  static int flexible_sched_in(struct perf_event *event, void *data)
>  {
>         struct sched_in_data *sid = data;
> +       int ret;
>
>         if (event->state <= PERF_EVENT_STATE_OFF)
>                 return 0;
> @@ -3325,21 +3321,15 @@ static int flexible_sched_in(struct perf
>                 return 0;
>
>         if (group_can_go_on(event, sid->cpuctx, sid->can_add_hw)) {
> -               if (!group_sched_in(event, sid->cpuctx, sid->ctx))
> -                       list_add_tail(&event->active_list, &sid->ctx->flexible_active);
> -               else
> +               ret = group_sched_in(event, sid->cpuctx, sid->ctx);
> +               if (ret) {
>                         sid->can_add_hw = 0;
> +                       sid->ctx->rotate_necessary = 1;
> +                       return ret;
> +               }
> +               list_add_tail(&event->active_list, &sid->ctx->flexible_active);
>         }
>
> -       /*
> -        * If the group wasn't scheduled then set that multiplexing is necessary
> -        * for the context. Note, this won't be set if the event wasn't
> -        * scheduled due to event_filter_match failing due to the earlier
> -        * return.
> -        */
> -       if (event->state == PERF_EVENT_STATE_INACTIVE)
> -               sid->ctx->rotate_necessary = 1;
> -
>         return 0;
>  }
>