* [PATCH] mm: append __GFP_COMP flag for trace_malloc @ 2021-04-27 2:43 Xiongwei Song 2021-04-27 2:53 ` Matthew Wilcox 0 siblings, 1 reply; 15+ messages in thread From: Xiongwei Song @ 2021-04-27 2:43 UTC (permalink / raw) To: cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka Cc: linux-mm, linux-kernel, Xiongwei Song From: Xiongwei Song <sxwjean@gmail.com> When calling kmalloc_order, the flags should include __GFP_COMP here, so that trace_malloc can trace the precise flags. Signed-off-by: Xiongwei Song <sxwjean@gmail.com> --- mm/slab_common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/slab_common.c b/mm/slab_common.c index 585a6f9..c23e1e8 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -928,7 +928,7 @@ EXPORT_SYMBOL(kmalloc_order); void *kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order) { void *ret = kmalloc_order(size, flags, order); - trace_kmalloc(_RET_IP_, ret, size, PAGE_SIZE << order, flags); + trace_kmalloc(_RET_IP_, ret, size, PAGE_SIZE << order, flags | __GFP_COMP); return ret; } EXPORT_SYMBOL(kmalloc_order_trace); -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc 2021-04-27 2:43 [PATCH] mm: append __GFP_COMP flag for trace_malloc Xiongwei Song @ 2021-04-27 2:53 ` Matthew Wilcox 2021-04-27 3:29 ` Xiongwei Song 0 siblings, 1 reply; 15+ messages in thread From: Matthew Wilcox @ 2021-04-27 2:53 UTC (permalink / raw) To: Xiongwei Song Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm, linux-kernel, Xiongwei Song On Tue, Apr 27, 2021 at 10:43:20AM +0800, Xiongwei Song wrote: > From: Xiongwei Song <sxwjean@gmail.com> > > When calling kmalloc_order, the flags should include __GFP_COMP here, > so that trace_malloc can trace the precise flags. I suppose that depends on your point of view. Should we report the flags used by the caller, or the flags that we used to allocate memory? And why does it matter? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc 2021-04-27 2:53 ` Matthew Wilcox @ 2021-04-27 3:29 ` Xiongwei Song 0 siblings, 0 replies; 15+ messages in thread From: Xiongwei Song @ 2021-04-27 3:29 UTC (permalink / raw) To: Matthew Wilcox Cc: Xiongwei Song, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm, Linux Kernel Mailing List On Tue, Apr 27, 2021 at 10:54 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Tue, Apr 27, 2021 at 10:43:20AM +0800, Xiongwei Song wrote: > > From: Xiongwei Song <sxwjean@gmail.com> > > > > When calling kmalloc_order, the flags should include __GFP_COMP here, > > so that trace_malloc can trace the precise flags. > > I suppose that depends on your point of view. Correct. Should we report the > flags used by the caller, or the flags that we used to allocate memory? > And why does it matter? When I capture kmem:kmalloc events on my env with perf: (perf record -p my_pid -e kmem:kmalloc) I got the result below: 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca4000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca8000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a6f80000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a6f84000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a6f88000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a6f8c000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.07% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4c80000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC The value of gfp_flags made me confused, I spent some time to find out which trace_malloc is here. So I think we should append __GFP_COMP. Regards ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc @ 2021-04-27 3:29 ` Xiongwei Song 0 siblings, 0 replies; 15+ messages in thread From: Xiongwei Song @ 2021-04-27 3:29 UTC (permalink / raw) To: Matthew Wilcox Cc: Xiongwei Song, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm, Linux Kernel Mailing List On Tue, Apr 27, 2021 at 10:54 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Tue, Apr 27, 2021 at 10:43:20AM +0800, Xiongwei Song wrote: > > From: Xiongwei Song <sxwjean@gmail.com> > > > > When calling kmalloc_order, the flags should include __GFP_COMP here, > > so that trace_malloc can trace the precise flags. > > I suppose that depends on your point of view. Correct. Should we report the > flags used by the caller, or the flags that we used to allocate memory? > And why does it matter? When I capture kmem:kmalloc events on my env with perf: (perf record -p my_pid -e kmem:kmalloc) I got the result below: 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca4000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca8000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a6f80000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a6f84000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a6f88000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a6f8c000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC 0.07% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4c80000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC The value of gfp_flags made me confused, I spent some time to find out which trace_malloc is here. So I think we should append __GFP_COMP. Regards ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc 2021-04-27 3:29 ` Xiongwei Song (?) @ 2021-04-27 3:36 ` Matthew Wilcox 2021-04-27 4:11 ` Xiongwei Song -1 siblings, 1 reply; 15+ messages in thread From: Matthew Wilcox @ 2021-04-27 3:36 UTC (permalink / raw) To: Xiongwei Song Cc: Xiongwei Song, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm, Linux Kernel Mailing List On Tue, Apr 27, 2021 at 11:29:32AM +0800, Xiongwei Song wrote: > On Tue, Apr 27, 2021 at 10:54 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Tue, Apr 27, 2021 at 10:43:20AM +0800, Xiongwei Song wrote: > > > From: Xiongwei Song <sxwjean@gmail.com> > > > > > > When calling kmalloc_order, the flags should include __GFP_COMP here, > > > so that trace_malloc can trace the precise flags. > > > > I suppose that depends on your point of view. > Correct. > > Should we report the > > flags used by the caller, or the flags that we used to allocate memory? > > And why does it matter? > When I capture kmem:kmalloc events on my env with perf: > (perf record -p my_pid -e kmem:kmalloc) > I got the result below: > 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 > bytes_req=10176 bytes_alloc=16384 > gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC Hmm ... if you have a lot of allocations about this size, that would argue in favour of adding a kmem_cache of 10880 [*] bytes. That way, we'd get 3 allocations per 32kB instead of 2. [*] 32768 / 3, rounded down to a 64 byte cacheline But I don't understand why this confused you. Your caller at ffffffff851d0cb0 didn't specify __GFP_COMP. I'd be more confused if this did report __GFP_COMP. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc 2021-04-27 3:36 ` Matthew Wilcox @ 2021-04-27 4:11 ` Xiongwei Song 0 siblings, 0 replies; 15+ messages in thread From: Xiongwei Song @ 2021-04-27 4:11 UTC (permalink / raw) To: Matthew Wilcox Cc: Xiongwei Song, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm, Linux Kernel Mailing List On Tue, Apr 27, 2021 at 11:36 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Tue, Apr 27, 2021 at 11:29:32AM +0800, Xiongwei Song wrote: > > On Tue, Apr 27, 2021 at 10:54 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Tue, Apr 27, 2021 at 10:43:20AM +0800, Xiongwei Song wrote: > > > > From: Xiongwei Song <sxwjean@gmail.com> > > > > > > > > When calling kmalloc_order, the flags should include __GFP_COMP here, > > > > so that trace_malloc can trace the precise flags. > > > > > > I suppose that depends on your point of view. > > Correct. > > > > Should we report the > > > flags used by the caller, or the flags that we used to allocate memory? > > > And why does it matter? > > When I capture kmem:kmalloc events on my env with perf: > > (perf record -p my_pid -e kmem:kmalloc) > > I got the result below: > > 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 > > bytes_req=10176 bytes_alloc=16384 > > gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC > > Hmm ... if you have a lot of allocations about this size, that would > argue in favour of adding a kmem_cache of 10880 [*] bytes. That way, > we'd get 3 allocations per 32kB instead of 2. I understand you. But I don't think our process needs this size. This size may be a bug in our code or somewhere, I don't know the RC for now. > [*] 32768 / 3, rounded down to a 64 byte cacheline > > But I don't understand why this confused you. Your caller at > ffffffff851d0cb0 didn't specify __GFP_COMP. I'd be more confused if > this did report __GFP_COMP. > I just wanted to save some time when debugging. Regards ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc @ 2021-04-27 4:11 ` Xiongwei Song 0 siblings, 0 replies; 15+ messages in thread From: Xiongwei Song @ 2021-04-27 4:11 UTC (permalink / raw) To: Matthew Wilcox Cc: Xiongwei Song, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm, Linux Kernel Mailing List On Tue, Apr 27, 2021 at 11:36 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Tue, Apr 27, 2021 at 11:29:32AM +0800, Xiongwei Song wrote: > > On Tue, Apr 27, 2021 at 10:54 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Tue, Apr 27, 2021 at 10:43:20AM +0800, Xiongwei Song wrote: > > > > From: Xiongwei Song <sxwjean@gmail.com> > > > > > > > > When calling kmalloc_order, the flags should include __GFP_COMP here, > > > > so that trace_malloc can trace the precise flags. > > > > > > I suppose that depends on your point of view. > > Correct. > > > > Should we report the > > > flags used by the caller, or the flags that we used to allocate memory? > > > And why does it matter? > > When I capture kmem:kmalloc events on my env with perf: > > (perf record -p my_pid -e kmem:kmalloc) > > I got the result below: > > 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 > > bytes_req=10176 bytes_alloc=16384 > > gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC > > Hmm ... if you have a lot of allocations about this size, that would > argue in favour of adding a kmem_cache of 10880 [*] bytes. That way, > we'd get 3 allocations per 32kB instead of 2. I understand you. But I don't think our process needs this size. This size may be a bug in our code or somewhere, I don't know the RC for now. > [*] 32768 / 3, rounded down to a 64 byte cacheline > > But I don't understand why this confused you. Your caller at > ffffffff851d0cb0 didn't specify __GFP_COMP. I'd be more confused if > this did report __GFP_COMP. > I just wanted to save some time when debugging. Regards ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc 2021-04-27 4:11 ` Xiongwei Song @ 2021-04-27 5:30 ` Xiongwei Song -1 siblings, 0 replies; 15+ messages in thread From: Xiongwei Song @ 2021-04-27 5:30 UTC (permalink / raw) To: Matthew Wilcox Cc: Xiongwei Song, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm, Linux Kernel Mailing List Hi Mattew, One more thing I should explain, the kmalloc_order() appends the __GFP_COMP flags, not by the caller. void *kmalloc_order(size_t size, gfp_t flags, unsigned int order) { ........................................................... flags |= __GFP_COMP; page = alloc_pages(flags, order); ........................................................... return ret; } EXPORT_SYMBOL(kmalloc_order); #ifdef CONFIG_TRACING void *kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order) { void *ret = kmalloc_order(size, flags, order); trace_kmalloc(_RET_IP_, ret, size, PAGE_SIZE << order, flags); return ret; } EXPORT_SYMBOL(kmalloc_order_trace); #endif Regards, Xiongwei On Tue, Apr 27, 2021 at 12:11 PM Xiongwei Song <sxwjean@gmail.com> wrote: > > On Tue, Apr 27, 2021 at 11:36 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Tue, Apr 27, 2021 at 11:29:32AM +0800, Xiongwei Song wrote: > > > On Tue, Apr 27, 2021 at 10:54 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > On Tue, Apr 27, 2021 at 10:43:20AM +0800, Xiongwei Song wrote: > > > > > From: Xiongwei Song <sxwjean@gmail.com> > > > > > > > > > > When calling kmalloc_order, the flags should include __GFP_COMP here, > > > > > so that trace_malloc can trace the precise flags. > > > > > > > > I suppose that depends on your point of view. > > > Correct. > > > > > > Should we report the > > > > flags used by the caller, or the flags that we used to allocate memory? > > > > And why does it matter? > > > When I capture kmem:kmalloc events on my env with perf: > > > (perf record -p my_pid -e kmem:kmalloc) > > > I got the result below: > > > 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 > > > bytes_req=10176 bytes_alloc=16384 > > > gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC > > > > Hmm ... if you have a lot of allocations about this size, that would > > argue in favour of adding a kmem_cache of 10880 [*] bytes. That way, > > we'd get 3 allocations per 32kB instead of 2. > I understand you. But I don't think our process needs this size. This size > may be a bug in our code or somewhere, I don't know the RC for now. > > > [*] 32768 / 3, rounded down to a 64 byte cacheline > > > > But I don't understand why this confused you. Your caller at > > ffffffff851d0cb0 didn't specify __GFP_COMP. I'd be more confused if > > this did report __GFP_COMP. > > > I just wanted to save some time when debugging. > > Regards ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc @ 2021-04-27 5:30 ` Xiongwei Song 0 siblings, 0 replies; 15+ messages in thread From: Xiongwei Song @ 2021-04-27 5:30 UTC (permalink / raw) To: Matthew Wilcox Cc: Xiongwei Song, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm, Linux Kernel Mailing List Hi Mattew, One more thing I should explain, the kmalloc_order() appends the __GFP_COMP flags, not by the caller. void *kmalloc_order(size_t size, gfp_t flags, unsigned int order) { ........................................................... flags |= __GFP_COMP; page = alloc_pages(flags, order); ........................................................... return ret; } EXPORT_SYMBOL(kmalloc_order); #ifdef CONFIG_TRACING void *kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order) { void *ret = kmalloc_order(size, flags, order); trace_kmalloc(_RET_IP_, ret, size, PAGE_SIZE << order, flags); return ret; } EXPORT_SYMBOL(kmalloc_order_trace); #endif Regards, Xiongwei On Tue, Apr 27, 2021 at 12:11 PM Xiongwei Song <sxwjean@gmail.com> wrote: > > On Tue, Apr 27, 2021 at 11:36 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Tue, Apr 27, 2021 at 11:29:32AM +0800, Xiongwei Song wrote: > > > On Tue, Apr 27, 2021 at 10:54 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > On Tue, Apr 27, 2021 at 10:43:20AM +0800, Xiongwei Song wrote: > > > > > From: Xiongwei Song <sxwjean@gmail.com> > > > > > > > > > > When calling kmalloc_order, the flags should include __GFP_COMP here, > > > > > so that trace_malloc can trace the precise flags. > > > > > > > > I suppose that depends on your point of view. > > > Correct. > > > > > > Should we report the > > > > flags used by the caller, or the flags that we used to allocate memory? > > > > And why does it matter? > > > When I capture kmem:kmalloc events on my env with perf: > > > (perf record -p my_pid -e kmem:kmalloc) > > > I got the result below: > > > 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 > > > bytes_req=10176 bytes_alloc=16384 > > > gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC > > > > Hmm ... if you have a lot of allocations about this size, that would > > argue in favour of adding a kmem_cache of 10880 [*] bytes. That way, > > we'd get 3 allocations per 32kB instead of 2. > I understand you. But I don't think our process needs this size. This size > may be a bug in our code or somewhere, I don't know the RC for now. > > > [*] 32768 / 3, rounded down to a 64 byte cacheline > > > > But I don't understand why this confused you. Your caller at > > ffffffff851d0cb0 didn't specify __GFP_COMP. I'd be more confused if > > this did report __GFP_COMP. > > > I just wanted to save some time when debugging. > > Regards ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc 2021-04-27 5:30 ` Xiongwei Song (?) @ 2021-04-27 11:25 ` Matthew Wilcox 2021-04-28 3:05 ` Xiongwei Song -1 siblings, 1 reply; 15+ messages in thread From: Matthew Wilcox @ 2021-04-27 11:25 UTC (permalink / raw) To: Xiongwei Song Cc: Xiongwei Song, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm, Linux Kernel Mailing List On Tue, Apr 27, 2021 at 01:30:48PM +0800, Xiongwei Song wrote: > Hi Mattew, > > One more thing I should explain, the kmalloc_order() appends the > __GFP_COMP flags, > not by the caller. > > void *kmalloc_order(size_t size, gfp_t flags, unsigned int order) > { > ........................................................... > > flags |= __GFP_COMP; > page = alloc_pages(flags, order); > ........................................................... > return ret; > } > EXPORT_SYMBOL(kmalloc_order); > > #ifdef CONFIG_TRACING > void *kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order) > { > void *ret = kmalloc_order(size, flags, order); > trace_kmalloc(_RET_IP_, ret, size, PAGE_SIZE << order, flags); > return ret; > } > EXPORT_SYMBOL(kmalloc_order_trace); > #endif Yes, I understood that. What I don't understand is why appending the __GFP_COMP to the trace would have been less confusing for you. Suppose I have some code which calls: kmalloc(10 * 1024, GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC); and I see in my logs 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC|__GFP_COMP That seems to me _more_ confusing because I would wonder "Where did that __GFP_COMP come from?" > > Regards, > Xiongwei > > On Tue, Apr 27, 2021 at 12:11 PM Xiongwei Song <sxwjean@gmail.com> wrote: > > > > On Tue, Apr 27, 2021 at 11:36 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Tue, Apr 27, 2021 at 11:29:32AM +0800, Xiongwei Song wrote: > > > > On Tue, Apr 27, 2021 at 10:54 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > > > On Tue, Apr 27, 2021 at 10:43:20AM +0800, Xiongwei Song wrote: > > > > > > From: Xiongwei Song <sxwjean@gmail.com> > > > > > > > > > > > > When calling kmalloc_order, the flags should include __GFP_COMP here, > > > > > > so that trace_malloc can trace the precise flags. > > > > > > > > > > I suppose that depends on your point of view. > > > > Correct. > > > > > > > > Should we report the > > > > > flags used by the caller, or the flags that we used to allocate memory? > > > > > And why does it matter? > > > > When I capture kmem:kmalloc events on my env with perf: > > > > (perf record -p my_pid -e kmem:kmalloc) > > > > I got the result below: > > > > 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 > > > > bytes_req=10176 bytes_alloc=16384 > > > > gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC > > > > > > Hmm ... if you have a lot of allocations about this size, that would > > > argue in favour of adding a kmem_cache of 10880 [*] bytes. That way, > > > we'd get 3 allocations per 32kB instead of 2. > > I understand you. But I don't think our process needs this size. This size > > may be a bug in our code or somewhere, I don't know the RC for now. > > > > > [*] 32768 / 3, rounded down to a 64 byte cacheline > > > > > > But I don't understand why this confused you. Your caller at > > > ffffffff851d0cb0 didn't specify __GFP_COMP. I'd be more confused if > > > this did report __GFP_COMP. > > > > > I just wanted to save some time when debugging. > > > > Regards ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc 2021-04-27 11:25 ` Matthew Wilcox @ 2021-04-28 3:05 ` Xiongwei Song 0 siblings, 0 replies; 15+ messages in thread From: Xiongwei Song @ 2021-04-28 3:05 UTC (permalink / raw) To: Matthew Wilcox Cc: Xiongwei Song, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm, Linux Kernel Mailing List On Tue, Apr 27, 2021 at 7:26 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Tue, Apr 27, 2021 at 01:30:48PM +0800, Xiongwei Song wrote: > > Hi Mattew, > > > > One more thing I should explain, the kmalloc_order() appends the > > __GFP_COMP flags, > > not by the caller. > > > > void *kmalloc_order(size_t size, gfp_t flags, unsigned int order) > > { > > ........................................................... > > > > flags |= __GFP_COMP; > > page = alloc_pages(flags, order); > > ........................................................... > > return ret; > > } > > EXPORT_SYMBOL(kmalloc_order); > > > > #ifdef CONFIG_TRACING > > void *kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order) > > { > > void *ret = kmalloc_order(size, flags, order); > > trace_kmalloc(_RET_IP_, ret, size, PAGE_SIZE << order, flags); > > return ret; > > } > > EXPORT_SYMBOL(kmalloc_order_trace); > > #endif > > Yes, I understood that. What I don't understand is why appending the > __GFP_COMP to the trace would have been less confusing for you. > > Suppose I have some code which calls: > > kmalloc(10 * 1024, GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC); > > and I see in my logs > > 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC|__GFP_COMP > > That seems to me _more_ confusing because I would wonder "Where did that > __GFP_COMP come from?" Thank you for the comments. But I disagree. When I use trace, I hope I can get the precise data rather than something changed that I don't know , then I can get the correct conclusion or direction on my issue. Here my question is what the trace events are for if they don't provide the real situation? I think that's not graceful and friendly. From my perspective, it'd be better to know my flags changed before checking code lines one by one. In other words, I need a warning to reminder me on this, then I can know quickly my process might do some incorrect things. Regards, Xiongwei ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc @ 2021-04-28 3:05 ` Xiongwei Song 0 siblings, 0 replies; 15+ messages in thread From: Xiongwei Song @ 2021-04-28 3:05 UTC (permalink / raw) To: Matthew Wilcox Cc: Xiongwei Song, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm, Linux Kernel Mailing List On Tue, Apr 27, 2021 at 7:26 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Tue, Apr 27, 2021 at 01:30:48PM +0800, Xiongwei Song wrote: > > Hi Mattew, > > > > One more thing I should explain, the kmalloc_order() appends the > > __GFP_COMP flags, > > not by the caller. > > > > void *kmalloc_order(size_t size, gfp_t flags, unsigned int order) > > { > > ........................................................... > > > > flags |= __GFP_COMP; > > page = alloc_pages(flags, order); > > ........................................................... > > return ret; > > } > > EXPORT_SYMBOL(kmalloc_order); > > > > #ifdef CONFIG_TRACING > > void *kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order) > > { > > void *ret = kmalloc_order(size, flags, order); > > trace_kmalloc(_RET_IP_, ret, size, PAGE_SIZE << order, flags); > > return ret; > > } > > EXPORT_SYMBOL(kmalloc_order_trace); > > #endif > > Yes, I understood that. What I don't understand is why appending the > __GFP_COMP to the trace would have been less confusing for you. > > Suppose I have some code which calls: > > kmalloc(10 * 1024, GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC); > > and I see in my logs > > 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC|__GFP_COMP > > That seems to me _more_ confusing because I would wonder "Where did that > __GFP_COMP come from?" Thank you for the comments. But I disagree. When I use trace, I hope I can get the precise data rather than something changed that I don't know , then I can get the correct conclusion or direction on my issue. Here my question is what the trace events are for if they don't provide the real situation? I think that's not graceful and friendly. From my perspective, it'd be better to know my flags changed before checking code lines one by one. In other words, I need a warning to reminder me on this, then I can know quickly my process might do some incorrect things. Regards, Xiongwei ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc 2021-04-28 3:05 ` Xiongwei Song (?) @ 2021-05-03 12:35 ` Vlastimil Babka 2021-05-07 5:41 ` Xiongwei Song -1 siblings, 1 reply; 15+ messages in thread From: Vlastimil Babka @ 2021-05-03 12:35 UTC (permalink / raw) To: Xiongwei Song, Matthew Wilcox Cc: Xiongwei Song, cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm, Linux Kernel Mailing List On 4/28/21 5:05 AM, Xiongwei Song wrote: > On Tue, Apr 27, 2021 at 7:26 PM Matthew Wilcox <willy@infradead.org> wrote: >> >> On Tue, Apr 27, 2021 at 01:30:48PM +0800, Xiongwei Song wrote: >> > Hi Mattew, >> > >> > One more thing I should explain, the kmalloc_order() appends the >> > __GFP_COMP flags, >> > not by the caller. >> > >> > void *kmalloc_order(size_t size, gfp_t flags, unsigned int order) >> > { >> > ........................................................... >> > >> > flags |= __GFP_COMP; >> > page = alloc_pages(flags, order); >> > ........................................................... >> > return ret; >> > } >> > EXPORT_SYMBOL(kmalloc_order); >> > >> > #ifdef CONFIG_TRACING >> > void *kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order) >> > { >> > void *ret = kmalloc_order(size, flags, order); >> > trace_kmalloc(_RET_IP_, ret, size, PAGE_SIZE << order, flags); >> > return ret; >> > } >> > EXPORT_SYMBOL(kmalloc_order_trace); >> > #endif >> >> Yes, I understood that. What I don't understand is why appending the >> __GFP_COMP to the trace would have been less confusing for you. >> >> Suppose I have some code which calls: >> >> kmalloc(10 * 1024, GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC); >> >> and I see in my logs >> >> 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC|__GFP_COMP >> >> That seems to me _more_ confusing because I would wonder "Where did that >> __GFP_COMP come from?" > > Thank you for the comments. But I disagree. FTR, I agree with Matthew. This is a tracepoint for kmalloc() so I would expect to see what flags were passed to kmalloc(). If I wanted to see how the flags translated to page allocator's flags, I would have used a page allocator's tracepoint which would show me that. > When I use trace, I hope I can get the precise data rather than something > changed that I don't know , then I can get the correct conclusion or > direction on my issue. It's precise from the point of the caller. > Here my question is what the trace events are for if they don't provide the > real situation? I think that's not graceful and friendly. > > From my perspective, it'd be better to know my flags changed before checking > code lines one by one. In other words, I need a warning to reminder me on this, > then I can know quickly my process might do some incorrect things. Your process should not care about __GFP_COMP if you use properly kmalloc()+kfree(). Once you start caring about __GFP_COMP, you should be using page allocator's API, not kmalloc(). > Regards, > Xiongwei > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc 2021-05-03 12:35 ` Vlastimil Babka @ 2021-05-07 5:41 ` Xiongwei Song 0 siblings, 0 replies; 15+ messages in thread From: Xiongwei Song @ 2021-05-07 5:41 UTC (permalink / raw) To: Vlastimil Babka Cc: Matthew Wilcox, Xiongwei Song, cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm, Linux Kernel Mailing List On Mon, May 3, 2021 at 8:35 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 4/28/21 5:05 AM, Xiongwei Song wrote: > > On Tue, Apr 27, 2021 at 7:26 PM Matthew Wilcox <willy@infradead.org> wrote: > >> > >> On Tue, Apr 27, 2021 at 01:30:48PM +0800, Xiongwei Song wrote: > >> > Hi Mattew, > >> > > >> > One more thing I should explain, the kmalloc_order() appends the > >> > __GFP_COMP flags, > >> > not by the caller. > >> > > >> > void *kmalloc_order(size_t size, gfp_t flags, unsigned int order) > >> > { > >> > ........................................................... > >> > > >> > flags |= __GFP_COMP; > >> > page = alloc_pages(flags, order); > >> > ........................................................... > >> > return ret; > >> > } > >> > EXPORT_SYMBOL(kmalloc_order); > >> > > >> > #ifdef CONFIG_TRACING > >> > void *kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order) > >> > { > >> > void *ret = kmalloc_order(size, flags, order); > >> > trace_kmalloc(_RET_IP_, ret, size, PAGE_SIZE << order, flags); > >> > return ret; > >> > } > >> > EXPORT_SYMBOL(kmalloc_order_trace); > >> > #endif > >> > >> Yes, I understood that. What I don't understand is why appending the > >> __GFP_COMP to the trace would have been less confusing for you. > >> > >> Suppose I have some code which calls: > >> > >> kmalloc(10 * 1024, GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC); > >> > >> and I see in my logs > >> > >> 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC|__GFP_COMP > >> > >> That seems to me _more_ confusing because I would wonder "Where did that > >> __GFP_COMP come from?" > > > > Thank you for the comments. But I disagree. > > FTR, I agree with Matthew. This is a tracepoint for kmalloc() so I would expect > to see what flags were passed to kmalloc(). > If I wanted to see how the flags translated to page allocator's flags, I would > have used a page allocator's tracepoint which would show me that. Make sense. Thank you. > > When I use trace, I hope I can get the precise data rather than something > > changed that I don't know , then I can get the correct conclusion or > > direction on my issue. > > It's precise from the point of the caller. > > > Here my question is what the trace events are for if they don't provide the > > real situation? I think that's not graceful and friendly. > > > > From my perspective, it'd be better to know my flags changed before checking > > code lines one by one. In other words, I need a warning to reminder me on this, > > then I can know quickly my process might do some incorrect things. > > Your process should not care about __GFP_COMP if you use properly > kmalloc()+kfree(). Once you start caring about __GFP_COMP, you should be using > page allocator's API, not kmalloc(). > > > Regards, > > Xiongwei > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: append __GFP_COMP flag for trace_malloc @ 2021-05-07 5:41 ` Xiongwei Song 0 siblings, 0 replies; 15+ messages in thread From: Xiongwei Song @ 2021-05-07 5:41 UTC (permalink / raw) To: Vlastimil Babka Cc: Matthew Wilcox, Xiongwei Song, cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm, Linux Kernel Mailing List On Mon, May 3, 2021 at 8:35 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 4/28/21 5:05 AM, Xiongwei Song wrote: > > On Tue, Apr 27, 2021 at 7:26 PM Matthew Wilcox <willy@infradead.org> wrote: > >> > >> On Tue, Apr 27, 2021 at 01:30:48PM +0800, Xiongwei Song wrote: > >> > Hi Mattew, > >> > > >> > One more thing I should explain, the kmalloc_order() appends the > >> > __GFP_COMP flags, > >> > not by the caller. > >> > > >> > void *kmalloc_order(size_t size, gfp_t flags, unsigned int order) > >> > { > >> > ........................................................... > >> > > >> > flags |= __GFP_COMP; > >> > page = alloc_pages(flags, order); > >> > ........................................................... > >> > return ret; > >> > } > >> > EXPORT_SYMBOL(kmalloc_order); > >> > > >> > #ifdef CONFIG_TRACING > >> > void *kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order) > >> > { > >> > void *ret = kmalloc_order(size, flags, order); > >> > trace_kmalloc(_RET_IP_, ret, size, PAGE_SIZE << order, flags); > >> > return ret; > >> > } > >> > EXPORT_SYMBOL(kmalloc_order_trace); > >> > #endif > >> > >> Yes, I understood that. What I don't understand is why appending the > >> __GFP_COMP to the trace would have been less confusing for you. > >> > >> Suppose I have some code which calls: > >> > >> kmalloc(10 * 1024, GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC); > >> > >> and I see in my logs > >> > >> 0.08% call_site=ffffffff851d0cb0 ptr=0xffff8c04a4ca0000 bytes_req=10176 bytes_alloc=16384 gfp_flags=GFP_ATOMIC|__GFP_NOWARN|__GFP_NOMEMALLOC|__GFP_COMP > >> > >> That seems to me _more_ confusing because I would wonder "Where did that > >> __GFP_COMP come from?" > > > > Thank you for the comments. But I disagree. > > FTR, I agree with Matthew. This is a tracepoint for kmalloc() so I would expect > to see what flags were passed to kmalloc(). > If I wanted to see how the flags translated to page allocator's flags, I would > have used a page allocator's tracepoint which would show me that. Make sense. Thank you. > > When I use trace, I hope I can get the precise data rather than something > > changed that I don't know , then I can get the correct conclusion or > > direction on my issue. > > It's precise from the point of the caller. > > > Here my question is what the trace events are for if they don't provide the > > real situation? I think that's not graceful and friendly. > > > > From my perspective, it'd be better to know my flags changed before checking > > code lines one by one. In other words, I need a warning to reminder me on this, > > then I can know quickly my process might do some incorrect things. > > Your process should not care about __GFP_COMP if you use properly > kmalloc()+kfree(). Once you start caring about __GFP_COMP, you should be using > page allocator's API, not kmalloc(). > > > Regards, > > Xiongwei > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-05-07 5:42 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-27 2:43 [PATCH] mm: append __GFP_COMP flag for trace_malloc Xiongwei Song 2021-04-27 2:53 ` Matthew Wilcox 2021-04-27 3:29 ` Xiongwei Song 2021-04-27 3:29 ` Xiongwei Song 2021-04-27 3:36 ` Matthew Wilcox 2021-04-27 4:11 ` Xiongwei Song 2021-04-27 4:11 ` Xiongwei Song 2021-04-27 5:30 ` Xiongwei Song 2021-04-27 5:30 ` Xiongwei Song 2021-04-27 11:25 ` Matthew Wilcox 2021-04-28 3:05 ` Xiongwei Song 2021-04-28 3:05 ` Xiongwei Song 2021-05-03 12:35 ` Vlastimil Babka 2021-05-07 5:41 ` Xiongwei Song 2021-05-07 5:41 ` Xiongwei Song
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.