[v5,bpf-next,0/9] bpf: implement variadic printk helper

This series introduces a new helper, bpf_trace_vprintk, which functions
like bpf_trace_printk but supports > 3 arguments via a pseudo-vararg u64
array. The bpf_printk libbpf convenience macro is modified to use
bpf_trace_vprintk when > 3 varargs are passed, otherwise the previous
behavior - using bpf_trace_printk - is retained.

Helper functions and macros added during the implementation of
bpf_seq_printf and bpf_snprintf do most of the heavy lifting for
bpf_trace_vprintk. There's no novel format string wrangling here.

Usecase here is straightforward: Giving BPF program writers a more
powerful printk will ease development of BPF programs, particularly
during debugging and testing, where printk tends to be used.

This feature was proposed by Andrii in libbpf mirror's issue tracker
[1].

[1] https://github.com/libbpf/libbpf/issues/315

v4 -> v5:

* patch 8: added test for "%pS" format string w/ NULL fmt arg [Daniel]
* patch 8: dmesg -> /sys/kernel/debug/tracing/trace_pipe in commit message [Andrii]
* patch 9: squash into patch 8, remove the added test in favor of just bpf_printk'ing in patch 8's test [Andrii]
    * migrate comment to /* */
* header comments improved$
    * uapi/linux/bpf.h: u64 -> long return type [Daniel]
    * uapi/linux/bpf.h: function description explains benefit of bpf_trace_vprintk over bpf_trace_printk [Daniel]
    * uapi/linux/bpf.h: added patch explaining that data_len should be a multiple of 8 in bpf_seq_printf, bpf_snprintf descriptions [Daniel]
    * tools/lib/bpf/bpf_helpers.h: move comment to new bpf_printk [Andrii]
* rebase

v3 -> v4:
* Add patch 2, which migrates reference_tracking prog_test away from 
  bpf_program__load. Could be placed a bit later in the series, but 
  wanted to keep the actual vprintk-related patches contiguous
* Add patch 9, which adds a program w/ 0 fmt arg bpf_printk to vprintk
  test
* bpf_printk convenience macro isn't multiline anymore, so simplify [Andrii]
* Add some comments to ___bpf_pick_printk to make it more obvious when
  implementation switches from printk to vprintk [Andrii]
* BPF_PRINTK_FMT_TYPE -> BPF_PRINTK_FMT_MOD for 'static const' fmt string
  in printk wrapper macro [Andrii]
    * checkpatch.pl doesn't like this, says "Macros with complex values 
      should be enclosed in parentheses". Strange that it didn't have similar
      complaints about v3's BPF_PRINTK_FMT_TYPE. Regardless, IMO the complaint
      is not highlighting a real issue in the case of this macro.
* Fix alignment of __bpf_vprintk and __bpf_pick_printk [Andrii]
* rebase

v2 -> v3:
* Clean up patch 3's commit message [Alexei]
* Add patch 4, which modifies __bpf_printk to use 'static const char' to
  store fmt string with fallback for older kernels [Andrii]
* rebase

v1 -> v2:

* Naming conversation seems to have gone in favor of keeping
  bpf_trace_vprintk, names are unchanged

* Patch 3 now modifies bpf_printk convenience macro to choose between
  __bpf_printk and __bpf_vprintk 'implementation' macros based on arg
  count. __bpf_vprintk is a renaming of bpf_vprintk convenience macro
  from v1, __bpf_printk is the existing bpf_printk implementation.

  This patch could use some scrutiny as I think current implementation
  may regress developer experience in a specific case, turning a
  compile-time error into a load-time error. Unclear to me how
  common the case is, or whether the macro magic I chose is ideal.

* char ___fmt[] to static const char ___fmt[] change was not done,
  wanted to leave __bpf_printk 'implementation' macro unchanged for v2
  to ease discussion of above point

* Removed __always_inline from __set_printk_clr_event [Andrii]
* Simplified bpf_trace_printk docstring to refer to other functions
  instead of copy/pasting and avoid specifying 12 vararg limit [Andrii]
* Migrated trace_printk selftest to use ASSERT_ instead of CHECK
  * Adds new patch 5, previous patch 5 is now 6
* Migrated trace_vprintk selftest to use ASSERT_ instead of CHECK,
  open_and_load instead of separate open, load [Andrii]
* Patch 2's commit message now correctly mentions trace_pipe instead of
  dmesg [Andrii]

Dave Marchevsky (9):
  bpf: merge printk and seq_printf VARARG max macros
  selftests/bpf: stop using bpf_program__load
  bpf: add bpf_trace_vprintk helper
  libbpf: Modify bpf_printk to choose helper based on arg count
  libbpf: use static const fmt string in __bpf_printk
  bpftool: only probe trace_vprintk feature in 'full' mode
  selftests/bpf: Migrate prog_tests/trace_printk CHECKs to ASSERTs
  selftests/bpf: add trace_vprintk test prog
  bpf: clarify data_len param in bpf_snprintf and bpf_seq_printf
    comments

 include/linux/bpf.h                           |  3 +
 include/uapi/linux/bpf.h                      | 16 ++++-
 kernel/bpf/core.c                             |  5 ++
 kernel/bpf/helpers.c                          |  6 +-
 kernel/trace/bpf_trace.c                      | 54 ++++++++++++++-
 tools/bpf/bpftool/feature.c                   |  1 +
 tools/include/uapi/linux/bpf.h                | 16 ++++-
 tools/lib/bpf/bpf_helpers.h                   | 51 +++++++++++---
 tools/testing/selftests/bpf/Makefile          |  3 +-
 .../bpf/prog_tests/reference_tracking.c       | 39 ++++++++---
 .../selftests/bpf/prog_tests/trace_printk.c   | 24 +++----
 .../selftests/bpf/prog_tests/trace_vprintk.c  | 68 +++++++++++++++++++
 .../selftests/bpf/progs/trace_vprintk.c       | 33 +++++++++
 tools/testing/selftests/bpf/test_bpftool.py   | 22 +++---
 14 files changed, 286 insertions(+), 55 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_vprintk.c
 create mode 100644 tools/testing/selftests/bpf/progs/trace_vprintk.c

On Sun, Sep 12, 2021 at 8:56 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> This series introduces a new helper, bpf_trace_vprintk, which functions
> like bpf_trace_printk but supports > 3 arguments via a pseudo-vararg u64
> array. The bpf_printk libbpf convenience macro is modified to use
> bpf_trace_vprintk when > 3 varargs are passed, otherwise the previous
> behavior - using bpf_trace_printk - is retained.
>
> Helper functions and macros added during the implementation of
> bpf_seq_printf and bpf_snprintf do most of the heavy lifting for
> bpf_trace_vprintk. There's no novel format string wrangling here.
>
> Usecase here is straightforward: Giving BPF program writers a more
> powerful printk will ease development of BPF programs, particularly
> during debugging and testing, where printk tends to be used.
>
> This feature was proposed by Andrii in libbpf mirror's issue tracker
> [1].
>
> [1] https://github.com/libbpf/libbpf/issues/315
>
> v4 -> v5:
>
> * patch 8: added test for "%pS" format string w/ NULL fmt arg [Daniel]
> * patch 8: dmesg -> /sys/kernel/debug/tracing/trace_pipe in commit message [Andrii]
> * patch 9: squash into patch 8, remove the added test in favor of just bpf_printk'ing in patch 8's test [Andrii]
>     * migrate comment to /* */
> * header comments improved$
>     * uapi/linux/bpf.h: u64 -> long return type [Daniel]
>     * uapi/linux/bpf.h: function description explains benefit of bpf_trace_vprintk over bpf_trace_printk [Daniel]
>     * uapi/linux/bpf.h: added patch explaining that data_len should be a multiple of 8 in bpf_seq_printf, bpf_snprintf descriptions [Daniel]
>     * tools/lib/bpf/bpf_helpers.h: move comment to new bpf_printk [Andrii]
> * rebase
>
> v3 -> v4:
> * Add patch 2, which migrates reference_tracking prog_test away from
>   bpf_program__load. Could be placed a bit later in the series, but
>   wanted to keep the actual vprintk-related patches contiguous
> * Add patch 9, which adds a program w/ 0 fmt arg bpf_printk to vprintk
>   test
> * bpf_printk convenience macro isn't multiline anymore, so simplify [Andrii]
> * Add some comments to ___bpf_pick_printk to make it more obvious when
>   implementation switches from printk to vprintk [Andrii]
> * BPF_PRINTK_FMT_TYPE -> BPF_PRINTK_FMT_MOD for 'static const' fmt string
>   in printk wrapper macro [Andrii]
>     * checkpatch.pl doesn't like this, says "Macros with complex values
>       should be enclosed in parentheses". Strange that it didn't have similar
>       complaints about v3's BPF_PRINTK_FMT_TYPE. Regardless, IMO the complaint
>       is not highlighting a real issue in the case of this macro.
> * Fix alignment of __bpf_vprintk and __bpf_pick_printk [Andrii]
> * rebase
>
> v2 -> v3:
> * Clean up patch 3's commit message [Alexei]
> * Add patch 4, which modifies __bpf_printk to use 'static const char' to
>   store fmt string with fallback for older kernels [Andrii]
> * rebase
>
> v1 -> v2:
>
> * Naming conversation seems to have gone in favor of keeping
>   bpf_trace_vprintk, names are unchanged
>
> * Patch 3 now modifies bpf_printk convenience macro to choose between
>   __bpf_printk and __bpf_vprintk 'implementation' macros based on arg
>   count. __bpf_vprintk is a renaming of bpf_vprintk convenience macro
>   from v1, __bpf_printk is the existing bpf_printk implementation.
>
>   This patch could use some scrutiny as I think current implementation
>   may regress developer experience in a specific case, turning a
>   compile-time error into a load-time error. Unclear to me how
>   common the case is, or whether the macro magic I chose is ideal.
>
> * char ___fmt[] to static const char ___fmt[] change was not done,
>   wanted to leave __bpf_printk 'implementation' macro unchanged for v2
>   to ease discussion of above point
>
> * Removed __always_inline from __set_printk_clr_event [Andrii]
> * Simplified bpf_trace_printk docstring to refer to other functions
>   instead of copy/pasting and avoid specifying 12 vararg limit [Andrii]
> * Migrated trace_printk selftest to use ASSERT_ instead of CHECK
>   * Adds new patch 5, previous patch 5 is now 6
> * Migrated trace_vprintk selftest to use ASSERT_ instead of CHECK,
>   open_and_load instead of separate open, load [Andrii]
> * Patch 2's commit message now correctly mentions trace_pipe instead of
>   dmesg [Andrii]
>
> Dave Marchevsky (9):
>   bpf: merge printk and seq_printf VARARG max macros
>   selftests/bpf: stop using bpf_program__load
>   bpf: add bpf_trace_vprintk helper
>   libbpf: Modify bpf_printk to choose helper based on arg count
>   libbpf: use static const fmt string in __bpf_printk
>   bpftool: only probe trace_vprintk feature in 'full' mode
>   selftests/bpf: Migrate prog_tests/trace_printk CHECKs to ASSERTs
>   selftests/bpf: add trace_vprintk test prog
>   bpf: clarify data_len param in bpf_snprintf and bpf_seq_printf
>     comments
>
>  include/linux/bpf.h                           |  3 +
>  include/uapi/linux/bpf.h                      | 16 ++++-
>  kernel/bpf/core.c                             |  5 ++
>  kernel/bpf/helpers.c                          |  6 +-
>  kernel/trace/bpf_trace.c                      | 54 ++++++++++++++-
>  tools/bpf/bpftool/feature.c                   |  1 +
>  tools/include/uapi/linux/bpf.h                | 16 ++++-
>  tools/lib/bpf/bpf_helpers.h                   | 51 +++++++++++---
>  tools/testing/selftests/bpf/Makefile          |  3 +-
>  .../bpf/prog_tests/reference_tracking.c       | 39 ++++++++---
>  .../selftests/bpf/prog_tests/trace_printk.c   | 24 +++----
>  .../selftests/bpf/prog_tests/trace_vprintk.c  | 68 +++++++++++++++++++
>  .../selftests/bpf/progs/trace_vprintk.c       | 33 +++++++++
>  tools/testing/selftests/bpf/test_bpftool.py   | 22 +++---
>  14 files changed, 286 insertions(+), 55 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_vprintk.c
>  create mode 100644 tools/testing/selftests/bpf/progs/trace_vprintk.c
>
> --
> 2.30.2
>

This is a great feature and nice and clean implementation, thanks.
Unfortunately it's conflicting with recently landed
bpf_get_branch_snapshot() series, so you'll have to rebase and
resubmit.

For the series:

Acked-by: Andrii Nakryiko <andrii@kernel.org>