On Wed, Sep 18, 2024 at 1:40 PM Mykyta Yatsenko
<mykyta.yatsenko5@xxxxxxxxx> wrote:
>
> From: Mykyta Yatsenko <yatsenko@xxxxxxxx>
>
> Production BPF programs are increasing in number of instructions and states
> to the point, where optimising verification process for them is necessary
> to avoid running into instruction limit. Authors of those BPF programs
> need to analyze verifier output, for example, collecting the most
> frequent source code lines to understand which part of the program has
> the biggest verification cost.
>
> This patch introduces `--top-src-lines` flag in veristat.
> `--top-src-lines=N` makes veristat output N the most popular sorce code
> lines, parsed from verification log.
>
> An example:
> ```
> $ sudo ./veristat --log-size=1000000000 --top-src-lines=4 pyperf600.bpf.o
> Processing 'pyperf600.bpf.o'...
> Top source lines (on_event):
> 4697: (pyperf.h:0)
> 2334: (pyperf.h:326) event->stack[i] = *symbol_id;
> 2334: (pyperf.h:118) pidData->offsets.String_data);
> 1176: (pyperf.h:92) bpf_probe_read_user(&frame->f_back,
> ...
> ```
>
> Signed-off-by: Mykyta Yatsenko <yatsenko@xxxxxxxx>
> ---
> tools/testing/selftests/bpf/veristat.c | 132 ++++++++++++++++++++++++-
> 1 file changed, 127 insertions(+), 5 deletions(-)
>
Looks pretty close to the final state, see some nits and some
potential problems below. We should figure out the verboseness bits
before applying this, though.
pw-bot: cr
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index 1ec5c4c47235..854fa4459b77 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
> @@ -143,6 +143,7 @@ static struct env {
> char **filenames;
> int filename_cnt;
> bool verbose;
> + bool print_verbose;
let's not do that, if we need to pass log_level to the kernel due to
top_src_lines, then that should be coded explicitly, not relying on
implicit "verbose" flag.
But also consider that there are two verbose levels for the kernel:
LOG_LEVEL=1 and LOG_LEVEL=2. We probably should default to LOG_LEVEL=2
if --top-src-lines is specified *unless* user explicitly specified
-vl1. Please play with different combinations and see if they make
sense.
log_level=2 is extremely verbose, so if the user specified -v (and not
-vl2), they will be surprised by extra verboseness.
> bool debug;
> bool quiet;
> bool force_checkpoints;
> @@ -179,11 +180,12 @@ static struct env {
> int files_skipped;
> int progs_processed;
> int progs_skipped;
> + int top_src_lines;
> } env;
[...]
> +static int print_top_src_lines(char * const buf, size_t buf_sz, const char *prog_name)
> +{
> + int lines_cap = 1;
> + int lines_size = 0;
> + char **lines;
> + char *line = NULL;
> + char *state;
> + struct line_cnt *freq = NULL;
> + struct line_cnt *cur;
> + int unique_lines;
> + int err;
> + int i;
> +
> + lines = calloc(lines_cap, sizeof(char *));
> + if (!lines)
> + return -ENOMEM;
start with lines_cap == 0, skip this calloc(), let realloc() do all
the work (lines should be initialized to NULL, of course)
> +
> + while ((line = strtok_r(line ? NULL : buf, "\n", &state))) {
> + if (strncmp(line, "; ", 2))
nit: with strncmp() and strcmp() I find it much more readable explicit
== 0 or != 0, otherwise my brain instinctively understands it in
exactly opposite way
> + continue;
> + line += 2;
> +
> + if (lines_size == lines_cap) {
> + char **tmp;
> +
> + lines_cap *= 2;
nit: generally speaking, it's common pattern to have some starting
minimal length that's a bit bigger than 1 or 2, so e.g., you'd do
lines_cap = max(16, 2 * lines_cap);
> + tmp = realloc(lines, lines_cap * sizeof(char *));
> + if (!tmp) {
> + err = -ENOMEM;
> + goto cleanup;
> + }
> + lines = tmp;
> + }
> + lines[lines_size] = line;
> + lines_size++;
> + }
> +
> + if (!lines_size)
nit: lines_size == 0 (it's not an error code and neither is it bool,
so explicit zero comparison makes more sense, IMO)
> + goto cleanup;
> +
> + qsort(lines, lines_size, sizeof(char *), str_cmp);
nit: probably would be better to use sizeof(*lines)
> +
> + freq = calloc(lines_size, sizeof(struct line_cnt));
> + if (!freq) {
> + err = -ENOMEM;
> + goto cleanup;
> + }
> +
> + cur = freq;
> + cur->line = lines[0];
> + cur->cnt = 1;
> + for (i = 1; i < lines_size; ++i) {
> + if (strcmp(lines[i], cur->line)) {
> + cur++;
> + cur->line = lines[i];
> + cur->cnt = 0;
> + }
> + cur->cnt++;
> + }
> + unique_lines = cur - freq + 1;
> +
> + qsort(freq, unique_lines, sizeof(struct line_cnt), line_cnt_cmp);
nit: sizeof(*freq), besides being shorter, it also won't need updating
if we rename the type *and* it shows connection to freq[] itself quite
explicitly
> +
> + printf("Top source lines (%s):\n", prog_name);
> + for (i = 0; i < min(unique_lines, env.top_src_lines); ++i) {
> + char *src_code;
> + char *src_line;
> +
> + src_code = strtok_r(freq[i].line, "@", &state);
what happens if the line doesn't have '@' in it, which will be the
case on older kernels? Can you please test all this on some old kernel
with a different verifier log output? It shouldn't crash.
tbh, it feels like strtok_r is a bit of an overkill here. I'd just
strrchr('@') (note reverse search), if it's there, extract everything
to the right as file name, otherwise assume the entire string is
source code line. This will handle both old and new verifier log
formats
> + src_line = strtok_r(NULL, "\0", &state);
> + if (src_line)
> + printf("%5d: (%s)\t%s\n", freq[i].cnt, src_line + 1, src_code);
> + else
> + printf("%5d: %s\n", freq[i].cnt, src_code);
> + }
> +
> +cleanup:
> + free(freq);
> + free(lines);
> + return err;
> +}
> +
[...]
