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(-)
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;
--
2.44.0.278.ge034bb2e1d-googLe 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;
>
© 2016 - 2026 Red Hat, Inc.