| Message ID | 20230522070905.16773-5-ying.huang@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | swap: cleanup get/put_swap_device() usage | expand |
On 22.05.23 09:09, Huang Ying wrote: > __swap_duplicate() is called by > > - swap_shmem_alloc(): the page lock of the swap cache is held. page lock of the swap cache? Did you really mean to say that or am I confused? "Page lock of the page that is in the swap cache?" > > - copy_nonpresent_pte() -> swap_duplicate() and try_to_unmap_one() -> > swap_duplicate(): the page table lock is held. > > - __read_swap_cache_async() -> swapcache_prepare(): enclosed with > get/put_swap_device() in __read_swap_cache_async() already. > > So, it's safe to remove get/put_swap_device() in __swap_duplicate().
David Hildenbrand <david@redhat.com> writes: > On 22.05.23 09:09, Huang Ying wrote: >> __swap_duplicate() is called by >> - swap_shmem_alloc(): the page lock of the swap cache is held. > > page lock of the swap cache? Did you really mean to say that or am I > confused? > > "Page lock of the page that is in the swap cache?" Sorry for my poor English. Or make it shorter? "the folio in the swap cache is locked" Best Regards, Huang, Ying >> - copy_nonpresent_pte() -> swap_duplicate() and try_to_unmap_one() >> -> >> swap_duplicate(): the page table lock is held. >> - __read_swap_cache_async() -> swapcache_prepare(): enclosed with >> get/put_swap_device() in __read_swap_cache_async() already. >> So, it's safe to remove get/put_swap_device() in __swap_duplicate().
On Mon, May 22, 2023 at 12:09 AM Huang Ying <ying.huang@intel.com> wrote: > > __swap_duplicate() is called by > > - swap_shmem_alloc(): the page lock of the swap cache is held. > > - copy_nonpresent_pte() -> swap_duplicate() and try_to_unmap_one() -> > swap_duplicate(): the page table lock is held. > > - __read_swap_cache_async() -> swapcache_prepare(): enclosed with > get/put_swap_device() in __read_swap_cache_async() already. > > So, it's safe to remove get/put_swap_device() in __swap_duplicate(). > > Signed-off-by: "Huang, Ying" <ying.huang@intel.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Hugh Dickins <hughd@google.com> > Cc: Johannes Weiner <hannes@cmpxchg.org> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Minchan Kim <minchan@kernel.org> > Cc: Tim Chen <tim.c.chen@linux.intel.com> > Cc: Yang Shi <shy828301@gmail.com> > Cc: Yu Zhao <yuzhao@google.com> > --- > mm/swapfile.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index e9cce775fb25..4dbaea64635d 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -3264,9 +3264,7 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage) I would add a comment above this function stating that the caller needs to provide protection against swapoff, and refer to the comment above get_swap_device(). Otherwise, LGTM with David's comment. Reviewed-by: Yosry Ahmed <yosryahmed@google.com> > unsigned char has_cache; > int err; > > - p = get_swap_device(entry); > - if (!p) > - return -EINVAL; > + p = swp_swap_info(entry); > > offset = swp_offset(entry); > ci = lock_cluster_or_swap_info(p, offset); > @@ -3313,7 +3311,6 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage) > > unlock_out: > unlock_cluster_or_swap_info(p, ci); > - put_swap_device(p); > return err; > } > > -- > 2.39.2 > >
On 23.05.23 02:56, Huang, Ying wrote: > David Hildenbrand <david@redhat.com> writes: > >> On 22.05.23 09:09, Huang Ying wrote: >>> __swap_duplicate() is called by >>> - swap_shmem_alloc(): the page lock of the swap cache is held. >> >> page lock of the swap cache? Did you really mean to say that or am I >> confused? >> >> "Page lock of the page that is in the swap cache?" > > Sorry for my poor English. Or make it shorter? > > "the folio in the swap cache is locked" Much clearer, thanks.
diff --git a/mm/swapfile.c b/mm/swapfile.c index e9cce775fb25..4dbaea64635d 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -3264,9 +3264,7 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage) unsigned char has_cache; int err; - p = get_swap_device(entry); - if (!p) - return -EINVAL; + p = swp_swap_info(entry); offset = swp_offset(entry); ci = lock_cluster_or_swap_info(p, offset); @@ -3313,7 +3311,6 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage) unlock_out: unlock_cluster_or_swap_info(p, ci); - put_swap_device(p); return err; }
__swap_duplicate() is called by - swap_shmem_alloc(): the page lock of the swap cache is held. - copy_nonpresent_pte() -> swap_duplicate() and try_to_unmap_one() -> swap_duplicate(): the page table lock is held. - __read_swap_cache_async() -> swapcache_prepare(): enclosed with get/put_swap_device() in __read_swap_cache_async() already. So, it's safe to remove get/put_swap_device() in __swap_duplicate(). Signed-off-by: "Huang, Ying" <ying.huang@intel.com> Cc: David Hildenbrand <david@redhat.com> Cc: Hugh Dickins <hughd@google.com> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: Matthew Wilcox <willy@infradead.org> Cc: Michal Hocko <mhocko@suse.com> Cc: Minchan Kim <minchan@kernel.org> Cc: Tim Chen <tim.c.chen@linux.intel.com> Cc: Yang Shi <shy828301@gmail.com> Cc: Yu Zhao <yuzhao@google.com> --- mm/swapfile.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)