diff mbox series

[4/5] swap: remove the swap lock in swap_cache_get_folio

Message ID 20221208180209.50845-5-ryncsn@gmail.com
State New
Headers
Series Clean up and fixes for swap |

Commit Message

Kairui Song Dec. 8, 2022, 6:02 p.m. UTC 
  From: Kairui Song <kasong@tencent.com>

There is only one caller not keep holding a reference or lock the
swap device while calling this function. Just move the lock out
of this function, it only used to prevent swapoff, and this helper
function is very short so there is no performance regression
issue. Help saves a few cycles.

Signed-off-by: Kairui Song <kasong@tencent.com>
---
 mm/shmem.c      | 8 +++++++-
 mm/swap_state.c | 8 ++------
 2 files changed, 9 insertions(+), 7 deletions(-)

Comments

Huang, Ying Dec. 11, 2022, 11:39 a.m. UTC | #1 
Kairui Song <ryncsn@gmail.com> writes:

> From: Kairui Song <kasong@tencent.com>
>
> There is only one caller not keep holding a reference or lock the
> swap device while calling this function. Just move the lock out
> of this function, it only used to prevent swapoff, and this helper
> function is very short so there is no performance regression
> issue. Help saves a few cycles.

> Subject: Re: [PATCH 4/5] swap: remove the swap lock in swap_cache_get_folio

I don't think you remove `swap lock` in swap_cache_get_folio().  Just
avoid to inc/dec the reference count.

And I think it's better to add '()' after swap_cache_get_folio to make
it clear it's a function.

> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
>  mm/shmem.c      | 8 +++++++-
>  mm/swap_state.c | 8 ++------
>  2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index c1d8b8a1aa3b..0183b6678270 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1725,6 +1725,7 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
>  	struct address_space *mapping = inode->i_mapping;
>  	struct shmem_inode_info *info = SHMEM_I(inode);
>  	struct mm_struct *charge_mm = vma ? vma->vm_mm : NULL;
> +	struct swap_info_struct *si;
>  	struct folio *folio = NULL;
>  	swp_entry_t swap;
>  	int error;
> @@ -1737,7 +1738,12 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
>  		return -EIO;
>  
>  	/* Look it up and read it in.. */
> -	folio = swap_cache_get_folio(swap, NULL, 0);
> +	si = get_swap_device(swap);
> +	if (si) {
> +		folio = swap_cache_get_folio(swap, NULL, 0);
> +		put_swap_device(si);

I'd rather to call put_swap_device() at the end of function.  That is,
whenever we get a swap entry without proper lock/reference to prevent
swapoff, we should call get_swap_device() to check its validity and
prevent the swap device from swapoff.

Best Regards,
Huang, Ying

> +	}
> +
>  	if (!folio) {
>  		/* Or update major stats only when swapin succeeds?? */
>  		if (fault_type) {
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 19089417abd1..eba388f67741 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -324,19 +324,15 @@ static inline bool swap_use_vma_readahead(void)
>   * unlocked and with its refcount incremented - we rely on the kernel
>   * lock getting page table operations atomic even if we drop the folio
>   * lock before returning.
> + *
> + * Caller must lock the swap device or hold a reference to keep it valid.
>   */
>  struct folio *swap_cache_get_folio(swp_entry_t entry,
>  		struct vm_area_struct *vma, unsigned long addr)
>  {
>  	struct folio *folio;
> -	struct swap_info_struct *si;
>  
> -	si = get_swap_device(entry);
> -	if (!si)
> -		return NULL;
>  	folio = filemap_get_folio(swap_address_space(entry), swp_offset(entry));
> -	put_swap_device(si);
> -
>  	if (folio) {
>  		bool vma_ra = swap_use_vma_readahead();
>  		bool readahead;
Kairui Song Dec. 11, 2022, 11:47 a.m. UTC | #2 
Huang, Ying <ying.huang@intel.com> 于2022年12月11日周日 19:40写道：
>
> Kairui Song <ryncsn@gmail.com> writes:
>
> > From: Kairui Song <kasong@tencent.com>
> >
> > There is only one caller not keep holding a reference or lock the
> > swap device while calling this function. Just move the lock out
> > of this function, it only used to prevent swapoff, and this helper
> > function is very short so there is no performance regression
> > issue. Help saves a few cycles.
>
> > Subject: Re: [PATCH 4/5] swap: remove the swap lock in swap_cache_get_folio
>
> I don't think you remove `swap lock` in swap_cache_get_folio().  Just
> avoid to inc/dec the reference count.

Yes, that's more accurate, it's kind of like 'locked the swap device
from being swapped off', so I used some inaccurate word. I'll correct
this in V2.

>
> And I think it's better to add '()' after swap_cache_get_folio to make
> it clear it's a function.

Good suggestion.

>
> > Signed-off-by: Kairui Song <kasong@tencent.com>
> > ---
> >  mm/shmem.c      | 8 +++++++-
> >  mm/swap_state.c | 8 ++------
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index c1d8b8a1aa3b..0183b6678270 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1725,6 +1725,7 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
> >       struct address_space *mapping = inode->i_mapping;
> >       struct shmem_inode_info *info = SHMEM_I(inode);
> >       struct mm_struct *charge_mm = vma ? vma->vm_mm : NULL;
> > +     struct swap_info_struct *si;
> >       struct folio *folio = NULL;
> >       swp_entry_t swap;
> >       int error;
> > @@ -1737,7 +1738,12 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
> >               return -EIO;
> >
> >       /* Look it up and read it in.. */
> > -     folio = swap_cache_get_folio(swap, NULL, 0);
> > +     si = get_swap_device(swap);
> > +     if (si) {
> > +             folio = swap_cache_get_folio(swap, NULL, 0);
> > +             put_swap_device(si);
>
> I'd rather to call put_swap_device() at the end of function.  That is,
> whenever we get a swap entry without proper lock/reference to prevent
> swapoff, we should call get_swap_device() to check its validity and
> prevent the swap device from swapoff.

Yes, that's the right way to do it, my code is buggy here, sorry for
being so careless, I'll fix it.

>
> Best Regards,
> Huang, Ying
>
> > +     }
> > +
> >       if (!folio) {
> >               /* Or update major stats only when swapin succeeds?? */
> >               if (fault_type) {
> > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > index 19089417abd1..eba388f67741 100644
> > --- a/mm/swap_state.c
> > +++ b/mm/swap_state.c
> > @@ -324,19 +324,15 @@ static inline bool swap_use_vma_readahead(void)
> >   * unlocked and with its refcount incremented - we rely on the kernel
> >   * lock getting page table operations atomic even if we drop the folio
> >   * lock before returning.
> > + *
> > + * Caller must lock the swap device or hold a reference to keep it valid.
> >   */
> >  struct folio *swap_cache_get_folio(swp_entry_t entry,
> >               struct vm_area_struct *vma, unsigned long addr)
> >  {
> >       struct folio *folio;
> > -     struct swap_info_struct *si;
> >
> > -     si = get_swap_device(entry);
> > -     if (!si)
> > -             return NULL;
> >       folio = filemap_get_folio(swap_address_space(entry), swp_offset(entry));
> > -     put_swap_device(si);
> > -
> >       if (folio) {
> >               bool vma_ra = swap_use_vma_readahead();
> >               bool readahead;
diff mbox series

Patch

diff --git a/mm/shmem.c b/mm/shmem.c
index c1d8b8a1aa3b..0183b6678270 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1725,6 +1725,7 @@  static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
 	struct address_space *mapping = inode->i_mapping;
 	struct shmem_inode_info *info = SHMEM_I(inode);
 	struct mm_struct *charge_mm = vma ? vma->vm_mm : NULL;
+	struct swap_info_struct *si;
 	struct folio *folio = NULL;
 	swp_entry_t swap;
 	int error;
@@ -1737,7 +1738,12 @@  static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
 		return -EIO;
 
 	/* Look it up and read it in.. */
-	folio = swap_cache_get_folio(swap, NULL, 0);
+	si = get_swap_device(swap);
+	if (si) {
+		folio = swap_cache_get_folio(swap, NULL, 0);
+		put_swap_device(si);
+	}
+
 	if (!folio) {
 		/* Or update major stats only when swapin succeeds?? */
 		if (fault_type) {
diff --git a/mm/swap_state.c b/mm/swap_state.c
index 19089417abd1..eba388f67741 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -324,19 +324,15 @@  static inline bool swap_use_vma_readahead(void)
  * unlocked and with its refcount incremented - we rely on the kernel
  * lock getting page table operations atomic even if we drop the folio
  * lock before returning.
+ *
+ * Caller must lock the swap device or hold a reference to keep it valid.
  */
 struct folio *swap_cache_get_folio(swp_entry_t entry,
 		struct vm_area_struct *vma, unsigned long addr)
 {
 	struct folio *folio;
-	struct swap_info_struct *si;
 
-	si = get_swap_device(entry);
-	if (!si)
-		return NULL;
 	folio = filemap_get_folio(swap_address_space(entry), swp_offset(entry));
-	put_swap_device(si);
-
 	if (folio) {
 		bool vma_ra = swap_use_vma_readahead();
 		bool readahead;