| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20100428211353.GK8591@Krystal>
Date: Wed, 28 Apr 2010 17:13:53 -0400
From: Mathieu Desnoyers <compudj@...stal.dyndns.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...e.hu>,
Andrew Morton <akpm@...ux-foundation.org>,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Frederic Weisbecker <fweisbec@...il.com>,
Arnaldo Carvalho de Melo <acme@...hat.com>,
Lai Jiangshan <laijs@...fujitsu.com>,
Li Zefan <lizf@...fujitsu.com>,
Masami Hiramatsu <mhiramat@...hat.com>,
Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 10/10][RFC] tracing: Combine event filter_active and
enable into single flags field
* Steven Rostedt (rostedt@...dmis.org) wrote:
> From: Steven Rostedt <srostedt@...hat.com>
>
> The filter_active and enable both use an int (4 bytes each) to
> set a single flag. We can save 4 bytes per event by combining the
> two into a single integer.
So you're adding extra masks on the tracing fast path to save 4 bytes
per event. That sounds like an acceptable tradeoff, since a mask is
incredibly cheap compared to a cache miss.
This patch could use slightly more verbose locking rule explanation of
the flags. The updates are protected by a mutex, but can reads happen
concurrently with updates ? (can we update the filter while tracing is
active ?)
Thanks,
Mathieu
>
> text data bss dec hex filename
> 5788186 1337252 9351592 16477030 fb6b66 vmlinux.orig
> 5761074 1262596 9351592 16375262 f9ddde vmlinux.id
> 5761007 1256916 9351592 16369515 f9c76b vmlinux.flags
>
> This gives us another 5K in savings.
>
> The modification of both the enable and filter fields are done
> under the event_mutex, so it is still safe to combine the two.
>
> Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
> ---
> include/linux/ftrace_event.h | 21 +++++++++++++++++++--
> kernel/trace/trace.h | 2 +-
> kernel/trace/trace_events.c | 14 +++++++-------
> kernel/trace/trace_events_filter.c | 10 +++++-----
> kernel/trace/trace_kprobe.c | 2 +-
> 5 files changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index b26507f..2e28c94 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -141,6 +141,16 @@ struct ftrace_event_class {
> int (*raw_init)(struct ftrace_event_call *);
> };
>
> +enum {
> + TRACE_EVENT_FL_ENABLED_BIT,
> + TRACE_EVENT_FL_FILTERED_BIT,
> +};
> +
> +enum {
> + TRACE_EVENT_FL_ENABLED = (1 << TRACE_EVENT_FL_ENABLED_BIT),
> + TRACE_EVENT_FL_FILTERED = (1 << TRACE_EVENT_FL_FILTERED_BIT),
> +};
> +
> struct ftrace_event_call {
> struct list_head list;
> struct ftrace_event_class *class;
> @@ -152,8 +162,15 @@ struct ftrace_event_call {
> void *mod;
> void *data;
>
> - int enabled;
> - int filter_active;
> + /*
> + * 32 bit flags:
> + * bit 1: enabled
> + * bit 2: filter_active
> + *
> + * Must hold event_mutex to change.
> + */
> + unsigned int flags;
> +
> int perf_refcount;
> };
>
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index ff63bee..51ee319 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -779,7 +779,7 @@ filter_check_discard(struct ftrace_event_call *call, void *rec,
> struct ring_buffer *buffer,
> struct ring_buffer_event *event)
> {
> - if (unlikely(call->filter_active) &&
> + if (unlikely(call->flags & TRACE_EVENT_FL_FILTERED) &&
> !filter_match_preds(call->filter, rec)) {
> ring_buffer_discard_commit(buffer, event);
> return 1;
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 8d2e28e..176b8be 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -144,8 +144,8 @@ static int ftrace_event_enable_disable(struct ftrace_event_call *call,
>
> switch (enable) {
> case 0:
> - if (call->enabled) {
> - call->enabled = 0;
> + if (call->flags & TRACE_EVENT_FL_ENABLED) {
> + call->flags &= ~TRACE_EVENT_FL_ENABLED;
> tracing_stop_cmdline_record();
> if (call->class->reg)
> call->class->reg(call, TRACE_REG_UNREGISTER);
> @@ -156,7 +156,7 @@ static int ftrace_event_enable_disable(struct ftrace_event_call *call,
> }
> break;
> case 1:
> - if (!call->enabled) {
> + if (!(call->flags & TRACE_EVENT_FL_ENABLED)) {
> tracing_start_cmdline_record();
> if (call->class->reg)
> ret = call->class->reg(call, TRACE_REG_REGISTER);
> @@ -170,7 +170,7 @@ static int ftrace_event_enable_disable(struct ftrace_event_call *call,
> "%s\n", call->name);
> break;
> }
> - call->enabled = 1;
> + call->flags |= TRACE_EVENT_FL_ENABLED;
> }
> break;
> }
> @@ -359,7 +359,7 @@ s_next(struct seq_file *m, void *v, loff_t *pos)
> (*pos)++;
>
> list_for_each_entry_continue(call, &ftrace_events, list) {
> - if (call->enabled)
> + if (call->flags & TRACE_EVENT_FL_ENABLED)
> return call;
> }
>
> @@ -418,7 +418,7 @@ event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
> struct ftrace_event_call *call = filp->private_data;
> char *buf;
>
> - if (call->enabled)
> + if (call->flags & TRACE_EVENT_FL_ENABLED)
> buf = "1\n";
> else
> buf = "0\n";
> @@ -493,7 +493,7 @@ system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
> * or if all events or cleared, or if we have
> * a mixture.
> */
> - set |= (1 << !!call->enabled);
> + set |= (1 << !!(call->flags & TRACE_EVENT_FL_ENABLED));
>
> /*
> * If we have a mixture, no need to look further.
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index b8e3bf3..fbc72ee 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -546,7 +546,7 @@ static void filter_disable_preds(struct ftrace_event_call *call)
> struct event_filter *filter = call->filter;
> int i;
>
> - call->filter_active = 0;
> + call->flags &= ~TRACE_EVENT_FL_FILTERED;
> filter->n_preds = 0;
>
> for (i = 0; i < MAX_FILTER_PRED; i++)
> @@ -573,7 +573,7 @@ void destroy_preds(struct ftrace_event_call *call)
> {
> __free_preds(call->filter);
> call->filter = NULL;
> - call->filter_active = 0;
> + call->flags &= ~TRACE_EVENT_FL_FILTERED;
> }
>
> static struct event_filter *__alloc_preds(void)
> @@ -612,7 +612,7 @@ static int init_preds(struct ftrace_event_call *call)
> if (call->filter)
> return 0;
>
> - call->filter_active = 0;
> + call->flags &= ~TRACE_EVENT_FL_FILTERED;
> call->filter = __alloc_preds();
> if (IS_ERR(call->filter))
> return PTR_ERR(call->filter);
> @@ -1267,7 +1267,7 @@ static int replace_system_preds(struct event_subsystem *system,
> if (err)
> filter_disable_preds(call);
> else {
> - call->filter_active = 1;
> + call->flags |= TRACE_EVENT_FL_FILTERED;
> replace_filter_string(filter, filter_string);
> }
> fail = false;
> @@ -1316,7 +1316,7 @@ int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
> if (err)
> append_filter_err(ps, call->filter);
> else
> - call->filter_active = 1;
> + call->flags |= TRACE_EVENT_FL_FILTERED;
> out:
> filter_opstack_clear(ps);
> postfix_clear(ps);
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 934078b..0e3ded6 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -1382,7 +1382,7 @@ static int register_probe_event(struct trace_probe *tp)
> kfree(call->print_fmt);
> return -ENODEV;
> }
> - call->enabled = 0;
> + call->flags = 0;
> call->class->reg = kprobe_register;
> call->data = tp;
> ret = trace_add_event_call(call);
> --
> 1.7.0
>
>
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists