vma->shared list_head initializations
Subsystems (8)
PATCH 1/1 vma->shared list_head initializations
more list_head debugging carnage. This time it was proc_pid_statm()
trying to do list_empty() on vma->shared in various bad ways. First
were head and next containing slab poison. with that fixed, it was then dying with a vma->shared pointing to a valid list that it wasn't a member of. I guessed that this was because we hadn't cleared it when we copied it whole-sale from another vma when it was allocated. sure enough, the attached patch makes ps run without exploding.
Andrew, am I on to something? If the attached patch isn't insane, can you marshall it and the previous list_head init fix on to linus?
There are probably still arch vma allocators that are doing it wrong, but I didn't look.
in any case, I think I'll submit the list_head debugging patch. It seems to do good things, and is prefaced on CONFIG_DEBUG_LIST_HEAD.. and the system appears to run fine with this and the previous fix applied.
- z
fs/exec.c renamed from fs/exec.c.fmuta +1 -0
@@ -400,6 +400,7 @@ mpnt->vm_ops =NULL ; mpnt->vm_pgoff =0 ; mpnt->vm_file =NULL ; +INIT_LIST_HEAD (&mpnt->shared); mpnt->vm_private_data = (void *)0 ;insert_vm_struct (mm, mpnt); mm->total_vm = (mpnt->vm_end - mpnt->vm_start) >>PAGE_SHIFT ;
kernel/fork.c renamed from kernel/fork.c.fmuta +1 -0
@@ -246,6 +246,7 @@ tmp->vm_mm = mm; tmp->vm_next =NULL ; file = tmp->vm_file; +INIT_LIST_HEAD (&tmp->shared);if (file) {struct inode *inode = file->f_dentry->d_inode;get_file (file);
mm/mmap.c renamed from mm/mmap.c.fmuta +4 -0
@@ -553,6 +553,7 @@ vma->vm_file =NULL ; vma->vm_private_data =NULL ; vma->vm_raend =0 ; +INIT_LIST_HEAD (&vma->shared);if (file) { error = -EINVAL ;
@@ -1052,6 +1053,8 @@/* most fields are the same, copy all, and then fixup */ *new = *vma; +INIT_LIST_HEAD (&new->shared); +if (new_below) { new->vm_end = addr; vma->vm_start = addr;
@@ -1215,6 +1218,7 @@ vma->vm_pgoff =0 ; vma->vm_file =NULL ; vma->vm_private_data =NULL ; +INIT_LIST_HEAD (&vma->shared);vma_link (mm, vma, prev, rb_link, rb_parent);
mm/mremap.c renamed from mm/mremap.c.fmuta +1 -0
@@ -200,6 +200,7 @@if (!move_page_tables (vma, new_addr, addr, old_len)) {if (allocated_vma) { *new_vma = *vma; +INIT_LIST_HEAD (&new_vma->shared); new_vma->vm_start = new_addr; new_vma->vm_end = new_addr+new_len; new_vma->vm_pgoff += (addr - vma->vm_start) >>PAGE_SHIFT ;
Quoted 3 lines · Zach Brown
more
list_headdebugging carnage.
yup
fs/exec.c Sat Sep 28 19:51:08 2002 line 406
Quoted 10 lines
fs/exec.c Sat Sep 28 19:51:08 2002 renamed from fs/exec.c.fmuta Sat Sep 28 19:50:20 2002 +1
-0@@ -400,6 +400,7 @@ mpnt->vm_ops = NULL; mpnt->vm_pgoff = 0; mpnt->vm_file = NULL; + INIT_LIST_HEAD(&mpnt->shared); mpnt->vm_private_data = (void *) 0; insert_vm_struct(mm, mpnt); mm->total_vm = (mpnt->vm_end - mpnt->vm_start) >> PAGE_SHIFT;
Fair enough, short-term. But what your patch is really saying is "this code stinks".
We need to lose all those open-coded accesses to vm_area_cachep, give that cache a constructor and possibly write some helper functions. To lose all this fragile "did I remember to initialise everything and has anyone added any more fields since I wrote that code" gunk.
<looks hopefully at Christoph>
PATCH 1/1 add callback back to slab pruning
Hi Andrew,
I posted this Thursday but it seems to have gotten lost in the storm of messages on slab.
This adds a callback to allow users of prunable cache familes to register their shrinker callback with the vm. This allows adding prunable cache familes without modifing vmscan. It also allows us to assign different 'seek costs' to familes (I have used a cost of 2 for all familes in this patch).
I have moved the code from slab.c to vmscan.c since this really is a vm function. This avoids an export and makes the code clearer.
We may well want a del_shrinker function. As it stands now nobody needs it. When/if its necessary its easy enought to add. Andrew would you like the patch?
Comments?
Ed
----------
fs/dcache.c +10 -5
@@ -570,9 +570,10 @@* This is called from kswapd when we think we need some * more memory. */ -int shrink_dcache_memory (int ratio,unsigned int gfp_mask) +int shrink_dcache_memory (int nr,unsigned int gfp_mask) { -int entries = dentry_stat.nr_dentry / ratio +1 ; +if (!nr) +return dentry_stat.nr_dentry;/* * Nasty deadlock avoidance. *
@@ -585,10 +586,10 @@* block allocations, but for now: */ if (!(gfp_mask & __GFP_FS)) -return 0 ; +return nr; -prune_dcache (entries); -return entries; +prune_dcache (nr); +return 0 ; }#define NAME_ALLOC_LEN (len) ((len+16) & ~15)
@@ -1328,6 +1329,8 @@ NULL, NULL);if (!dentry_cache)panic ("Cannot create dentry cache" ); + +set_shrinker (DEFAULT_SEEKS , shrink_dcache_memory);#if PAGE_SHIFT <13 mempages >>= (13 -PAGE_SHIFT );
@@ -1401,6 +1404,8 @@ SLAB_HWCACHE_ALIGN, NULL, NULL);if (!dquot_cachep)panic ("Cannot create dquot SLAB cache" ); + +set_shrinker (DEFAULT_SEEKS , shrink_dquot_memory);#endif dcache_init (mempages);
fs/dquot.c +6 -4
@@ -55,6 +55,7 @@#include <linux/errno.h> #include <linux/kernel.h> #include <linux/fs.h> +#include <linux/mm.h> #include <linux/time.h> #include <linux/types.h> #include <linux/string.h>
@@ -483,14 +484,15 @@* more memory */ -int shrink_dqcache_memory (int ratio,unsigned int gfp_mask) +int shrink_dqcache_memory (int nr,unsigned int gfp_mask) { -int entries = dqstats.allocated_dquots / ratio +1 ; +if (!nr) +return dqstats.allocated_dquots;lock_kernel (); -prune_dqcache (entries); +prune_dqcache (nr);unlock_kernel (); -return entries; +return 0 ; }/*
fs/inode.c +8 -5
@@ -419,9 +419,10 @@* This is called from kswapd when we think we need some * more memory. */ -int shrink_icache_memory (int ratio,unsigned int gfp_mask) +int shrink_icache_memory (int nr,unsigned int gfp_mask) { -int entries = inodes_stat.nr_inodes / ratio +1 ; +if (!nr) +return inodes_stat.nr_inodes;/* * Nasty deadlock avoidance.. *
@@ -430,10 +431,10 @@* in clear_inode() and friends.. */ if (!(gfp_mask & __GFP_FS)) -return 0 ; +return nr; -prune_icache (entries); -return entries; +prune_icache (nr); +return 0 ; }EXPORT_SYMBOL (shrink_icache_memory);
@@ -1098,4 +1099,6 @@ NULL);if (!inode_cachep)panic ("cannot create inode slab cache" ); + +set_shrinker (DEFAULT_SEEKS , shrink_icache_memory); }
include/linux/mm.h +24 -0
@@ -395,6 +395,30 @@/* +* Prototype to add a shrinker callback for ageable caches. +* +* These functions are passed a count and a gfpmask. They should +* return one of three results. +* +* when nr = 0 return number of entries in the cache(s) +* when nr > 0 and we can age return 0 +* when nr > 0 and we cannot age return nr +* +* if the cache(s) 'disappears' passing nr = 0 must return 0 +*/ +typedef int (*shrinker_t )(int ,unsigned int ); + +/* +* Add an aging callback. The int is the number of 'seeks' it takes +* to recreate one of the objects that these functions age. +*/ + +#define DEFAULT_SEEKS 2 + +extern void set_shrinker (int ,shrinker_t ); + + +/* * If the mapping doesn't provide a set_page_dirty a_op, then * just fall through and assume that it wants buffer_heads. * FIXME: make the method unconditional.
mm/vmscan.c +68 -29
116 lines not on this page · raw message
----------
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/
Re: [PATCH] add callback back to slab pruning
Quoted 6 lines · Ed Tomlinson
Hi Andrew,
I posted this Thursday but it seems to have gotten lost in the storm of messages on slab.
Ah, sorry, I neglected to answer. Yes, I have been testing this for a few days, works fine thanks.
Calling out to the shrinker to find out how many objects they have is sneaky.
I haven't looked super-closely at the code, but it'd be nice to make shrinker_lock go away ;)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/
Re: [PATCH] add callback back to slab pruning
Quoted 8 lines · Andrew Morton
Quoted 4 lines · Ed Tomlinson
Hi Andrew,
I posted this Thursday but it seems to have gotten lost in the storm of messages on slab.
Ah, sorry, I neglected to answer. Yes, I have been testing this for a few days, works fine thanks.
Been busy have you? <grin>
Quoted 2 lines
Calling out to the shrinker to find out how many objects they have is sneaky.
Yes. However it does let us have the ratio calculation in one place in vmscan.
Quoted 2 lines
I haven't looked super-closely at the code, but it'd be nice to make
shrinker_lockgo away ;)
Will keep this in mind.
Thanks
Ed
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/