Re: [PATCHv3 bpf-next 3/3] bpf: Remove trace_printk_lock

[Yonghong Song <yhs@xxxxxxxx>]

On 12/15/22 1:44 PM, Jiri Olsa wrote:
> Both bpf_trace_printk and bpf_trace_vprintk helpers use static buffer
> guarded with trace_printk_lock spin lock.
>
> The spin lock contention causes issues with bpf programs attached to
> contention_begin tracepoint [1] [2].
>
> Andrii suggested we could get rid of the contention by using trylock,
> but we could actually get rid of the spinlock completely by using
> percpu buffers the same way as for bin_args in bpf_bprintf_prepare
> function.
>
> Adding new return 'buf' argument to struct bpf_bprintf_data and making
> bpf_bprintf_prepare to return also the buffer for printk helpers.
>
> [1] https://lore.kernel.org/bpf/CACkBjsakT_yWxnSWr4r-0TpPvbKm9-OBmVUhJb7hV3hY8fdCkw@xxxxxxxxxxxxxx/
> [2] https://lore.kernel.org/bpf/CACkBjsaCsTovQHFfkqJKto6S4Z8d02ud1D7MPESrHa1cVNNTrw@xxxxxxxxxxxxxx/
>
> Reported-by: Hao Sun <sunhao.th@xxxxxxxxx>
> Suggested-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>

Ack with a small nit below.

Acked-by: Yonghong Song <yhs@xxxxxx>

> ---
>   include/linux/bpf.h      |  3 +++
>   kernel/bpf/helpers.c     | 31 +++++++++++++++++++------------
>   kernel/trace/bpf_trace.c | 20 ++++++--------------
>   3 files changed, 28 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 656879385fbf..5fec2d1be6d7 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2795,10 +2795,13 @@ struct btf_id_set;
>   bool btf_id_set_contains(const struct btf_id_set *set, u32 id);
>   
>   #define MAX_BPRINTF_VARARGS		12
> +#define MAX_BPRINTF_BUF			1024
>   
>   struct bpf_bprintf_data {
>   	u32 *bin_args;
> +	char *buf;
>   	bool get_bin_args;
> +	bool get_buf;
>   };
>   
>   int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 9cca02e13f2e..23aa8cf8fd1a 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -756,19 +756,20 @@ static int bpf_trace_copy_string(char *buf, void *unsafe_ptr, char fmt_ptype,
>   /* Per-cpu temp buffers used by printf-like helpers to store the bprintf binary
>    * arguments representation.
>    */
> -#define MAX_BPRINTF_BUF_LEN	512
> +#define MAX_BPRINTF_BIN_ARGS	512
>   
>   /* Support executing three nested bprintf helper calls on a given CPU */
>   #define MAX_BPRINTF_NEST_LEVEL	3
>   struct bpf_bprintf_buffers {
> -	char tmp_bufs[MAX_BPRINTF_NEST_LEVEL][MAX_BPRINTF_BUF_LEN];
> +	char bin_args[MAX_BPRINTF_BIN_ARGS];
> +	char buf[MAX_BPRINTF_BUF];
>   };
> -static DEFINE_PER_CPU(struct bpf_bprintf_buffers, bpf_bprintf_bufs);
> +
> +static DEFINE_PER_CPU(struct bpf_bprintf_buffers[MAX_BPRINTF_NEST_LEVEL], bpf_bprintf_bufs);
>   static DEFINE_PER_CPU(int, bpf_bprintf_nest_level);
>   
> -static int try_get_fmt_tmp_buf(char **tmp_buf)
> +static int try_get_buffers(struct bpf_bprintf_buffers **bufs)
>   {
> -	struct bpf_bprintf_buffers *bufs;
>   	int nest_level;
>   
>   	preempt_disable();
> @@ -778,15 +779,14 @@ static int try_get_fmt_tmp_buf(char **tmp_buf)
>   		preempt_enable();
>   		return -EBUSY;
>   	}
> -	bufs = this_cpu_ptr(&bpf_bprintf_bufs);
> -	*tmp_buf = bufs->tmp_bufs[nest_level - 1];
> +	*bufs = this_cpu_ptr(&bpf_bprintf_bufs[nest_level - 1]);
>   
>   	return 0;
>   }
>   
>   void bpf_bprintf_cleanup(struct bpf_bprintf_data *data)
>   {
> -	if (!data->bin_args)
> +	if (!data->bin_args && !data->buf)
>   		return;
>   	if (WARN_ON_ONCE(this_cpu_read(bpf_bprintf_nest_level) == 0))
>   		return;
> @@ -811,7 +811,9 @@ void bpf_bprintf_cleanup(struct bpf_bprintf_data *data)
>   int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
>   			u32 num_args, struct bpf_bprintf_data *data)
>   {
> +	bool get_buffers = (data->get_bin_args && num_args) || data->get_buf;

We might waste some memory if num_args is 0 here. This is unlikely case
and it is not worthwhile to optimize for that, so current
implementation sounds good to me.

>   	char *unsafe_ptr = NULL, *tmp_buf = NULL, *tmp_buf_end, *fmt_end;
> +	struct bpf_bprintf_buffers *buffers = NULL;
>   	size_t sizeof_cur_arg, sizeof_cur_ip;
>   	int err, i, num_spec = 0;
>   	u64 cur_arg;
[...]