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

[Oleg Nesterov <oleg@redhat.com>]

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.