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

KOSAKI Motohiro
2008-06-07T05:38:57Z

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.