[Crash-utility] Test patch to fix kmem -s bug

[Dave Anderson]

Sharyathi Nagesh wrote:

> Hi
>         This is a test patch that will fix the problem with kmem -s. Which used
> to fail due to modification of kmem_cache structure. But it needs
> testing to validate the results, both in Uninode and multi node (NUMA)
> architectures.
>         Please go through and let me know of your opinions.
> Thanks
> Sharyathi Nagesh

Hi Sharyathi,

Thanks again for undertaking this effort.

I haven't done any compiling/testing, but upon reviewing the patch,
the update to gather_cpudata_list_v2():

       if(vt->flags  & PERCPU_KMALLOC_V2_NODES)
        {
                start_address = (ulong *) GETBUF(sizeof(ulong) * vt->kmem_cache_len_nodes);
                if (!readmem(si->cache+OFFSET(kmem_cache_s_lists),
                      KVADDR, &start_address[0],
                     sizeof(ulong) * vt->kmem_cache_len_nodes ,
                      "array nodelist array", RETURN_ON_ERROR))
                          error(INFO, "Error encountered with reading nodelists");

                for(index=0,avail=0 ; index < vt->kmem_cache_len_nodes && start_address[index] ; index++)
                {
                 if (!VALID_MEMBER(kmem_list3_shared) ||
                    !VALID_MEMBER(kmem_cache_s_lists) ||
                    !readmem(start_address[index] + OFFSET(kmem_list3_shared), KVADDR, &shared, sizeof(void *),
                    "kmem_list3 shared", RETURN_ON_ERROR|QUIET) ||
                    !readmem(shared + OFFSET(array_cache_avail),
                    KVADDR, &temp, sizeof(int), "shared array_cache avail",
                    RETURN_ON_ERROR|QUIET) || !temp)
                        return;
                   avail+=temp;
                }

        }

leads me to suggest that there should be a new gather_cpudata_list_v2_nodes()
function that gets called in the 3 places that gather_cpudata_list_v2() is currently
called.

First, the for loop is initializing "avail" to zero, which is a cumulative counter
that contains the number of kmem_cache.array per-cpu objects that it
calculated above, so that value is being wiped out.  It should just be
incremented by "temp" each time around the loop.

Secondly, this function as written won't work because there are now
several "shared" pointers being referenced instead of just one.  Note
that later on in the function, the last reference to "shared" is used:

       readmem(shared+SIZE(array_cache), KVADDR, si->shared_array_cache,
                sizeof(void *) * avail, "shared array_cache avail",
                FAULT_ON_ERROR);

to gather the list of share objects.  In the "nodes" case, the "shared" pointer
is left pointing the last value that you assigned to it, and only reads those
objects.  (If nodes are used, there's only just the one)  Anyway, in the nodes
case, they will have to cumulatively gathered/appended.  This kind of change
is why a new function should be created -- it's making the original to hard
to understand/maintain.

And another thing I'm not sure about -- in the new do_slab_chain_percpu_v2_nodes()
function -- in the SLAB_WALKTHROUGH case, you've removed (commented out) the
"specified slab" functionality, i.e., if you enter "kmem -[sS] <slab_address>.  Did you
mean to do that?

A few other minor points...

To avoid proliferation of internally allocated buffers, if either of the post-readmem
returns are done, a FREEBUF() of the start_address array should be done.
The buffer will be free after the whole command is complete, but it look like
it would be possible for this to fail thousands of times over the course of one
command, so a FREEBUF() would be in order.

Also, in the new do_slab_chain_percpu_v2_nodes() function, the same
thing applies for the start_address array -- if you return prematurely, please
do a FREEBUF() on it.

Third, in the loop shown above, there's no need to re-check for
!VALID_MEMBER(kmem_cache_s_lists) in the for loop -- we know at this
point that it can't be valid.

Lastly, I usually end up doing this myself, but whenever a new flag or field
gets added to one of the key data structures (as in the vm_table in this patch)
please print it in the associated table-dump function, which in this case
would be in dump_vm_table().

Finally  make sure your patch compiles cleanly -- and I didn't try this myself,
so I'm not implying that your patch does not -- by doing this before sending
it in:

$ touch defs.h
$ make Warn

Thanks,
   Dave