Messages in this thread | | | Date | Sun, 17 Mar 2024 15:42:28 +0100 | Subject | Re: [RFC 03/14] fork: Clean-up naming of vm_strack/vm_struct variables in vmap stacks code | From | Christophe JAILLET <> |
| |
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@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_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;
| |