Re: [RESEND][PATCH v2 3/3] schedutil: trace: Add tracing to capture filter out requests

[Lukasz Luba]

Hi Rafael,

On 6/20/23 18:40, Rafael J. Wysocki wrote:
> On Mon, May 22, 2023 at 4:57 PM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
> > Some of the frequency update requests coming form the task scheduler
> > might be filter out. It can happen when the previous request was served
> > not that long ago (in a period smaller than provided by the cpufreq driver
> > as minimum for frequency update). In such case, we want to know if some of
> > the frequency updates cannot make through.
> > Export the new tracepoint as well. That would allow to handle it by a
> > toolkit for trace analyzes.
> >
> > Reported-by: kernel test robot <lkp@xxxxxxxxx> # solved tricky build
> > Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> > ---
> >   include/trace/events/sched.h     |  4 ++++
> >   kernel/sched/cpufreq_schedutil.c | 10 ++++++++--
> >   2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
> > index dbfb30809f15..e34b7cd5de73 100644
> > --- a/include/trace/events/sched.h
> > +++ b/include/trace/events/sched.h
> > @@ -739,6 +739,10 @@ DECLARE_TRACE(uclamp_update_tsk_tp,
> >          TP_PROTO(struct task_struct *tsk, int uclamp_id,  unsigned int value),
> >          TP_ARGS(tsk, uclamp_id, value));
> >
> > +DECLARE_TRACE(schedutil_update_filtered_tp,
> > +       TP_PROTO(int cpu),
> > +       TP_ARGS(cpu));
> > +
> >   #endif /* _TRACE_SCHED_H */
> >
> >   /* This part must be outside protection */
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index f462496e5c07..4f9daf258a65 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -6,6 +6,8 @@
> >    * Author: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >    */
> >
> > +EXPORT_TRACEPOINT_SYMBOL_GPL(schedutil_update_filtered_tp);
> > +
> >   #define IOWAIT_BOOST_MIN       (SCHED_CAPACITY_SCALE / 8)
> >
> >   struct sugov_tunables {
> > @@ -318,8 +320,10 @@ static inline bool sugov_update_single_common(struct sugov_cpu *sg_cpu,
> >
> >          ignore_dl_rate_limit(sg_cpu);
> >
> > -       if (!sugov_should_update_freq(sg_cpu->sg_policy, time))
> > +       if (!sugov_should_update_freq(sg_cpu->sg_policy, time)) {
> > +               trace_schedutil_update_filtered_tp(sg_cpu->cpu);
>
> It looks like the tracepoint can be added to
> sugov_should_update_freq() for less code duplication.

Make sense. I will move that trace there.

In such case, of movement that trace call...
Based on your comment for patch 2/3 I got impression
that you still want it. For me it looks more 'aligned' w/ that
patch 2/3. The two functions code flows:
sugov_update_shared() and sugov_update_single_common() - how
they call and interpret result from
sugov_should_update_freq() - is more clear IMO.

So I will keep that patch 2/3 in the next version. Although,
if you don't like it - please tell me and I will drop it.

Thanks for the review!

Lukasz