[PATCH v2 0/2] mm: mmap: add mmap trace point

[PATCH v2 0/2] mm: mmap: add mmap trace point

[Jaewon Kim]

Create mmap trace file and add trace point of vm_unmapped_area.

To include mmap trace, remove inline of vm_unmapped_area and move code
to mmap.c. There is no logical change.

Jaewon Kim (2):
  mmap: remove inline of vm_unmapped_area
  mm: mmap: add trace point of vm_unmapped_area

 include/linux/mm.h          | 21 +-----------------
 include/trace/events/mmap.h | 52 +++++++++++++++++++++++++++++++++++++++++++++
 mm/mmap.c                   | 24 +++++++++++++++++++++
 3 files changed, 77 insertions(+), 20 deletions(-)
 create mode 100644 include/trace/events/mmap.h

-- 
2.13.7

[PATCH v2 1/2] mmap: remove inline of vm_unmapped_area

[Jaewon Kim]

In prepration for next patch remove inline of vm_unmapped_area and move
code to mmap.c. There is no logical change.

Also remove unmapped_area[_topdown] out of mm.h, there is no code
calling to them.

Signed-off-by: Jaewon Kim <jaewon31.kim@samsung.com>
---
 include/linux/mm.h | 21 +--------------------
 mm/mmap.c          | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 52269e56c514..1cb01f4a83c9 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2364,26 +2364,7 @@ struct vm_unmapped_area_info {
 	unsigned long align_offset;
 };
 
-extern unsigned long unmapped_area(struct vm_unmapped_area_info *info);
-extern unsigned long unmapped_area_topdown(struct vm_unmapped_area_info *info);
-
-/*
- * Search for an unmapped address range.
- *
- * We are looking for a range that:
- * - does not intersect with any VMA;
- * - is contained within the [low_limit, high_limit) interval;
- * - is at least the desired size.
- * - satisfies (begin_addr & align_mask) == (align_offset & align_mask)
- */
-static inline unsigned long
-vm_unmapped_area(struct vm_unmapped_area_info *info)
-{
-	if (info->flags & VM_UNMAPPED_AREA_TOPDOWN)
-		return unmapped_area_topdown(info);
-	else
-		return unmapped_area(info);
-}
+extern unsigned long vm_unmapped_area(struct vm_unmapped_area_info *info);
 
 /* truncate.c */
 extern void truncate_inode_pages(struct address_space *, loff_t);
diff --git a/mm/mmap.c b/mm/mmap.c
index d681a20eb4ea..eeaddb76286c 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2050,6 +2050,22 @@ unsigned long unmapped_area_topdown(struct vm_unmapped_area_info *info)
 	return gap_end;
 }
 
+/*
+ * Search for an unmapped address range.
+ *
+ * We are looking for a range that:
+ * - does not intersect with any VMA;
+ * - is contained within the [low_limit, high_limit) interval;
+ * - is at least the desired size.
+ * - satisfies (begin_addr & align_mask) == (align_offset & align_mask)
+ */
+unsigned long vm_unmapped_area(struct vm_unmapped_area_info *info)
+{
+	if (info->flags & VM_UNMAPPED_AREA_TOPDOWN)
+		return unmapped_area_topdown(info);
+	else
+		return unmapped_area(info);
+}
 
 #ifndef arch_get_mmap_end
 #define arch_get_mmap_end(addr)	(TASK_SIZE)
-- 
2.13.7

Re: [PATCH v2 1/2] mmap: remove inline of vm_unmapped_area

[Andrew Morton]

On Fri, 13 Mar 2020 10:14:19 +0900 Jaewon Kim <jaewon31.kim@samsung.com> wrote:

> In prepration for next patch remove inline of vm_unmapped_area and move
> code to mmap.c. There is no logical change.
> 
> Also remove unmapped_area[_topdown] out of mm.h, there is no code
> calling to them.
> 

Patches seem reasonable.

> -extern unsigned long unmapped_area(struct vm_unmapped_area_info *info);
> -extern unsigned long unmapped_area_topdown(struct vm_unmapped_area_info *info);

I believe these can now be made static to mmap.c

Re: [PATCH v2 1/2] mmap: remove inline of vm_unmapped_area

[Jaewon Kim]

On 2020년 03월 13일 12:02, Andrew Morton wrote:
> On Fri, 13 Mar 2020 10:14:19 +0900 Jaewon Kim <jaewon31.kim@samsung.com> wrote:
>
>> In prepration for next patch remove inline of vm_unmapped_area and move
>> code to mmap.c. There is no logical change.
>>
>> Also remove unmapped_area[_topdown] out of mm.h, there is no code
>> calling to them.
>>
> Patches seem reasonable.
>
>> -extern unsigned long unmapped_area(struct vm_unmapped_area_info *info);
>> -extern unsigned long unmapped_area_topdown(struct vm_unmapped_area_info *info);
> I believe these can now be made static to mmap.c
Correct.
Let me wait for the 2/2 patch to be reviewed, and resubmit this 1/2 patch with having static if need.
Thank you for your comment.
>
>
>
>

Re: [PATCH v2 1/2] mmap: remove inline of vm_unmapped_area

[Vlastimil Babka]

On 3/13/20 2:14 AM, Jaewon Kim wrote:
> In prepration for next patch remove inline of vm_unmapped_area and move
> code to mmap.c. There is no logical change.
> 
> Also remove unmapped_area[_topdown] out of mm.h, there is no code
> calling to them.
> 
> Signed-off-by: Jaewon Kim <jaewon31.kim@samsung.com>

Assuming the 'static' is added as Andrew pointed out,

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>


> ---
>  include/linux/mm.h | 21 +--------------------
>  mm/mmap.c          | 16 ++++++++++++++++
>  2 files changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 52269e56c514..1cb01f4a83c9 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2364,26 +2364,7 @@ struct vm_unmapped_area_info {
>  	unsigned long align_offset;
>  };
>  
> -extern unsigned long unmapped_area(struct vm_unmapped_area_info *info);
> -extern unsigned long unmapped_area_topdown(struct vm_unmapped_area_info *info);
> -
> -/*
> - * Search for an unmapped address range.
> - *
> - * We are looking for a range that:
> - * - does not intersect with any VMA;
> - * - is contained within the [low_limit, high_limit) interval;
> - * - is at least the desired size.
> - * - satisfies (begin_addr & align_mask) == (align_offset & align_mask)
> - */
> -static inline unsigned long
> -vm_unmapped_area(struct vm_unmapped_area_info *info)
> -{
> -	if (info->flags & VM_UNMAPPED_AREA_TOPDOWN)
> -		return unmapped_area_topdown(info);
> -	else
> -		return unmapped_area(info);
> -}
> +extern unsigned long vm_unmapped_area(struct vm_unmapped_area_info *info);
>  
>  /* truncate.c */
>  extern void truncate_inode_pages(struct address_space *, loff_t);
> diff --git a/mm/mmap.c b/mm/mmap.c
> index d681a20eb4ea..eeaddb76286c 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2050,6 +2050,22 @@ unsigned long unmapped_area_topdown(struct vm_unmapped_area_info *info)
>  	return gap_end;
>  }
>  
> +/*
> + * Search for an unmapped address range.
> + *
> + * We are looking for a range that:
> + * - does not intersect with any VMA;
> + * - is contained within the [low_limit, high_limit) interval;
> + * - is at least the desired size.
> + * - satisfies (begin_addr & align_mask) == (align_offset & align_mask)
> + */
> +unsigned long vm_unmapped_area(struct vm_unmapped_area_info *info)
> +{
> +	if (info->flags & VM_UNMAPPED_AREA_TOPDOWN)
> +		return unmapped_area_topdown(info);
> +	else
> +		return unmapped_area(info);
> +}
>  
>  #ifndef arch_get_mmap_end
>  #define arch_get_mmap_end(addr)	(TASK_SIZE)
> 

[PATCH v2 2/2] mm: mmap: add trace point of vm_unmapped_area

[Jaewon Kim]

Even on 64 bit kernel, the mmap failure can happen for a 32 bit task.
Virtual memory space shortage of a task on mmap is reported to userspace
as -ENOMEM. It can be confused as physical memory shortage of overall
system.

The vm_unmapped_area can be called to by some drivers or other kernel
core system like filesystem. In my platform, GPU driver calls to
vm_unmapped_area and the driver returns -ENOMEM even in GPU side
shortage. It can be hard to distinguish which code layer returns the
-ENOMEM.

Create mmap trace file and add trace point of vm_unmapped_area.

i.e.)
265.214275: vm_unmapped_area: addr=fffffffffffffff4 err=-12 tvm=0xffb08 flags=0x1 len=0x100000 lo=0x8000 hi=0xf7182000 mask=0x0 ofs=0x22
265.214276: vm_unmapped_area: addr=fffffffffffffff4 err=-12 tvm=0xffb08 flags=0x0 len=0x100000 lo=0x40000000 hi=0xfffff000 mask=0x0 ofs=0x22
510.827472: vm_unmapped_area: addr=7377060000 err=0 tvm=0x262f flags=0x1 len=0x7f000 lo=0x8000 hi=0x73795f7000 mask=0x0 ofs=0x22
510.827668: vm_unmapped_area: addr=73794d1000 err=0 tvm=0x25cc flags=0x1 len=0x1000 lo=0x8000 hi=0x73795f7000 mask=0x0 ofs=0x22

Signed-off-by: Jaewon Kim <jaewon31.kim@samsung.com>
---
 include/trace/events/mmap.h | 52 +++++++++++++++++++++++++++++++++++++++++++++
 mm/mmap.c                   | 12 +++++++++--
 2 files changed, 62 insertions(+), 2 deletions(-)
 create mode 100644 include/trace/events/mmap.h

diff --git a/include/trace/events/mmap.h b/include/trace/events/mmap.h
new file mode 100644
index 000000000000..a51dd8af6f8c
--- /dev/null
+++ b/include/trace/events/mmap.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM mmap
+
+#if !defined(_TRACE_MMAP_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_MMAP_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(vm_unmapped_area,
+
+	TP_PROTO(unsigned long addr, struct vm_unmapped_area_info *info),
+
+	TP_ARGS(addr, info),
+
+	TP_STRUCT__entry(
+		__field(unsigned long,	addr)
+		__field(unsigned long,	error)
+		__field(unsigned long,	total_vm)
+		__field(unsigned long,	flags)
+		__field(unsigned long,	length)
+		__field(unsigned long,	low_limit)
+		__field(unsigned long,	high_limit)
+		__field(unsigned long,	align_mask)
+		__field(unsigned long,	align_offset)
+	),
+
+	TP_fast_assign(
+		__entry->addr = addr;
+		if (IS_ERR_VALUE(addr))
+			__entry->error = addr;
+		else
+			__entry->error = 0;
+		__entry->total_vm = current->mm->total_vm;
+		__entry->flags = info->flags;
+		__entry->length = info->length;
+		__entry->low_limit = info->low_limit;
+		__entry->high_limit = info->high_limit;
+		__entry->align_mask = info->align_mask;
+		__entry->align_offset = info->align_offset;
+	),
+
+	TP_printk("addr=%lx err=%ld tvm=0x%lx flags=0x%lx len=0x%lx lo=0x%lx hi=0x%lx mask=0x%lx ofs=0x%lx\n",
+		__entry->addr, __entry->error, __entry->total_vm,
+		__entry->flags, __entry->length, __entry->low_limit,
+		__entry->high_limit, __entry->align_mask,
+		__entry->align_offset)
+);
+#endif
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
diff --git a/mm/mmap.c b/mm/mmap.c
index eeaddb76286c..68ccbd7d5256 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -53,6 +53,9 @@
 #include <asm/tlb.h>
 #include <asm/mmu_context.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/mmap.h>
+
 #include "internal.h"
 
 #ifndef arch_mmap_check
@@ -2061,10 +2064,15 @@ unsigned long unmapped_area_topdown(struct vm_unmapped_area_info *info)
  */
 unsigned long vm_unmapped_area(struct vm_unmapped_area_info *info)
 {
+	unsigned long addr;
+
 	if (info->flags & VM_UNMAPPED_AREA_TOPDOWN)
-		return unmapped_area_topdown(info);
+		addr = unmapped_area_topdown(info);
 	else
-		return unmapped_area(info);
+		addr = unmapped_area(info);
+
+	trace_vm_unmapped_area(addr, info);
+	return addr;
 }
 
 #ifndef arch_get_mmap_end
-- 
2.13.7

Re: [PATCH v2 2/2] mm: mmap: add trace point of vm_unmapped_area

[Vlastimil Babka]

On 3/13/20 2:14 AM, Jaewon Kim wrote:
> Even on 64 bit kernel, the mmap failure can happen for a 32 bit task.
> Virtual memory space shortage of a task on mmap is reported to userspace
> as -ENOMEM. It can be confused as physical memory shortage of overall
> system.
> 
> The vm_unmapped_area can be called to by some drivers or other kernel
> core system like filesystem. In my platform, GPU driver calls to
> vm_unmapped_area and the driver returns -ENOMEM even in GPU side
> shortage. It can be hard to distinguish which code layer returns the
> -ENOMEM.
> 
> Create mmap trace file and add trace point of vm_unmapped_area.
> 
> i.e.)
> 265.214275: vm_unmapped_area: addr=fffffffffffffff4 err=-12 tvm=0xffb08 flags=0x1 len=0x100000 lo=0x8000 hi=0xf7182000 mask=0x0 ofs=0x22
> 265.214276: vm_unmapped_area: addr=fffffffffffffff4 err=-12 tvm=0xffb08 flags=0x0 len=0x100000 lo=0x40000000 hi=0xfffff000 mask=0x0 ofs=0x22

I think it's nicer to print addr as 0 if it was in fact an error.
Also "tvm" is cryptic, I would just use total_vm?

> 510.827472: vm_unmapped_area: addr=7377060000 err=0 tvm=0x262f flags=0x1 len=0x7f000 lo=0x8000 hi=0x73795f7000 mask=0x0 ofs=0x22
> 510.827668: vm_unmapped_area: addr=73794d1000 err=0 tvm=0x25cc flags=0x1 len=0x1000 lo=0x8000 hi=0x73795f7000 mask=0x0 ofs=0x22
> 
> Signed-off-by: Jaewon Kim <jaewon31.kim@samsung.com>
> ---
>  include/trace/events/mmap.h | 52 +++++++++++++++++++++++++++++++++++++++++++++
>  mm/mmap.c                   | 12 +++++++++--
>  2 files changed, 62 insertions(+), 2 deletions(-)
>  create mode 100644 include/trace/events/mmap.h
> 
> diff --git a/include/trace/events/mmap.h b/include/trace/events/mmap.h
> new file mode 100644
> index 000000000000..a51dd8af6f8c
> --- /dev/null
> +++ b/include/trace/events/mmap.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM mmap
> +
> +#if !defined(_TRACE_MMAP_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_MMAP_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(vm_unmapped_area,
> +
> +	TP_PROTO(unsigned long addr, struct vm_unmapped_area_info *info),
> +
> +	TP_ARGS(addr, info),
> +
> +	TP_STRUCT__entry(
> +		__field(unsigned long,	addr)
> +		__field(unsigned long,	error)

That seems wasteful to store both, coming from the same value.

> +		__field(unsigned long,	total_vm)
> +		__field(unsigned long,	flags)
> +		__field(unsigned long,	length)
> +		__field(unsigned long,	low_limit)
> +		__field(unsigned long,	high_limit)
> +		__field(unsigned long,	align_mask)
> +		__field(unsigned long,	align_offset)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->addr = addr;
> +		if (IS_ERR_VALUE(addr))
> +			__entry->error = addr;
> +		else
> +			__entry->error = 0;

fast_assign should be also as fast as possible, i.e. avoid these decision, just
store addr...

> +		__entry->total_vm = current->mm->total_vm;
> +		__entry->flags = info->flags;
> +		__entry->length = info->length;
> +		__entry->low_limit = info->low_limit;
> +		__entry->high_limit = info->high_limit;
> +		__entry->align_mask = info->align_mask;
> +		__entry->align_offset = info->align_offset;
> +	),
> +
> +	TP_printk("addr=%lx err=%ld tvm=0x%lx flags=0x%lx len=0x%lx lo=0x%lx hi=0x%lx mask=0x%lx ofs=0x%lx\n",
> +		__entry->addr, __entry->error, __entry->total_vm,

... and only here do the decisions if you print addr as zero and err as non-zero,
or vice versa. Something like the patch at the end of the e-mail.
Except then /sys/kernel/debug/tracing/events/mmap/vm_unmapped_area/format contains:

print fmt: "addr=%lx err=%ld tvm=0x%lx flags=0x%lx len=0x%lx lo=0x%lx hi=0x%lx mask=0x%lx ofs=0x%lx
", __builtin_expect(!!((unsigned long)(void *)(REC->addr) >= (unsigned long)-4095), 0) ? 0 : REC->addr, __builtin_expect(!!((unsigned long)(void *)(REC->addr) >= (unsigned long)-4095), 0) ? REC->addr : 0, REC->total_vm, REC->flags, REC->length, REC->low_limit, REC->high_limit, REC->align_mask, REC->align_offset

and trace-cmd doesn't like it:

           sleep-381   [001]    93.434157: vm_unmapped_area:     [FAILED TO PARSE] addr=0x7f7f32c47000 total_vm=0x2a flags=0x1 length=0x2d000 low_limit=0x1000 high_limit=0x7f7f32c74000 align_mask=0x0 align_offset=0x0

CC Steven, any idea? Does it not like the __builtin_epect, i.e. unlikely()
that's part of IS_ERR_VALUE?

Thanks,
Vlastimil

> +		__entry->flags, __entry->length, __entry->low_limit,
> +		__entry->high_limit, __entry->align_mask,
> +		__entry->align_offset)
> +);
> +#endif
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index eeaddb76286c..68ccbd7d5256 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -53,6 +53,9 @@
>  #include <asm/tlb.h>
>  #include <asm/mmu_context.h>
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/mmap.h>
> +
>  #include "internal.h"
>  
>  #ifndef arch_mmap_check
> @@ -2061,10 +2064,15 @@ unsigned long unmapped_area_topdown(struct vm_unmapped_area_info *info)
>   */
>  unsigned long vm_unmapped_area(struct vm_unmapped_area_info *info)
>  {
> +	unsigned long addr;
> +
>  	if (info->flags & VM_UNMAPPED_AREA_TOPDOWN)
> -		return unmapped_area_topdown(info);
> +		addr = unmapped_area_topdown(info);
>  	else
> -		return unmapped_area(info);
> +		addr = unmapped_area(info);
> +
> +	trace_vm_unmapped_area(addr, info);
> +	return addr;
>  }
>  
>  #ifndef arch_get_mmap_end
> 


------8<------
diff --git a/include/trace/events/mmap.h b/include/trace/events/mmap.h
index a51dd8af6f8c..57cccf731721 100644
--- a/include/trace/events/mmap.h
+++ b/include/trace/events/mmap.h
@@ -15,7 +15,6 @@ TRACE_EVENT(vm_unmapped_area,
 
        TP_STRUCT__entry(
                __field(unsigned long,  addr)
-               __field(unsigned long,  error)
                __field(unsigned long,  total_vm)
                __field(unsigned long,  flags)
                __field(unsigned long,  length)
@@ -27,10 +26,6 @@ TRACE_EVENT(vm_unmapped_area,
 
        TP_fast_assign(
                __entry->addr = addr;
-               if (IS_ERR_VALUE(addr))
-                       __entry->error = addr;
-               else
-                       __entry->error = 0;
                __entry->total_vm = current->mm->total_vm;
                __entry->flags = info->flags;
                __entry->length = info->length;
@@ -41,8 +36,8 @@ TRACE_EVENT(vm_unmapped_area,
        ),
 
        TP_printk("addr=%lx err=%ld tvm=0x%lx flags=0x%lx len=0x%lx lo=0x%lx hi=0x%lx mask=0x%lx ofs=0x%lx\n",
-               __entry->addr, __entry->error, __entry->total_vm,
-               __entry->flags, __entry->length, __entry->low_limit,
+               IS_ERR_VALUE(__entry->addr) ? 0 : __entry->addr, IS_ERR_VALUE(__entry->addr) ? __entry->addr : 0,
+               __entry->total_vm, __entry->flags, __entry->length, __entry->low_limit,
                __entry->high_limit, __entry->align_mask,
                __entry->align_offset)
 );