On Tue, Jan 25, 2022 at 10:28 AM HAGIO KAZUHITO(萩尾 一仁) <[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]>
> > ---
> > 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. :-)
> > +
> > + 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
[email protected]
https://listman.redhat.com/mailman/listinfo/crash-utility
