On Tue, Jan 25, 2022 at 10:28 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> 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@xxxxxxxxxx>
> ---
> 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)
{
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;
#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. :-)
> +
> + 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?
> +
> + 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 Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility
