Received-SPF: pass (google.com: domain of linux-kernel+bounces-126399-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <3be0a35e-9e10-49b5-8fa2-71f6ddc6f704@bytedance.com>
Date: Mon, 1 Apr 2024 09:50:02 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [External] Re: [PATCH 1/2] perf sched timehist: Fix
 -g/--call-graph option failure
Content-Language: en-US
To: Arnaldo Carvalho de Melo <acme@kernel.org>,
 Ian Rogers <irogers@google.com>
Cc: peterz@infradead.org, mingo@redhat.com, namhyung@kernel.org,
 mark.rutland@arm.com, alexander.shishkin@linux.intel.com, jolsa@kernel.org,
 adrian.hunter@intel.com, kan.liang@linux.intel.com, james.clark@arm.com,
 linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240328055857.383180-1-yangjihong@bytedance.com>
 <CAP-5=fXms1noWT8PXqBu89QogcwVsvAGpxq3Q_bNUYOYL7PpKA@mail.gmail.com>
 <2e44ae50-d4af-4788-9274-aaf345bac066@bytedance.com>
 <CAP-5=fWDY9Aj+qHNOuwJ9yE==G=vmCzECXYEZAifOhGHX7yr6w@mail.gmail.com>
 <ZggmNWYjBtid_hCZ@x1>
From: Yang Jihong <yangjihong@bytedance.com>
In-Reply-To: <ZggmNWYjBtid_hCZ@x1>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hello,

On 3/30/24 22:48, Arnaldo Carvalho de Melo wrote:
> On Fri, Mar 29, 2024 at 09:08:01AM -0700, Ian Rogers wrote:
>> On Thu, Mar 28, 2024 at 8:02 PM Yang Jihong <yangjihong@bytedance.com> wrote:
>>>
>>> Hello,
>>>
>>> Sorry, due to the new email settings, the last reply email was in html
>>> format, resend it now.
>>>
>>> On 3/29/24 00:02, Ian Rogers wrote:
>>>> On Wed, Mar 27, 2024 at 10:59 PM Yang Jihong <yangjihong@bytedance.com> wrote:
>>>>>
>>>>> When perf-sched enables the call-graph recording, sample_type of dummy
>>>>> event does not have PERF_SAMPLE_CALLCHAIN, timehist_check_attr() checks
>>>>> that the evsel does not have a callchain, and set show_callchain to 0.
>>>>>
>>>>> Currently perf sched timehist only saves callchain when processing
>>>>> sched:sched_switch event, timehist_check_attr() only needs to determine
>>>>> whether the event has PERF_SAMPLE_CALLCHAIN.
>>>>>
>>>>> Before:
>>>>>     # perf sched record -g true
>>>>>     [ perf record: Woken up 0 times to write data ]
>>>>>     [ perf record: Captured and wrote 4.153 MB perf.data (7536 samples) ]
>>>>>     # perf sched timehist
>>>>>     Samples do not have callchains.
>>>>>                time    cpu  task name                       wait time  sch delay   run time
>>>>>                             [tid/pid]                          (msec)     (msec)     (msec)
>>>>>     --------------- ------  ------------------------------  ---------  ---------  ---------
>>>>>       147851.826019 [0000]  perf[285035]                        0.000      0.000      0.000
>>>>>       147851.826029 [0000]  migration/0[15]                     0.000      0.003      0.009
>>>>>       147851.826063 [0001]  perf[285035]                        0.000      0.000      0.000
>>>>>       147851.826069 [0001]  migration/1[21]                     0.000      0.003      0.006
>>>>>     <SNIP>
>>>>>
>>>>> After:
>>>>>     # perf sched record -g true
>>>>>     [ perf record: Woken up 1 times to write data ]
>>>>>     [ perf record: Captured and wrote 2.572 MB perf.data (822 samples) ]
>>>>>     # perf sched timehist
>>>>>                time    cpu  task name                       wait time  sch delay   run time
>>>>>                             [tid/pid]                          (msec)     (msec)     (msec)
>>>>>     --------------- ------  ------------------------------  ---------  ---------  ---------
>>>>>       144193.035164 [0000]  perf[277062]                        0.000      0.000      0.000    __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
>>>>>       144193.035174 [0000]  migration/0[15]                     0.000      0.003      0.009    __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
>>>>>       144193.035207 [0001]  perf[277062]                        0.000      0.000      0.000    __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
>>>>>       144193.035214 [0001]  migration/1[21]                     0.000      0.003      0.007    __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
>>>>> <SNIP>
>>>>
>>>> This looks good, should there be a Fixes tag for the sake of backports?
>>>>
>>> The direct cause is commit 9c95e4ef0657 ("perf evlist: Add
>>> evlist__findnew_tracking_event() helper"). perf-record uses
>>> evlist__add_aux_dummy() to replace evlist__add_dummy() to add a dummy
>>> event. The difference is that evlist__add_aux_dummy() sets
>>> no_aux_samples to true (this is expected behavior, for dummy event, no
>>> need to sample aux data), resulting in evsel__config() not adding the
>>> PERF_SAMPLE_CALLCHAIN bit to dummy's sample_type.
>>>
>>> In summary, the direct cause is the problem introduced by commit
>>> 9c95e4ef0657 ("perf evlist: Add evlist__findnew_tracking_event()
>>> helper"), but the root cause is the timehist_check_attr() logic problem,
>>> The dummy event itself does not need to have PERF_SAMPLE_CALLCHAIN, so
>>> there is no need to check it.
>>>
>>>
>>> So, maybe add fixes-tag:
>>>
>>> Fixes: 9c95e4ef0657 ("perf evlist: Add evlist__findnew_tracking_event()
>>> helper")
>>>
>>> If it is ok, I will send v2 version with this fixes-tag.
>>
>> I think the maintainer can add the fixes tag when they add the reviewed-by tag:
> 
> I usually do this, but if the submitter does it I'll have just to check
> that it is the right tag, helps a bit in processing.
Okay, will send v2 version.

> 
> - Arnaldo
>   
>> Reviewed-by: Ian Rogers <irogers@google.com>
Thanks for reviewed-by tag.

Thanks,
Yang