On Thu, May 23, 2024 at 10:45 PM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> > From: Uros Bizjak <ubizjak@xxxxxxxxx>
> > 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.
