[PATCH 0/5] mm: convert to vma_is_heap/stack()

[PATCH 0/5] mm: convert to vma_is_heap/stack()

[Kefeng Wang]

Add vma_is_stack() and vma_is_heap() helper and use them to
simplify code.

Kefeng Wang (5):
  mm: introduce vma_is_stack() and vma_is_heap()
  mm: use vma_is_stack() and vma_is_heap()
  drm/amdkfd: use vma_is_stack() and vma_is_heap()
  selinux: use vma_is_stack() and vma_is_heap()
  perf/core: use vma_is_stack() and vma_is_heap()

 drivers/gpu/drm/amd/amdkfd/kfd_svm.c |  5 +----
 fs/proc/task_mmu.c                   | 24 ++++--------------------
 fs/proc/task_nommu.c                 | 15 +--------------
 include/linux/mm.h                   | 12 ++++++++++++
 kernel/events/core.c                 | 22 +++++++---------------
 security/selinux/hooks.c             |  7 ++-----
 6 files changed, 27 insertions(+), 58 deletions(-)

-- 
2.41.0

[PATCH 1/5] mm: introduce vma_is_stack() and vma_is_heap()

[Kefeng Wang]

Introduce the two helpers for general use.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 include/linux/mm.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 1462cf15badf..0bbeb31ac750 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -926,6 +926,18 @@ static inline bool vma_is_anonymous(struct vm_area_struct *vma)
 	return !vma->vm_ops;
 }
 
+static inline bool vma_is_heap(struct vm_area_struct *vma)
+{
+       return vma->vm_start <= vma->vm_mm->brk &&
+		vma->vm_end >= vma->vm_mm->start_brk;
+}
+
+static inline bool vma_is_stack(struct vm_area_struct *vma)
+{
+       return vma->vm_start <= vma->vm_mm->start_stack &&
+	       vma->vm_end >= vma->vm_mm->start_stack;
+}
+
 static inline bool vma_is_temporary_stack(struct vm_area_struct *vma)
 {
 	int maybe_stack = vma->vm_flags & (VM_GROWSDOWN | VM_GROWSUP);
-- 
2.41.0

Re: [PATCH 1/5] mm: introduce vma_is_stack() and vma_is_heap()

[David Hildenbrand]

On 12.07.23 16:38, Kefeng Wang wrote:
> Introduce the two helpers for general use.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   include/linux/mm.h | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 1462cf15badf..0bbeb31ac750 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -926,6 +926,18 @@ static inline bool vma_is_anonymous(struct vm_area_struct *vma)
>   	return !vma->vm_ops;
>   }
>   
> +static inline bool vma_is_heap(struct vm_area_struct *vma)
> +{
> +       return vma->vm_start <= vma->vm_mm->brk &&
> +		vma->vm_end >= vma->vm_mm->start_brk;
> +}
> +
> +static inline bool vma_is_stack(struct vm_area_struct *vma)
> +{
> +       return vma->vm_start <= vma->vm_mm->start_stack &&
> +	       vma->vm_end >= vma->vm_mm->start_stack;
> +}
> +
>   static inline bool vma_is_temporary_stack(struct vm_area_struct *vma)
>   {
>   	int maybe_stack = vma->vm_flags & (VM_GROWSDOWN | VM_GROWSUP);

Looking at the comments in patch #3, should these functions be called

vma_is_initial_heap / vma_is_initial_stack ?

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/5] mm: introduce vma_is_stack() and vma_is_heap()

[Christian Göttsche]

On Wed, 12 Jul 2023 at 16:25, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
> Introduce the two helpers for general use.
>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  include/linux/mm.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 1462cf15badf..0bbeb31ac750 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -926,6 +926,18 @@ static inline bool vma_is_anonymous(struct vm_area_struct *vma)
>         return !vma->vm_ops;
>  }
>
> +static inline bool vma_is_heap(struct vm_area_struct *vma)

What about declaring the parameters const to document in code these
functions do not modify any state, and allow callers to pass pointers
to const?

> +{
> +       return vma->vm_start <= vma->vm_mm->brk &&
> +               vma->vm_end >= vma->vm_mm->start_brk;
> +}
> +
> +static inline bool vma_is_stack(struct vm_area_struct *vma)
> +{
> +       return vma->vm_start <= vma->vm_mm->start_stack &&
> +              vma->vm_end >= vma->vm_mm->start_stack;
> +}
> +
>  static inline bool vma_is_temporary_stack(struct vm_area_struct *vma)
>  {
>         int maybe_stack = vma->vm_flags & (VM_GROWSDOWN | VM_GROWSUP);
> --
> 2.41.0
>

[PATCH 2/5] mm: use vma_is_stack() and vma_is_heap()

[Kefeng Wang]

Use the helpers to simplify code.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 fs/proc/task_mmu.c   | 24 ++++--------------------
 fs/proc/task_nommu.c | 15 +--------------
 2 files changed, 5 insertions(+), 34 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index cfab855fe7e9..05e9893552ce 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -236,21 +236,6 @@ static int do_maps_open(struct inode *inode, struct file *file,
 				sizeof(struct proc_maps_private));
 }
 
-/*
- * Indicate if the VMA is a stack for the given task; for
- * /proc/PID/maps that is the stack of the main task.
- */
-static int is_stack(struct vm_area_struct *vma)
-{
-	/*
-	 * We make no effort to guess what a given thread considers to be
-	 * its "stack".  It's not even well-defined for programs written
-	 * languages like Go.
-	 */
-	return vma->vm_start <= vma->vm_mm->start_stack &&
-		vma->vm_end >= vma->vm_mm->start_stack;
-}
-
 static void show_vma_header_prefix(struct seq_file *m,
 				   unsigned long start, unsigned long end,
 				   vm_flags_t flags, unsigned long long pgoff,
@@ -327,13 +312,12 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
 			goto done;
 		}
 
-		if (vma->vm_start <= mm->brk &&
-		    vma->vm_end >= mm->start_brk) {
+		if (vma_is_heap(vma)) {
 			name = "[heap]";
 			goto done;
 		}
 
-		if (is_stack(vma)) {
+		if (vma_is_stack(vma)) {
 			name = "[stack]";
 			goto done;
 		}
@@ -1974,9 +1958,9 @@ static int show_numa_map(struct seq_file *m, void *v)
 	if (file) {
 		seq_puts(m, " file=");
 		seq_file_path(m, file, "\n\t= ");
-	} else if (vma->vm_start <= mm->brk && vma->vm_end >= mm->start_brk) {
+	} else if (vma_is_heap(vma)) {
 		seq_puts(m, " heap");
-	} else if (is_stack(vma)) {
+	} else if (vma_is_stack(vma)) {
 		seq_puts(m, " stack");
 	}
 
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 2c8b62265981..f42c84172b9e 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -121,19 +121,6 @@ unsigned long task_statm(struct mm_struct *mm,
 	return size;
 }
 
-static int is_stack(struct vm_area_struct *vma)
-{
-	struct mm_struct *mm = vma->vm_mm;
-
-	/*
-	 * We make no effort to guess what a given thread considers to be
-	 * its "stack".  It's not even well-defined for programs written
-	 * languages like Go.
-	 */
-	return vma->vm_start <= mm->start_stack &&
-		vma->vm_end >= mm->start_stack;
-}
-
 /*
  * display a single VMA to a sequenced file
  */
@@ -171,7 +158,7 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma)
 	if (file) {
 		seq_pad(m, ' ');
 		seq_file_path(m, file, "");
-	} else if (mm && is_stack(vma)) {
+	} else if (mm && vma_is_stack(vma)) {
 		seq_pad(m, ' ');
 		seq_puts(m, "[stack]");
 	}
-- 
2.41.0

Re: [PATCH 2/5] mm: use vma_is_stack() and vma_is_heap()

[David Hildenbrand]

On 12.07.23 16:38, Kefeng Wang wrote:
> Use the helpers to simplify code.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   fs/proc/task_mmu.c   | 24 ++++--------------------
>   fs/proc/task_nommu.c | 15 +--------------
>   2 files changed, 5 insertions(+), 34 deletions(-)
> 

Please squash patch #1 and this patch and call it something like

"mm: factor out VMA stack and heap checks"

And then, maybe also keep the comments in these functions, they sound 
reasonable to have.

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/5] mm: use vma_is_stack() and vma_is_heap()

[Kefeng Wang]

On 2023/7/17 18:25, David Hildenbrand wrote:
> On 12.07.23 16:38, Kefeng Wang wrote:
>> Use the helpers to simplify code.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   fs/proc/task_mmu.c   | 24 ++++--------------------
>>   fs/proc/task_nommu.c | 15 +--------------
>>   2 files changed, 5 insertions(+), 34 deletions(-)
>>
> 
> Please squash patch #1 and this patch and call it something like
> 
> "mm: factor out VMA stack and heap checks"
> 
> And then, maybe also keep the comments in these functions, they sound 
> reasonable to have.

Thanks, will re-post them.
> 

[PATCH 3/5] drm/amdkfd: use vma_is_stack() and vma_is_heap()

[Kefeng Wang]

Use the helpers to simplify code.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 479c4f66afa7..19ce68a7e1a8 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2623,10 +2623,7 @@ svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
 		return -EFAULT;
 	}
 
-	*is_heap_stack = (vma->vm_start <= vma->vm_mm->brk &&
-			  vma->vm_end >= vma->vm_mm->start_brk) ||
-			 (vma->vm_start <= vma->vm_mm->start_stack &&
-			  vma->vm_end >= vma->vm_mm->start_stack);
+	*is_heap_stack = vma_is_heap(vma) || vma_is_stack(vma);
 
 	start_limit = max(vma->vm_start >> PAGE_SHIFT,
 		      (unsigned long)ALIGN_DOWN(addr, 2UL << 8));
-- 
2.41.0

Re: [PATCH 3/5] drm/amdkfd: use vma_is_stack() and vma_is_heap()

[Christoph Hellwig]

On Wed, Jul 12, 2023 at 10:38:29PM +0800, Kefeng Wang wrote:
> Use the helpers to simplify code.

Nothing against your addition of a helper, but a GPU driver really
should have no business even looking at this information..

Re: [PATCH 3/5] drm/amdkfd: use vma_is_stack() and vma_is_heap()

[Felix Kuehling]

Allocations in the heap and stack tend to be small, with several 
allocations sharing the same page. Sharing the same page for different 
allocations with different access patterns leads to thrashing when we 
migrate data back and forth on GPU and CPU access. To avoid this we 
disable HMM migrations for head and stack VMAs.

Regards,
   Felix


Am 2023-07-12 um 10:42 schrieb Christoph Hellwig:
> On Wed, Jul 12, 2023 at 10:38:29PM +0800, Kefeng Wang wrote:
>> Use the helpers to simplify code.
> Nothing against your addition of a helper, but a GPU driver really
> should have no business even looking at this information..
>
>

Re: [PATCH 3/5] drm/amdkfd: use vma_is_stack() and vma_is_heap()

[Vlastimil Babka]

On 7/12/23 18:24, Felix Kuehling wrote:
> Allocations in the heap and stack tend to be small, with several 
> allocations sharing the same page. Sharing the same page for different 
> allocations with different access patterns leads to thrashing when we 
> migrate data back and forth on GPU and CPU access. To avoid this we 
> disable HMM migrations for head and stack VMAs.

Wonder how well does it really work in practice? AFAIK "heaps" (malloc())
today uses various arenas obtained by mmap() and not a single brk() managed
space anymore? And programs might be multithreaded, thus have multiple
stacks, while vma_is_stack() will recognize only the initial one...

Vlastimil

> Regards,
>    Felix
> 
> 
> Am 2023-07-12 um 10:42 schrieb Christoph Hellwig:
>> On Wed, Jul 12, 2023 at 10:38:29PM +0800, Kefeng Wang wrote:
>>> Use the helpers to simplify code.
>> Nothing against your addition of a helper, but a GPU driver really
>> should have no business even looking at this information..
>>
>>
> 

Re: [PATCH 3/5] drm/amdkfd: use vma_is_stack() and vma_is_heap()

[Felix Kuehling]

Am 2023-07-14 um 10:26 schrieb Vlastimil Babka:
> On 7/12/23 18:24, Felix Kuehling wrote:
>> Allocations in the heap and stack tend to be small, with several
>> allocations sharing the same page. Sharing the same page for different
>> allocations with different access patterns leads to thrashing when we
>> migrate data back and forth on GPU and CPU access. To avoid this we
>> disable HMM migrations for head and stack VMAs.
> Wonder how well does it really work in practice? AFAIK "heaps" (malloc())
> today uses various arenas obtained by mmap() and not a single brk() managed
> space anymore? And programs might be multithreaded, thus have multiple
> stacks, while vma_is_stack() will recognize only the initial one...

Thanks for these pointers. I have not heard of such problems with mmap 
arenas and multiple thread stacks in practice. But I'll keep it in mind 
in case we observe unexpected thrashing in the future. FWIW, we once had 
the opposite problem of a custom malloc implementation that used sbrk 
for very large allocations. This disabled migrations of large buffers 
unexpectedly.

I agree that eventually we'll want a more dynamic way of detecting and 
suppressing thrashing that's based on observed memory access patterns. 
Getting this right is probably trickier than it sounds, so I'd prefer to 
have some more experience with real workloads to use as benchmarks. 
Compared to other things we're working on, this is fairly low on our 
priority list at the moment. Using the VMA flags is a simple and 
effective method for now, at least until we see it failing in real 
workloads.

Regards,
   Felix


>
> Vlastimil
>
>> Regards,
>>     Felix
>>
>>
>> Am 2023-07-12 um 10:42 schrieb Christoph Hellwig:
>>> On Wed, Jul 12, 2023 at 10:38:29PM +0800, Kefeng Wang wrote:
>>>> Use the helpers to simplify code.
>>> Nothing against your addition of a helper, but a GPU driver really
>>> should have no business even looking at this information..
>>>
>>>

[PATCH 4/5] selinux: use vma_is_stack() and vma_is_heap()

[Kefeng Wang]

Use the helpers to simplify code.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 security/selinux/hooks.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4e46cf3d67b6..289ef2d6a427 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3775,13 +3775,10 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
 	if (default_noexec &&
 	    (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) {
 		int rc = 0;
-		if (vma->vm_start >= vma->vm_mm->start_brk &&
-		    vma->vm_end <= vma->vm_mm->brk) {
+		if (vma_is_heap(vma)) {
 			rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
 					  PROCESS__EXECHEAP, NULL);
-		} else if (!vma->vm_file &&
-			   ((vma->vm_start <= vma->vm_mm->start_stack &&
-			     vma->vm_end >= vma->vm_mm->start_stack) ||
+		} else if (!vma->vm_file && vma_is_stack(vma) ||
 			    vma_is_stack_for_current(vma))) {
 			rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
 					  PROCESS__EXECSTACK, NULL);
-- 
2.41.0

Re: [PATCH 4/5] selinux: use vma_is_stack() and vma_is_heap()

[Paul Moore]

On Wed, Jul 12, 2023 at 10:25 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
> Use the helpers to simplify code.
>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  security/selinux/hooks.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 4e46cf3d67b6..289ef2d6a427 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3775,13 +3775,10 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
>         if (default_noexec &&
>             (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) {
>                 int rc = 0;
> -               if (vma->vm_start >= vma->vm_mm->start_brk &&
> -                   vma->vm_end <= vma->vm_mm->brk) {
> +               if (vma_is_heap(vma)) {
>                         rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
>                                           PROCESS__EXECHEAP, NULL);
> -               } else if (!vma->vm_file &&
> -                          ((vma->vm_start <= vma->vm_mm->start_stack &&
> -                            vma->vm_end >= vma->vm_mm->start_stack) ||
> +               } else if (!vma->vm_file && vma_is_stack(vma) ||
>                             vma_is_stack_for_current(vma))) {

With the parens fix that Andrew already provided.

Acked-by: Paul Moore <paul@paul-moore.com>

>                         rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
>                                           PROCESS__EXECSTACK, NULL);
> --
> 2.41.0

-- 
paul-moore.com

[PATCH 5/5] perf/core: use vma_is_stack() and vma_is_heap()

[Kefeng Wang]

Use the helpers to simplify code, also kill unneeded goto cpy_name.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 kernel/events/core.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 78ae7b6f90fd..cb271f449b81 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8685,22 +8685,14 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		}
 
 		name = (char *)arch_vma_name(vma);
-		if (name)
-			goto cpy_name;
-
-		if (vma->vm_start <= vma->vm_mm->start_brk &&
-				vma->vm_end >= vma->vm_mm->brk) {
-			name = "[heap]";
-			goto cpy_name;
+		if (!name) {
+			if (vma_is_heap(vma))
+				name = "[heap]";
+			else if (vma_is_stack(vma))
+				name = "[stack]";
+			else
+				name = "//anon";
 		}
-		if (vma->vm_start <= vma->vm_mm->start_stack &&
-				vma->vm_end >= vma->vm_mm->start_stack) {
-			name = "[stack]";
-			goto cpy_name;
-		}
-
-		name = "//anon";
-		goto cpy_name;
 	}
 
 cpy_name:
-- 
2.41.0