| Message ID | 11aa614c82976adbfa4ea763dbe885b5fb01d59c.1676063532.git.zanussi@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | tracing/histogram: Some fixes for new stacktrace variables | expand |
Hi Tom,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20230210]
[cannot apply to linus/master rostedt-trace/for-next rostedt-trace/for-next-urgent v6.2-rc7 v6.2-rc6 v6.2-rc5 v6.2-rc7]
[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/Tom-Zanussi/tracing-histogram-Don-t-use-strlen-to-find-length-of-stacktrace-variables/20230211-053647
patch link: https://lore.kernel.org/r/11aa614c82976adbfa4ea763dbe885b5fb01d59c.1676063532.git.zanussi%40kernel.org
patch subject: [PATCH 3/4] tracing/histogram: Fix stacktrace key
config: parisc-allyesconfig (https://download.01.org/0day-ci/archive/20230211/202302110636.O2hlhbxt-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/797160b4aa615acf656dc6c8ef6fe41b3c2b84a2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Tom-Zanussi/tracing-histogram-Don-t-use-strlen-to-find-length-of-stacktrace-variables/20230211-053647
git checkout 797160b4aa615acf656dc6c8ef6fe41b3c2b84a2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash kernel/trace/
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/202302110636.O2hlhbxt-lkp@intel.com/
All warnings (new ones prefixed by >>):
kernel/trace/trace_events_hist.c: In function 'event_hist_trigger':
>> kernel/trace/trace_events_hist.c:5261:41: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
5261 | stack = (unsigned long *)field_contents;
| ^
vim +5261 kernel/trace/trace_events_hist.c
5231
5232 static void event_hist_trigger(struct event_trigger_data *data,
5233 struct trace_buffer *buffer, void *rec,
5234 struct ring_buffer_event *rbe)
5235 {
5236 struct hist_trigger_data *hist_data = data->private_data;
5237 bool use_compound_key = (hist_data->n_keys > 1);
5238 unsigned long entries[HIST_STACKTRACE_DEPTH];
5239 u64 var_ref_vals[TRACING_MAP_VARS_MAX];
5240 char compound_key[HIST_KEY_SIZE_MAX];
5241 struct tracing_map_elt *elt = NULL;
5242 struct hist_field *key_field;
5243 u64 field_contents;
5244 void *key = NULL;
5245 unsigned int i;
5246
5247 if (unlikely(!rbe))
5248 return;
5249
5250 memset(compound_key, 0, hist_data->key_size);
5251
5252 for_each_hist_key_field(i, hist_data) {
5253 key_field = hist_data->fields[i];
5254
5255 if (key_field->flags & HIST_FIELD_FL_STACKTRACE) {
5256 memset(entries, 0, HIST_STACKTRACE_SIZE);
5257 if (key_field->field) {
5258 unsigned long *stack, n_entries;
5259
5260 field_contents = hist_fn_call(key_field, elt, buffer, rbe, rec);
> 5261 stack = (unsigned long *)field_contents;
5262 n_entries = *stack;
5263 memcpy(entries, ++stack, n_entries * sizeof(unsigned long));
5264 } else {
5265 stack_trace_save(entries, HIST_STACKTRACE_DEPTH,
5266 HIST_STACKTRACE_SKIP);
5267 }
5268 key = entries;
5269 } else {
5270 field_contents = hist_fn_call(key_field, elt, buffer, rbe, rec);
5271 if (key_field->flags & HIST_FIELD_FL_STRING) {
5272 key = (void *)(unsigned long)field_contents;
5273 use_compound_key = true;
5274 } else
5275 key = (void *)&field_contents;
5276 }
5277
5278 if (use_compound_key)
5279 add_to_key(compound_key, key, key_field, rec);
5280 }
5281
5282 if (use_compound_key)
5283 key = compound_key;
5284
5285 if (hist_data->n_var_refs &&
5286 !resolve_var_refs(hist_data, key, var_ref_vals, false))
5287 return;
5288
5289 elt = tracing_map_insert(hist_data->map, key);
5290 if (!elt)
5291 return;
5292
5293 hist_trigger_elt_update(hist_data, elt, buffer, rec, rbe, var_ref_vals);
5294
5295 if (resolve_var_refs(hist_data, key, var_ref_vals, true))
5296 hist_trigger_actions(hist_data, elt, buffer, rec, rbe, key, var_ref_vals);
5297 }
5298
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index a58380702491..eb812cfdaa88 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -135,6 +135,7 @@ enum hist_field_fn { HIST_FIELD_FN_DIV_NOT_POWER2, HIST_FIELD_FN_DIV_MULT_SHIFT, HIST_FIELD_FN_EXECNAME, + HIST_FIELD_FN_STACK, }; /* @@ -1982,7 +1983,10 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, } if (flags & HIST_FIELD_FL_STACKTRACE) { - hist_field->fn_num = HIST_FIELD_FN_NOP; + if (field) + hist_field->fn_num = HIST_FIELD_FN_STACK; + else + hist_field->fn_num = HIST_FIELD_FN_NOP; hist_field->size = HIST_STACKTRACE_SIZE; hist_field->type = kstrdup_const("unsigned long[]", GFP_KERNEL); if (!hist_field->type) @@ -4269,6 +4273,19 @@ static u64 hist_field_execname(struct hist_field *hist_field, return (u64)(unsigned long)(elt_data->comm); } +static u64 hist_field_stack(struct hist_field *hist_field, + struct tracing_map_elt *elt, + struct trace_buffer *buffer, + struct ring_buffer_event *rbe, + void *event) +{ + u32 str_item = *(u32 *)(event + hist_field->field->offset); + int str_loc = str_item & 0xffff; + char *addr = (char *)(event + str_loc); + + return (u64)(unsigned long)addr; +} + static u64 hist_fn_call(struct hist_field *hist_field, struct tracing_map_elt *elt, struct trace_buffer *buffer, @@ -4332,6 +4349,8 @@ static u64 hist_fn_call(struct hist_field *hist_field, return div_by_mult_and_shift(hist_field, elt, buffer, rbe, event); case HIST_FIELD_FN_EXECNAME: return hist_field_execname(hist_field, elt, buffer, rbe, event); + case HIST_FIELD_FN_STACK: + return hist_field_stack(hist_field, elt, buffer, rbe, event); default: return 0; } @@ -5233,8 +5252,17 @@ static void event_hist_trigger(struct event_trigger_data *data, if (key_field->flags & HIST_FIELD_FL_STACKTRACE) { memset(entries, 0, HIST_STACKTRACE_SIZE); - stack_trace_save(entries, HIST_STACKTRACE_DEPTH, - HIST_STACKTRACE_SKIP); + if (key_field->field) { + unsigned long *stack, n_entries; + + field_contents = hist_fn_call(key_field, elt, buffer, rbe, rec); + stack = (unsigned long *)field_contents; + n_entries = *stack; + memcpy(entries, ++stack, n_entries * sizeof(unsigned long)); + } else { + stack_trace_save(entries, HIST_STACKTRACE_DEPTH, + HIST_STACKTRACE_SKIP); + } key = entries; } else { field_contents = hist_fn_call(key_field, elt, buffer, rbe, rec);
The current code will always use the current stacktrace as a key even if a stacktrace contained in a specific event field was specified. For example, we expect to use the 'unsigned long[] stack' field in the below event in the histogram: # echo 's:block_lat pid_t pid; u64 delta; unsigned long[] stack;' > /sys/kernel/debug/tracing/dynamic_events # echo 'hist:keys=delta.buckets=100,stack.stacktrace:sort=delta' > /sys/kernel/debug/tracing/events/synthetic/block_lat/trigger But in fact, when we type out the trigger, we see that it's using the plain old global 'stacktrace' as the key, which is just the stacktrace when the event was hit and not the stacktrace contained in the event, which is what we want: # cat /sys/kernel/debug/tracing/events/synthetic/block_lat/trigger hist:keys=delta.buckets=100,stacktrace:vals=hitcount:sort=delta.buckets=100:size=2048 [active] And in fact, there's no code to actually retrieve it from the event, so we need to add HIST_FIELD_FN_STACK and hist_field_stack() to get it and hook it into the trigger code. For now, since the stack is just using dynamic strings, this could just use the dynamic string function, but it seems cleaner to have a dedicated function an be able to tweak independently as necessary. Signed-off-by: Tom Zanussi <zanussi@kernel.org> --- kernel/trace/trace_events_hist.c | 34 +++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-)