[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1af02bc8-1b20-4508-909d-342f3b06bf78@wanadoo.fr>
Date: Sun, 17 Mar 2024 15:42:28 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Pasha Tatashin <pasha.tatashin@...een.com>, linux-kernel@...r.kernel.org,
linux-mm@...ck.org, akpm@...ux-foundation.org, x86@...nel.org, bp@...en8.de,
brauner@...nel.org, bristot@...hat.com, bsegall@...gle.com,
dave.hansen@...ux.intel.com, dianders@...omium.org,
dietmar.eggemann@....com, eric.devolder@...cle.com, hca@...ux.ibm.com,
hch@...radead.org, hpa@...or.com, jacob.jun.pan@...ux.intel.com,
jgg@...pe.ca, jpoimboe@...nel.org, jroedel@...e.de, juri.lelli@...hat.com,
kent.overstreet@...ux.dev, kinseyho@...gle.com,
kirill.shutemov@...ux.intel.com, lstoakes@...il.com, luto@...nel.org,
mgorman@...e.de, mic@...ikod.net, michael.christie@...cle.com,
mingo@...hat.com, mjguzik@...il.com, mst@...hat.com, npiggin@...il.com,
peterz@...radead.org, pmladek@...e.com, rick.p.edgecombe@...el.com,
rostedt@...dmis.org, surenb@...gle.com, tglx@...utronix.de,
urezki@...il.com, vincent.guittot@...aro.org
Subject: Re: [RFC 03/14] fork: Clean-up naming of vm_strack/vm_struct
variables in vmap stacks code
Le 11/03/2024 à 17:46, Pasha Tatashin a écrit :
> 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@...een.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_area)
> {
> unsigned int i;
>
> for (i = 0; i < NR_CACHED_STACKS; i++) {
> - if (this_cpu_cmpxchg(cached_stacks[i], NULL, vm) != NULL)
> + if (this_cpu_cmpxchg(cached_stacks[i], NULL, vm_area) != NULL)
> continue;
> return true;
> }
> @@ -207,11 +207,12 @@ static bool try_release_thread_stack_to_cache(struct vm_struct *vm)
> static void thread_stack_free_rcu(struct rcu_head *rh)
> {
> struct vm_stack *vm_stack = container_of(rh, struct vm_stack, rcu);
> + struct vm_struct *vm_area = 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?
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 = 0; i < NR_CACHED_STACKS; i++) {
> - struct vm_struct *vm_stack = cached_vm_stacks[i];
> + struct vm_struct *vm_area = cached_vm_stacks[i];
>
> - if (!vm_stack)
> + if (!vm_area)
> continue;
>
> - vfree(vm_stack->addr);
> + vfree(vm_area->addr);
> cached_vm_stacks[i] = NULL;
> }
>
> @@ -263,32 +264,29 @@ static int memcg_charge_kernel_stack(struct vm_struct *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 = 0; i < NR_CACHED_STACKS; i++) {
> - struct vm_struct *s;
> -
> - s = this_cpu_xchg(cached_stacks[i], NULL);
> -
> - if (!s)
> + vm_area = 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 = kasan_reset_tag(s->addr);
> + stack = 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 = s;
> + tsk->stack_vm_area = vm_area;
> tsk->stack = stack;
> return 0;
> }
> @@ -306,8 +304,8 @@ static int alloc_thread_stack_node(struct task_struct *tsk, int node)
> if (!stack)
> return -ENOMEM;
>
> - vm = find_vm_area(stack);
> - if (memcg_charge_kernel_stack(vm)) {
> + vm_area = 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_struct *tsk, int node)
> * free_thread_stack() can be called in interrupt context,
> * so cache the vm_struct.
> */
> - tsk->stack_vm_area = vm;
> + tsk->stack_vm_area = vm_area;
> stack = kasan_reset_tag(stack);
> tsk->stack = stack;
> return 0;
Powered by blists - more mailing lists