Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1724241 > unrolled thread

Re: [PATCH 1/5] tracing, mm: Record pfn instead of pointer to struct page

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/5] tracing, mm: Record pfn instead of pointer to  struct page Steven Rostedt <rostedt@goodmis.org> - 2017-08-31 15:50 +0200
    Re: [PATCH 1/5] tracing, mm: Record pfn instead of pointer to struct  page Vlastimil Babka <vbabka@suse.cz> - 2017-08-31 16:40 +0200
      Re: [PATCH 1/5] tracing, mm: Record pfn instead of pointer to  struct page Steven Rostedt <rostedt@goodmis.org> - 2017-08-31 16:50 +0200
        Re: [PATCH 1/5] tracing, mm: Record pfn instead of pointer to struct  page Vlastimil Babka <vbabka@suse.cz> - 2017-09-01 10:20 +0200
          Re: [PATCH 1/5] tracing, mm: Record pfn instead of pointer to  struct page Steven Rostedt <rostedt@goodmis.org> - 2017-09-01 13:20 +0200

#1724241 — Re: [PATCH 1/5] tracing, mm: Record pfn instead of pointer to struct page

On Mon, 31 Jul 2017 09:43:41 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:

> On 04/14/2015 12:14 AM, Arnaldo Carvalho de Melo wrote:
> > From: Namhyung Kim <namhyung@kernel.org>
> > 
> > The struct page is opaque for userspace tools, so it'd be better to save
> > pfn in order to identify page frames.
> > 
> > The textual output of $debugfs/tracing/trace file remains unchanged and
> > only raw (binary) data format is changed - but thanks to libtraceevent,
> > userspace tools which deal with the raw data (like perf and trace-cmd)
> > can parse the format easily.  
> 
> Hmm it seems trace-cmd doesn't work that well, at least on current
> x86_64 kernel where I noticed it:
> 
>  trace-cmd-22020 [003] 105219.542610: mm_page_alloc:        [FAILED TO PARSE] pfn=0x165cb4 order=0 gfp_flags=29491274 migratetype=1

Which version of trace-cmd failed? It parses for me. Hmm, the
vmemmap_base isn't in the event format file. It's the actually address.
That's probably what failed to parse.

> 
> I'm quite sure it's due to the "page=%p" part, which uses pfn_to_page().
> The events/kmem/mm_page_alloc/format file contains this for page:
> 
> REC->pfn != -1UL ? (((struct page *)vmemmap_base) + (REC->pfn)) : ((void *)0)

But yeah, I think the output is wrong. I just ran this:

 page=0xffffea00000a62f4 pfn=680692 order=0 migratetype=0 gfp_flags=GFP_KERNEL_ACCOUNT|__GFP_ZERO|__GFP_NOTRACK

But running it with trace-cmd report -R (raw format):

 mm_page_alloc:         pfn=0xa62f4 order=0 gfp_flags=24150208 migratetype=0

The parser currently ignores types, so it doesn't do pointer
arithmetic correctly, and would be hard to here as it doesn't know the
size of the struct page. What could work is if we changed the printf
fmt to be:

  (unsigned long)(0xffffea0000000000UL) + (REC->pfn * sizeof(struct page))


> 
> I think userspace can't know vmmemap_base nor the implied sizeof(struct
> page) for pointer arithmetic?
> 
> On older 4.4-based kernel:
> 
> REC->pfn != -1UL ? (((struct page *)(0xffffea0000000000UL)) + (REC->pfn)) : ((void *)0)

This is what I have on 4.13-rc7

> 
> This also fails to parse, so it must be the struct page part?

Again, what version of trace-cmd do you have?


> 
> I think the problem is, even if ve solve this with some more
> preprocessor trickery to make the format file contain only constant
> numbers, pfn_to_page() on e.g. sparse memory model without vmmemap is
> more complicated than simple arithmetic, and can't be exported in the
> format file.
> 
> I'm afraid that to support userspace parsing of the trace data, we will
> have to store both struct page and pfn... or perhaps give up on reporting
> the struct page pointer completely. Thoughts?

Had some thoughts up above.

-- Steve

[toc] | [next] | [standalone]

#1724290 — Re: [PATCH 1/5] tracing, mm: Record pfn instead of pointer to struct page

On 08/31/2017 03:43 PM, Steven Rostedt wrote:
> On Mon, 31 Jul 2017 09:43:41 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:
> 
>> On 04/14/2015 12:14 AM, Arnaldo Carvalho de Melo wrote:
>>> From: Namhyung Kim <namhyung@kernel.org>
>>>
>>> The struct page is opaque for userspace tools, so it'd be better to save
>>> pfn in order to identify page frames.
>>>
>>> The textual output of $debugfs/tracing/trace file remains unchanged and
>>> only raw (binary) data format is changed - but thanks to libtraceevent,
>>> userspace tools which deal with the raw data (like perf and trace-cmd)
>>> can parse the format easily.  
>>
>> Hmm it seems trace-cmd doesn't work that well, at least on current
>> x86_64 kernel where I noticed it:
>>
>>  trace-cmd-22020 [003] 105219.542610: mm_page_alloc:        [FAILED TO PARSE] pfn=0x165cb4 order=0 gfp_flags=29491274 migratetype=1
> 
> Which version of trace-cmd failed? It parses for me. Hmm, the
> vmemmap_base isn't in the event format file. It's the actually address.
> That's probably what failed to parse.

Mine says 2.6. With 4.13-rc6 I get FAILED TO PARSE.

> 
>>
>> I'm quite sure it's due to the "page=%p" part, which uses pfn_to_page().
>> The events/kmem/mm_page_alloc/format file contains this for page:
>>
>> REC->pfn != -1UL ? (((struct page *)vmemmap_base) + (REC->pfn)) : ((void *)0)
> 
> But yeah, I think the output is wrong. I just ran this:
> 
>  page=0xffffea00000a62f4 pfn=680692 order=0 migratetype=0 gfp_flags=GFP_KERNEL_ACCOUNT|__GFP_ZERO|__GFP_NOTRACK
> 
> But running it with trace-cmd report -R (raw format):
> 
>  mm_page_alloc:         pfn=0xa62f4 order=0 gfp_flags=24150208 migratetype=0
> 
> The parser currently ignores types, so it doesn't do pointer
> arithmetic correctly, and would be hard to here as it doesn't know the
> size of the struct page. What could work is if we changed the printf
> fmt to be:
> 
>   (unsigned long)(0xffffea0000000000UL) + (REC->pfn * sizeof(struct page))
> 
> 
>>
>> I think userspace can't know vmmemap_base nor the implied sizeof(struct
>> page) for pointer arithmetic?
>>
>> On older 4.4-based kernel:
>>
>> REC->pfn != -1UL ? (((struct page *)(0xffffea0000000000UL)) + (REC->pfn)) : ((void *)0)
> 
> This is what I have on 4.13-rc7
> 
>>
>> This also fails to parse, so it must be the struct page part?
> 
> Again, what version of trace-cmd do you have?

On the older distro it was 2.0.4

> 
>>
>> I think the problem is, even if ve solve this with some more
>> preprocessor trickery to make the format file contain only constant
>> numbers, pfn_to_page() on e.g. sparse memory model without vmmemap is
>> more complicated than simple arithmetic, and can't be exported in the
>> format file.
>>
>> I'm afraid that to support userspace parsing of the trace data, we will
>> have to store both struct page and pfn... or perhaps give up on reporting
>> the struct page pointer completely. Thoughts?
> 
> Had some thoughts up above.

Yeah, it could be made to work for some configurations, but see the part
about "sparse memory model without vmemmap" above.

> -- Steve
> 

[toc] | [prev] | [next] | [standalone]

#1724293

On Thu, 31 Aug 2017 16:31:36 +0200
Vlastimil Babka <vbabka@suse.cz> wrote:


> > Which version of trace-cmd failed? It parses for me. Hmm, the
> > vmemmap_base isn't in the event format file. It's the actually address.
> > That's probably what failed to parse.  
> 
> Mine says 2.6. With 4.13-rc6 I get FAILED TO PARSE.

Right, but you have the vmemmap_base in the event format, which can't
be parsed by userspace because it has no idea what the value of the
vmemmap_base is.

> 
> >   
> >>
> >> I'm quite sure it's due to the "page=%p" part, which uses pfn_to_page().
> >> The events/kmem/mm_page_alloc/format file contains this for page:
> >>
> >> REC->pfn != -1UL ? (((struct page *)vmemmap_base) + (REC->pfn)) : ((void *)0)  
> > 


> >> On older 4.4-based kernel:
> >>
> >> REC->pfn != -1UL ? (((struct page *)(0xffffea0000000000UL)) + (REC->pfn)) : ((void *)0)  
> > 
> > This is what I have on 4.13-rc7
> >   
> >>
> >> This also fails to parse, so it must be the struct page part?  
> > 
> > Again, what version of trace-cmd do you have?  
> 
> On the older distro it was 2.0.4

Right. That's probably why it failed to parse here. If you installed
the latest trace-cmd from the git repo, it probably will parse fine.

> 
> >   
> >>
> >> I think the problem is, even if ve solve this with some more
> >> preprocessor trickery to make the format file contain only constant
> >> numbers, pfn_to_page() on e.g. sparse memory model without vmmemap is
> >> more complicated than simple arithmetic, and can't be exported in the
> >> format file.
> >>
> >> I'm afraid that to support userspace parsing of the trace data, we will
> >> have to store both struct page and pfn... or perhaps give up on reporting
> >> the struct page pointer completely. Thoughts?  
> > 
> > Had some thoughts up above.  
> 
> Yeah, it could be made to work for some configurations, but see the part
> about "sparse memory model without vmemmap" above.

Right, but that should work with the latest trace-cmd. Does it?

-- Steve

[toc] | [prev] | [next] | [standalone]

#1724866 — Re: [PATCH 1/5] tracing, mm: Record pfn instead of pointer to struct page

On 08/31/2017 04:44 PM, Steven Rostedt wrote:
> On Thu, 31 Aug 2017 16:31:36 +0200
> Vlastimil Babka <vbabka@suse.cz> wrote:
> 
> 
>>> Which version of trace-cmd failed? It parses for me. Hmm, the
>>> vmemmap_base isn't in the event format file. It's the actually address.
>>> That's probably what failed to parse.  
>>
>> Mine says 2.6. With 4.13-rc6 I get FAILED TO PARSE.
> 
> Right, but you have the vmemmap_base in the event format, which can't
> be parsed by userspace because it has no idea what the value of the
> vmemmap_base is.

This seems to be caused by CONFIG_RANDOMIZE_MEMORY. If we somehow put the value
in the format file, it's an info leak? (but I guess kernels that care must have
ftrace disabled anyway :)

>>
>>>   
>>>>
>>>> I'm quite sure it's due to the "page=%p" part, which uses pfn_to_page().
>>>> The events/kmem/mm_page_alloc/format file contains this for page:
>>>>
>>>> REC->pfn != -1UL ? (((struct page *)vmemmap_base) + (REC->pfn)) : ((void *)0)  
>>>>
>>>> I think the problem is, even if ve solve this with some more
>>>> preprocessor trickery to make the format file contain only constant
>>>> numbers, pfn_to_page() on e.g. sparse memory model without vmmemap is
>>>> more complicated than simple arithmetic, and can't be exported in the
>>>> format file.
>>>>
>>>> I'm afraid that to support userspace parsing of the trace data, we will
>>>> have to store both struct page and pfn... or perhaps give up on reporting
>>>> the struct page pointer completely. Thoughts?  
>>>
>>> Had some thoughts up above.  
>>
>> Yeah, it could be made to work for some configurations, but see the part
>> about "sparse memory model without vmemmap" above.
> 
> Right, but that should work with the latest trace-cmd. Does it?

Hmm, by "sparse memory model without vmemmap" I don't mean there's a
number instead of "vmemmap_base". I mean CONFIG_SPARSEMEM=y

Then __pfn_to_page() looks like this:

#define __page_to_pfn(pg)                                       \
({      const struct page *__pg = (pg);                         \
        int __sec = page_to_section(__pg);                      \
        (unsigned long)(__pg - __section_mem_map_addr(__nr_to_section(__sec))); \
})

Then the part of format file looks like this:

REC->pfn != -1UL ? ({ unsigned long __pfn = (REC->pfn); struct mem_section *__sec = __pfn_to_section(__pfn); __section_mem_map_addr(__sec) + __pfn; }) : ((void *)0)

The section things involve some array lookups, so I don't see how we
could pass it to tracing userspace. Would we want to special-case
this config to store both pfn and struct page in the trace frame? And
make sure the simpler ones work despite all the exsisting gotchas?
I'd rather say we should either store both pfn and page pointer, or
just throw away the page pointer as the pfn is enough to e.g. match
alloc and free, and also much more deterministic.
 
> -- Steve
> 

[toc] | [prev] | [next] | [standalone]

#1724979

On Fri, 1 Sep 2017 10:16:21 +0200
Vlastimil Babka <vbabka@suse.cz> wrote:
 
> > Right, but that should work with the latest trace-cmd. Does it?  
> 
> Hmm, by "sparse memory model without vmemmap" I don't mean there's a
> number instead of "vmemmap_base". I mean CONFIG_SPARSEMEM=y
> 
> Then __pfn_to_page() looks like this:
> 
> #define __page_to_pfn(pg)                                       \
> ({      const struct page *__pg = (pg);                         \
>         int __sec = page_to_section(__pg);                      \
>         (unsigned long)(__pg - __section_mem_map_addr(__nr_to_section(__sec))); \
> })
> 
> Then the part of format file looks like this:
> 
> REC->pfn != -1UL ? ({ unsigned long __pfn = (REC->pfn); struct mem_section *__sec = __pfn_to_section(__pfn); __section_mem_map_addr(__sec) + __pfn; }) : ((void *)0)

Ouch.

> 
> The section things involve some array lookups, so I don't see how we
> could pass it to tracing userspace. Would we want to special-case
> this config to store both pfn and struct page in the trace frame? And
> make sure the simpler ones work despite all the exsisting gotchas?
> I'd rather say we should either store both pfn and page pointer, or
> just throw away the page pointer as the pfn is enough to e.g. match
> alloc and free, and also much more deterministic.

Write up a patch and we'll take a look.

-- Steve

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web