From: Tom Zanussi <[email protected]>
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
From: Tom Zanussi <[email protected]>
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 <[email protected]>
Cc: Joe Perches <[email protected]>
---
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
From: Tom Zanussi <[email protected]>
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 <[email protected]>
Cc: Namhyung Kim <[email protected]>
---
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
Ping...
On Mon, 2019-02-04 at 15:07 -0600, Tom Zanussi wrote:
> From: Tom Zanussi <[email protected]>
>
> 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(-)
>
On Mon, 04 Mar 2019 14:11:03 -0600
Tom Zanussi <[email protected]> wrote:
> Ping...
>
>
Thanks for the ping. It got buried.
-- Steve
On Mon, 4 Feb 2019 15:07:24 -0600
Tom Zanussi <[email protected]> wrote:
> From: Tom Zanussi <[email protected]>
>
> 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 <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> ---
> 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
On Mon, 4 Mar 2019 16:50:00 -0500
Steven Rostedt <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]> 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
On Mon, 04 Mar 2019 16:31:40 -0600
Tom Zanussi <[email protected]> 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
On Mon, 2019-03-04 at 18:45 -0500, Steven Rostedt wrote:
> On Mon, 04 Mar 2019 16:31:40 -0600
> Tom Zanussi <[email protected]> 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