Message-ID: <a4ef5e31-551e-481e-8da1-1476ca827f97@intel.com>
Date: Fri, 3 Jan 2025 12:36:52 -0800
From: "Chen, Zide" <zide.chen@...el.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
 David Hildenbrand <david@...hat.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
 Adrian Hunter <adrian.hunter@...el.com>,
 Kan Liang <kan.liang@...ux.intel.com>, linux-perf-users@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-mm@...ck.org,
 Matthew Wilcox <willy@...radead.org>, Yi Lai <yi1.lai@...ux.intel.com>
Subject: Re: [PATCH v3] perf: map pages in advance



On 1/3/2025 6:45 AM, Lorenzo Stoakes wrote:
> On Mon, Dec 23, 2024 at 10:12:42PM +0100, David Hildenbrand wrote:
>> On 23.12.24 12:10, Lorenzo Stoakes wrote:
>>> Peter - could you drop this patch for now until I have a chance to take a
>>> look at this issue on my return on 2nd Jan?
>>>
>>> On Fri, Dec 20, 2024 at 10:53:14PM +0100, David Hildenbrand wrote:
>>>> On 20.12.24 22:29, David Hildenbrand wrote:
>>>>> On 20.12.24 20:36, Chen, Zide wrote:
>>>>>>
>>>>>>
>>>>>> On 12/20/2024 1:56 AM, David Hildenbrand wrote:
>>>>>>> On 20.12.24 10:31, Lorenzo Stoakes wrote:
>>>>>>>> On Thu, Dec 19, 2024 at 01:17:44PM -0800, Chen, Zide wrote:
>>>>>>>>
>>>>>>>>> With this patch, it seems perf tool has some problems in capturing the
>>>>>>>>> kernel data with Intel PT.
>>>>>>>>>
>>>>>>>>> Running the following commands, the size of perf.data is very small, and
>>>>>>>>> perf script can't find any valid records.
>>>>>>>>>
>>>>>>>>> perf record -e intel_pt//u -- /bin/ls
>>>>>>>>> perf script --insn-trace
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I'm on leave (and should really go back to relaxing :>), returning on 2nd
>>>>>>>> Jan so can't really dig into this.
>>>>>>>>
>>>>>>>> But I tried it on my intel box and it 'works on my machine' with and
>>>>>>>> without patch with commands provided, so I'm not sure this is actually a
>>>>>>>> product of this change (which shouldn't impact this).
>>>>>>>
>>>>>>> Zide Chen, can you try with and without this patch to see if it
>>>>>>> introduces the issue?
>>>>>>
>>>>>> Yes, I re-did the test on a SPR server, and the result is same. Without
>>>>>> the patch, it went well; But with it, "perf script --insn-trace" doesn't
>>>>>> show valid records.
>>>>>>
>>>>>> This time I tested it on the clean 6.13-rc1 tag, base commit
>>>>>> 40384c840ea1944d7c5a392e8975ed088ecf0b37
>>>>>>
>>>>>> Also, with this patch, running tools/perf/tests/shell/test_intel_pt.sh:
>>>>>>
>>>>>> Error:
>>>>>> The - data has no samples!
>>>>>
>>>>> I just tested it on 6.13-rc1  vs. 6.13-rc1  with this patch.
>>>>>
>>>>> Indeed, there is quite difference. Below are the main parts that changed, only.
>>>>>
>>>>> We seem to be recording data, but maybe what we record gets corrupted somehow?
>>>>
>>>> Huge parts of the new file are full of 0s. Either we are mapping the wrong
>>>> pages, or reading from the pages (via PFNMAP) does not work as expected.
>>>>
>>>
>>> Thanks David, and apologies Zide, appears there is an issue here clearly.
>>>
>>> Could you try this with sudo operations? I was doing this locally and I
>>> wonder if there is now a permissioning error?
>>
>> I ran it under root.
>>
>>>
>>> I'd be surprised if pfn map would cause an issue here as it should just
>>> directly map the kernel memory, however if the PT code assumes there will
>>> be faults there could be an issue. I did take a brief look at this last
>>> week and it seems the PT stuff relies on the aux functionality, so that
>>> could also be a source of problems here.
>>
>> I started a bit at that code, no clue yet what's happening.
>>
>> I was wondering if we end up mapping the wrong pages, meaning: the pages at
>> mmap time end up being different to the pages later at fault time. The code
>> is a bit confusing, but I thought we cannot change the effective event/pages
>> while we have an active mmap. Maybe there is some corner case ...
> 
> OK I figured it out... it's a very silly mistake on my part (oh how this is so
> often the case :).
> 
> When we map the pages, we do not offset by vma->vm_pgoff when looking up the
> page, so if you map with an offset (as presumably the intel pt stuff is), it is
> then retrieving the wrong pages).
> 
> This also resolves the apparent need for sudo...
> 
> Very silly mistake. Apologies :>)
> 
> I will send a v4 in a second.
> 
> Zide - could you give v4 a test when I send it out just to confirm it resolves
> your issue? I will cc- you on this.
> 
> Thanks again for your report, and apologies for the noise!

I can confirm that v4 works for my Intel PT tests!

-Zide


>>
>> Nothing else really jumped at me ... moving the mapping og pages after the
>> event_mapped() callback also didn't change anything.
>>
>>>
>>> I am on leave at the moment returning on 2nd Jan, I will look at this as a
>>> priority when I return, as you can see above I've asked Peter to drop this
>>> for now.
>>
>> Enjoy your time off an Happy Holidays!
>>
>> --
>> Cheers,
>>
>> David / dhildenb
>>
> 
> Cheers, Lorenzo

Powered by blists - more mailing lists