From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Alan Maguire <alan.maguire@oracle.com>
Cc: ast@kernel.org, daniel@iogearbox.net, andriin@fb.com, yhs@fb.com,
linux@rasmusvillemoes.dk, andriy.shevchenko@linux.intel.com,
pmladek@suse.com, kafai@fb.com, songliubraving@fb.com,
john.fastabend@gmail.com, kpsingh@chromium.org, shuah@kernel.org,
rdna@fb.com, scott.branden@broadcom.com, quentin@isovalent.com,
cneirabustos@gmail.com, jakub@cloudflare.com, mingo@redhat.com,
rostedt@goodmis.org, bpf@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
acme@kernel.org
Subject: Re: [PATCH v6 bpf-next 2/6] bpf: move to generic BTF show support, apply it to seq files/strings
Date: Thu, 24 Sep 2020 17:33:23 -0700 [thread overview]
Message-ID: <20200925003323.u2s2vyyqq2uhtij7@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <1600883188-4831-3-git-send-email-alan.maguire@oracle.com>
On Wed, Sep 23, 2020 at 06:46:24PM +0100, Alan Maguire wrote:
>
> +/* Chunk size we use in safe copy of data to be shown. */
> +#define BTF_SHOW_OBJ_SAFE_SIZE 256
sizeof(struct btf_show) == 472
It's allocated on stack and called from bpf prog.
It's a leaf function, but it still worries me a bit.
I've trimmed it down to 32 and everything seems to be printing fine.
There will be more calls to copy_from_kernel_nofault(), but so what?
Is there a downside to make it that small?
Similarly state.name is 128 bytes. May be use 80 there?
I think that should be plenty still.
> + * Another problem is we want to ensure the data for display is safe to
> + * access. To support this, the "struct obj" is used to track the data
'struct obj' doesn't exist. It's an anon field 'struct {} obj;' inside btf_show
that you're referring to, right?
Would be good to fix this comment.
> +struct btf_show {
> + u64 flags;
> + void *target; /* target of show operation (seq file, buffer) */
> + void (*showfn)(struct btf_show *show, const char *fmt, ...);
buildbot complained that this field needs to be annotated.
> +#define btf_show(show, ...) \
> + do { \
> + if (!show->state.depth_check) \
> + show->showfn(show, __VA_ARGS__); \
> + } while (0)
Does it have to be a macro? What are you gaining from macro
instead of vararg function?
> +static inline const char *__btf_show_indent(struct btf_show *show)
please remove all 'inline' from .c file.
There is no need to give such hints to the compiler.
> +#define btf_show_indent(show) \
> + ((show->flags & BTF_SHOW_COMPACT) ? "" : __btf_show_indent(show))
> +
> +#define btf_show_newline(show) \
> + ((show->flags & BTF_SHOW_COMPACT) ? "" : "\n")
> +
> +#define btf_show_delim(show) \
> + (show->state.depth == 0 ? "" : \
> + ((show->flags & BTF_SHOW_COMPACT) && show->state.type && \
> + BTF_INFO_KIND(show->state.type->info) == BTF_KIND_UNION) ? "|" : ",")
> +
> +#define btf_show_type_value(show, fmt, value) \
> + do { \
> + if ((value) != 0 || (show->flags & BTF_SHOW_ZERO) || \
> + show->state.depth == 0) { \
> + btf_show(show, "%s%s" fmt "%s%s", \
> + btf_show_indent(show), \
> + btf_show_name(show), \
> + value, btf_show_delim(show), \
> + btf_show_newline(show)); \
> + if (show->state.depth > show->state.depth_to_show) \
> + show->state.depth_to_show = show->state.depth; \
> + } \
> + } while (0)
> +
> +#define btf_show_type_values(show, fmt, ...) \
> + do { \
> + btf_show(show, "%s%s" fmt "%s%s", btf_show_indent(show), \
> + btf_show_name(show), \
> + __VA_ARGS__, btf_show_delim(show), \
> + btf_show_newline(show)); \
> + if (show->state.depth > show->state.depth_to_show) \
> + show->state.depth_to_show = show->state.depth; \
> + } while (0)
> +
> +/* How much is left to copy to safe buffer after @data? */
> +#define btf_show_obj_size_left(show, data) \
> + (show->obj.head + show->obj.size - data)
> +
> +/* Is object pointed to by @data of @size already copied to our safe buffer? */
> +#define btf_show_obj_is_safe(show, data, size) \
> + (data >= show->obj.data && \
> + (data + size) < (show->obj.data + BTF_SHOW_OBJ_SAFE_SIZE))
> +
> +/*
> + * If object pointed to by @data of @size falls within our safe buffer, return
> + * the equivalent pointer to the same safe data. Assumes
> + * copy_from_kernel_nofault() has already happened and our safe buffer is
> + * populated.
> + */
> +#define __btf_show_obj_safe(show, data, size) \
> + (btf_show_obj_is_safe(show, data, size) ? \
> + show->obj.safe + (data - show->obj.data) : NULL)
Similarly I don't understand the benefit of macros.
They all could have been normal functions.
> +static inline void *btf_show_obj_safe(struct btf_show *show,
> + const struct btf_type *t,
> + void *data)
drop 'inline' pls.
> +{
> + int size_left, size;
> + void *safe = NULL;
> +
> + if (show->flags & BTF_SHOW_UNSAFE)
> + return data;
> +
> + (void) btf_resolve_size(show->btf, t, &size);
Is this ok to ignore the error?
next prev parent reply other threads:[~2020-09-25 0:33 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-23 17:46 [PATCH v6 bpf-next 0/6] bpf: add helpers to support BTF-based kernel data display Alan Maguire
2020-09-23 17:46 ` [PATCH v6 bpf-next 1/6] bpf: provide function to get vmlinux BTF information Alan Maguire
2020-09-23 17:46 ` [PATCH v6 bpf-next 2/6] bpf: move to generic BTF show support, apply it to seq files/strings Alan Maguire
2020-09-25 0:33 ` Alexei Starovoitov [this message]
2020-09-23 17:46 ` [PATCH v6 bpf-next 3/6] bpf: add bpf_snprintf_btf helper Alan Maguire
2020-09-25 0:42 ` Alexei Starovoitov
2020-09-23 17:46 ` [PATCH v6 bpf-next 4/6] selftests/bpf: add bpf_snprintf_btf helper tests Alan Maguire
2020-09-25 0:50 ` Alexei Starovoitov
2020-09-25 17:36 ` Andrii Nakryiko
2020-09-23 17:46 ` [PATCH v6 bpf-next 5/6] bpf: add bpf_seq_printf_btf helper Alan Maguire
2020-09-23 17:46 ` [PATCH v6 bpf-next 6/6] selftests/bpf: add test for " Alan Maguire
2020-09-25 1:26 ` Alexei Starovoitov
2020-09-28 14:12 ` Alan Maguire
2020-09-28 17:51 ` Andrii Nakryiko
2020-09-29 1:54 ` Alexei Starovoitov
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=20200925003323.u2s2vyyqq2uhtij7@ast-mbp.dhcp.thefacebook.com \
--to=alexei.starovoitov@gmail.com \
--cc=acme@kernel.org \
--cc=alan.maguire@oracle.com \
--cc=andriin@fb.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=cneirabustos@gmail.com \
--cc=daniel@iogearbox.net \
--cc=jakub@cloudflare.com \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=kpsingh@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
--cc=mingo@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=pmladek@suse.com \
--cc=quentin@isovalent.com \
--cc=rdna@fb.com \
--cc=rostedt@goodmis.org \
--cc=scott.branden@broadcom.com \
--cc=shuah@kernel.org \
--cc=songliubraving@fb.com \
--cc=yhs@fb.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).