Re: [PATCH v6 bpf-next 6/6] selftests/bpf: add test for bpf_seq_printf_btf helper

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Mon, Sep 28, 2020 at 7:14 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
>
>
> On Thu, 24 Sep 2020, Alexei Starovoitov wrote:
>
> > to whatever number, but printing single task_struct needs ~800 lines and
> > ~18kbytes. Humans can scroll through that much spam, but can we make it less
> > verbose by default somehow?
> > May be not in this patch set, but in the follow up?
> >
>
> One approach that might work would be to devote 4 bits or so of
> flag space to a "maximum depth" specifier; i.e. at depth 1,
> only base types are displayed, no aggregate types like arrays,
> structs and unions.  We've already got depth processing in the
> code to figure out if possibly zeroed nested data needs to be
> displayed, so it should hopefully be a simple follow-up.
>
> One way to express it would be to use "..." to denote field(s)
> were omitted. We could even use the number of "."s to denote
> cases where multiple fields were omitted, giving a visual sense
> of how much data was omitted.  So for example with
> BTF_F_MAX_DEPTH(1), task_struct looks like this:
>
> (struct task_struct){
>  .state = ()1,
>  .stack = ( *)0x00000000029d1e6f,
>  ...
>  .flags = (unsigned int)4194560,
>  ...
>  .cpu = (unsigned int)36,
>  .wakee_flips = (unsigned int)11,
>  .wakee_flip_decay_ts = (long unsigned int)4294914874,
>  .last_wakee = (struct task_struct *)0x000000006c7dfe6d,
>  .recent_used_cpu = (int)19,
>  .wake_cpu = (int)36,
>  .prio = (int)120,
>  .static_prio = (int)120,
>  .normal_prio = (int)120,
>  .sched_class = (struct sched_class *)0x00000000ad1561e6,
>  ...
>  .exec_start = (u64)674402577156,
>  .sum_exec_runtime = (u64)5009664110,
>  .vruntime = (u64)167038057,
>  .prev_sum_exec_runtime = (u64)5009578167,
>  .nr_migrations = (u64)54,
>  .depth = (int)1,
>  .parent = (struct sched_entity *)0x00000000cba60e7d,
>  .cfs_rq = (struct cfs_rq *)0x0000000014f353ed,
>  ...
>
> ...etc. What do you think?

It's not clear to me what exactly is omitted with ... ? Would it make
sense to still at least list a field name and "abbreviated" value.
E.g., for arrays:

.array_field = (int[16]){ ... },

Similarly for struct:

.struct_field = (struct my_struct){ ... },

? With just '...' I get a very strong and unsettling feeling of
missing out on the important stuff :)

>
> > > +SEC("iter/task")
> > > +int dump_task_fs_struct(struct bpf_iter__task *ctx)
> > > +{
> > > +   static const char fs_type[] = "struct fs_struct";
> > > +   struct seq_file *seq = ctx->meta->seq;
> > > +   struct task_struct *task = ctx->task;
> > > +   struct fs_struct *fs = (void *)0;
> > > +   static struct btf_ptr ptr = { };
> > > +   long ret;
> > > +
> > > +   if (task)
> > > +           fs = task->fs;
> > > +
> > > +   ptr.type = fs_type;
> > > +   ptr.ptr = fs;
> >
> > imo the following is better:
> >        ptr.type_id = __builtin_btf_type_id(*fs, 1);
> >        ptr.ptr = fs;
> >
>
> I'm still seeing lookup failures using __builtin_btf_type_id(,1) -
> whereas both __builtin_btf_type_id(,0) and Andrii's
> suggestion of bpf_core_type_id_kernel() work. Not sure what's
> going on - pahole is v1.17, clang is

bpf_core_type_id_kernel() is

__builtin_btf_type_id(*(typeof(type) *)0, BPF_TYPE_ID_TARGET)

BPF_TYPE_ID_TARGET is exactly 1. So I bet it's because of the type
capturing through typeof() and pointer casting/dereferencing, which
preserves type information properly. Regardless, just use the helper,
IMO.


>
> clang version 12.0.0 (/mnt/src/llvm-project/clang
> 7ab7b979d29e1e43701cf690f5cf1903740f50e3)
>

[...]