Re: [Crash-utility] [PATCH] Fix for "bpf -m|-M" options to appropriately display MEMLOCK and UID

-----Original Message----- > On Tue, Jan 25, 2022 at 10:28 AM HAGIO KAZUHITO(萩尾 一仁) < k-hagio-ab(a)nec.com <mailto:k-hagio-ab@nec.com> > > wrote: > > > Hi Lianbo, > > thanks for working on this. > > -----Original Message----- > > Kernel commit <80ee81e0403c> ("bpf: Eliminate rlimit-based memory > > accounting infra for bpf maps") removed the struct bpf_map_memory > > member from struct bpf_map. Without the patch, "bpf -m|-M" options > > will print the following errors: > > > > crash> bpf -m 1 > > ID BPF_MAP BPF_MAP_TYPE MAP_FLAGS > > 1 ffff96ba41804400 ARRAY 00000000 > > KEY_SIZE: 4 VALUE_SIZE: 8 MAX_ENTRIES: 64 MEMLOCK: (unknown) > > ^^^^^^^ > > NAME: "dist" UID: (unknown) > > ^^^^^^^ > > > > Signed-off-by: Lianbo Jiang <lijiang(a)redhat.com <mailto: lijiang(a)redhat.com&gt; > > > --- > > bpf.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 64 insertions(+), 3 deletions(-) > > > > diff --git a/bpf.c b/bpf.c > > index cb6b0ed385f9..d45e9ab9311b 100644 > > --- a/bpf.c > > +++ b/bpf.c > > @@ -15,6 +15,7 @@ > > */ > > > > #include "defs.h" > > +#include <stdbool.h> > > > > struct bpf_info { > > ulong status; > > @@ -63,6 +64,66 @@ static int do_old_idr(int, ulong, struct list_pair *); > > #define PROG_VERBOSE (0x40) > > #define MAP_VERBOSE (0x80) > > > > +static bool map_is_per_cpu(ulong type) > > I think that int is enough here and stdbool.h can be removed. > > (also type is int originally.) > > > > Thank you for the comment and suggestions, Kazu. > > Other several changes look good to me, But there are two issues, I have the following comments. > > > > > +{ > > + /* > > + * See the definition of bpf_map_type: > > + * include/uapi/linux/bpf.h > > + */ > > + #define BPF_MAP_TYPE_PERCPU_HASH (5UL) > > + #define BPF_MAP_TYPE_PERCPU_ARRAY (6UL) > > + #define BPF_MAP_TYPE_LRU_PERCPU_HASH (10UL) > > + #define BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE (21UL) > > This #define style in function looks unusual.. please let me change. > > > > The above code style is intentional, the intention is to enhance the readability of this code, and the > important thing is that these #define macros are not used in the other functions. > > In addition, it has the same #define style in crash utility functions, such as the netdump_memory_dump(). > And the similar definitions can also be found in other c source files, for example: set_kvm_iohole(), > symbol_dump(), show_ps_summary()... > > int > netdump_memory_dump(FILE *fp) > { > ... > #define DUMP_EXCLUDE_CACHE 0x00000001 /* Exclude LRU & SwapCache pages*/ > #define DUMP_EXCLUDE_CLEAN 0x00000002 /* Exclude all-zero pages */ > #define DUMP_EXCLUDE_FREE 0x00000004 /* Exclude free pages */ > #define DUMP_EXCLUDE_ANON 0x00000008 /* Exclude Anon pages */ > #define DUMP_SAVE_PRIVATE 0x00000010 /* Save private pages */ > > others = 0; > > ... > } > > Furthermore, the same style can be seen in the upstream kernels. :-) ok, that's fine with me. If you do so, could you remove the indents at the beginning of a line? I've not seen this style in function.

> + #define BPF_MAP_TYPE_PERCPU_HASH (5UL) > + #define BPF_MAP_TYPE_PERCPU_ARRAY (6UL) > + #define BPF_MAP_TYPE_LRU_PERCPU_HASH (10UL) > + #define BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE (21UL) ^^^^^

> > > > > + > > + return type == BPF_MAP_TYPE_PERCPU_HASH || > > + type == BPF_MAP_TYPE_PERCPU_ARRAY || > > + type == BPF_MAP_TYPE_LRU_PERCPU_HASH || > > + type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE; > > +} > > + > > +static bool map_is_fd_map(ulong type) > > +{ > > + /* > > + * See the definition of bpf_map_type: > > + * include/uapi/linux/bpf.h > > + */ > > + #define BPF_MAP_TYPE_PROG_ARRAY (3UL) > > + #define BPF_MAP_TYPE_PERF_EVENT_ARRAY (4UL) > > + #define BPF_MAP_TYPE_CGROUP_ARRAY (8UL) > > + #define BPF_MAP_TYPE_ARRAY_OF_MAPS (12UL) > > + #define BPF_MAP_TYPE_HASH_OF_MAPS (13UL) > > Ditto. > > > + > > + return type == BPF_MAP_TYPE_PROG_ARRAY || > > + type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || > > + type == BPF_MAP_TYPE_CGROUP_ARRAY || > > + type == BPF_MAP_TYPE_ARRAY_OF_MAPS || > > + type == BPF_MAP_TYPE_HASH_OF_MAPS; > > + > > +} > > + > > +static ulong bpf_map_memory_size(ulong map_type, ulong vsize, ulong ksize, ulong esize) > > The arguments are int and uint, and let's sync with kernel for readability. > > static ulong bpf_map_memory_size(int map_type, uint value_size, > uint key_size, uint max_entries) > > > +{ > > + ulong memsize,valsize; > > + int cpus = 0; > > + > > + valsize = vsize; > > + > > + if (map_is_fd_map(map_type)) > > + valsize = sizeof(ulong); > > This should be uint. > > else if (IS_FD_MAP(map)) > return sizeof(u32); > > > + > > + if (map_is_per_cpu(map_type)) { > > + cpus = get_cpus_possible(); > > + if (!cpus) > > + error(WARNING, "cpu_possible_map does not exist, pissible cpus: %d\n", > cpus); > > s/pissible/possible/ > > And if this fails, I think it would be better to print "(unknown)", so > let's return 0 here. > > > > When the cpu_possible_map does not exist, could it be better to set the default number of cpus to 1? In > fact, it has at least one cpu even if the get_cpus_possible() failed. It may not be an exact value, but > it is the closest value for the memlock(with a warning). > > And the value of memlock itself is approximate, not a completely exact value. What do you think? The value is an approximation, but it's the same as bpftool command output and this is an important aspect. I think that it's better to print "(unknown)" if they can be wrong, because they can be confusing/misleading to users.

Thanks, Kazu > > > > > + > > + valsize = roundup(vsize, 8) * cpus; > > + } > > + > > + memsize = roundup((ksize + valsize), 8); > > + > > + return roundup((esize * memsize), PAGESIZE()); > > +} > > + > > void > > cmd_bpf(void) > > { > > @@ -332,7 +393,7 @@ do_bpf(ulong flags, ulong prog_id, ulong map_id, int radix) > > { > > struct bpf_info *bpf; > > int i, c, found, entries, type; > > - uint uid, map_pages, key_size, value_size, max_entries; > > + uint uid, map_pages, key_size = 0, value_size = 0, max_entries = 0; > > ulong bpf_prog_aux, bpf_func, end_func, addr, insnsi, user; > > ulong do_progs, do_maps; > > ulonglong load_time; > > @@ -603,7 +664,7 @@ do_map_only: > > map_pages = UINT(bpf->bpf_map_buf + OFFSET(bpf_map_pages)); > > fprintf(fp, "%d\n", map_pages * PAGESIZE()); > > } else > > - fprintf(fp, "(unknown)\n"); > > + fprintf(fp, "%ld\n", bpf_map_memory_size(type, value_size, > key_size, > > max_entries)); > > Then, how about this? > > + } else if (memory = bpf_map_memory_size(type, value_size, key_size, > max_entries)) > + fprintf(fp, "%ld\n", memory); > + else > + fprintf(fp, "(unknown)"); > > I've attached a modified patch, could you check? > > > > > Thank you for writing the patch in the attachment. > > Thanks. > Lianbo > > > > > Thanks, > Kazu > > > > > fprintf(fp, " NAME: "); > > if (VALID_MEMBER(bpf_map_name)) { > > @@ -632,7 +693,7 @@ do_map_only: > > else > > fprintf(fp, "(unknown)\n"); > > } else > > - fprintf(fp, "(unknown)\n"); > > + fprintf(fp, "(unused)\n"); > > } > > > > if (flags & DUMP_STRUCT) { > > -- > > 2.20.1 >