* + fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch added to mm-nonmm-unstable branch
@ 2024-05-23 19:07 Andrew Morton
2024-05-23 20:43 ` Oleg Nesterov
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2024-05-23 19:07 UTC (permalink / raw)
To: mm-commits, oleg, keescook, ubizjak, akpm
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: + fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch added to mm-nonmm-unstable branch
2024-05-23 19:07 + fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch added to mm-nonmm-unstable branch Andrew Morton
@ 2024-05-23 20:43 ` Oleg Nesterov
2024-05-23 21:00 ` Uros Bizjak
0 siblings, 1 reply; 5+ messages in thread
From: Oleg Nesterov @ 2024-05-23 20:43 UTC (permalink / raw)
To: Andrew Morton; +Cc: mm-commits, keescook, ubizjak
> 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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: + fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch added to mm-nonmm-unstable branch
2024-05-23 20:43 ` Oleg Nesterov
@ 2024-05-23 21:00 ` Uros Bizjak
2024-05-23 21:17 ` Oleg Nesterov
0 siblings, 1 reply; 5+ messages in thread
From: Uros Bizjak @ 2024-05-23 21:00 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, mm-commits, keescook
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: + fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch added to mm-nonmm-unstable branch
2024-05-23 21:00 ` Uros Bizjak
@ 2024-05-23 21:17 ` Oleg Nesterov
2024-05-23 21:43 ` [PATCH] fork: Use this_cpu_try_cmpxchg() in try_release_thread_stack_to_cache()-fix Uros Bizjak
0 siblings, 1 reply; 5+ messages in thread
From: Oleg Nesterov @ 2024-05-23 21:17 UTC (permalink / raw)
To: Uros Bizjak; +Cc: Andrew Morton, mm-commits, keescook
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] fork: Use this_cpu_try_cmpxchg() in try_release_thread_stack_to_cache()-fix
2024-05-23 21:17 ` Oleg Nesterov
@ 2024-05-23 21:43 ` Uros Bizjak
0 siblings, 0 replies; 5+ messages in thread
From: Uros Bizjak @ 2024-05-23 21:43 UTC (permalink / raw)
To: mm-commits; +Cc: Uros Bizjak, Andrew Morton, Oleg Nestrov
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-05-23 21:44 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-23 19:07 + fork-use-this_cpu_try_cmpxchg-in-try_release_thread_stack_to_cache.patch added to mm-nonmm-unstable branch Andrew Morton
2024-05-23 20:43 ` Oleg Nesterov
2024-05-23 21:00 ` Uros Bizjak
2024-05-23 21:17 ` Oleg Nesterov
2024-05-23 21:43 ` [PATCH] fork: Use this_cpu_try_cmpxchg() in try_release_thread_stack_to_cache()-fix Uros Bizjak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.