diff mbox series

[RFC,03/14] fork: Clean-up naming of vm_strack/vm_struct variables in vmap stacks code

Message ID 20240311164638.2015063-4-pasha.tatashin@soleen.com (mailing list archive)
State New
Headers show
Series Dynamic Kernel Stacks | expand

Commit Message

Pasha Tatashin March 11, 2024, 4:46 p.m. UTC 
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(-)

Comments

Christophe JAILLET March 17, 2024, 2:42 p.m. UTC | #1 
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;
Pasha Tatashin March 19, 2024, 4:32 p.m. UTC | #2 
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 mbox series

Patch

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;