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

Re: [PATCH] add a new command: ipcs

----- 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 ------
> 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 ------
 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 ------

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:


crash> ipcs
------ Shared Memory Segments ------
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

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.


Crash-utility mailing list

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


Powered by Linux