Received-SPF: pass (google.com: domain of linux-kernel+bounces-107839-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240311164638.2015063-1-pasha.tatashin@soleen.com>
 <20240311164638.2015063-4-pasha.tatashin@soleen.com> <1af02bc8-1b20-4508-909d-342f3b06bf78@wanadoo.fr>
In-Reply-To: <1af02bc8-1b20-4508-909d-342f3b06bf78@wanadoo.fr>
From: Pasha Tatashin <pasha.tatashin@soleen.com>
Date: Tue, 19 Mar 2024 12:32:18 -0400
Message-ID: <CA+CK2bB-7fUnMXcRLma60J-wFZ9Y-W45O0okU2fseuh-VxG8fA@mail.gmail.com>
Subject: Re: [RFC 03/14] fork: Clean-up naming of vm_strack/vm_struct
 variables in vmap stacks code
To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, 
	akpm@linux-foundation.org, x86@kernel.org, bp@alien8.de, brauner@kernel.org, 
	bristot@redhat.com, bsegall@google.com, dave.hansen@linux.intel.com, 
	dianders@chromium.org, dietmar.eggemann@arm.com, eric.devolder@oracle.com, 
	hca@linux.ibm.com, hch@infradead.org, hpa@zytor.com, 
	jacob.jun.pan@linux.intel.com, jgg@ziepe.ca, jpoimboe@kernel.org, 
	jroedel@suse.de, juri.lelli@redhat.com, kent.overstreet@linux.dev, 
	kinseyho@google.com, kirill.shutemov@linux.intel.com, lstoakes@gmail.com, 
	luto@kernel.org, mgorman@suse.de, mic@digikod.net, 
	michael.christie@oracle.com, mingo@redhat.com, mjguzik@gmail.com, 
	mst@redhat.com, npiggin@gmail.com, peterz@infradead.org, pmladek@suse.com, 
	rick.p.edgecombe@intel.com, rostedt@goodmis.org, surenb@google.com, 
	tglx@linutronix.de, urezki@gmail.com, vincent.guittot@linaro.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, Mar 17, 2024 at 10:42=E2=80=AFAM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 11/03/2024 =C3=A0 17:46, Pasha Tatashin a =C3=A9crit :
> > There are two data types: "struct vm_struct" and "struct vm_stack" that
> > have the same local variable names: vm_stack, or vm, or s, which makes
> > code confusing to read.
> >
> > Change the code so the naming is consisent:
>
> Nit: consistent
>
> >
> > struct vm_struct is always called vm_area
> > struct vm_stack is always called vm_stack
> >
> > Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> > ---
> >   kernel/fork.c | 38 ++++++++++++++++++--------------------
> >   1 file changed, 18 insertions(+), 20 deletions(-)
> >
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 32600bf2422a..60e812825a7a 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -192,12 +192,12 @@ struct vm_stack {
> >       struct vm_struct *stack_vm_area;
> >   };
> >
> > -static bool try_release_thread_stack_to_cache(struct vm_struct *vm)
> > +static bool try_release_thread_stack_to_cache(struct vm_struct *vm_are=
a)
> >   {
> >       unsigned int i;
> >
> >       for (i =3D 0; i < NR_CACHED_STACKS; i++) {
> > -             if (this_cpu_cmpxchg(cached_stacks[i], NULL, vm) !=3D NUL=
L)
> > +             if (this_cpu_cmpxchg(cached_stacks[i], NULL, vm_area) !=
=3D NULL)
> >                       continue;
> >               return true;
> >       }
> > @@ -207,11 +207,12 @@ static bool try_release_thread_stack_to_cache(str=
uct vm_struct *vm)
> >   static void thread_stack_free_rcu(struct rcu_head *rh)
> >   {
> >       struct vm_stack *vm_stack =3D container_of(rh, struct vm_stack, r=
cu);
> > +     struct vm_struct *vm_area =3D vm_stack->stack_vm_area;
> >
> >       if (try_release_thread_stack_to_cache(vm_stack->stack_vm_area))
> >               return;
> >
> > -     vfree(vm_stack);
> > +     vfree(vm_area->addr);
>
> This does not look like only a renaming of a variable. Is it?
>
> If no, should there be a Fixes tag and should it be detailed in the
> commit description?

This change is only for readability purposes. vm_stack is stored in
vm_area, so vfree(vm_stack) equals to vfree(vm_area->addr), but is
hard to read. I will add it to changelog.

>
> CJ
>
> >   }
> >
> >   static void thread_stack_delayed_free(struct task_struct *tsk)
> > @@ -228,12 +229,12 @@ static int free_vm_stack_cache(unsigned int cpu)
> >       int i;
> >
> >       for (i =3D 0; i < NR_CACHED_STACKS; i++) {
> > -             struct vm_struct *vm_stack =3D cached_vm_stacks[i];
> > +             struct vm_struct *vm_area =3D cached_vm_stacks[i];
> >
> > -             if (!vm_stack)
> > +             if (!vm_area)
> >                       continue;
> >
> > -             vfree(vm_stack->addr);
> > +             vfree(vm_area->addr);
> >               cached_vm_stacks[i] =3D NULL;
> >       }
> >
> > @@ -263,32 +264,29 @@ static int memcg_charge_kernel_stack(struct vm_st=
ruct *vm)
> >
> >   static int alloc_thread_stack_node(struct task_struct *tsk, int node)
> >   {
> > -     struct vm_struct *vm;
> > +     struct vm_struct *vm_area;
> >       void *stack;
> >       int i;
> >
> >       for (i =3D 0; i < NR_CACHED_STACKS; i++) {
> > -             struct vm_struct *s;
> > -
> > -             s =3D this_cpu_xchg(cached_stacks[i], NULL);
> > -
> > -             if (!s)
> > +             vm_area =3D this_cpu_xchg(cached_stacks[i], NULL);
> > +             if (!vm_area)
> >                       continue;
> >
> >               /* Reset stack metadata. */
> > -             kasan_unpoison_range(s->addr, THREAD_SIZE);
> > +             kasan_unpoison_range(vm_area->addr, THREAD_SIZE);
> >
> > -             stack =3D kasan_reset_tag(s->addr);
> > +             stack =3D kasan_reset_tag(vm_area->addr);
> >
> >               /* Clear stale pointers from reused stack. */
> >               memset(stack, 0, THREAD_SIZE);
> >
> > -             if (memcg_charge_kernel_stack(s)) {
> > -                     vfree(s->addr);
> > +             if (memcg_charge_kernel_stack(vm_area)) {
> > +                     vfree(vm_area->addr);
> >                       return -ENOMEM;
> >               }
> >
> > -             tsk->stack_vm_area =3D s;
> > +             tsk->stack_vm_area =3D vm_area;
> >               tsk->stack =3D stack;
> >               return 0;
> >       }
> > @@ -306,8 +304,8 @@ static int alloc_thread_stack_node(struct task_stru=
ct *tsk, int node)
> >       if (!stack)
> >               return -ENOMEM;
> >
> > -     vm =3D find_vm_area(stack);
> > -     if (memcg_charge_kernel_stack(vm)) {
> > +     vm_area =3D find_vm_area(stack);
> > +     if (memcg_charge_kernel_stack(vm_area)) {
> >               vfree(stack);
> >               return -ENOMEM;
> >       }
> > @@ -316,7 +314,7 @@ static int alloc_thread_stack_node(struct task_stru=
ct *tsk, int node)
> >        * free_thread_stack() can be called in interrupt context,
> >        * so cache the vm_struct.
> >        */
> > -     tsk->stack_vm_area =3D vm;
> > +     tsk->stack_vm_area =3D vm_area;
> >       stack =3D kasan_reset_tag(stack);
> >       tsk->stack =3D stack;
> >       return 0;
>