+ fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch added to mm-nonmm-unstable branch

+ fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch added to mm-nonmm-unstable branch

[Andrew Morton]

The patch titled
     Subject: fork: use this_cpu_try_cmpxchg() in try_release_thread_stack_to_cache()
has been added to the -mm mm-nonmm-unstable branch.  Its filename is
     fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch

This patch will shortly appear at
     https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch

This patch will later appear in the mm-nonmm-unstable branch at
    git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/process/submit-checklist.rst when testing your code ***

The -mm tree is included into linux-next via the mm-everything
branch at git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
and is updated there every 2-3 working days

------------------------------------------------------
From: Uros Bizjak <ubizjak@gmail.com>
Subject: fork: use this_cpu_try_cmpxchg() in try_release_thread_stack_to_cache()
Date: Thu, 23 May 2024 09:35:14 +0200

Use this_cpu_try_cmpxchg() instead of this_cpu_cmpxchg (*ptr, old, new) ==
old in try_release_thread_stack_to_cache.  x86 CMPXCHG instruction returns
success in ZF flag, so this change saves a compare after cmpxchg (and
related move instruction in front of cmpxchg).

No functional change intended.

Link: https://lkml.kernel.org/r/20240523073530.8128-1-ubizjak@gmail.com
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/fork.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- a/kernel/fork.c~fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache
+++ a/kernel/fork.c
@@ -205,7 +205,9 @@ static bool try_release_thread_stack_to_
 	unsigned int i;
 
 	for (i = 0; i < NR_CACHED_STACKS; i++) {
-		if (this_cpu_cmpxchg(cached_stacks[i], NULL, vm) != NULL)
+		struct vm_struct *tmp = NULL;
+
+		if (!this_cpu_try_cmpxchg(cached_stacks[i], &tmp, vm))
 			continue;
 		return true;
 	}
_

Patches currently in -mm which might be from ubizjak@gmail.com are

fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch

Re: + fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch added to mm-nonmm-unstable branch

[Oleg Nesterov]

> From: Uros Bizjak <ubizjak@gmail.com>
> Subject: fork: use this_cpu_try_cmpxchg() in try_release_thread_stack_to_cache()
> Date: Thu, 23 May 2024 09:35:14 +0200
>
> Use this_cpu_try_cmpxchg() instead of this_cpu_cmpxchg (*ptr, old, new) ==
> old in try_release_thread_stack_to_cache.  x86 CMPXCHG instruction returns
> success in ZF flag, so this change saves a compare after cmpxchg (and
> related move instruction in front of cmpxchg).
>
> No functional change intended.

Sorry, cosmetic nit...

> @@ -205,7 +205,9 @@ static bool try_release_thread_stack_to_
>  	unsigned int i;
>  
>  	for (i = 0; i < NR_CACHED_STACKS; i++) {
> -		if (this_cpu_cmpxchg(cached_stacks[i], NULL, vm) != NULL)
> +		struct vm_struct *tmp = NULL;
> +
> +		if (!this_cpu_try_cmpxchg(cached_stacks[i], &tmp, vm))
>  			continue;
>  		return true;
>  	}

Why not

	static bool try_release_thread_stack_to_cache(struct vm_struct *vm)
	{
		unsigned int i;

		for (i = 0; i < NR_CACHED_STACKS; i++) {
			struct vm_struct *tmp = NULL;

			if (this_cpu_try_cmpxchg(cached_stacks[i], &tmp, vm))
				return true;
		}
		return false;
	}

?

To me this "continue" just adds the unnecessary confusion with or without
this patch, but I won't insist.

Oleg.

Re: + fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch added to mm-nonmm-unstable branch

[Uros Bizjak]

On Thu, May 23, 2024 at 10:45 PM Oleg Nesterov <oleg@redhat.com> wrote:
>
> > From: Uros Bizjak <ubizjak@gmail.com>
> > Subject: fork: use this_cpu_try_cmpxchg() in try_release_thread_stack_to_cache()
> > Date: Thu, 23 May 2024 09:35:14 +0200
> >
> > Use this_cpu_try_cmpxchg() instead of this_cpu_cmpxchg (*ptr, old, new) ==
> > old in try_release_thread_stack_to_cache.  x86 CMPXCHG instruction returns
> > success in ZF flag, so this change saves a compare after cmpxchg (and
> > related move instruction in front of cmpxchg).
> >
> > No functional change intended.
>
> Sorry, cosmetic nit...
>
> > @@ -205,7 +205,9 @@ static bool try_release_thread_stack_to_
> >       unsigned int i;
> >
> >       for (i = 0; i < NR_CACHED_STACKS; i++) {
> > -             if (this_cpu_cmpxchg(cached_stacks[i], NULL, vm) != NULL)
> > +             struct vm_struct *tmp = NULL;
> > +
> > +             if (!this_cpu_try_cmpxchg(cached_stacks[i], &tmp, vm))
> >                       continue;
> >               return true;
> >       }
>
> Why not
>
>         static bool try_release_thread_stack_to_cache(struct vm_struct *vm)
>         {
>                 unsigned int i;
>
>                 for (i = 0; i < NR_CACHED_STACKS; i++) {
>                         struct vm_struct *tmp = NULL;
>
>                         if (this_cpu_try_cmpxchg(cached_stacks[i], &tmp, vm))
>                                 return true;
>                 }
>                 return false;
>         }
>
> ?
>
> To me this "continue" just adds the unnecessary confusion with or without
> this patch, but I won't insist.

To ease review, I was trying to make the patch as simple as possible
(almost mechanical), because the logic around cmpxchg() and
try_cmpxchg() is somehow hard to follow (e.g. cmpxchg() returns the
previous value at location when it succeeds, the "tmp" variable is
only updated when try_cmpxchg() fails). Any additional change would
just unnecessarily complicate the patch.

Uros.

Re: + fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch added to mm-nonmm-unstable branch

[Oleg Nesterov]

On 05/23, Uros Bizjak wrote:
>
> On Thu, May 23, 2024 at 10:45 PM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Why not
> >
> >         static bool try_release_thread_stack_to_cache(struct vm_struct *vm)
> >         {
> >                 unsigned int i;
> >
> >                 for (i = 0; i < NR_CACHED_STACKS; i++) {
> >                         struct vm_struct *tmp = NULL;
> >
> >                         if (this_cpu_try_cmpxchg(cached_stacks[i], &tmp, vm))
> >                                 return true;
> >                 }
> >                 return false;
> >         }
> >
> > ?
> >
> > To me this "continue" just adds the unnecessary confusion with or without
> > this patch, but I won't insist.
>
> To ease review, I was trying to make the patch as simple as possible
> (almost mechanical),

I understand,

> because the logic around cmpxchg() and
> try_cmpxchg() is somehow hard to follow

Yes,

> Any additional change would
> just unnecessarily complicate the patch.

I disagree.

If you change the code for any reason, it is always good to try to make
it more readable. And to me

	for (i = 0; i < NR_CACHED_STACKS; i++) {
		struct vm_struct *tmp = NULL;

		if (this_cpu_try_cmpxchg(cached_stacks[i], &tmp, vm))
			return true;
	}

is certainly more readable than

	for (i = 0; i < NR_CACHED_STACKS; i++) {
		struct vm_struct *tmp = NULL;

		if (!this_cpu_try_cmpxchg(cached_stacks[i], &tmp, vm))
			continue;
		return true;
	}

but of course this is subjective, and as I said I won't insist.

Oleg.

[PATCH] fork: Use this_cpu_try_cmpxchg() in try_release_thread_stack_to_cache()-fix

[Uros Bizjak]

Simplify the for loop a bit.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nestrov <oleg@redhat.com>
---
 kernel/fork.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index c9e994d66930..f1c16b3dd8ac 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -207,9 +207,8 @@ static bool try_release_thread_stack_to_cache(struct vm_struct *vm)
 	for (i = 0; i < NR_CACHED_STACKS; i++) {
 		struct vm_struct *tmp = NULL;
 
-		if (!this_cpu_try_cmpxchg(cached_stacks[i], &tmp, vm))
-			continue;
-		return true;
+		if (this_cpu_try_cmpxchg(cached_stacks[i], &tmp, vm))
+			return true;
 	}
 	return false;
 }
-- 
2.42.0