From: Mykola Lysenko <mykolal@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Mykola Lysenko <mykolal@fb.com>, bpf <bpf@vger.kernel.org>,
"Alexei Starovoitov" <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>,
"Daniel Borkmann" <daniel@iogearbox.net>
Subject: Re: [PATCH bpf-next] Small BPF verifier log improvements
Date: Tue, 22 Feb 2022 18:34:09 +0000 [thread overview]
Message-ID: <05EBE285-0588-4B7B-925D-056E5352D47A@fb.com> (raw)
In-Reply-To: <CAEf4BzbXqnchq5yejT3jhrRLizBRWQcuwb8U43r2-c6GpQEMBg@mail.gmail.com>
> On Feb 19, 2022, at 5:10 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Feb 18, 2022 at 4:36 PM Mykola Lysenko <mykolal@fb.com> wrote:
>>
>> In particular:
>> 1) remove output of inv for scalars
>> 2) remove _value suffixes for umin/umax/s32_min/etc (except map_value)
>> 3) remove output of id=0
>> 4) remove output of ref_obj_id=0
>>
>> Signed-off-by: Mykola Lysenko <mykolal@fb.com>
>> ---
>
> Few nitpicks below, but this is a great improvement!
Thanks a lot for the super quick review!
>
>> kernel/bpf/verifier.c | 72 +++---
>> .../testing/selftests/bpf/prog_tests/align.c | 218 +++++++++---------
>> .../selftests/bpf/prog_tests/log_buf.c | 4 +-
>> 3 files changed, 156 insertions(+), 138 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index d7473fee247c..a43bb0cf4c46 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -539,7 +539,7 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
>> char postfix[16] = {0}, prefix[32] = {0};
>> static const char * const str[] = {
>> [NOT_INIT] = "?",
>> - [SCALAR_VALUE] = "inv",
>> + [SCALAR_VALUE] = "",
>> [PTR_TO_CTX] = "ctx",
>> [CONST_PTR_TO_MAP] = "map_ptr",
>> [PTR_TO_MAP_VALUE] = "map_value",
>> @@ -666,6 +666,15 @@ static void scrub_spilled_slot(u8 *stype)
>> *stype = STACK_MISC;
>> }
>>
>> +#define verbose_append(fmt, ...) \
>> +({ \
>> + if (is_first_item) \
>> + is_first_item = false; \
>> + else \
>> + verbose(env, ","); \
>> + verbose(env, fmt, __VA_ARGS__); \
>> +})
>> +
>
> let's move this inside print_verifier_state() and #undef it at the end
> of that function, given it should only be used inside it.
>
> I don't know if it's better (it sucks either way that we need to
> define extra macro for this, but alternative approach would be to
> define separator:
>
> const char *sep = "";
>
> #define verbose_append(fmt, ...) ({ verbose(env, "%s" fmt, sep,
> __VA_ARGS__); sep = ","; })
I will use your approach as it is more compact than mine
>
>> static void print_verifier_state(struct bpf_verifier_env *env,
>> const struct bpf_func_state *state,
>> bool print_all)
>> @@ -693,65 +702,74 @@ static void print_verifier_state(struct bpf_verifier_env *env,
>> /* reg->off should be 0 for SCALAR_VALUE */
>> verbose(env, "%lld", reg->var_off.value + reg->off);
>> } else {
>> + bool is_first_item = true;
>> +
>> if (base_type(t) == PTR_TO_BTF_ID ||
>> base_type(t) == PTR_TO_PERCPU_BTF_ID)
>> verbose(env, "%s", kernel_type_name(reg->btf, reg->btf_id));
>> - verbose(env, "(id=%d", reg->id);
>> - if (reg_type_may_be_refcounted_or_null(t))
>> - verbose(env, ",ref_obj_id=%d", reg->ref_obj_id);
>> +
>> + verbose(env, "(");
>> +
>> + if (reg->id) {
>> + verbose(env, "id=%d", reg->id);
>> + is_first_item = false;
>
> should be also verbose_append?
yes
>
>> + }
>> +
>> + if (reg_type_may_be_refcounted_or_null(t) && reg->ref_obj_id)
>> + verbose_append("ref_obj_id=%d", reg->ref_obj_id);
>> if (t != SCALAR_VALUE)
>> - verbose(env, ",off=%d", reg->off);
>> + verbose_append("off=%d", reg->off);
>> if (type_is_pkt_pointer(t))
>> - verbose(env, ",r=%d", reg->range);
>> + verbose_append("r=%d", reg->range);
>> else if (base_type(t) == CONST_PTR_TO_MAP ||
>> base_type(t) == PTR_TO_MAP_KEY ||
>> base_type(t) == PTR_TO_MAP_VALUE)
>> - verbose(env, ",ks=%d,vs=%d",
>> - reg->map_ptr->key_size,
>> - reg->map_ptr->value_size);
>> + verbose_append("ks=%d,vs=%d",
>> + reg->map_ptr->key_size,
>> + reg->map_ptr->value_size);
>> if (tnum_is_const(reg->var_off)) {
>> /* Typically an immediate SCALAR_VALUE, but
>> * could be a pointer whose offset is too big
>> * for reg->off
>> */
>> - verbose(env, ",imm=%llx", reg->var_off.value);
>> + verbose_append("imm=%llx", reg->var_off.value);
>> } else {
>> if (reg->smin_value != reg->umin_value &&
>> reg->smin_value != S64_MIN)
>> - verbose(env, ",smin_value=%lld",
>> - (long long)reg->smin_value);
>> + verbose_append("smin=%lld",
>> + (long long)reg->smin_value);
>
> a bunch of these should fit within 100 character single line, given
> the code churn anyways, let's "straighten" those wrapped lines where
> possible? if we go with something shorter than "verbose_append" even
> more lines would fit (verbose_add, don't know, naming is hard).
sounds good
>
>> if (reg->smax_value != reg->umax_value &&
>> reg->smax_value != S64_MAX)
>> - verbose(env, ",smax_value=%lld",
>> - (long long)reg->smax_value);
>> + verbose_append("smax=%lld",
>> + (long long)reg->smax_value);
>> if (reg->umin_value != 0)
>> - verbose(env, ",umin_value=%llu",
>> - (unsigned long long)reg->umin_value);
>> + verbose_append("umin=%llu",
>> + (unsigned long long)reg->umin_value);
>> if (reg->umax_value != U64_MAX)
>> - verbose(env, ",umax_value=%llu",
>> - (unsigned long long)reg->umax_value);
>> + verbose_append("umax=%llu",
>> + (unsigned long long)reg->umax_value);
>> if (!tnum_is_unknown(reg->var_off)) {
>> char tn_buf[48];
>>
>> tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
>> - verbose(env, ",var_off=%s", tn_buf);
>> + verbose_append("var_off=%s", tn_buf);
>> }
>> if (reg->s32_min_value != reg->smin_value &&
>> reg->s32_min_value != S32_MIN)
>> - verbose(env, ",s32_min_value=%d",
>> - (int)(reg->s32_min_value));
>> + verbose_append("s32_min=%d",
>> + (int)(reg->s32_min_value));
>> if (reg->s32_max_value != reg->smax_value &&
>> reg->s32_max_value != S32_MAX)
>> - verbose(env, ",s32_max_value=%d",
>> - (int)(reg->s32_max_value));
>> + verbose_append("s32_max=%d",
>> + (int)(reg->s32_max_value));
>> if (reg->u32_min_value != reg->umin_value &&
>> reg->u32_min_value != U32_MIN)
>> - verbose(env, ",u32_min_value=%d",
>> - (int)(reg->u32_min_value));
>> + verbose_append("u32_min=%d",
>> + (int)(reg->u32_min_value));
>> if (reg->u32_max_value != reg->umax_value &&
>> reg->u32_max_value != U32_MAX)
>> - verbose(env, ",u32_max_value=%d",
>> - (int)(reg->u32_max_value));
>> + verbose_append("u32_max=%d",
>> + (int)(reg->u32_max_value));
>> }
>> verbose(env, ")");
>> }
>
> […]
prev parent reply other threads:[~2022-02-22 18:34 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-19 0:36 [PATCH bpf-next] Small BPF verifier log improvements Mykola Lysenko
2022-02-20 1:10 ` Andrii Nakryiko
2022-02-22 18:34 ` Mykola Lysenko [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=05EBE285-0588-4B7B-925D-056E5352D47A@fb.com \
--to=mykolal@fb.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.