Re: [PATCH 1/2] ps: Add support to "ps -l" to properly display process list

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2022년 2월 25일 (금) 오후 6:47, lijiang <lijiang@xxxxxxxxxx>님이 작성:
>
> Thank you for the patch, Austin.
>
> On Fri, Feb 25, 2022 at 4:52 PM <crash-utility-request@xxxxxxxxxx> wrote:
>>
>> Date: Fri, 25 Feb 2022 07:19:32 +0000
>> From: Austin Kim <austindh.kim@xxxxxxxxx>
>> To: k-hagio-ab@xxxxxxx, crash-utility@xxxxxxxxxx
>> Cc: kernel-team@xxxxxxx, mikeseohyungjin@xxxxxxxxx
>> Subject:  [PATCH 1/2] ps: Add support to "ps -l" to
>>         properly display process list
>> Message-ID: <20220225071932.GA1097@raspberrypi>
>> Content-Type: text/plain; charset=us-ascii
>>
>> Sometimes kernel image is generated without CONFIG_SCHED_STAT or CONFIG_SCHED_INFO.
>>
>> Running crash-utility with above kernel image,
>> "ps -l" options cannot display all processes sorted with most recently-run process
>>
>> crash> ps -l
>> ps: last-run timestamps do not exist in this kernel
>> Usage:
>>   ps [-k|-u|-G] [-s] [-p|-c|-t|-[l|m][-C cpu]|-a|-g|-r|-S]
>>      [pid | task | command] ...
>> Enter "help ps" for details.
>>
>> This is because output of 'ps -l' depends on task_struct.sched_info.last_arrival.
>> Without CONFIG_SCHED_STAT or CONFIG_SCHED_INFO, 'sched_info' field is not included
>> in task_struct.
>>
>> So we make 'ps -e' option to access 'exec_start' field of sched_entity.
>> where 'exec_start' is task_struct.se.exec_start.
>>
>> With this patch, "ps -l" option works well without CONFIG_SCHED_STAT or
>> CONFIG_SCHED_INFO.
>>
>> The history of CONFIG_SCHED_INFO and CONFIG_SCHED_STAT is as below;
>>
>>   - CONFIG_SCHED_INFO: KERNEL_VERSION >= LINUX(4,2,0)
>>   - CONFIG_SCHED_STAT: KERNEL_VERSION < LINUX(4,2,0)
>>
>
> Could you please add the kernel commit ID for the relevant changes?
>

Relevant commit I think of is below.
f6db83479932 ("sched/stat: Simplify the sched_info accounting")

Let me add above commit ID to the commit description with v2 patch.

> In addition, I would suggest to fold these two patches as one patch and change the subject as:
> "ps: Add support to "ps -l|-m" to properly display process list", what do you think?

Thanks for good suggetion. I guess one patch is enough with "ps -l|-m"
commit title.
Let me send v2 patch which includes 1 commit.

>
>> Signed-off-by: Austin Kim <austindh.kim@xxxxxxxxx>
>> ---
>>  defs.h    |  2 ++
>>  symbols.c |  2 ++
>>  task.c    | 17 ++++++++++++++---
>>  3 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/defs.h b/defs.h
>> index 7d386d2..ed2f5ca 100644
>> --- a/defs.h
>> +++ b/defs.h
>> @@ -1768,6 +1768,8 @@ struct offset_table {                    /* stash of commonly-used offsets */
>>         long vcpu_struct_rq;
>>         long task_struct_sched_info;
>>         long sched_info_last_arrival;
>> +       long task_struct_sched_entity;
>> +       long se_exec_start;
>
>
> This can be only appended to the end of the offset_table.
> For more details, refer to the section "writing patches" in wiki:
> https://github.com/crash-utility/crash/wiki
>

Thanks for the information.
Let me make sure to append these to the end of the offset_table.

I will resend patch v2 version soon.

Thanks for detailed review and tips.

BR,
Austin Kim

> Thanks.
> Lianbo
>
>>         long page_objects;
>>         long kmem_cache_oo;
>>         long char_device_struct_cdev;
>> diff --git a/symbols.c b/symbols.c
>> index 97fb778..5e2032a 100644
>> --- a/symbols.c
>> +++ b/symbols.c
>> @@ -8892,6 +8892,8 @@ dump_offset_table(char *spec, ulong makestruct)
>>                  OFFSET(sched_rt_entity_run_list));
>>         fprintf(fp, "       sched_info_last_arrival: %ld\n",
>>                  OFFSET(sched_info_last_arrival));
>> +               fprintf(fp, "       se_exec_start: %ld\n",
>> +                               OFFSET(se_exec_start));
>>          fprintf(fp, "       task_struct_thread_info: %ld\n",
>>                  OFFSET(task_struct_thread_info));
>>          fprintf(fp, "             task_struct_stack: %ld\n",
>> diff --git a/task.c b/task.c
>> index 864c838..e6fde74 100644
>> --- a/task.c
>> +++ b/task.c
>> @@ -334,9 +334,15 @@ task_init(void)
>>         if (VALID_MEMBER(task_struct_sched_info))
>>                 MEMBER_OFFSET_INIT(sched_info_last_arrival,
>>                         "sched_info", "last_arrival");
>> +       MEMBER_OFFSET_INIT(task_struct_sched_entity, "task_struct", "se");
>> +       if (VALID_MEMBER(task_struct_sched_entity)) {
>> +               STRUCT_SIZE_INIT(sched_entity, "sched_entity");
>> +               MEMBER_OFFSET_INIT(se_exec_start, "sched_entity", "exec_start");
>> +       }
>>         if (VALID_MEMBER(task_struct_last_run) ||
>>             VALID_MEMBER(task_struct_timestamp) ||
>> -           VALID_MEMBER(sched_info_last_arrival)) {
>> +           VALID_MEMBER(sched_info_last_arrival) ||
>> +           VALID_MEMBER(se_exec_start)) {
>>                 char buf[BUFSIZE];
>>                 strcpy(buf, "alias last ps -l");
>>                 alias_init(buf);
>> @@ -3574,7 +3580,8 @@ cmd_ps(void)
>>                 case 'l':
>>                         if (INVALID_MEMBER(task_struct_last_run) &&
>>                             INVALID_MEMBER(task_struct_timestamp) &&
>> -                           INVALID_MEMBER(sched_info_last_arrival)) {
>> +                           INVALID_MEMBER(sched_info_last_arrival) &&
>> +                           INVALID_MEMBER(se_exec_start)) {
>>                                 error(INFO,
>>                              "last-run timestamps do not exist in this kernel\n");
>>                                 argerrs++;
>> @@ -6020,7 +6027,11 @@ task_last_run(ulong task)
>>                 timestamp = tt->last_task_read ?  ULONGLONG(tt->task_struct +
>>                         OFFSET(task_struct_sched_info) +
>>                         OFFSET(sched_info_last_arrival)) : 0;
>> -
>> +       else if (VALID_MEMBER(se_exec_start))
>> +                       timestamp = tt->last_task_read ?  ULONGLONG(tt->task_struct +
>> +                       OFFSET(task_struct_sched_entity) +
>> +                       OFFSET(se_exec_start)) : 0;
>> +
>>          return timestamp;
>>  }
>>
>> --
>> 2.20.1


--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux