2024-02-21 21:09:35

by Barry Song

Subject: [PATCH v2] mm/swapfile:__swap_duplicate: drop redundant WRITE_ONCE on swap_map for err cases

From: Barry Song <[email protected]>

The code is quite hard to read, we are still writing swap_map after
errors happen. Though the written value is as before,

has_cache = count & SWAP_HAS_CACHE;
count &= ~SWAP_HAS_CACHE;
[snipped]
WRITE_ONCE(p->swap_map[offset], count | has_cache);

It would be better to entirely drop the WRITE_ONCE for both
performance and readability.

Signed-off-by: Barry Song <[email protected]>
---
-v2: drop goto according to Andrew, Thanks!

mm/swapfile.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 556ff7347d5f..7cb6d9a2d51d 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3320,7 +3320,8 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
} else
err = -ENOENT; /* unused swap entry */

- WRITE_ONCE(p->swap_map[offset], count | has_cache);
+ if (!err)
+ WRITE_ONCE(p->swap_map[offset], count | has_cache);

unlock_out:
unlock_cluster_or_swap_info(p, ci);
--
2.34.1

2024-03-11 14:38:02

by David Hildenbrand

Subject: Re: [PATCH v2] mm/swapfile:__swap_duplicate: drop redundant WRITE_ONCE on swap_map for err cases

On 21.02.24 22:08, Barry Song wrote:
> From: Barry Song <[email protected]>
>
> The code is quite hard to read, we are still writing swap_map after
> errors happen. Though the written value is as before,
>
> has_cache = count & SWAP_HAS_CACHE;
> count &= ~SWAP_HAS_CACHE;
> [snipped]
> WRITE_ONCE(p->swap_map[offset], count | has_cache);
>
> It would be better to entirely drop the WRITE_ONCE for both
> performance and readability.
>
> Signed-off-by: Barry Song <[email protected]>
> ---
> -v2: drop goto according to Andrew, Thanks!
>
> mm/swapfile.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 556ff7347d5f..7cb6d9a2d51d 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3320,7 +3320,8 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
> } else
> err = -ENOENT; /* unused swap entry */
>
> - WRITE_ONCE(p->swap_map[offset], count | has_cache);
> + if (!err)
> + WRITE_ONCE(p->swap_map[offset], count | has_cache);
>
> unlock_out:
> unlock_cluster_or_swap_info(p, ci);

Maybe too late, but LGTM

Reviewed-by: David Hildenbrand <[email protected]>

--
Cheers,

David / dhildenb

2024-03-11 23:14:44

by Barry Song

Subject: Re: [PATCH v2] mm/swapfile:__swap_duplicate: drop redundant WRITE_ONCE on swap_map for err cases

On Mon, Mar 11, 2024 at 10:37 PM David Hildenbrand <[email protected]> wrote:
>
> On 21.02.24 22:08, Barry Song wrote:
> > From: Barry Song <[email protected]>
> >
> > The code is quite hard to read, we are still writing swap_map after
> > errors happen. Though the written value is as before,
> >
> > has_cache = count & SWAP_HAS_CACHE;
> > count &= ~SWAP_HAS_CACHE;
> > [snipped]
> > WRITE_ONCE(p->swap_map[offset], count | has_cache);
> >
> > It would be better to entirely drop the WRITE_ONCE for both
> > performance and readability.
> >
> > Signed-off-by: Barry Song <[email protected]>
> > ---
> > -v2: drop goto according to Andrew, Thanks!
> >
> > mm/swapfile.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 556ff7347d5f..7cb6d9a2d51d 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -3320,7 +3320,8 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
> > } else
> > err = -ENOENT; /* unused swap entry */
> >
> > - WRITE_ONCE(p->swap_map[offset], count | has_cache);
> > + if (!err)
> > + WRITE_ONCE(p->swap_map[offset], count | has_cache);
> >
> > unlock_out:
> > unlock_cluster_or_swap_info(p, ci);
>
> Maybe too late, but LGTM
>
> Reviewed-by: David Hildenbrand <[email protected]>

David, thanks! Though it is probably too late as this one has been in
mm-stable for a long time.

>
> --
> Cheers,
>
> David / dhildenb

Thanks
Barry