Thank you for the review, Kazu. On Wed, Jan 26, 2022 at 1:07 PM HAGIO KAZUHITO(萩尾 一仁) <[email protected]> wrote:
> -----Original Message----- > > On Tue, Jan 25, 2022 at 10:28 AM HAGIO KAZUHITO(萩尾 一仁) < > [email protected] <mailto:[email protected]> > > > 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 <[email protected] <mailto: > [email protected]> > > > > --- > > > 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. > > Yes, sure, I will remove the indents. > > + #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) > ^^^^^ > Thank you for pointing out this issue. > > > > > > > > > > > + > > > + 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. > Sounds good. Thanks. Lianbo > > 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 > > > >
-- Crash-utility mailing list [email protected] https://listman.redhat.com/mailman/listinfo/crash-utility
