Re: [patch][v2] swap: virtual swap readahead

Johannes Weiner
2009-06-03T13:30:03Z

On Wed, Jun 03, 2009 at 01:34:57AM +0200, Andi Kleen wrote:
> On Wed, Jun 03, 2009 at 12:37:39AM +0200, Johannes Weiner wrote:
> > + *
> > + * Caller must hold down_read on the vma->vm_mm if vma is not NULL.
> > + */
> > +struct page *swapin_readahead(swp_entry_t entry, gfp_t gfp_mask,
> > + struct vm_area_struct *vma, unsigned long addr)
> > +{
> > + unsigned long start, pos, end;
> > + unsigned long pmin, pmax;
> > + int cluster, window;
> > +
> > + if (!vma || !vma->vm_mm) /* XXX: shmem case */
> > + return swapin_readahead_phys(entry, gfp_mask, vma, addr);
> > +
> > + cluster = 1 << page_cluster;
> > + window = cluster << PAGE_SHIFT;
> > +
> > + /* Physical range to read from */
> > + pmin = swp_offset(entry) & ~(cluster - 1);
>
> Is cluster really properly sign extended on 64bit? Looks a little
> dubious. long from the start would be safer

Fixed.

> > + /* Virtual range to read from */
> > + start = addr & ~(window - 1);
>
> Same.

Fixed.

> > + pgd = pgd_offset(vma->vm_mm, pos);
> > + if (!pgd_present(*pgd))
> > + continue;
> > + pud = pud_offset(pgd, pos);
> > + if (!pud_present(*pud))
> > + continue;
> > + pmd = pmd_offset(pud, pos);
> > + if (!pmd_present(*pmd))
> > + continue;
> > + pte = pte_offset_map_lock(vma->vm_mm, pmd, pos, &ptl);
>
> You could be more efficient here by using the standard mm/* nested loop
> pattern that avoids relookup of everything in each iteration. I suppose
> it would mainly make a difference with 32bit highpte where mapping a pte
> can be somewhat costly. And you would take less locks this way.

I ran into weird problems here. The above version is actually faster
in the benchmarks than writing a nested level walker or using
walk_page_range(). Still digging but it can take some time. Busy
week :(

> > + page = read_swap_cache_async(swp, gfp_mask, vma, pos);
> > + if (!page)
> > + continue;
>
> That's out of memory, break would be better here because prefetch
> while oom is usually harmful.

It can also happen due to a race with something releasing the swap
slot (i.e. swap_duplicate() fails). But the old version did a break
too and this patch shouldn't do it differently. Fixed.

> > + page_cache_release(page);
> > + }
> > + lru_add_drain(); /* Push any new pages onto the LRU now */
> > + return read_swap_cache_async(entry, gfp_mask, vma, addr);
>
> Shouldn't that page be already handled in the loop earlier? Why doing that
> again? It would be better to remember it from there.

When doing the nested page table level walker, communicating even more
state back and forth gets pretty ugly. I see what I can do.

Thanks for your input Andi,

Hannes