| Message ID | 20230322151843.14390-2-lukasz.luba@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add basic tracing for uclamp and schedutil | expand |
Hi Lukasz! On 03/22/23 15:18, Lukasz Luba wrote: > The user-space can set uclamp value for a given task. It impacts task > placement decisions made by the scheduler. This is very useful information > and helps to understand the system behavior or track improvements in > middleware and applications which start using uclamp mechanisms and report > better performance in tests. We do have uclamp trace events in sched_tp, why are they not sufficient? https://github.com/qais-yousef/sched_tp/blob/main/sched_events.h#L233 Do you really want to know the exact time the value has changed? Would it make sense to introduce a generic sched_setscheduler tracepoint instead? Although this might not be necessary as I think we can use register_kprobe() to register a callback and create a new event without any additional tracepoint. sched_setscheduler() is not inlined so should be easy to hook into and create events, no? Thanks -- Qais Yousef > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > include/trace/events/sched.h | 4 ++++ > kernel/sched/core.c | 5 +++++ > 2 files changed, 9 insertions(+) > > diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h > index fbb99a61f714..dbfb30809f15 100644 > --- a/include/trace/events/sched.h > +++ b/include/trace/events/sched.h > @@ -735,6 +735,10 @@ DECLARE_TRACE(sched_update_nr_running_tp, > TP_PROTO(struct rq *rq, int change), > TP_ARGS(rq, change)); > > +DECLARE_TRACE(uclamp_update_tsk_tp, > + TP_PROTO(struct task_struct *tsk, int uclamp_id, unsigned int value), > + TP_ARGS(tsk, uclamp_id, value)); > + > #endif /* _TRACE_SCHED_H */ > > /* This part must be outside protection */ > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 488655f2319f..882c92e3ccf0 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -110,6 +110,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(sched_overutilized_tp); > EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_cfs_tp); > EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_se_tp); > EXPORT_TRACEPOINT_SYMBOL_GPL(sched_update_nr_running_tp); > +EXPORT_TRACEPOINT_SYMBOL_GPL(uclamp_update_tsk_tp); > > DEFINE_PER_CPU_SHARED_ALIGNED(struct rq, runqueues); > > @@ -1936,12 +1937,16 @@ static void __setscheduler_uclamp(struct task_struct *p, > attr->sched_util_min != -1) { > uclamp_se_set(&p->uclamp_req[UCLAMP_MIN], > attr->sched_util_min, true); > + trace_uclamp_update_tsk_tp(p, UCLAMP_MIN, > + attr->sched_util_min); > } > > if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MAX && > attr->sched_util_max != -1) { > uclamp_se_set(&p->uclamp_req[UCLAMP_MAX], > attr->sched_util_max, true); > + trace_uclamp_update_tsk_tp(p, UCLAMP_MAX, > + attr->sched_util_max); > } > } > > -- > 2.17.1 >
Hi Qais, On 4/3/23 14:46, Qais Yousef wrote: > Hi Lukasz! > > On 03/22/23 15:18, Lukasz Luba wrote: >> The user-space can set uclamp value for a given task. It impacts task >> placement decisions made by the scheduler. This is very useful information >> and helps to understand the system behavior or track improvements in >> middleware and applications which start using uclamp mechanisms and report >> better performance in tests. > > We do have uclamp trace events in sched_tp, why are they not sufficient? > > https://github.com/qais-yousef/sched_tp/blob/main/sched_events.h#L233 > > Do you really want to know the exact time the value has changed? Yes, that's why these new are triggered instantly after userspace wanted to set the new uclamp values. We are going to have a few different uclamp implementations: one in mainline and X in Android vendor kernels. The goal is to have only one... We will have to experiment to find the behavior of those algorithms and understand the differences. Since uclamp is part of this 'control-chain' of CPU frequency and also task placement - it would be really tricky to figure our everything. The analysis on traces are crucial for this. > > Would it make sense to introduce a generic sched_setscheduler tracepoint > instead? Although this might not be necessary as I think we can use > register_kprobe() to register a callback and create a new event without any > additional tracepoint. sched_setscheduler() is not inlined so should be easy to > hook into and create events, no? This looks very complex and we already have our LISA tool with the module to change the tracepoints into trace events and build them. I wanted to be aligned with that design, which might look a bit old-fashion but is simple IMO. The 'sched_setscheduler tracepoint' might be a too big for this purpose. Thanks for the comments :) Lukasz > > > Thanks > > -- > Qais Yousef > >> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >> --- >> include/trace/events/sched.h | 4 ++++ >> kernel/sched/core.c | 5 +++++ >> 2 files changed, 9 insertions(+) >> >> diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h >> index fbb99a61f714..dbfb30809f15 100644 >> --- a/include/trace/events/sched.h >> +++ b/include/trace/events/sched.h >> @@ -735,6 +735,10 @@ DECLARE_TRACE(sched_update_nr_running_tp, >> TP_PROTO(struct rq *rq, int change), >> TP_ARGS(rq, change)); >> >> +DECLARE_TRACE(uclamp_update_tsk_tp, >> + TP_PROTO(struct task_struct *tsk, int uclamp_id, unsigned int value), >> + TP_ARGS(tsk, uclamp_id, value)); >> + >> #endif /* _TRACE_SCHED_H */ >> >> /* This part must be outside protection */ >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c >> index 488655f2319f..882c92e3ccf0 100644 >> --- a/kernel/sched/core.c >> +++ b/kernel/sched/core.c >> @@ -110,6 +110,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(sched_overutilized_tp); >> EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_cfs_tp); >> EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_se_tp); >> EXPORT_TRACEPOINT_SYMBOL_GPL(sched_update_nr_running_tp); >> +EXPORT_TRACEPOINT_SYMBOL_GPL(uclamp_update_tsk_tp); >> >> DEFINE_PER_CPU_SHARED_ALIGNED(struct rq, runqueues); >> >> @@ -1936,12 +1937,16 @@ static void __setscheduler_uclamp(struct task_struct *p, >> attr->sched_util_min != -1) { >> uclamp_se_set(&p->uclamp_req[UCLAMP_MIN], >> attr->sched_util_min, true); >> + trace_uclamp_update_tsk_tp(p, UCLAMP_MIN, >> + attr->sched_util_min); >> } >> >> if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MAX && >> attr->sched_util_max != -1) { >> uclamp_se_set(&p->uclamp_req[UCLAMP_MAX], >> attr->sched_util_max, true); >> + trace_uclamp_update_tsk_tp(p, UCLAMP_MAX, >> + attr->sched_util_max); >> } >> } >> >> -- >> 2.17.1 >>
On 04/03/23 17:47, Lukasz Luba wrote: > Hi Qais, > > On 4/3/23 14:46, Qais Yousef wrote: > > Hi Lukasz! > > > > On 03/22/23 15:18, Lukasz Luba wrote: > > > The user-space can set uclamp value for a given task. It impacts task > > > placement decisions made by the scheduler. This is very useful information > > > and helps to understand the system behavior or track improvements in > > > middleware and applications which start using uclamp mechanisms and report > > > better performance in tests. > > > > We do have uclamp trace events in sched_tp, why are they not sufficient? > > > > https://github.com/qais-yousef/sched_tp/blob/main/sched_events.h#L233 > > > > Do you really want to know the exact time the value has changed? > > Yes, that's why these new are triggered instantly after userspace wanted > to set the new uclamp values. We are going to have a few different > uclamp implementations: one in mainline and X in Android vendor kernels. This is not true. As you can see everyone tries to push fixes for issues they find, but this not happening fast enough and they get forced to carry out of tree stuff at the end. Out of tree stuff for the broken bits that is. > The goal is to have only one... We will have to experiment to find This statement gives a very strong message to everyone out here. And I'd like to stress strongly that this is NOT true. Broken bits, yes. But essential bits that works are used the same. We can do a better job and fix things faster upstream. I am committed to sorting all these stuff out anyway. > the behavior of those algorithms and understand the differences. Since > uclamp is part of this 'control-chain' of CPU frequency and also > task placement - it would be really tricky to figure our everything. > The analysis on traces are crucial for this. I think the existing uclamp trace event is enough to be honest. But up to the maintainer if they want to add this new specific one. The two tps seem a bit of a clutter to me. With kprobes and bpf a lot can be done on the fly if you want to reverse engineer some stuff. > > > > > Would it make sense to introduce a generic sched_setscheduler tracepoint > > instead? Although this might not be necessary as I think we can use > > register_kprobe() to register a callback and create a new event without any > > additional tracepoint. sched_setscheduler() is not inlined so should be easy to > > hook into and create events, no? > > This looks very complex and we already have our LISA tool with the It's not. Here's a PoC that only does a trace_printk(), it's simple. You don't need to do it in a module even, see below. https://github.com/qais-yousef/sched_tp/compare/main...kprobe-poc It did highlight that sugov_should_update_freq() ends up actually being inlined though :(. It should work for sched_setscheduler(). You'd want to use register_kprobe() instead for that. > module to change the tracepoints into trace events and build them. > I wanted to be aligned with that design, which might look a bit > old-fashion but is simple IMO. trace-cmd, bpf and I believe perf, all can do the same; and they support kprobes not just tracepoints. And they all boil down to the same underlying mechanism https://www.kernel.org/doc/html/v6.1/trace/kprobetrace.html No need to re-invent a new wheel ;-) > The 'sched_setscheduler tracepoint' might be a too big for this > purpose. Sorry I am usually supportive for more tracepoints, but I feel skeptical this time. That said, I really don't mind if the maintainers are okay with it. So while I'm not convinced, but I don't have any objection either. > Thanks for the comments :) Thanks! -- Qais Yousef
On 4/4/23 18:17, Qais Yousef wrote: > On 04/03/23 17:47, Lukasz Luba wrote: >> Hi Qais, >> >> On 4/3/23 14:46, Qais Yousef wrote: >>> Hi Lukasz! >>> >>> On 03/22/23 15:18, Lukasz Luba wrote: >>>> The user-space can set uclamp value for a given task. It impacts task >>>> placement decisions made by the scheduler. This is very useful information >>>> and helps to understand the system behavior or track improvements in >>>> middleware and applications which start using uclamp mechanisms and report >>>> better performance in tests. >>> >>> We do have uclamp trace events in sched_tp, why are they not sufficient? >>> >>> https://github.com/qais-yousef/sched_tp/blob/main/sched_events.h#L233 >>> >>> Do you really want to know the exact time the value has changed? >> >> Yes, that's why these new are triggered instantly after userspace wanted >> to set the new uclamp values. We are going to have a few different >> uclamp implementations: one in mainline and X in Android vendor kernels. > > This is not true. As you can see everyone tries to push fixes for issues they Could you clarify which part is not true? 1. We have X vendor hook implementations how uclamp is interpreted + one upstream 2. we are going to have such situation for a while (till we don't meet requirements in mainline uclamp, so the hooks can be removed and we would have the same mechanism under the hood that we understand) 3. the user-space sets uclamp value via syscall > find, but this not happening fast enough and they get forced to carry out of > tree stuff at the end. Out of tree stuff for the broken bits that is. > >> The goal is to have only one... We will have to experiment to find > > This statement gives a very strong message to everyone out here. And I'd like > to stress strongly that this is NOT true. Broken bits, yes. But essential bits > that works are used the same. Could you elaborate this? This is how I view the solution with uclamp, but I'm afraid we have different views: Uclamp is part of the control algorithms in a closed feedback loop. The apps want to use (as stated in the latest kernel doc) uclamp in order get desired performance (e.g. no frame drops) in an environment, which: 1. can change with a CPUs configuration that the app was installed (single app from a repo want to run efficient on various devices; those devices create different environment, from the beginning) 2. the thermal conditions can change in run-time, so the capacity of the platform might change (even to a point that some CPUs is out) 3. has access to a dedicated accelerators, which can handle some of heavy computations from the CPUs (e.g. those with neural networks) 4. drivers and devices have different capabilities e.g. in terms of fast DVFS change, which can be and issue in this case When the uclamp algorithm, which interprets the individual tasks' values and runqueue value, can be different, then this is a different solution. In such situation every platform might have different solution. We are going to focus on mainline uclamp development. We want to know if the frequency cannot be changed due to some slow HW or a huge traffic in the system of various tasks/runqueues wanted to change the freq, or just wrong setting in the schedutil filter. We are also interested in measuring the 'delay' from the uclamp new request till the HW getting it actually. All of this would shape the solutions 'quality'. I don't know if vendors justify their vendor hook uclamp implementation as a fix for 'broken bits' or a 'value-added'. > > We can do a better job and fix things faster upstream. I am committed to > sorting all these stuff out anyway. > >> the behavior of those algorithms and understand the differences. Since >> uclamp is part of this 'control-chain' of CPU frequency and also >> task placement - it would be really tricky to figure our everything. >> The analysis on traces are crucial for this. > > I think the existing uclamp trace event is enough to be honest. But up to the > maintainer if they want to add this new specific one. The two tps seem a bit of > a clutter to me. With kprobes and bpf a lot can be done on the fly if you want > to reverse engineer some stuff. > >> >>> >>> Would it make sense to introduce a generic sched_setscheduler tracepoint >>> instead? Although this might not be necessary as I think we can use >>> register_kprobe() to register a callback and create a new event without any >>> additional tracepoint. sched_setscheduler() is not inlined so should be easy to >>> hook into and create events, no? >> >> This looks very complex and we already have our LISA tool with the > > It's not. Here's a PoC that only does a trace_printk(), it's simple. You don't > need to do it in a module even, see below. > > https://github.com/qais-yousef/sched_tp/compare/main...kprobe-poc > > It did highlight that sugov_should_update_freq() ends up actually being inlined > though :(. It should work for sched_setscheduler(). You'd want to use > register_kprobe() instead for that. Issues with that approach: - as you said, it won't work with inline functions, so in this patch set example it would work 'partially' ;) - uses trace_printk() which is not aligned method to our tool and AFAIK Perfetto - would create mix of mechanisms that would push the complexity for our analysis tool, which we don't want. This is the complexity that I had in mind from SW engineering perspective. Maintainability and understanding of the mechanics across teams or even companies is not for free. Then explaining someone in different company that this approach won't work if the compiler inlined a function, so they have to rewrite the code and use the other approach (with tracepoint in the code) doesn't sound good. > >> module to change the tracepoints into trace events and build them. >> I wanted to be aligned with that design, which might look a bit >> old-fashion but is simple IMO. > > trace-cmd, bpf and I believe perf, all can do the same; and they support > kprobes not just tracepoints. > > And they all boil down to the same underlying mechanism > > https://www.kernel.org/doc/html/v6.1/trace/kprobetrace.html > > No need to re-invent a new wheel ;-) I don't understand why you are calling this 're-invent a new wheel', when I said this is aligned with our SW design for trace analysis. BTW, the LISA tool is open source and anyone can use it for free [1]. I'm not developing another framework for tracepoints here, I just use this approach. I agree, there is a few ways of doing this. The worse thing would be for a us to use a mix (and mess) a few of them. As you know, our tool takes the path with one of them: tracepoints explicitly visible to everyone in the kernel code, so you can point engineers into it. I know, you can basically trace almost any function dynamically which creates huge potential space. The big space of opportunity isn't good when you want to solve the problem and share the knowledge to others how to follow. Look at the example of tracepoints which are used for deriving the task wake-up latency. It's easier for engineers to see that in the code with the tracepoints, what parts are important. Where and why the flow is going, which is a nice example how the vast code space can been reduced. > >> The 'sched_setscheduler tracepoint' might be a too big for this >> purpose. > > Sorry I am usually supportive for more tracepoints, but I feel skeptical this > time. That said, I really don't mind if the maintainers are okay with it. So > while I'm not convinced, but I don't have any objection either. I hope I would convince you with those examples, so you would be less skeptical. Cheers, Lukasz [1] https://lisa-linux-integrated-system-analysis.readthedocs.io/en/latest/setup.html
diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h index fbb99a61f714..dbfb30809f15 100644 --- a/include/trace/events/sched.h +++ b/include/trace/events/sched.h @@ -735,6 +735,10 @@ DECLARE_TRACE(sched_update_nr_running_tp, TP_PROTO(struct rq *rq, int change), TP_ARGS(rq, change)); +DECLARE_TRACE(uclamp_update_tsk_tp, + TP_PROTO(struct task_struct *tsk, int uclamp_id, unsigned int value), + TP_ARGS(tsk, uclamp_id, value)); + #endif /* _TRACE_SCHED_H */ /* This part must be outside protection */ diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 488655f2319f..882c92e3ccf0 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -110,6 +110,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(sched_overutilized_tp); EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_cfs_tp); EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_se_tp); EXPORT_TRACEPOINT_SYMBOL_GPL(sched_update_nr_running_tp); +EXPORT_TRACEPOINT_SYMBOL_GPL(uclamp_update_tsk_tp); DEFINE_PER_CPU_SHARED_ALIGNED(struct rq, runqueues); @@ -1936,12 +1937,16 @@ static void __setscheduler_uclamp(struct task_struct *p, attr->sched_util_min != -1) { uclamp_se_set(&p->uclamp_req[UCLAMP_MIN], attr->sched_util_min, true); + trace_uclamp_update_tsk_tp(p, UCLAMP_MIN, + attr->sched_util_min); } if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MAX && attr->sched_util_max != -1) { uclamp_se_set(&p->uclamp_req[UCLAMP_MAX], attr->sched_util_max, true); + trace_uclamp_update_tsk_tp(p, UCLAMP_MAX, + attr->sched_util_max); } }
The user-space can set uclamp value for a given task. It impacts task placement decisions made by the scheduler. This is very useful information and helps to understand the system behavior or track improvements in middleware and applications which start using uclamp mechanisms and report better performance in tests. Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> --- include/trace/events/sched.h | 4 ++++ kernel/sched/core.c | 5 +++++ 2 files changed, 9 insertions(+)