[Crash-utility] [PATCH] add a new command: ipcs
Dave Anderson
anderson at redhat.com
Fri Mar 30 19:16:35 UTC 2012
----- Original Message -----
> Hello Dave,
>
> I have been working on a new command to provide information of ipc
> facilities. Recently, the function displaying shared memory segments has
> been implemented.
>
> The output is like below:
>
> crash> ipcs
> ------ Shared Memory Segments ------
> SHM_KERNEL KEY SHMID RSS SWAP UID PERMS BYTES NATTCH SHM_INODE
> ffff8804683b0310 0x00000000 0 7 15 0 600 393216 2 ffff880470512d98
> ffff880470987910 0x00000000 32769 6 16 0 600 393216 2 ffff880470512758
> ffff880471621190 0x00000000 65538 46 0 0 600 393216 2 ffff880474202d98
> ffff8804747f1a50 0x00000000 98307 12 12 0 600 393216 2 ffff880471264758
> ffff8804704ad510 0x00000000 131076 96 0 0 600 393216 2 ffff880474094d98
>
> To see more details, please refer to the help information and the patch.
>
> --
> --
> Regards
> Qiao Nuohan
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
More information about the Crash-utility
mailing list