* [PATCH 0/2] A couple hist trigger patches
@ 2019-02-04 21:07 Tom Zanussi
2019-02-04 21:07 ` [PATCH 1/2] tracing: Use str_has_prefix() in synth_event_create() Tom Zanussi
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Tom Zanussi @ 2019-02-04 21:07 UTC (permalink / raw)
To: rostedt; +Cc: linux-kernel, linux-rt-users, Tom Zanussi
From: Tom Zanussi <tom.zanussi@linux.intel.com>
Hi,
Here are a couple miscellaneous hist trigger patches which can be
applied independently of the onchange/snapshot patches.
The first is just an additional use of str_has_prefix() that was
apparently missed in the original str_has_prefix() conversion, with an
update suggested by Joe Perches.
The second fixes a bug I noticed when testing the onchange/snapshot
fixes.
I pulled these out into this separate patchset because they should be
applied regardless of what happens with the onchange/snapshot patchset.
Thanks,
Tom
The following changes since commit 67748dbeaf2b1240c0b87588df4bb0fb9471a751:
sh: ftrace: Fix missing parenthesis in WARN_ON() (2019-01-08 10:19:02 -0600)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/hist-fixes-v1
Tom Zanussi (2):
tracing: Use str_has_prefix() in synth_event_create()
tracing: Use strncpy instead of memcpy for string keys in hist
triggers
kernel/trace/trace_events_hist.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
--
2.14.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] tracing: Use str_has_prefix() in synth_event_create() 2019-02-04 21:07 [PATCH 0/2] A couple hist trigger patches Tom Zanussi @ 2019-02-04 21:07 ` Tom Zanussi 2019-02-04 21:07 ` [PATCH 2/2] tracing: Use strncpy instead of memcpy for string keys in hist triggers Tom Zanussi 2019-03-04 20:11 ` [PATCH 0/2] A couple hist trigger patches Tom Zanussi 2 siblings, 0 replies; 11+ messages in thread From: Tom Zanussi @ 2019-02-04 21:07 UTC (permalink / raw) To: rostedt; +Cc: linux-kernel, linux-rt-users, Tom Zanussi, Joe Perches From: Tom Zanussi <tom.zanussi@linux.intel.com> Since we now have a str_has_prefix() that returns the length, we can use that instead of explicitly calculating it. Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com> Cc: Joe Perches <joe@perches.com> --- kernel/trace/trace_events_hist.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 449d90cfa151..c4a667503bf0 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -1200,8 +1200,8 @@ static int synth_event_create(int argc, const char **argv) /* This interface accepts group name prefix */ if (strchr(name, '/')) { - len = sizeof(SYNTH_SYSTEM "/") - 1; - if (strncmp(name, SYNTH_SYSTEM "/", len)) + len = str_has_prefix(name, SYNTH_SYSTEM "/"); + if (len == 0) return -EINVAL; name += len; } -- 2.14.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] tracing: Use strncpy instead of memcpy for string keys in hist triggers 2019-02-04 21:07 [PATCH 0/2] A couple hist trigger patches Tom Zanussi 2019-02-04 21:07 ` [PATCH 1/2] tracing: Use str_has_prefix() in synth_event_create() Tom Zanussi @ 2019-02-04 21:07 ` Tom Zanussi 2019-03-04 21:50 ` Steven Rostedt 2019-03-04 20:11 ` [PATCH 0/2] A couple hist trigger patches Tom Zanussi 2 siblings, 1 reply; 11+ messages in thread From: Tom Zanussi @ 2019-02-04 21:07 UTC (permalink / raw) To: rostedt; +Cc: linux-kernel, linux-rt-users, Tom Zanussi, Namhyung Kim From: Tom Zanussi <tom.zanussi@linux.intel.com> Because there may be random garbage beyond a string's null terminator, it's not correct to copy the the complete character array for use as a hist trigger key. This results in multiple histogram entries for the 'same' string key. So, in the case of a string key, use strncpy instead of memcpy to avoid copying in the extra bytes. Before, using the gdbus entries in the following hist trigger as an example: # echo 'hist:key=comm' > /sys/kernel/debug/tracing/events/sched/sched_waking/trigger # cat /sys/kernel/debug/tracing/events/sched/sched_waking/hist ... { comm: ImgDecoder #4 } hitcount: 203 { comm: gmain } hitcount: 213 { comm: gmain } hitcount: 216 { comm: StreamTrans #73 } hitcount: 221 { comm: mozStorage #3 } hitcount: 230 { comm: gdbus } hitcount: 233 { comm: StyleThread#5 } hitcount: 253 { comm: gdbus } hitcount: 256 { comm: gdbus } hitcount: 260 { comm: StyleThread#4 } hitcount: 271 ... # cat /sys/kernel/debug/tracing/events/sched/sched_waking/hist | egrep gdbus | wc -l 51 After: # cat /sys/kernel/debug/tracing/events/sched/sched_waking/hist | egrep gdbus | wc -l 1 Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com> Cc: Namhyung Kim <namhyung@kernel.org> --- kernel/trace/trace_events_hist.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index c4a667503bf0..1b349689b897 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -4695,9 +4695,10 @@ static inline void add_to_key(char *compound_key, void *key, /* ensure NULL-termination */ if (size > key_field->size - 1) size = key_field->size - 1; - } - memcpy(compound_key + key_field->offset, key, size); + strncpy(compound_key + key_field->offset, (char *)key, size); + } else + memcpy(compound_key + key_field->offset, key, size); } static void -- 2.14.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tracing: Use strncpy instead of memcpy for string keys in hist triggers 2019-02-04 21:07 ` [PATCH 2/2] tracing: Use strncpy instead of memcpy for string keys in hist triggers Tom Zanussi @ 2019-03-04 21:50 ` Steven Rostedt 2019-03-04 21:56 ` Steven Rostedt 0 siblings, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2019-03-04 21:50 UTC (permalink / raw) To: Tom Zanussi; +Cc: linux-kernel, linux-rt-users, Tom Zanussi, Namhyung Kim On Mon, 4 Feb 2019 15:07:24 -0600 Tom Zanussi <zanussi@kernel.org> wrote: > From: Tom Zanussi <tom.zanussi@linux.intel.com> > > Because there may be random garbage beyond a string's null terminator, > it's not correct to copy the the complete character array for use as a > hist trigger key. This results in multiple histogram entries for the > 'same' string key. > > So, in the case of a string key, use strncpy instead of memcpy to > avoid copying in the extra bytes. > > Before, using the gdbus entries in the following hist trigger as an > example: > > # echo 'hist:key=comm' > /sys/kernel/debug/tracing/events/sched/sched_waking/trigger > # cat /sys/kernel/debug/tracing/events/sched/sched_waking/hist > > ... > > { comm: ImgDecoder #4 } hitcount: 203 > { comm: gmain } hitcount: 213 > { comm: gmain } hitcount: 216 > { comm: StreamTrans #73 } hitcount: 221 > { comm: mozStorage #3 } hitcount: 230 > { comm: gdbus } hitcount: 233 > { comm: StyleThread#5 } hitcount: 253 > { comm: gdbus } hitcount: 256 > { comm: gdbus } hitcount: 260 > { comm: StyleThread#4 } hitcount: 271 > > ... > > # cat /sys/kernel/debug/tracing/events/sched/sched_waking/hist | egrep gdbus | wc -l > 51 > > After: > > # cat /sys/kernel/debug/tracing/events/sched/sched_waking/hist | egrep gdbus | wc -l > 1 > > Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com> > Cc: Namhyung Kim <namhyung@kernel.org> > --- > kernel/trace/trace_events_hist.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index c4a667503bf0..1b349689b897 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -4695,9 +4695,10 @@ static inline void add_to_key(char *compound_key, void *key, > /* ensure NULL-termination */ > if (size > key_field->size - 1) > size = key_field->size - 1; > - } > > - memcpy(compound_key + key_field->offset, key, size); > + strncpy(compound_key + key_field->offset, (char *)key, size); > + } else > + memcpy(compound_key + key_field->offset, key, size); > } > Shouldn't we use strncpy() in save_comm() too. Feels safer. -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tracing: Use strncpy instead of memcpy for string keys in hist triggers 2019-03-04 21:50 ` Steven Rostedt @ 2019-03-04 21:56 ` Steven Rostedt 2019-03-04 22:22 ` Tom Zanussi 0 siblings, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2019-03-04 21:56 UTC (permalink / raw) To: Tom Zanussi; +Cc: linux-kernel, linux-rt-users, Tom Zanussi, Namhyung Kim On Mon, 4 Mar 2019 16:50:00 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > > +++ b/kernel/trace/trace_events_hist.c > > @@ -4695,9 +4695,10 @@ static inline void add_to_key(char *compound_key, void *key, > > /* ensure NULL-termination */ > > if (size > key_field->size - 1) > > size = key_field->size - 1; > > - } > > > > - memcpy(compound_key + key_field->offset, key, size); > > + strncpy(compound_key + key_field->offset, (char *)key, size); > > + } else > > + memcpy(compound_key + key_field->offset, key, size); > > } > > > > Shouldn't we use strncpy() in save_comm() too. Feels safer. Note, if that is changed, it can be another patch. This one is fine as is. I just was looking at other use cases of memcpy() in that file. -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tracing: Use strncpy instead of memcpy for string keys in hist triggers 2019-03-04 21:56 ` Steven Rostedt @ 2019-03-04 22:22 ` Tom Zanussi 2019-03-04 22:31 ` Tom Zanussi 0 siblings, 1 reply; 11+ messages in thread From: Tom Zanussi @ 2019-03-04 22:22 UTC (permalink / raw) To: Steven Rostedt, Tom Zanussi; +Cc: linux-kernel, linux-rt-users, Namhyung Kim Hi Steve, On Mon, 2019-03-04 at 16:56 -0500, Steven Rostedt wrote: > On Mon, 4 Mar 2019 16:50:00 -0500 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > > +++ b/kernel/trace/trace_events_hist.c > > > @@ -4695,9 +4695,10 @@ static inline void add_to_key(char > > > *compound_key, void *key, > > > /* ensure NULL-termination */ > > > if (size > key_field->size - 1) > > > size = key_field->size - 1; > > > - } > > > > > > - memcpy(compound_key + key_field->offset, key, size); > > > + strncpy(compound_key + key_field->offset, (char *)key, > > > size); > > > + } else > > > + memcpy(compound_key + key_field->offset, key, size); > > > } > > > > > > > Shouldn't we use strncpy() in save_comm() too. Feels safer. > > Note, if that is changed, it can be another patch. This one is fine > as > is. I just was looking at other use cases of memcpy() in that file. > Hmm, I don't think it's really necessary - it's not used in a key so don't care about anything after the null, and TASK_COMM_LEN is used in the memcpy. Tom > -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tracing: Use strncpy instead of memcpy for string keys in hist triggers 2019-03-04 22:22 ` Tom Zanussi @ 2019-03-04 22:31 ` Tom Zanussi 2019-03-04 23:45 ` Steven Rostedt 0 siblings, 1 reply; 11+ messages in thread From: Tom Zanussi @ 2019-03-04 22:31 UTC (permalink / raw) To: Steven Rostedt, Tom Zanussi; +Cc: linux-kernel, linux-rt-users, Namhyung Kim Hi Steve, On Mon, 2019-03-04 at 16:22 -0600, Tom Zanussi wrote: > Hi Steve, > > On Mon, 2019-03-04 at 16:56 -0500, Steven Rostedt wrote: > > On Mon, 4 Mar 2019 16:50:00 -0500 > > Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > +++ b/kernel/trace/trace_events_hist.c > > > > @@ -4695,9 +4695,10 @@ static inline void add_to_key(char > > > > *compound_key, void *key, > > > > /* ensure NULL-termination */ > > > > if (size > key_field->size - 1) > > > > size = key_field->size - 1; > > > > - } > > > > > > > > - memcpy(compound_key + key_field->offset, key, size); > > > > + strncpy(compound_key + key_field->offset, (char > > > > *)key, > > > > size); > > > > + } else > > > > + memcpy(compound_key + key_field->offset, key, > > > > size); > > > > } > > > > > > > > > > Shouldn't we use strncpy() in save_comm() too. Feels safer. > > > > Note, if that is changed, it can be another patch. This one is fine > > as > > is. I just was looking at other use cases of memcpy() in that file. > > > > Hmm, I don't think it's really necessary - it's not used in a key so > don't care about anything after the null, and TASK_COMM_LEN is used > in > the memcpy. Never mind, yeah, it would make sense to do this, will create another patch... Tom > > Tom > > > -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tracing: Use strncpy instead of memcpy for string keys in hist triggers 2019-03-04 22:31 ` Tom Zanussi @ 2019-03-04 23:45 ` Steven Rostedt 2019-03-05 0:02 ` Tom Zanussi 0 siblings, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2019-03-04 23:45 UTC (permalink / raw) To: Tom Zanussi; +Cc: Tom Zanussi, linux-kernel, linux-rt-users, Namhyung Kim On Mon, 04 Mar 2019 16:31:40 -0600 Tom Zanussi <tom.zanussi@linux.intel.com> wrote: > > Hmm, I don't think it's really necessary - it's not used in a key so > > don't care about anything after the null, and TASK_COMM_LEN is used > > in > > the memcpy. > > Never mind, yeah, it would make sense to do this, will create another > patch... And probably should change the memcpy() of comm in kernel/trace/trace.c too. It could be that memcpy() is a little bit faster than strncpy(), and this is done on scheduling switches when tracing is active, but still, I'm starting to think that isn't a good choice. -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] tracing: Use strncpy instead of memcpy for string keys in hist triggers 2019-03-04 23:45 ` Steven Rostedt @ 2019-03-05 0:02 ` Tom Zanussi 0 siblings, 0 replies; 11+ messages in thread From: Tom Zanussi @ 2019-03-05 0:02 UTC (permalink / raw) To: Steven Rostedt, Tom Zanussi; +Cc: linux-kernel, linux-rt-users, Namhyung Kim On Mon, 2019-03-04 at 18:45 -0500, Steven Rostedt wrote: > On Mon, 04 Mar 2019 16:31:40 -0600 > Tom Zanussi <tom.zanussi@linux.intel.com> wrote: > > > > Hmm, I don't think it's really necessary - it's not used in a key > > > so > > > don't care about anything after the null, and TASK_COMM_LEN is > > > used > > > in > > > the memcpy. > > > > Never mind, yeah, it would make sense to do this, will create > > another > > patch... > > And probably should change the memcpy() of comm in > kernel/trace/trace.c > too. It could be that memcpy() is a little bit faster than strncpy(), > and this is done on scheduling switches when tracing is active, but > still, I'm starting to think that isn't a good choice. > OK, will add that too. Tom > -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] A couple hist trigger patches 2019-02-04 21:07 [PATCH 0/2] A couple hist trigger patches Tom Zanussi 2019-02-04 21:07 ` [PATCH 1/2] tracing: Use str_has_prefix() in synth_event_create() Tom Zanussi 2019-02-04 21:07 ` [PATCH 2/2] tracing: Use strncpy instead of memcpy for string keys in hist triggers Tom Zanussi @ 2019-03-04 20:11 ` Tom Zanussi 2019-03-04 21:26 ` Steven Rostedt 2 siblings, 1 reply; 11+ messages in thread From: Tom Zanussi @ 2019-03-04 20:11 UTC (permalink / raw) To: rostedt; +Cc: linux-kernel, linux-rt-users, Tom Zanussi Ping... On Mon, 2019-02-04 at 15:07 -0600, Tom Zanussi wrote: > From: Tom Zanussi <tom.zanussi@linux.intel.com> > > Hi, > > Here are a couple miscellaneous hist trigger patches which can be > applied independently of the onchange/snapshot patches. > > The first is just an additional use of str_has_prefix() that was > apparently missed in the original str_has_prefix() conversion, with > an > update suggested by Joe Perches. > > The second fixes a bug I noticed when testing the onchange/snapshot > fixes. > > I pulled these out into this separate patchset because they should be > applied regardless of what happens with the onchange/snapshot > patchset. > > Thanks, > > Tom > > > The following changes since commit > 67748dbeaf2b1240c0b87588df4bb0fb9471a751: > > sh: ftrace: Fix missing parenthesis in WARN_ON() (2019-01-08 > 10:19:02 -0600) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux- > trace.git ftrace/hist-fixes-v1 > > Tom Zanussi (2): > tracing: Use str_has_prefix() in synth_event_create() > tracing: Use strncpy instead of memcpy for string keys in hist > triggers > > kernel/trace/trace_events_hist.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] A couple hist trigger patches 2019-03-04 20:11 ` [PATCH 0/2] A couple hist trigger patches Tom Zanussi @ 2019-03-04 21:26 ` Steven Rostedt 0 siblings, 0 replies; 11+ messages in thread From: Steven Rostedt @ 2019-03-04 21:26 UTC (permalink / raw) To: Tom Zanussi; +Cc: linux-kernel, linux-rt-users, Tom Zanussi On Mon, 04 Mar 2019 14:11:03 -0600 Tom Zanussi <tzanussi@gmail.com> wrote: > Ping... > > Thanks for the ping. It got buried. -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-03-05 0:02 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-04 21:07 [PATCH 0/2] A couple hist trigger patches Tom Zanussi 2019-02-04 21:07 ` [PATCH 1/2] tracing: Use str_has_prefix() in synth_event_create() Tom Zanussi 2019-02-04 21:07 ` [PATCH 2/2] tracing: Use strncpy instead of memcpy for string keys in hist triggers Tom Zanussi 2019-03-04 21:50 ` Steven Rostedt 2019-03-04 21:56 ` Steven Rostedt 2019-03-04 22:22 ` Tom Zanussi 2019-03-04 22:31 ` Tom Zanussi 2019-03-04 23:45 ` Steven Rostedt 2019-03-05 0:02 ` Tom Zanussi 2019-03-04 20:11 ` [PATCH 0/2] A couple hist trigger patches Tom Zanussi 2019-03-04 21:26 ` Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).