<!doctype html public "-//w3c//dtd html 4.0 transitional//en"> <html> Sharyathi Nagesh wrote: <blockquote TYPE=CITE>This is the patch after making following changes o Created a new function gather_cpudata_list_v2_nodes(). o gather_cpudata_list_v2_nodes() will be called for each node and it will update avail with corresponding value of shared memory.. o gather_cpudata_list_v2_nodes() is called inside do_slab_chain_percpu_v2_nodes during SLAB_WALKTHROUGH instead outside as earlier.. o Have removed the commented out section of SLAB_WALKTHROUGH (specified slab). o updated with FREEBUF at possible exit points. o updated dump_vm_table() to dump vt->kmem_cache_len_nodes Opens o The si->found field was not getting set for the dump I analysed. so if(si->found) part of the code was not getting executed. Needs to be checked for this case. Please go through the patch and let me know of your opinions. Thanks Sharyathi Nagesh</blockquote> <tt>Hi Sharyathi,</tt><tt></tt> <tt>OK -- it's getting there, but there are a couple problems with</tt> <tt>this second patch.</tt><tt></tt> <tt>The purpose of gather_cpudata_list_v2_nodes() is to fill</tt> <tt>several arrays of cached objects:</tt><tt></tt> <tt>(1) the list of per-cpu objects attached to the kmem_cache,</tt> <tt> referenced by the "array_cache[NR_CPUS]" member:</tt><tt></tt> <tt> struct kmem_cache {</tt> <tt> ...</tt> <tt> struct array_cache *array[NR_CPUS];</tt> <tt> ...</tt><tt></tt> <tt> So, for each cpu, its cached per-cpu objects from the</tt> <tt> above get stored in crash in each dynamically-allocated,</tt> <tt> per-cpu "cpudata" array:</tt><tt></tt> <tt> struct si_meminfo {</tt> <tt> ...</tt> <tt> ulong *cpudata[NR_CPUS];</tt><tt></tt> <tt> Now that gather_cpudata_list_v2_nodes() is being called</tt> <tt> multiple times instead of just once, the loading of each</tt> <tt> of the cpudata[NR_CPUS] array is being done redundantly.</tt> <tt> This is harmless, although you could probably just look</tt> <tt> at the "index" argument you now pass in, and if it's</tt> <tt> non-zero, the redundant reading of the cached per-cpu</tt> <tt> objects above can be avoided.</tt><tt></tt> <tt>(2) The second function of the current gather_cpudata_list_v2()</tt> <tt> is to gather the "shared" list from the singular kmem_list3</tt> <tt> structure:</tt><tt></tt> <tt> struct kmem_list3 {</tt> <tt> ...</tt> <tt> struct array_cache *shared;</tt><tt></tt> <tt> Since this has (until now) been a single list per kmem_cache,</tt> <tt> crash has been storing the list of shared objects in a singular</tt> <tt> dynamically-allocated array:</tt> <tt> struct si_meminfo {</tt> <tt> ...</tt> <tt> ulong *shared_array_cache;</tt> <tt> </tt> <tt>and that's the problem with your new gather_cpudata_list_v2_nodes().</tt> <tt>It's using a singular "shared_array_cache" array multiple times,</tt> <tt>once for each "index" of kmem_list3 structure. So it ends up</tt> <tt>over-writing the array each time, and so the si->shared_array_cache</tt> <tt>ends up containing the list of objects from the *last* "index"</tt> <tt>only.</tt><tt></tt> <tt>> o The si->found field was not getting set for the dump I analysed.</tt> <tt>> so if(si->found) part of the code was not getting executed.</tt><tt></tt> <tt>Given the above, this makes sense, since later on, when the</tt> <tt>shared_array_cache list is searched for a particular object, it</tt> <tt>most likely won't find it unless it happened to be contained in</tt> <tt>the last "index'd" set of objects.</tt><tt></tt> <tt>Now this can be solved in a couple of ways:</tt><tt></tt> <tt>The dynamic allocation of the singular shared_array_cache array</tt> <tt>can be increased by multiplying its currently predetermined</tt> <tt>max-size *times* the vt->kmem_cache_len_nodes. And then each</tt> <tt>time gather_cpudata_list_v2_nodes() is called, it could read</tt> <tt>the next set of objects into the location in the array where</tt> <tt>it left off the last time, i.e., the first non-zero entry.</tt><tt></tt> <tt>Alternatively, a different array could be used, although</tt> <tt>it couldn't use NR_CPUS as a delimiter, but would have</tt> <tt>to be aware of a maximum-number-of-numa-nodes, which</tt> <tt>would be kind of ugly. And if you use a new array, then</tt> <tt>the check_shared_list() function would have to be updated</tt> <tt>to check all of the lists instead of the current single</tt> <tt>list.</tt><tt></tt> <tt>BTW, there's a bug in check_shared_list() that I just</tt> <tt>noticed now -- it's harmless, but dumb:</tt><tt></tt> <tt>static int</tt> <tt>check_shared_list(struct meminfo *si, ulong obj)</tt> <tt>{</tt> <tt> int i;</tt><tt></tt> <tt> if (INVALID_MEMBER(kmem_list3_shared) ||</tt> <tt> !si->shared_array_cache)</tt> <tt> return FALSE;</tt><tt></tt> <tt> for (i = 0; i < si->shared_array_cache[i]; i++) {</tt> <tt> if (si->shared_array_cache[i] == obj)</tt> <tt> return TRUE;</tt> <tt> }</tt><tt></tt> <tt> return FALSE;</tt> <tt>}</tt><tt></tt> <tt>The for loop should be:</tt><tt></tt> <tt> for (i = 0; si->shared_array_cache[i]; i++) {</tt><tt></tt> <tt>It works now by dumb luck, stopping at the first non-zero</tt> <tt>entry (object address), which is what it's supposed</tt> <tt>to do.</tt><tt></tt> <tt>Finally, there's one other "gotcha" with this scheme.</tt> <tt>During intitialization, the value of vt->kmem_max_limit</tt> <tt>is determined in max_cpudata_limit, and later on it's</tt> <tt>used to allocate the object array:</tt><tt></tt> <tt> si->shared_array_cache = (ulong *)</tt> <tt> GETBUF(vt->kmem_max_limit * sizeof(ulong));</tt><tt></tt> <tt>vt->kmem_max_limit is calculated during initialization</tt> <tt>by determining the maximum cached object list size amongst</tt> <tt>both the kmem_cache->array[NR_CPUS] and the singular</tt> <tt>kmem_list3->array. Therefore, the max_cpudata_limit()</tt> <tt>function is only checking the first one:</tt><tt></tt> <tt> /*</tt> <tt> * If the shared list can be accessed, check its size as well.</tt> <tt> */</tt> <tt> if (VALID_MEMBER(kmem_list3_shared) &&</tt> <tt> VALID_MEMBER(kmem_cache_s_lists) &&</tt> <tt> readmem(cache+OFFSET(kmem_cache_s_lists)+OFFSET(kmem_list3_shared),</tt> <tt> KVADDR, &shared, sizeof(void *), "kmem_list3 shared",</tt> <tt> RETURN_ON_ERROR|QUIET) &&</tt> <tt> readmem(shared+OFFSET(array_cache_limit),</tt> <tt> KVADDR, &limit, sizeof(int), "shared array_cache limit",</tt> <tt> RETURN_ON_ERROR|QUIET)) {</tt> <tt> if (limit > max_limit)</tt> <tt> max_limit = limit;</tt> <tt> }</tt><tt></tt> <tt>Like your other new functions, if there's more than one list,</tt> <tt>they should probably all be checked for the largest one.</tt><tt></tt> <tt>Clear as mud?</tt><tt></tt> <tt>Dave</tt> <tt></tt> </html>