Re: [patch 2/2] fs, proc: Introduce the /proc/<pid>/map_files/directory v6

[Cyrill Gorcunov]

On Thu, Sep 01, 2011 at 03:49:46PM -0700, Andrew Morton wrote:
> On Thu, 1 Sep 2011 14:46:34 +0400
> Cyrill Gorcunov <gorcunov@xxxxxxxxx> wrote:
> 
> > On Wed, Aug 31, 2011 at 03:10:23PM -0700, Andrew Morton wrote:
> > > 
> > > This function would benefit from a code comment.
> > > 
> > > Given that it's pretty generic (indeed there might be open-coded code
> > > which already does this elsewhere), perhaps it should be in mm/mmap.c
> > > as a kernel-wide utility function.  That will add a little overhead to
> > > CONFIG_PROC_FS=n builds, which doesn't seem terribly important.
> > > 
> > 
> > Andrew, here is an attempt to address concerns. Please review.
> > Complains are welcome as always!
> 
> Changelog still doesn't explain why /proc/pid/maps is unfixably unsuitable.
> 

Will update.

> >
> > ...
> >
> > +static int map_files_d_revalidate(struct dentry *dentry, struct nameidata *nd)
> > +{
> > +	unsigned long vm_start, vm_end;
> > +	struct task_struct *task;
> > +	const struct cred *cred;
> > +	struct mm_struct *mm;
> > +	struct inode *inode;
> > +
> > +	bool exact_vma_exists = false;
> >
> 
> Extraneous newline there.

ok, thanks

...
> > +
> > +		/*
> > +		 * We need two passes here:
> > +		 *
> > +		 *  1) Collect vmas of mapped files with mmap_sem taken
> > +		 *  2) Release mmap_sem and instantiate entries
> > +		 *
> > +		 * otherwise we get lockdep complained, since filldir()
> > +		 * routine might require mmap_sem taken in might_fault().
> > +		 */
> > +
> > +		for (vma = mm->mmap; vma; vma = vma->vm_next) {
> > +			if (vma->vm_file)
> > +				nr_files++;
> > +		}
> > +
> > +		if (nr_files) {
> > +			mem_size = nr_files * sizeof(*info);
> > +			if (mem_size <= KMALLOC_MAX_SIZE)
> > +				info = kmalloc(mem_size, GFP_KERNEL);
> > +			else
> > +				info = vmalloc(mem_size);
> 
> This still sucks :(
> 
> A KMALLOC_MAX_SIZE allocation is huuuuuuuuuuuuge!  I don't know how big

kmalloc_sizes.h said it could be quite a big :(

> it is nowadays, but over 100 kbytes.  This will frequently send page
> reclaim on a berzerk rampage freeing *thousands* of pages (or
> relocating pages) until it manages to generate 20 or 30 physically
> contiguous free pages.
> 
> Also, vmalloc sucks.  The more often we perform vmallocs (with a mix of
> differently-sized ones), the more internally fragmented the vmalloc
> arena will become.  With some workloads we'll run out of
> sufficiently-large contiguous free spaces and things will start
> failing.  This doesn't happen often.  Yet.  But the more vmalloc()
> callsites we add, the more likely and the more frequent it becomes.  So
> vmalloc is something we should only use as a last resort.
> 
> The most robust implementation here is to allocate a large number of
> small objects - one per vma.  A list would be a suitable way of
> managing them.

Actually I though about slab-cache with 64 bytes per object, but it
requires more code to push here, that is why I stopped. Still this
doesn't justify me. So yes, bad idea. Thanks!

> 
> But do we *really* need to do it in two passes?  Avoiding the temporary
> storage would involve doing more work under mmap_sem, and a put_filp()
> under mmap_sem might be problematic.
> 

In real, for the particular case where we use this /proc/pid/map_files
it could be done in one pass without mmap_sem taken (since we use it
when task is frozen and we know noone is poking vmas) but the problem
is that people might start using it not for c/r but in various different
cases when task is pretty running and I wanna give them more-less robust
result in ls -l over this directory.

Andrew, I'll think some more, probably I'll find a way to drop this two
passes requirement.

	Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/