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