Re: [Crash-utility] [PATCH] add a new command: ipcs

----- Original Message ----- Hello Qiao, Interestingly enough, I remember looking into doing something like this to dump the three IPCS facilities quite a while ago. And although I cannot recall exactly what my reasons were, I think I decided against it because there were too many changes to kernel's IPC code over several kernel versions. So I certainly appreciate the amount of work that you have done. I built and tested your patch, and have the following comments. First, when posting a patch, can you please use the current upstream version of the crash utility? I think that your patch must be against crash-6.0.4: $ patch -p1 < ./0001-add-ipcs-command.patch patching file Makefile patching file defs.h Hunk #1 FAILED at 1662. Hunk #2 FAILED at 1820. Hunk #3 succeeded at 3695 (offset 121 lines). Hunk #4 succeeded at 4055 (offset 2 lines). 2 out of 4 hunks FAILED -- saving rejects to file defs.h.rej patching file global_data.c patching file help.c Hunk #1 succeeded at 5566 (offset 75 lines). patching file ipcs.c $ As part of my automated testing process, I run the same command(s), against ~150 saved dumpfiles consisting of various kernel versions. And not surprisingly, when testing the "ipcs" command, it failed a considerable number of times. These are samples of the errors: ipcs: invalid structure member offset: ipc_id_ary_p FILE: ipcs.c LINE: 281 FUNCTION: ipc_search_array() ipcs: invalid structure member offset: idr_layer_layer FILE: ipcs.c LINE: 338 FUNCTION: idr_find() ipcs: zero-size memory allocation! (called from 53ffb3) ipcs: invalid kernel virtual address: 8 type: "ipc_id_ary.p" ipcs: cannot resolve "hugetlbfs_file_operations" and eventually, when running against a 2.6.24 kernel, the "ipcs" command just quietly goes into an infinite loop. (I didn't check where...) And strangely enough, it worked OK on some RHEL5 dumpfiles, but failed on other RHEL5 dumpfiles. For example, on a 2.6.18-157.el5 kernel it failed like this: crash> ipcs ------ Shared Memory Segments ------ SHM_KERNEL KEY SHMID RSS SWAP UID PERMS BYTES NATTCH SHM_INODE ipcs: zero-size memory allocation! (called from 53ffb3) which comes from the GETBUF() call ipc_search_array(). And it's not because there's no IPC segments existing, because many of the dumpfiles just show this: crash> ipcs ------ Shared Memory Segments ------ SHM_KERNEL KEY SHMID RSS SWAP UID PERMS BYTES NATTCH SHM_INODE In that case, you should show something like "(none allocated)" after the header. Anyway, it would be preferable if the command worked on all 2.6-era kernels, but it should at least work on all kernels from RHEL5 (2.6.18) and up. The command output violates the 80-column rule. If the data displayed by a command is fixed (i.e., without things like file pathnames) , then it should be formatted so that it fits into 80-columns: (1) So in this case, I would omit the SHM_INODE column completely. If the user actually wants to see the inode, it can be found easily enough by following the trail from the shmid_kernel structure. (2) Don't waste space in the output with the "0x" prepended to the KEY value -- if you feel that a particular column's value is not obviously decimal or hexadecimal, then indicate what it is in the command's help page. (3) Your SHMID column should probably be made larger: 12345678901234567890123456789012345678901234567890123456789012345678901234567890 crash> ipcs ------ Shared Memory Segments ------ SHM_KERNEL KEY SHMID RSS SWAP UID PERMS BYTES NATTCH SHM_INODE ffff81003c537390 0x00000000 32768 0 96 42 600 393216 2 ffff81003bc3a718 ffff8100230cf1d0 0x7800801c 65537 0 1 0 666 4096 0 ffff81003d4cd418 ffff8100101b35d0 0x6600800a 135036930 0 1 0 666 4096 0 ffff81003c861118 ffff81002428b790 0x7600801e 269975555 1 0 0 666 4096 0 ffff81003bc3a118 ffff810018f9a0d0 0x6a0081bd 393221 0 0 99 600 8192 0 ffff81003d919a18 ffff81003c429d90 0x71008105 135364615 0 0 99 600 8192 0 ffff81002103a418 ffff8100300256d0 0x7000801c 270303241 0 0 99 600 8192 0 ffff810031304118 crash> Also, when creating new functions, can you please put the function name on its own line? It helps when using "grep" or "vi" to quickly find the actual function if the function name is the first thing on the line. In other words, change all instances like this: static void ipc_search_array(ulong ipc_ids_p, int id, int (*fn)(ulong, int)) { to this: static void ipc_search_array(ulong ipc_ids_p, int id, int (*fn)(ulong, int)) { Also, whenever you add new entries to the offset_table[] or size_table[] arrays, please dump their values in dump_offset_table() so that their values can be seen by "help -o". It makes it much easier to debug offset-related bugs and/or kernel changes over time. And if the command is going to be "ipcs", then it should deal with all 3 SYSV IPCS facilities. I wouldn't consider taking a half-baked command with just the shared memory stats. A few other minor nits: + /* + * global data + */ + static int idr_bits; + static ulong init_flags; + static ulong hugetlbfs_f_op_addr; + static ulong shm_f_op_addr; + static ulong shm_f_op_huge_addr; + static int use_shm_f_op; + static struct total_shm_info total_shm_info; + static unsigned int page_size; + static unsigned int page_shift; Can you put these items into one global data structure, and make them available for dumping during runtime with a "help" command? For now, you can add a "hidden" option letter to cmd_ipcs() to dump them. And page_size and page_shift are redundant -- you can just use the existing PAGESIZE() and PAGESHIFT() macros. Here's my suggestion. Work on this command as a standalone extension module. It will be very easy to transform what you have in place now into an extension module -- the most work will be involved with creating your own "MYOFFSET()" and MYSIZE() macros, and storing your offsets and sizes locally. And if/when it eventually is worthy of making it a new crash command or option, then it will be mostly a matter of doing a global search-and-replace of your MYOFFSET()/MYSIZE() callers with OFFSET() and SIZE(), and putting the members/sizes into the global offset_table[] and size_table[] arrays. And in the meantime, you won't have to constantly update the patch for each subsequent crash release. Thanks, Dave