Re: [PATCH v3 1/2] perf tools: add 'perf irq' to measure the hardware interrupts

[Namhyung Kim]

Hello,

On Sat, Jan 16, 2021 at 10:20 AM Bixuan Cui <cuibixuan@xxxxxxxxxx> wrote:
>
> Add 'perf irq' to trace/measure the hardware interrupts.
>
> Now three functions are provided:
>   1. 'perf irq record <command>' to record the irq handler events.
>   2. 'perf irq script' to see a detailed trace of the workload that
>    was recorded.
>   3. 'perf irq report' to calculate the time consumed by each
>    hardware interrupt processing function.
>
> Signed-off-by: Bixuan Cui <cuibixuan@xxxxxxxxxx>
> ---
>  tools/perf/Build         |   1 +
>  tools/perf/builtin-irq.c | 283 +++++++++++++++++++++++++++++++++++++++
>  tools/perf/builtin.h     |   1 +
>  tools/perf/perf.c        |   1 +
>  4 files changed, 286 insertions(+)
>  create mode 100644 tools/perf/builtin-irq.c
>
> diff --git a/tools/perf/Build b/tools/perf/Build
> index 5f392dbb88fc..d52a1e1d6d8a 100644
> --- a/tools/perf/Build
> +++ b/tools/perf/Build
> @@ -24,6 +24,7 @@ perf-y += builtin-mem.o
>  perf-y += builtin-data.o
>  perf-y += builtin-version.o
>  perf-y += builtin-c2c.o
> +perf-y += builtin-irq.o
>
>  perf-$(CONFIG_TRACE) += builtin-trace.o
>  perf-$(CONFIG_LIBELF) += builtin-probe.o
> diff --git a/tools/perf/builtin-irq.c b/tools/perf/builtin-irq.c
> new file mode 100644
> index 000000000000..25ba0669a875
> --- /dev/null
> +++ b/tools/perf/builtin-irq.c
[SNIP]
> +
> +#define IRQ_NAME_LEN           20
> +#define MAX_CPUS               4096
> +
> +static const char *cpu_list;
> +static DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
> +
> +struct perf_irq;

Seems unnecessary.

> +
> +struct perf_irq {
> +       struct perf_tool tool;
> +       bool force;
> +
> +       u32 irq_entry_irq;
> +       char irq_name[IRQ_NAME_LEN];
> +       u32 cpu;
> +       u64 irq_entry_time;
> +       u32 irq_entry_pid;
> +       u32 irq_exit_irq;
> +       u64 irq_exit_time;
> +       u32 irq_exit_pid;
> +};
> +
> +typedef int (*irq_handler)(struct perf_tool *tool,
> +                         union perf_event *event,
> +                         struct evsel *evsel,
> +                         struct perf_sample *sample,
> +                         struct machine *machine);

You don't need to pass all the arguments if unused.

> +
> +static int perf_report_process_sample(struct perf_tool *tool,
> +                                        union perf_event *event,
> +                                        struct perf_sample *sample,
> +                                        struct evsel *evsel,
> +                                        struct machine *machine)
> +{
> +       int err = 0;
> +
> +       if (evsel->handler != NULL) {
> +               irq_handler f = evsel->handler;
> +               err = f(tool, event, evsel, sample, machine);
> +       }
> +
> +       return err;
> +}
> +
> +static void output_report(struct perf_irq *irq)
> +{
> +       int ret, i;
> +       char irq_entry_time[30], irq_exit_time[30], irq_diff[30];
> +
> +       /* The entry and exit of the hardware irq function
> +        * exist at the same time. Check it by irq and pid.
> +        */
> +       if (irq->irq_entry_pid != irq->irq_exit_pid ||
> +           irq->irq_entry_irq != irq->irq_exit_irq)
> +               return;

Is there only a single instance of the perf_irq here?
Then I don't think this is correct and you should keep
pairs of irq entry/exit per cpu.  Otherwise overlapped
irqs from different cpus will be discarded (wrongly).

> +
> +       timestamp__scnprintf_usec(irq->irq_entry_time,
> +                                 irq_entry_time, sizeof(irq_entry_time));
> +       timestamp__scnprintf_usec(irq->irq_exit_time,
> +                                 irq_exit_time, sizeof(irq_exit_time));
> +       timestamp__scnprintf_usec(irq->irq_exit_time - irq->irq_entry_time,
> +                                 irq_diff, sizeof(irq_diff));
> +
> +       ret = printf("   %s ", irq->irq_name);
> +       for (i = 0; i < IRQ_NAME_LEN - ret; i++)
> +               printf(" ");
> +
> +       printf("| [%04d] | %13s s | %16s s | %16s s\n",
> +               irq->cpu, irq_diff, irq_entry_time, irq_exit_time);
> +}
> +
> +static int report_irq_handler_entry_event(struct perf_tool *tool,
> +                                     union perf_event *event __maybe_unused,
> +                                     struct evsel *evsel,
> +                                     struct perf_sample *sample,
> +                                     struct machine *machine __maybe_unused)
> +{
> +       int err = 0;
> +       struct perf_irq *irq = container_of(tool, struct perf_irq, tool);
> +
> +       const char *name = evsel__strval(evsel, sample, "name");
> +
> +       irq->irq_entry_pid = evsel__intval(evsel, sample, "pid");
> +       irq->irq_entry_irq = evsel__intval(evsel, sample, "irq");
> +       irq->irq_entry_time = sample->time;
> +       strncpy(irq->irq_name, name, IRQ_NAME_LEN);

Note that strncpy doesn't guarantee the NUL-termination.
You'd better do it by yourself just in case.

Thanks,
Namhyung

> +
> +       return err;
> +}
> +
> +static int report_irq_handler_exit_event(struct perf_tool *tool,
> +                                     union perf_event *event __maybe_unused,
> +                                     struct evsel *evsel,
> +                                     struct perf_sample *sample,
> +                                     struct machine *machine __maybe_unused)
> +{
> +       int err = 0;
> +       struct perf_irq *irq = container_of(tool, struct perf_irq, tool);
> +
> +       irq->irq_exit_pid = evsel__intval(evsel, sample, "pid");
> +       irq->irq_exit_irq = evsel__intval(evsel, sample, "irq");
> +       irq->irq_exit_time = sample->time;
> +       irq->cpu = sample->cpu;
> +
> +       if (cpu_list && !test_bit(sample->cpu, cpu_bitmap))
> +               return err;
> +
> +       output_report(irq);
> +
> +       return err;
> +}
> +