From: Kairui Song <[email protected]>
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 <[email protected]>
---
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);
+ }
+
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;
--
2.35.2
Huang, Ying <[email protected]> 于2022年12月11日周日 19:40写道：
>
> Kairui Song <[email protected]> writes:
>
> > From: Kairui Song <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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;
Kairui Song <[email protected]> writes:
> From: Kairui Song <[email protected]>
>
> 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 <[email protected]>
> ---
> 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;