| Message ID | 20230322151843.14390-4-lukasz.luba@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add basic tracing for uclamp and schedutil | expand |
Hi Lukasz,
I love your patch! Perhaps something to improve:
[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on tip/sched/core linus/master v6.3-rc3 next-20230322]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Lukasz-Luba/sched-tp-Add-new-tracepoint-to-track-uclamp-set-from-user-space/20230322-232206
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20230322151843.14390-4-lukasz.luba%40arm.com
patch subject: [PATCH 3/3] schedutil: trace: Add tracing to capture filter out requests
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230323/202303230130.BVOhzn4o-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/9eba04b3431f67b8e0c18ff57ea2976111673bea
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Lukasz-Luba/sched-tp-Add-new-tracepoint-to-track-uclamp-set-from-user-space/20230322-232206
git checkout 9eba04b3431f67b8e0c18ff57ea2976111673bea
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303230130.BVOhzn4o-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from include/trace/trace_events.h:27,
from include/trace/define_trace.h:102,
from include/trace/events/schedutil.h:17,
from kernel/sched/cpufreq_schedutil.c:10,
from kernel/sched/build_utility.c:69:
>> include/trace/stages/init.h:2:23: warning: 'str__schedutil__trace_system_name' defined but not used [-Wunused-const-variable=]
2 | #define __app__(x, y) str__##x##y
| ^~~~~
include/trace/stages/init.h:3:21: note: in expansion of macro '__app__'
3 | #define __app(x, y) __app__(x, y)
| ^~~~~~~
include/trace/stages/init.h:5:29: note: in expansion of macro '__app'
5 | #define TRACE_SYSTEM_STRING __app(TRACE_SYSTEM_VAR,__trace_system_name)
| ^~~~~
include/trace/stages/init.h:8:27: note: in expansion of macro 'TRACE_SYSTEM_STRING'
8 | static const char TRACE_SYSTEM_STRING[] = \
| ^~~~~~~~~~~~~~~~~~~
include/trace/stages/init.h:11:1: note: in expansion of macro 'TRACE_MAKE_SYSTEM_STR'
11 | TRACE_MAKE_SYSTEM_STR();
| ^~~~~~~~~~~~~~~~~~~~~
vim +/str__schedutil__trace_system_name +2 include/trace/stages/init.h
af6b9668e85ffd Steven Rostedt (Google 2022-03-03 @2) #define __app__(x, y) str__##x##y
af6b9668e85ffd Steven Rostedt (Google 2022-03-03 3) #define __app(x, y) __app__(x, y)
af6b9668e85ffd Steven Rostedt (Google 2022-03-03 4)
On 03/22/23 15:18, Lukasz Luba 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. I think you can use register_kretprobe() for this one. When functions are not inlined it should be easy to hook into them and you can get the return value of the function too. Check the usage in lib/test_kprobes.c. Creating the event in sched_tp should be the same way when registering for a tracepoint. They both are essentially the same. Patches to sched_tp module would be welcome ;-) Cheers -- Qais Yousef > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > include/trace/events/schedutil.h | 17 +++++++++++++++++ > kernel/sched/cpufreq_schedutil.c | 14 ++++++++++++-- > 2 files changed, 29 insertions(+), 2 deletions(-) > create mode 100644 include/trace/events/schedutil.h > > diff --git a/include/trace/events/schedutil.h b/include/trace/events/schedutil.h > new file mode 100644 > index 000000000000..7f25122f7257 > --- /dev/null > +++ b/include/trace/events/schedutil.h > @@ -0,0 +1,17 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#undef TRACE_SYSTEM > +#define TRACE_SYSTEM schedutil > + > +#if !defined(_TRACE_SCHEDUTIL_H) || defined(TRACE_HEADER_MULTI_READ) > +#define _TRACE_SCHEDUTIL_H > + > +#include <linux/tracepoint.h> > + > +DECLARE_TRACE(schedutil_update_filtered_tp, > + TP_PROTO(int cpu), > + TP_ARGS(cpu)); > + > +#endif /* _TRACE_SCHEDUTIL_H */ > + > +/* This part must be outside protection */ > +#include <trace/define_trace.h> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index f462496e5c07..45c18559f3a8 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -6,6 +6,12 @@ > * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > */ > > +#define CREATE_TRACE_POINTS > +#include <trace/events/schedutil.h> > +#undef CREATE_TRACE_POINTS > + > +EXPORT_TRACEPOINT_SYMBOL_GPL(schedutil_update_filtered_tp); > + > #define IOWAIT_BOOST_MIN (SCHED_CAPACITY_SCALE / 8) > > struct sugov_tunables { > @@ -318,8 +324,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); > return false; > + } > > sugov_get_util(sg_cpu); > sugov_iowait_apply(sg_cpu, time, max_cap); > @@ -446,8 +454,10 @@ sugov_update_shared(struct update_util_data *hook, u64 time, unsigned int flags) > > ignore_dl_rate_limit(sg_cpu); > > - if (!sugov_should_update_freq(sg_policy, time)) > + if (!sugov_should_update_freq(sg_policy, time)) { > + trace_schedutil_update_filtered_tp(sg_cpu->cpu); > goto unlock; > + } > > next_f = sugov_next_freq_shared(sg_cpu, time); > > -- > 2.17.1 >
On 4/3/23 14:46, Qais Yousef wrote: > On 03/22/23 15:18, Lukasz Luba 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. > > I think you can use register_kretprobe() for this one. When functions are not > inlined it should be easy to hook into them and you can get the return value of > the function too. Could be possible, but that's a different pattern to what we have in terms of LISa. It's also a bit complicated for downstream folks to understand when I would ask about the trace with a particular name. The integration would be a bit more complicated and our tooling and CI might be impacted but this different design approach. Therefore, I prefer to be aligned with the old-school. > > Check the usage in lib/test_kprobes.c. Creating the event in sched_tp should be > the same way when registering for a tracepoint. They both are essentially the > same. > > Patches to sched_tp module would be welcome ;-) I cannot promise, but I'll try to find some cycles for it :) Cheers, Lukasz > > > Cheers > > -- > Qais Yousef >
diff --git a/include/trace/events/schedutil.h b/include/trace/events/schedutil.h new file mode 100644 index 000000000000..7f25122f7257 --- /dev/null +++ b/include/trace/events/schedutil.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#undef TRACE_SYSTEM +#define TRACE_SYSTEM schedutil + +#if !defined(_TRACE_SCHEDUTIL_H) || defined(TRACE_HEADER_MULTI_READ) +#define _TRACE_SCHEDUTIL_H + +#include <linux/tracepoint.h> + +DECLARE_TRACE(schedutil_update_filtered_tp, + TP_PROTO(int cpu), + TP_ARGS(cpu)); + +#endif /* _TRACE_SCHEDUTIL_H */ + +/* This part must be outside protection */ +#include <trace/define_trace.h> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c index f462496e5c07..45c18559f3a8 100644 --- a/kernel/sched/cpufreq_schedutil.c +++ b/kernel/sched/cpufreq_schedutil.c @@ -6,6 +6,12 @@ * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> */ +#define CREATE_TRACE_POINTS +#include <trace/events/schedutil.h> +#undef CREATE_TRACE_POINTS + +EXPORT_TRACEPOINT_SYMBOL_GPL(schedutil_update_filtered_tp); + #define IOWAIT_BOOST_MIN (SCHED_CAPACITY_SCALE / 8) struct sugov_tunables { @@ -318,8 +324,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); return false; + } sugov_get_util(sg_cpu); sugov_iowait_apply(sg_cpu, time, max_cap); @@ -446,8 +454,10 @@ sugov_update_shared(struct update_util_data *hook, u64 time, unsigned int flags) ignore_dl_rate_limit(sg_cpu); - if (!sugov_should_update_freq(sg_policy, time)) + if (!sugov_should_update_freq(sg_policy, time)) { + trace_schedutil_update_filtered_tp(sg_cpu->cpu); goto unlock; + } next_f = sugov_next_freq_shared(sg_cpu, time);
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. Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> --- include/trace/events/schedutil.h | 17 +++++++++++++++++ kernel/sched/cpufreq_schedutil.c | 14 ++++++++++++-- 2 files changed, 29 insertions(+), 2 deletions(-) create mode 100644 include/trace/events/schedutil.h