diff mbox series

[-V2,4/5] swap: remove get/put_swap_device() in __swap_duplicate()

Message ID 20230522070905.16773-5-ying.huang@intel.com
State New
Headers
Series swap: cleanup get/put_swap_device() usage |

Commit Message

Huang, Ying May 22, 2023, 7:09 a.m. UTC 
  __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(-)

Comments

David Hildenbrand May 22, 2023, 12:06 p.m. UTC | #1 
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().
Huang, Ying May 23, 2023, 12:56 a.m. UTC | #2 
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().
Yosry Ahmed May 23, 2023, 1:39 a.m. UTC | #3 
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
>
>
David Hildenbrand May 23, 2023, 7:59 a.m. UTC | #4 
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 mbox series

Patch

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;
 }