| 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: <3082FF59-4A9B-4E6C-8132-40EEE2891B6C@fb.com>
Date: Wed, 16 Jan 2019 14:32:00 +0000
From: Song Liu <songliubraving@...com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
CC: lkml <linux-kernel@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"peterz@...radead.org" <peterz@...radead.org>,
"ast@...nel.org" <ast@...nel.org>,
"daniel@...earbox.net" <daniel@...earbox.net>,
Kernel Team <Kernel-team@...com>,
"dsahern@...il.com" <dsahern@...il.com>
Subject: Re: [PATCH v9 perf, bpf-next 5/9] perf util: handle
PERF_RECORD_KSYMBOL
> On Jan 16, 2019, at 6:22 AM, Arnaldo Carvalho de Melo <acme@...nel.org> wrote:
>
> Em Wed, Jan 16, 2019 at 02:12:37PM +0000, Song Liu escreveu:
>>
>>
>>> On Jan 16, 2019, at 6:00 AM, Arnaldo Carvalho de Melo <acme@...nel.org> wrote:
>>>
>>> Em Tue, Jan 15, 2019 at 04:14:06PM -0800, Song Liu escreveu:
>>>> This patch handles PERF_RECORD_KSYMBOL in perf record/report.
>>>> Specifically, map and symbol are created for ksymbol register, and
>>>> removed for ksymbol unregister.
>>>>
>>>> This patch also set perf_event_attr.ksymbol properly. The flag is
>>>> ON by default.
>>>>
>>>> Signed-off-by: Song Liu <songliubraving@...com>
>>>> ---
>>>> tools/perf/util/event.c | 21 +++++++++++++++
>>>> tools/perf/util/event.h | 20 ++++++++++++++
>>>> tools/perf/util/evsel.c | 12 ++++++++-
>>>> tools/perf/util/evsel.h | 1 +
>>>> tools/perf/util/machine.c | 57 +++++++++++++++++++++++++++++++++++++++
>>>> tools/perf/util/machine.h | 3 +++
>>>> tools/perf/util/session.c | 4 +++
>>>> tools/perf/util/tool.h | 4 ++-
>>>> 8 files changed, 120 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
>>>> index 937a5a4f71cc..3c8a6a8dd260 100644
>>>> --- a/tools/perf/util/event.c
>>>> +++ b/tools/perf/util/event.c
>>>> @@ -24,6 +24,7 @@
>>>> #include "symbol/kallsyms.h"
>>>> #include "asm/bug.h"
>>>> #include "stat.h"
>>>> +#include "session.h"
>>>>
>>>> #define DEFAULT_PROC_MAP_PARSE_TIMEOUT 500
>>>>
>>>> @@ -45,6 +46,7 @@ static const char *perf_event__names[] = {
>>>> [PERF_RECORD_SWITCH] = "SWITCH",
>>>> [PERF_RECORD_SWITCH_CPU_WIDE] = "SWITCH_CPU_WIDE",
>>>> [PERF_RECORD_NAMESPACES] = "NAMESPACES",
>>>> + [PERF_RECORD_KSYMBOL] = "KSYMBOL",
>>>> [PERF_RECORD_HEADER_ATTR] = "ATTR",
>>>> [PERF_RECORD_HEADER_EVENT_TYPE] = "EVENT_TYPE",
>>>> [PERF_RECORD_HEADER_TRACING_DATA] = "TRACING_DATA",
>>>> @@ -1329,6 +1331,14 @@ int perf_event__process_switch(struct perf_tool *tool __maybe_unused,
>>>> return machine__process_switch_event(machine, event);
>>>> }
>>>>
>>>> +int perf_event__process_ksymbol(struct perf_tool *tool __maybe_unused,
>>>> + union perf_event *event,
>>>> + struct perf_sample *sample __maybe_unused,
>>>> + struct machine *machine)
>>>> +{
>>>> + return machine__process_ksymbol(machine, event, sample);
>>>> +}
>>>> +
>>>> size_t perf_event__fprintf_mmap(union perf_event *event, FILE *fp)
>>>> {
>>>> return fprintf(fp, " %d/%d: [%#" PRIx64 "(%#" PRIx64 ") @ %#" PRIx64 "]: %c %s\n",
>>>> @@ -1461,6 +1471,14 @@ static size_t perf_event__fprintf_lost(union perf_event *event, FILE *fp)
>>>> return fprintf(fp, " lost %" PRIu64 "\n", event->lost.lost);
>>>> }
>>>>
>>>> +size_t perf_event__fprintf_ksymbol(union perf_event *event, FILE *fp)
>>>> +{
>>>> + return fprintf(fp, " ksymbol event with addr %lx len %u type %u flags 0x%x name %s\n",
>>>> + event->ksymbol_event.addr, event->ksymbol_event.len,
>>>> + event->ksymbol_event.ksym_type,
>>>> + event->ksymbol_event.flags, event->ksymbol_event.name);
>>>> +}
>>>> +
>>>> size_t perf_event__fprintf(union perf_event *event, FILE *fp)
>>>> {
>>>> size_t ret = fprintf(fp, "PERF_RECORD_%s",
>>>> @@ -1496,6 +1514,9 @@ size_t perf_event__fprintf(union perf_event *event, FILE *fp)
>>>> case PERF_RECORD_LOST:
>>>> ret += perf_event__fprintf_lost(event, fp);
>>>> break;
>>>> + case PERF_RECORD_KSYMBOL:
>>>> + ret += perf_event__fprintf_ksymbol(event, fp);
>>>> + break;
>>>> default:
>>>> ret += fprintf(fp, "\n");
>>>> }
>>>> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
>>>> index eb95f3384958..018322f2a13e 100644
>>>> --- a/tools/perf/util/event.h
>>>> +++ b/tools/perf/util/event.h
>>>> @@ -5,6 +5,7 @@
>>>> #include <limits.h>
>>>> #include <stdio.h>
>>>> #include <linux/kernel.h>
>>>> +#include <linux/bpf.h>
>>>>
>>>> #include "../perf.h"
>>>> #include "build-id.h"
>>>> @@ -84,6 +85,19 @@ struct throttle_event {
>>>> u64 stream_id;
>>>> };
>>>>
>>>> +#ifndef KSYM_NAME_LEN
>>>> +#define KSYM_NAME_LEN 256
>>>> +#endif
>>>> +
>>>> +struct ksymbol_event {
>>>> + struct perf_event_header header;
>>>> + u64 addr;
>>>> + u32 len;
>>>> + u16 ksym_type;
>>>> + u16 flags;
>>>> + char name[KSYM_NAME_LEN];
>>>> +};
>>>> +
>>>> #define PERF_SAMPLE_MASK \
>>>> (PERF_SAMPLE_IP | PERF_SAMPLE_TID | \
>>>> PERF_SAMPLE_TIME | PERF_SAMPLE_ADDR | \
>>>> @@ -651,6 +665,7 @@ union perf_event {
>>>> struct stat_round_event stat_round;
>>>> struct time_conv_event time_conv;
>>>> struct feature_event feat;
>>>> + struct ksymbol_event ksymbol_event;
>>>> };
>>>>
>>>> void perf_event__print_totals(void);
>>>> @@ -748,6 +763,10 @@ int perf_event__process_exit(struct perf_tool *tool,
>>>> union perf_event *event,
>>>> struct perf_sample *sample,
>>>> struct machine *machine);
>>>> +int perf_event__process_ksymbol(struct perf_tool *tool,
>>>> + union perf_event *event,
>>>> + struct perf_sample *sample,
>>>> + struct machine *machine);
>>>> int perf_tool__process_synth_event(struct perf_tool *tool,
>>>> union perf_event *event,
>>>> struct machine *machine,
>>>> @@ -811,6 +830,7 @@ size_t perf_event__fprintf_switch(union perf_event *event, FILE *fp);
>>>> size_t perf_event__fprintf_thread_map(union perf_event *event, FILE *fp);
>>>> size_t perf_event__fprintf_cpu_map(union perf_event *event, FILE *fp);
>>>> size_t perf_event__fprintf_namespaces(union perf_event *event, FILE *fp);
>>>> +size_t perf_event__fprintf_ksymbol(union perf_event *event, FILE *fp);
>>>> size_t perf_event__fprintf(union perf_event *event, FILE *fp);
>>>>
>>>> int kallsyms__get_function_start(const char *kallsyms_filename,
>>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>>>> index dbc0466db368..a2c75aace409 100644
>>>> --- a/tools/perf/util/evsel.c
>>>> +++ b/tools/perf/util/evsel.c
>>>> @@ -1035,6 +1035,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
>>>> attr->mmap = track;
>>>> attr->mmap2 = track && !perf_missing_features.mmap2;
>>>> attr->comm = track;
>>>> + attr->ksymbol = track && !perf_missing_features.ksymbol;
>>>>
>>>> if (opts->record_namespaces)
>>>> attr->namespaces = track;
>>>> @@ -1652,6 +1653,7 @@ int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
>>>> PRINT_ATTRf(context_switch, p_unsigned);
>>>> PRINT_ATTRf(write_backward, p_unsigned);
>>>> PRINT_ATTRf(namespaces, p_unsigned);
>>>> + PRINT_ATTRf(ksymbol, p_unsigned);
>>>>
>>>> PRINT_ATTRn("{ wakeup_events, wakeup_watermark }", wakeup_events, p_unsigned);
>>>> PRINT_ATTRf(bp_type, p_unsigned);
>>>> @@ -1811,6 +1813,8 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>>>> PERF_SAMPLE_BRANCH_NO_CYCLES);
>>>> if (perf_missing_features.group_read && evsel->attr.inherit)
>>>> evsel->attr.read_format &= ~(PERF_FORMAT_GROUP|PERF_FORMAT_ID);
>>>> + if (perf_missing_features.ksymbol)
>>>> + evsel->attr.ksymbol = 0;
>>>> retry_sample_id:
>>>> if (perf_missing_features.sample_id_all)
>>>> evsel->attr.sample_id_all = 0;
>>>> @@ -1930,7 +1934,13 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>>>> * Must probe features in the order they were added to the
>>>> * perf_event_attr interface.
>>>> */
>>>> - if (!perf_missing_features.write_backward && evsel->attr.write_backward) {
>>>> + if (!perf_missing_features.ksymbol &&
>>>> + evsel->attr.ksymbol) {
>>>> + perf_missing_features.ksymbol = true;
>>>> + pr_debug2("switching off ksymbol\n");
>>>> + goto fallback_missing_features;
>>>> + } else if (!perf_missing_features.write_backward &&
>>>> + evsel->attr.write_backward) {
>>>
>>> Please keep the existing coding style, i.e. no need to change this line
>>> into two:
>>>
>>> if (!perf_missing_features.write_backward && evsel->attr.write_backward) {
>>>
>>> Just add the '} else' in front of it, ditto for the new line.
>>>
>>> I'm not that strict about it, perhaps I should, to avoid my upstreamers
>>> to block things due to this ;-)
>>>
>>> And also this is not the only issue I found with this specific patch,
>>> see below for one more functional issue.
>>>
>>> I saw no issues in the previous patches (don't recall right now if there
>>> where coding style minor issues), so in your next patchset version you
>>> may add my:
>>>
>>> Reviewed-by: Arnaldo Carvalho de Melo <acme@...hat.com>
>>>
>>
>> Thanks Arnaldo!
>>
>> Just to be sure, did you mean to add Reviewed-by to patch 1/9 through 5/9?
>
> 1/9 to 4/9, 5/9 is this one and we're still going thru it, so not
> finished yet, right?
>
> - Arnaldo
I see. Thanks for clarifying.
Song
>
>>> To those.
>>>
>>>> perf_missing_features.write_backward = true;
>>>> pr_debug2("switching off write_backward\n");
>>>> goto out_close;
>>>> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>>>> index 82a289ce8b0c..4a8c3e7f4808 100644
>>>> --- a/tools/perf/util/evsel.h
>>>> +++ b/tools/perf/util/evsel.h
>>>> @@ -168,6 +168,7 @@ struct perf_missing_features {
>>>> bool lbr_flags;
>>>> bool write_backward;
>>>> bool group_read;
>>>> + bool ksymbol;
>>>> };
>>>>
>>>> extern struct perf_missing_features perf_missing_features;
>>>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
>>>> index 143f7057d581..c8f6daac9d2d 100644
>>>> --- a/tools/perf/util/machine.c
>>>> +++ b/tools/perf/util/machine.c
>>>> @@ -681,6 +681,61 @@ int machine__process_switch_event(struct machine *machine __maybe_unused,
>>>> return 0;
>>>> }
>>>>
>>>> +static int machine__process_ksymbol_register(
>>>> + struct machine *machine,
>>>> + union perf_event *event,
>>>> + struct perf_sample *sample __maybe_unused)
>>>> +{
>>>> + struct symbol *sym;
>>>> + struct map *map;
>>>> +
>>>> + map = map_groups__find(&machine->kmaps, event->ksymbol_event.addr);
>>>> + if (!map) {
>>>> + map = dso__new_map("bpf_prog");
>>>
>>> Humm, why not use event->ksymbol_event.name as the name of the map as
>>> well? Otherwise we may end up thinking its all the same map "bpf_prog",
>>> when we have lots.
>>
>> I will fix this in next version.
>>
>> Song
>>
>>>
>>>> + if (!map)
>>>> + return -ENOMEM;
>>>> +
>>>> + map->start = event->ksymbol_event.addr;
>>>> + map->pgoff = map->start;
>>>> + map->end = map->start + event->ksymbol_event.len;
>>>> + map_groups__insert(&machine->kmaps, map);
>>>> + }
>>>> +
>>>> + sym = symbol__new(event->ksymbol_event.addr, event->ksymbol_event.len,
>>>> + 0, 0, event->ksymbol_event.name);
>>>> + if (!sym)
>>>> + return -ENOMEM;
>>>> + dso__insert_symbol(map->dso, sym);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int machine__process_ksymbol_unregister(
>>>> + struct machine *machine,
>>>> + union perf_event *event,
>>>> + struct perf_sample *sample __maybe_unused)
>>>
>>> Please follow the existing style, the above should be:
>>>
>>> static int machine__process_ksymbol_unregister(struct machine *machine,
>>> union perf_event *event,
>>> struct perf_sample *sample __maybe_unused)
>>>
>>>> +{
>>>> + struct map *map;
>>>> +
>>>> + map = map_groups__find(&machine->kmaps, event->ksymbol_event.addr);
>>>> + if (map)
>>>> + map_groups__remove(&machine->kmaps, map);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +int machine__process_ksymbol(struct machine *machine __maybe_unused,
>>>> + union perf_event *event,
>>>> + struct perf_sample *sample)
>>>
>>> This one follows the existing style, great.
>>>
>>>> +{
>>>> + if (dump_trace)
>>>> + perf_event__fprintf_ksymbol(event, stdout);
>>>> +
>>>> + if (event->ksymbol_event.flags & PERF_RECORD_KSYMBOL_FLAGS_UNREGISTER)
>>>> + return machine__process_ksymbol_unregister(machine, event,
>>>> + sample);
>>>> + return machine__process_ksymbol_register(machine, event, sample);
>>>> +}
>>>> +
>>>> static void dso__adjust_kmod_long_name(struct dso *dso, const char *filename)
>>>> {
>>>> const char *dup_filename;
>>>> @@ -1812,6 +1867,8 @@ int machine__process_event(struct machine *machine, union perf_event *event,
>>>> case PERF_RECORD_SWITCH:
>>>> case PERF_RECORD_SWITCH_CPU_WIDE:
>>>> ret = machine__process_switch_event(machine, event); break;
>>>> + case PERF_RECORD_KSYMBOL:
>>>> + ret = machine__process_ksymbol(machine, event, sample); break;
>>>> default:
>>>> ret = -1;
>>>> break;
>>>> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
>>>> index a5d1da60f751..4ecd380ce1b4 100644
>>>> --- a/tools/perf/util/machine.h
>>>> +++ b/tools/perf/util/machine.h
>>>> @@ -130,6 +130,9 @@ int machine__process_mmap_event(struct machine *machine, union perf_event *event
>>>> struct perf_sample *sample);
>>>> int machine__process_mmap2_event(struct machine *machine, union perf_event *event,
>>>> struct perf_sample *sample);
>>>> +int machine__process_ksymbol(struct machine *machine,
>>>> + union perf_event *event,
>>>> + struct perf_sample *sample);
>>>> int machine__process_event(struct machine *machine, union perf_event *event,
>>>> struct perf_sample *sample);
>>>>
>>>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>>>> index 5456c84c7dd1..2efa75bb0c0a 100644
>>>> --- a/tools/perf/util/session.c
>>>> +++ b/tools/perf/util/session.c
>>>> @@ -376,6 +376,8 @@ void perf_tool__fill_defaults(struct perf_tool *tool)
>>>> tool->itrace_start = perf_event__process_itrace_start;
>>>> if (tool->context_switch == NULL)
>>>> tool->context_switch = perf_event__process_switch;
>>>> + if (tool->ksymbol == NULL)
>>>> + tool->ksymbol = perf_event__process_ksymbol;
>>>> if (tool->read == NULL)
>>>> tool->read = process_event_sample_stub;
>>>> if (tool->throttle == NULL)
>>>> @@ -1305,6 +1307,8 @@ static int machines__deliver_event(struct machines *machines,
>>>> case PERF_RECORD_SWITCH:
>>>> case PERF_RECORD_SWITCH_CPU_WIDE:
>>>> return tool->context_switch(tool, event, sample, machine);
>>>> + case PERF_RECORD_KSYMBOL:
>>>> + return tool->ksymbol(tool, event, sample, machine);
>>>> default:
>>>> ++evlist->stats.nr_unknown_events;
>>>> return -1;
>>>> diff --git a/tools/perf/util/tool.h b/tools/perf/util/tool.h
>>>> index 56e4ca54020a..9c81ca2f3cf7 100644
>>>> --- a/tools/perf/util/tool.h
>>>> +++ b/tools/perf/util/tool.h
>>>> @@ -53,7 +53,9 @@ struct perf_tool {
>>>> itrace_start,
>>>> context_switch,
>>>> throttle,
>>>> - unthrottle;
>>>> + unthrottle,
>>>> + ksymbol;
>>>> +
>>>> event_attr_op attr;
>>>> event_attr_op event_update;
>>>> event_op2 tracing_data;
>>>> --
>>>> 2.17.1
>>>
>>> --
>>>
>>> - Arnaldo
>
> --
>
> - Arnaldo
Powered by blists - more mailing lists