| Message ID | 20221208180209.50845-5-ryncsn@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Clean up and fixes for swap | expand |
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;
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 --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;