Re: [Crash-utility] [PATCH] add "files -n" command for an inode

On 2023/10/11 10:40, Huang Shijie wrote: > In the NUMA machine, it is useful to know the memory distribution of > an inode page cache: > How many pages in the node 0? > How many pages in the node 1? > > Add "files -n" command to get the memory distribution information: > 1.) Add new argument for dump_inode_page_cache_info() > 2.) make page_to_nid() a global function. > 3.) Add sumary_inode_page() to check each page's node > information. > 4.) Use print_inode_sumary_info() to print the > memory distribution information of an inode. > > Signed-off-by: Huang Shijie <shijie(a)os.amperecomputing.com&gt; Thanks for the patch. I tried it, but most of file inodes emit an error like this: crash> files 2249 | grep messages 81 ffff8915f73f7300 ffff8915d6bb03c0 ffff8915edb12788 REG /var/log/messages crash> files -n ffff8915edb12788 INODE NRPAGES ffff8915edb12788 200 files: do_xarray: callback operation failed: entry: 0 item: fffff3e1f84f8540 Am I missing anything?

> --- > defs.h | 1 + > filesys.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++----- > help.c | 10 ++++++++++ > memory.c | 3 +-- > 4 files changed, 59 insertions(+), 7 deletions(-) > > diff --git a/defs.h b/defs.h > index 96a7a2a..dfcc33d 100644 > --- a/defs.h > +++ b/defs.h > @@ -5748,6 +5748,7 @@ int dump_inode_page(ulong); > ulong valid_section_nr(ulong); > void display_memory_from_file_offset(ulonglong, long, void *); > void swap_info_init(void); > +int page_to_nid(ulong); > > /* > * filesys.c > diff --git a/filesys.c b/filesys.c > index 1d0ee7f..3c6a6bd 100644 > --- a/filesys.c > +++ b/filesys.c > @@ -49,7 +49,7 @@ static int match_file_string(char *, char *, char *); > static ulong get_root_vfsmount(char *); > static void check_live_arch_mismatch(void); > static long get_inode_nrpages(ulong); > -static void dump_inode_page_cache_info(ulong); > +static void dump_inode_page_cache_info(ulong, void *callback); > > #define DENTRY_CACHE (20) > #define INODE_CACHE (20) > @@ -2192,8 +2192,30 @@ get_inode_nrpages(ulong i_mapping) > return nrpages; > } > > +/* Used to collect the numa information for an inode */ > +static unsigned long *numa_node; nitpicking, but usually we use ulong for unsigned long.

> + > +static void > +print_inode_sumary_info(void) > +{ > + int i; > + > + fprintf(fp, "The physical memory in the page cache:\n"); > + for (i = 0; i < vt->numnodes; i++) > + fprintf(fp, "\tNode[%d]: %16ld pages\n", i, numa_node[i]); I would prefer more crash-like output, e.g. crash> files -n ffff8915edb12788 INODE NRPAGES ffff8915edb12788 200 NODE PAGES 0 150 1 50 crash> o

> +} > + > +static int > +sumary_inode_page(ulong page) Is "sumary" a typo of "summary"? or a common word?

And a warning is observed: filesys.c: In function 'sumary_inode_page': filesys.c:2215:1: warning: no return statement in function returning non-void [-Wreturn-type] } ^ (Probably no return value causes the callback failure?) > +{ > + int node = page_to_nid(page); > + > + if (node >= 0) > + numa_node[node]++; > +} > + > static void > -dump_inode_page_cache_info(ulong inode) > +dump_inode_page_cache_info(ulong inode, void *callback) > { > char *inode_buf; > ulong i_mapping, nrpages, root_rnode, xarray, count; > @@ -2236,7 +2258,7 @@ dump_inode_page_cache_info(ulong inode) > root_rnode = i_mapping + OFFSET(address_space_page_tree); > > lp.index = 0; > - lp.value = (void *)&dump_inode_page; > + lp.value = callback; > > if (root_rnode) > count = do_radix_tree(root_rnode, RADIX_TREE_DUMP_CB, &lp); > @@ -2276,7 +2298,7 @@ cmd_files(void) > ref = NULL; > refarg = NULL; > > - while ((c = getopt(argcnt, args, "d:R:p:c")) != EOF) { > + while ((c = getopt(argcnt, args, "d:n:R:p:c")) != EOF) { > switch(c) > { > case 'R': > @@ -2295,11 +2317,31 @@ cmd_files(void) > display_dentry_info(value); > return; > > + case 'n': > + if (VALID_MEMBER(address_space_page_tree) && > + VALID_MEMBER(inode_i_mapping)) { > + value = htol(optarg, FAULT_ON_ERROR, NULL); > + > + /* Allocate the array for this inode */ > + numa_node = malloc(sizeof(unsigned long) * vt->numnodes); > + BZERO(numa_node, sizeof(unsigned long) * vt->numnodes); > + > + dump_inode_page_cache_info(value, (void *)&sumary_inode_page); > + > + /* Print out the NUMA node information for this inode */ > + print_inode_sumary_info(); > + > + free(numa_node); > + numa_node = NULL; > + } else > + option_not_supported('n'); > + return; > + > case 'p': > if (VALID_MEMBER(address_space_page_tree) && > VALID_MEMBER(inode_i_mapping)) { > value = htol(optarg, FAULT_ON_ERROR, NULL); > - dump_inode_page_cache_info(value); > + dump_inode_page_cache_info(value, (void *)&dump_inode_page); > } else > option_not_supported('p'); > return; > diff --git a/help.c b/help.c > index cc7ab20..9b70940 100644 > --- a/help.c > +++ b/help.c > @@ -7863,6 +7863,8 @@ char *help_files[] = { > " specific, and only shows the data requested.\n", > " -d dentry given a hexadecimal dentry address, display its inode,", > " super block, file type, and full pathname.", > +" -n inode given a hexadecimal inode address, check all the pages", > +" in the page cache, and give a NUMA node distribution.", I would prefer "display" instead of "give" in crash help.