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

Sharyathi Nagesh wrote: 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