| Message ID | 20240311164638.2015063-4-pasha.tatashin@soleen.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Dynamic Kernel Stacks | expand |
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;
On Sun, Mar 17, 2024 at 10:42 AM Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > 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? 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 = 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; >
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); } 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;
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: 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(-)