Re: [PATCH 2/2] mm:swap: unify cluster-based and vma-based swap readahead
From: Minchan Kim
Date: Wed Nov 01 2017 - 02:25:20 EST
On Wed, Nov 01, 2017 at 02:17:17PM +0800, Huang, Ying wrote:
> Minchan Kim <minchan@xxxxxxxxxx> writes:
>
> > This patch makes do_swap_page no need to be aware of two different
> > swap readahead algorithm. Just unify cluster-based and vma-based
> > readahead function call.
> >
> > Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
> > ---
> > include/linux/swap.h | 17 ++++++++++++-----
> > mm/memory.c | 11 ++++-------
> > mm/shmem.c | 5 ++++-
> > mm/swap_state.c | 21 +++++++++++++++------
> > 4 files changed, 35 insertions(+), 19 deletions(-)
> >
> > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > index 7c7c8b344bc9..9cc330360eac 100644
> > --- a/include/linux/swap.h
> > +++ b/include/linux/swap.h
> > @@ -425,9 +425,11 @@ extern struct page *read_swap_cache_async(swp_entry_t, gfp_t,
> > extern struct page *__read_swap_cache_async(swp_entry_t, gfp_t,
> > struct vm_area_struct *vma, unsigned long addr,
> > bool *new_page_allocated);
> > -extern struct page *swapin_readahead(swp_entry_t, gfp_t,
> > - struct vm_area_struct *vma, unsigned long addr);
> > -extern struct page *do_swap_page_readahead(swp_entry_t fentry, gfp_t gfp_mask,
> > +extern struct page *cluster_readahead(swp_entry_t entry, gfp_t flag,
> > + struct vm_fault *vmf);
>
> In addition to swap readahead, there are file readahead too. So better
> add swap in name, such as swap_cluster_readahead()?
Yub.
>
> > +extern struct page *swapin_readahead(swp_entry_t entry, gfp_t flag,
> > + struct vm_fault *vmf);
> > +extern struct page *vma_readahead(swp_entry_t fentry, gfp_t gfp_mask,
> > struct vm_fault *vmf);
>
> I don't find vma_readahead() is used outside of page_state.c, why
> declare it here?
By wrapping function, it's pointless to declare.
Yub, Let's drop it.
>
> >
> > /* linux/mm/swapfile.c */
> > @@ -536,8 +538,13 @@ static inline void put_swap_page(struct page *page, swp_entry_t swp)
> > {
> > }
> >
> > +static inline struct page *cluster_readahead(swp_entry_t, gfp_t gfp_mask
> > + struct vm_fault *vmf)
> > +{
> > +}
> > +
> > static inline struct page *swapin_readahead(swp_entry_t swp, gfp_t gfp_mask,
> > - struct vm_area_struct *vma, unsigned long addr)
> > + struct vm_fault *vmf)
> > {
> > return NULL;
> > }
> > @@ -547,7 +554,7 @@ static inline bool swap_use_vma_readahead(void)
> > return false;
> > }
>
> Now swap_use_vma_readahead() is used in swap_state.c only, so we can
> remove it from the header file?
Will do.
>
> > -static inline struct page *do_swap_page_readahead(swp_entry_t fentry,
> > +static inline struct page *vma_readahead(swp_entry_t fentry,
> > gfp_t gfp_mask, struct vm_fault *vmf)
> > {
> > return NULL;
> > diff --git a/mm/memory.c b/mm/memory.c
> > index e955298e4290..ce5e3d7ccc5c 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -2889,7 +2889,8 @@ int do_swap_page(struct vm_fault *vmf)
> > if (si->flags & SWP_SYNCHRONOUS_IO &&
> > __swap_count(si, entry) == 1) {
> > /* skip swapcache */
> > - page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, vmf->address);
> > + page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma,
> > + vmf->address);
> > if (page) {
> > __SetPageLocked(page);
> > __SetPageSwapBacked(page);
> > @@ -2898,12 +2899,8 @@ int do_swap_page(struct vm_fault *vmf)
> > swap_readpage(page, true);
> > }
> > } else {
> > - if (swap_use_vma_readahead())
> > - page = do_swap_page_readahead(entry,
> > - GFP_HIGHUSER_MOVABLE, vmf);
> > - else
> > - page = swapin_readahead(entry,
> > - GFP_HIGHUSER_MOVABLE, vma, vmf->address);
> > + page = swapin_readahead(entry, GFP_HIGHUSER_MOVABLE,
> > + vmf);
> > swapcache = page;
> > }
> >
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 62dfdc097e44..2522bc0958e1 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1413,9 +1413,12 @@ static struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
> > {
> > struct vm_area_struct pvma;
> > struct page *page;
> > + struct vm_fault vmf;
> >
> > shmem_pseudo_vma_init(&pvma, info, index);
> > - page = swapin_readahead(swap, gfp, &pvma, 0);
> > + vmf.vma = &pvma;
> > + vmf.address = 0;
> > + page = cluster_readahead(swap, gfp, &vmf);
> > shmem_pseudo_vma_destroy(&pvma);
> >
> > return page;
> > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > index e3c535fcd2df..5ee53d4ee047 100644
> > --- a/mm/swap_state.c
> > +++ b/mm/swap_state.c
> > @@ -538,11 +538,10 @@ static unsigned long swapin_nr_pages(unsigned long offset)
> > }
> >
> > /**
> > - * swapin_readahead - swap in pages in hope we need them soon
> > + * cluster_readahead - swap in pages in hope we need them soon
> > * @entry: swap entry of this memory
> > * @gfp_mask: memory allocation flags
> > - * @vma: user vma this address belongs to
> > - * @addr: target address for mempolicy
> > + * @vmf: fault information
> > *
> > * Returns the struct page for entry and addr, after queueing swapin.
> > *
> > @@ -556,8 +555,8 @@ static unsigned long swapin_nr_pages(unsigned long offset)
> > *
> > * 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)
> > +struct page *cluster_readahead(swp_entry_t entry, gfp_t gfp_mask,
> > + struct vm_fault *vmf)
> > {
> > struct page *page;
> > unsigned long entry_offset = swp_offset(entry);
> > @@ -566,6 +565,8 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t gfp_mask,
> > unsigned long mask;
> > struct blk_plug plug;
> > bool do_poll = true, page_allocated;
> > + struct vm_area_struct *vma = vmf->vma;
> > + unsigned long addr = vmf->address;
> >
> > mask = swapin_nr_pages(offset) - 1;
> > if (!mask)
> > @@ -603,6 +604,14 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t gfp_mask,
> > return read_swap_cache_async(entry, gfp_mask, vma, addr, do_poll);
> > }
> >
>
> No function document for swapin_readahead()? It is the main interface
> for swap readahead.
Okay, I will remain original desciription and append some more.
Thanks for the quick review, Huang.