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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



-----Original Message-----
> On Tue, Jan 25, 2022 at 10:28 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx <mailto: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 <mailto: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)
> {
> ...
> #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
> 


--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux