Received-SPF: pass (google.com: domain of linux-kernel+bounces-205404-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240606210840.1650341-1-yosryahmed@google.com> <871q598qy0.fsf@yhuang6-desk2.ccr.corp.intel.com>
In-Reply-To: <871q598qy0.fsf@yhuang6-desk2.ccr.corp.intel.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Thu, 6 Jun 2024 21:55:55 -0700
Message-ID: <CAJD7tkbCHkQ1dO0ps5rTLo2pRMmjriWvMT+nBtOA3hfdD6O43w@mail.gmail.com>
Subject: Re: [PATCH] mm: swap: remove 'synchronous' argument to swap_read_folio()
To: "Huang, Ying" <ying.huang@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jun 6, 2024 at 6:48=E2=80=AFPM Huang, Ying <ying.huang@intel.com> w=
rote:
>
> Yosry Ahmed <yosryahmed@google.com> writes:
>
> > swap_read_folio() reads the folio synchronously if synchronous is passe=
d
> > as true or if SWP_SYNCHRONOUS_IO is set in swap_info_struct. The only
> > caller that passes synchronous=3Dtrue is in do_swap_page() in the
> > SWP_SYNCHRONOUS_IO case.
> >
> > Hence, the argument is redundant, it is only set to true when the swap
> > read would have been synchronous anyway. Remove it.
> >
> > Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
>
> LGTM, Thanks!
>
> Reviewed-by: "Huang, Ying" <ying.huang@intel.com>

Thanks!

>
> And, there's some history information in commit b243dcbf2f13 ("swap:
> remove remnants of polling from read_swap_cache_async").
>
> "
>     Commit [1] introduced IO polling support duding swapin to reduce swap=
 read
>     latency for block devices that can be polled.  However later commit [=
2]
>     removed polling support.  Therefore it seems safe to remove do_poll
>     parameter in read_swap_cache_async and always call swap_readpage with
>     synchronous=3Dfalse waiting for IO completion in folio_lock_or_retry.
>
>     [1] commit 23955622ff8d ("swap: add block io poll in swapin path")
>     [2] commit 9650b453a3d4 ("block: ignore RWF_HIPRI hint for sync dio")
> "
>
> IMO, it will help people to understand the code change to add some
> history information as above, or refer to the commit.

I sent a v2 with some history in the commit log.

>
> --
> Best Regards,
> Huang, Ying
>
> > ---
> >  mm/memory.c     |  2 +-
> >  mm/page_io.c    |  6 +++---
> >  mm/swap.h       |  6 ++----
> >  mm/swap_state.c | 10 +++++-----
> >  4 files changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index db91304882312..2b3ef08e8bb7d 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -4113,7 +4113,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> >
> >                               /* To provide entry to swap_read_folio() =
*/
> >                               folio->swap =3D entry;
> > -                             swap_read_folio(folio, true, NULL);
> > +                             swap_read_folio(folio, NULL);
> >                               folio->private =3D NULL;
> >                       }
> >               } else {
> > diff --git a/mm/page_io.c b/mm/page_io.c
> > index 41e8d738c6d28..f1a9cfab6e748 100644
> > --- a/mm/page_io.c
> > +++ b/mm/page_io.c
> > @@ -493,10 +493,10 @@ static void swap_read_folio_bdev_async(struct fol=
io *folio,
> >       submit_bio(bio);
> >  }
> >
> > -void swap_read_folio(struct folio *folio, bool synchronous,
> > -             struct swap_iocb **plug)
> > +void swap_read_folio(struct folio *folio, struct swap_iocb **plug)
> >  {
> >       struct swap_info_struct *sis =3D swp_swap_info(folio->swap);
> > +     bool synchronous =3D sis->flags & SWP_SYNCHRONOUS_IO;
> >       bool workingset =3D folio_test_workingset(folio);
> >       unsigned long pflags;
> >       bool in_thrashing;
> > @@ -521,7 +521,7 @@ void swap_read_folio(struct folio *folio, bool sync=
hronous,
> >               folio_unlock(folio);
> >       } else if (data_race(sis->flags & SWP_FS_OPS)) {
> >               swap_read_folio_fs(folio, plug);
> > -     } else if (synchronous || (sis->flags & SWP_SYNCHRONOUS_IO)) {
> > +     } else if (synchronous) {
> >               swap_read_folio_bdev_sync(folio, sis);
> >       } else {
> >               swap_read_folio_bdev_async(folio, sis);
> > diff --git a/mm/swap.h b/mm/swap.h
> > index 2c0e96272d498..baa1fa946b347 100644
> > --- a/mm/swap.h
> > +++ b/mm/swap.h
> > @@ -11,8 +11,7 @@ struct mempolicy;
> >  /* linux/mm/page_io.c */
> >  int sio_pool_init(void);
> >  struct swap_iocb;
> > -void swap_read_folio(struct folio *folio, bool do_poll,
> > -             struct swap_iocb **plug);
> > +void swap_read_folio(struct folio *folio, struct swap_iocb **plug);
> >  void __swap_read_unplug(struct swap_iocb *plug);
> >  static inline void swap_read_unplug(struct swap_iocb *plug)
> >  {
> > @@ -83,8 +82,7 @@ static inline unsigned int folio_swap_flags(struct fo=
lio *folio)
> >  }
> >  #else /* CONFIG_SWAP */
> >  struct swap_iocb;
> > -static inline void swap_read_folio(struct folio *folio, bool do_poll,
> > -             struct swap_iocb **plug)
> > +static inline void swap_read_folio(struct folio *folio, struct swap_io=
cb **plug)
> >  {
> >  }
> >  static inline void swap_write_unplug(struct swap_iocb *sio)
> > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > index 0803eedeabe3d..994723cef8217 100644
> > --- a/mm/swap_state.c
> > +++ b/mm/swap_state.c
> > @@ -567,7 +567,7 @@ struct folio *read_swap_cache_async(swp_entry_t ent=
ry, gfp_t gfp_mask,
> >       mpol_cond_put(mpol);
> >
> >       if (page_allocated)
> > -             swap_read_folio(folio, false, plug);
> > +             swap_read_folio(folio, plug);
> >       return folio;
> >  }
> >
> > @@ -684,7 +684,7 @@ struct folio *swap_cluster_readahead(swp_entry_t en=
try, gfp_t gfp_mask,
> >               if (!folio)
> >                       continue;
> >               if (page_allocated) {
> > -                     swap_read_folio(folio, false, &splug);
> > +                     swap_read_folio(folio, &splug);
> >                       if (offset !=3D entry_offset) {
> >                               folio_set_readahead(folio);
> >                               count_vm_event(SWAP_RA);
> > @@ -701,7 +701,7 @@ struct folio *swap_cluster_readahead(swp_entry_t en=
try, gfp_t gfp_mask,
> >                                       &page_allocated, false);
> >       if (unlikely(page_allocated)) {
> >               zswap_folio_swapin(folio);
> > -             swap_read_folio(folio, false, NULL);
> > +             swap_read_folio(folio, NULL);
> >       }
> >       return folio;
> >  }
> > @@ -834,7 +834,7 @@ static struct folio *swap_vma_readahead(swp_entry_t=
 targ_entry, gfp_t gfp_mask,
> >               if (!folio)
> >                       continue;
> >               if (page_allocated) {
> > -                     swap_read_folio(folio, false, &splug);
> > +                     swap_read_folio(folio, &splug);
> >                       if (addr !=3D vmf->address) {
> >                               folio_set_readahead(folio);
> >                               count_vm_event(SWAP_RA);
> > @@ -853,7 +853,7 @@ static struct folio *swap_vma_readahead(swp_entry_t=
 targ_entry, gfp_t gfp_mask,
> >                                       &page_allocated, false);
> >       if (unlikely(page_allocated)) {
> >               zswap_folio_swapin(folio);
> > -             swap_read_folio(folio, false, NULL);
> > +             swap_read_folio(folio, NULL);
> >       }
> >       return folio;
> >  }