diff mbox series

[-V2,1/5] swap: Remove get/put_swap_device() in __swap_count()

Message ID 20230522070905.16773-2-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_count() is called in do_swap_page() only, which encloses the
call site with get/put_swap_device() already.

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 | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

Comments

David Hildenbrand May 22, 2023, 11:54 a.m. UTC | #1 
On 22.05.23 09:09, Huang Ying wrote:
> __swap_count() is called in do_swap_page() only, which encloses the
> call site with get/put_swap_device() already.
> 
> 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 | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 274bbf797480..8419cba9c192 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1432,16 +1432,10 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
>   
>   int __swap_count(swp_entry_t entry)
>   {
> -	struct swap_info_struct *si;
> +	struct swap_info_struct *si = swp_swap_info(entry);
>   	pgoff_t offset = swp_offset(entry);
> -	int count = 0;
>   
> -	si = get_swap_device(entry);
> -	if (si) {
> -		count = swap_count(si->swap_map[offset]);
> -		put_swap_device(si);
> -	}
> -	return count;
> +	return swap_count(si->swap_map[offset]);
>   }
>   
>   /*

That locking was added in eb085574a752 ("mm, swap: fix race between 
swapoff and some swap operations"). Before 2799e77529c ("swap: fix 
do_swap_page() race with swapoff") added the get_swap_device() to 
do_swap_page().

Reviewed-by: David Hildenbrand <david@redhat.com>
Yosry Ahmed May 23, 2023, 1:22 a.m. UTC | #2 
On Mon, May 22, 2023 at 12:09 AM Huang Ying <ying.huang@intel.com> wrote:
>
> __swap_count() is called in do_swap_page() only, which encloses the
> call site with get/put_swap_device() already.
>
> 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 | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 274bbf797480..8419cba9c192 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1432,16 +1432,10 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
>

nit: I would add a comment here that the caller needs get/put_swap_device().

Reviewed-by: Yosry Ahmed <yosryahmed@google.com>

>  int __swap_count(swp_entry_t entry)
>  {
> -       struct swap_info_struct *si;
> +       struct swap_info_struct *si = swp_swap_info(entry);
>         pgoff_t offset = swp_offset(entry);
> -       int count = 0;
>
> -       si = get_swap_device(entry);
> -       if (si) {
> -               count = swap_count(si->swap_map[offset]);
> -               put_swap_device(si);
> -       }
> -       return count;
> +       return swap_count(si->swap_map[offset]);
>  }
>
>  /*
> --
> 2.39.2
>
>
Huang, Ying May 23, 2023, 1:47 a.m. UTC | #3 
Yosry Ahmed <yosryahmed@google.com> writes:

> On Mon, May 22, 2023 at 12:09 AM Huang Ying <ying.huang@intel.com> wrote:
>>
>> __swap_count() is called in do_swap_page() only, which encloses the
>> call site with get/put_swap_device() already.
>>
>> 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 | 10 ++--------
>>  1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 274bbf797480..8419cba9c192 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -1432,16 +1432,10 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
>>
>
> nit: I would add a comment here that the caller needs get/put_swap_device().

It's default behavior to call get/put_swap_device() in the caller for
all almost all swap functions.  I would rather comment the swap
functions needn't to do that, as the comments for
read_swap_cache_async() in [2/5].

> Reviewed-by: Yosry Ahmed <yosryahmed@google.com>

Thanks!

>>  int __swap_count(swp_entry_t entry)
>>  {
>> -       struct swap_info_struct *si;
>> +       struct swap_info_struct *si = swp_swap_info(entry);
>>         pgoff_t offset = swp_offset(entry);
>> -       int count = 0;
>>
>> -       si = get_swap_device(entry);
>> -       if (si) {
>> -               count = swap_count(si->swap_map[offset]);
>> -               put_swap_device(si);
>> -       }
>> -       return count;
>> +       return swap_count(si->swap_map[offset]);
>>  }
>>
>>  /*

Best Regards,
Huang, Ying
Yosry Ahmed May 23, 2023, 1:51 a.m. UTC | #4 
On Mon, May 22, 2023 at 6:48 PM Huang, Ying <ying.huang@intel.com> wrote:
>
> Yosry Ahmed <yosryahmed@google.com> writes:
>
> > On Mon, May 22, 2023 at 12:09 AM Huang Ying <ying.huang@intel.com> wrote:
> >>
> >> __swap_count() is called in do_swap_page() only, which encloses the
> >> call site with get/put_swap_device() already.
> >>
> >> 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 | 10 ++--------
> >>  1 file changed, 2 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/mm/swapfile.c b/mm/swapfile.c
> >> index 274bbf797480..8419cba9c192 100644
> >> --- a/mm/swapfile.c
> >> +++ b/mm/swapfile.c
> >> @@ -1432,16 +1432,10 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
> >>
> >
> > nit: I would add a comment here that the caller needs get/put_swap_device().
>
> It's default behavior to call get/put_swap_device() in the caller for
> all almost all swap functions.  I would rather comment the swap
> functions needn't to do that, as the comments for
> read_swap_cache_async() in [2/5].

Fair enough. The comment that you added above get_swap_device() states
that all swap-related functions should have some sort of protection
against swapoff, so I guess that's sufficient.

My concern is that sometimes people don't know where to look, and
having a comment above the function might be helpful. I do agree
though that having a comment above ~all swap-related functions
pointing to the comment above get_swap_device() is too much.

>
> > Reviewed-by: Yosry Ahmed <yosryahmed@google.com>
>
> Thanks!
>
> >>  int __swap_count(swp_entry_t entry)
> >>  {
> >> -       struct swap_info_struct *si;
> >> +       struct swap_info_struct *si = swp_swap_info(entry);
> >>         pgoff_t offset = swp_offset(entry);
> >> -       int count = 0;
> >>
> >> -       si = get_swap_device(entry);
> >> -       if (si) {
> >> -               count = swap_count(si->swap_map[offset]);
> >> -               put_swap_device(si);
> >> -       }
> >> -       return count;
> >> +       return swap_count(si->swap_map[offset]);
> >>  }
> >>
> >>  /*
>
> Best Regards,
> Huang, Ying
diff mbox series

Patch

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 274bbf797480..8419cba9c192 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1432,16 +1432,10 @@  void swapcache_free_entries(swp_entry_t *entries, int n)
 
 int __swap_count(swp_entry_t entry)
 {
-	struct swap_info_struct *si;
+	struct swap_info_struct *si = swp_swap_info(entry);
 	pgoff_t offset = swp_offset(entry);
-	int count = 0;
 
-	si = get_swap_device(entry);
-	if (si) {
-		count = swap_count(si->swap_map[offset]);
-		put_swap_device(si);
-	}
-	return count;
+	return swap_count(si->swap_map[offset]);
 }
 
 /*