Re: [PATCH -mm 17/25] Mlocked Pages are non-reclaimable

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

Hi

> > +	if (!isolate_lru_page(page)) {
> > +		putback_lru_page(page);
> > +	} else {
> > +		/*
> > +		 * Try hard not to leak this page ...
> > +		 */
> > +		lru_add_drain_all();
> > +		if (!isolate_lru_page(page))
> > +			putback_lru_page(page);
> > +	}
> > +}
> 
> When I review code I often come across stuff which I don't understand
> (at least, which I don't understand sufficiently easily).  So I'll ask
> questions, and I do think the best way in which those questions should
> be answered is by adding a code comment to fix the problem for ever.
> 
> When I look at the isolate_lru_page()-failed cases above I wonder what
> just happened.  We now have a page which is still on the LRU (how did
> it get there in the first place?). Well no.  I _think_ what happened is
> that this function is using isolate_lru_page() and putback_lru_page()
> to move a page off a now-inappropriate LRU list and to put it back onto
> the proper one.  But heck, maybe I just don't know what this function
> is doing at all?
> 
> If I _am_ right, and if the isolate_lru_page() _did_ fail (and under
> what circumstances?) then...  what?  We now have a page which is on an
> inappropriate LRU?  Why is this OK?  Do we handle it elsewhere?  How?

I think this code is OK, 
but "Try hard not to leak this page ..." is wrong comment and not true.

isolate_lru_page() failure mean this page is isolated by another one.
later, Another one put back page to proper LRU by putback_lru_page().
(putback_lru_page() alway put back right LRU.)

no leak happebnd.



> > +static int __mlock_vma_pages_range(struct vm_area_struct *vma,
> > +			unsigned long start, unsigned long end)
> > +{
> > +	struct mm_struct *mm = vma->vm_mm;
> > +	unsigned long addr = start;
> > +	struct page *pages[16]; /* 16 gives a reasonable batch */
> > +	int write = !!(vma->vm_flags & VM_WRITE);
> > +	int nr_pages = (end - start) / PAGE_SIZE;
> > +	int ret;
> > +
> > +	VM_BUG_ON(start & ~PAGE_MASK || end & ~PAGE_MASK);
> > +	VM_BUG_ON(start < vma->vm_start || end > vma->vm_end);
> > +	VM_BUG_ON(!rwsem_is_locked(&vma->vm_mm->mmap_sem));
> > +
> > +	lru_add_drain_all();	/* push cached pages to LRU */
> > +
> > +	while (nr_pages > 0) {
> > +		int i;
> > +
> > +		cond_resched();
> > +
> > +		/*
> > +		 * get_user_pages makes pages present if we are
> > +		 * setting mlock.
> > +		 */
> > +		ret = get_user_pages(current, mm, addr,
> > +				min_t(int, nr_pages, ARRAY_SIZE(pages)),
> > +				write, 0, pages, NULL);
> 
> Doesn't mlock already do a make_pages_present(), or did that get
> removed and moved to here?

I think, 

vanilla:     call make_pages_present() when mlock.
this series: call __mlock_vma_pages_range() when mlock.

thus, this code is right.