9:06 a.m.
----- Original Message -----
On 09/06/2014 04:11 AM, Dave Anderson wrote:
> Hello Pan,
>
> The patch looks to run OK. Here are my comments and suggestions:
>
> # make warn
> cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 memory.c -Wall -O2
> -Wstrict-prototypes -Wmissing-prototypes -fstack-protector
> -Wformat-security
> memory.c: In function 'get_task_mem_usage':
> memory.c:4157:10: warning: variable 'tgid' set but not used
> [-Wunused-but-set-variable]
> ...
>
> And speaking of tgid, I don't see why it's necessary to keep two
> copies of the tgid:?
>
> @@ -758,13 +758,23 @@ struct task_context { /* context
> stored for each task */
> int processor;
> ulong ptask;
> ulong mm_struct;
> + ulong tgid;
> + ulong tgid_task_context_index;
> struct task_context *tc_next;
> };
>
> +struct tgid_task_context{ /* context and tgid stored for
> each task */
> + ulong tgid;
> + struct task_context *tc;
> +};
> +
>
> These 3 functions are always called together, in task_init(),
> and in exec_command() if (ct->flags& REFRESH_TASK_TABLE):
>
> tt->refresh_task_table() -> calls store_context()
> sort_context_array()
> set_tgid_context_array()
>
> It would seem that store_context(), which reads the tgid value,
> could call a new function, say, something like:
>
> store_tgid_context(struct task_context *tc, ulong tgid);
>
> The store_tgid_context() function could determine the index
> into the tt->context_array from the address "tc", and therefore
> could populate both fields of each tgid_task_context array entry.
>
> Then, the set_tgid_context_array() function could be renamed
> to sort_tgid_context_array() and it could just do the sorting
> operation on the array that has already been initialized.
>
> Unfortunately I can't see a obvious way to remove the new
> tc->tgid_context_index field...
>
> Another question: why is it necessary to call sort_tgid_context_array()
> both here:
>
> @ -2916,7 +2981,10 @@ cmd_ps(void)
> cmd_usage(pc->curcmd, SYNOPSIS);
>
> if (flag& (PS_LAST_RUN|PS_MSECS))
> + {
> sort_context_array_by_last_run();
> + set_tgid_context_array();
> + }
>
> and here:
>
> @@ -5966,7 +6034,10 @@ foreach(struct foreach_data *fd)
> }
> }
> if (fd->flags& (FOREACH_l_FLAG|FOREACH_m_FLAG))
> + {
> sort_context_array_by_last_run();
> + set_tgid_context_array();
> +
>
> The "ps -l" and "ps -m" options don't need anything in the
> tgid_context_array, and everything will get re-sorted when
> the next "ps" command is attempted.
>
> Lastly, the tgid_task_context array should be dumped at the
> end of the "help -T" display after the task_context array.
>
> Dave
>
>
> .
>
I see. But I can't understand the meaning of "The store_tgid_context()
function could determine the index into the tt->context_array from
the address "tc", and therefore could populate both fields of each
tgid_task_context array entry.", because the sort_context_array() function
would change the tt->context_array, so it is different with the tc of
tgid_context_array.
Ah yes, you're right.
Can I do it like this:
"
+struct task_table { /* kernel/local task table data */
+ struct task_context *current;
+ struct task_context *context_array, *tgid_context_array;
+ void (*refresh_task_table)(void);
" the context_array sort by pid, or the tgid_context_array sort by tgid.
That would be a waste of space because of the unused fields in the
task_context structure used by the new tt->tgid_context_array.
Or like this:
"
+struct tgid_task_context{ /* context and tgid stored for each
task */
+ ulong tgid;
+ ulong task;
+};
+struct task_table { /* kernel/local task table data */
+ struct task_context *current;
+ struct task_context *context_array;
+ struct tgid_task_context *ttc_array;
the ttc_array sort by tgid
That would be much better! And if you did it that way, *then* my suggestion
of populating the unsorted ttc_array during store_context() would make sense,
i.e., when the tgid is readily available in the "tp" task_struct buffer.
Thanks,
Dave
3:01 a.m.
New subject: improve ps performance
Hello Dave,
I have modified the patch by your idea.
Please check.
Thanks,
Pan
5:01 p.m.
New subject: improve ps performance
----- Original Message -----
Hello Dave,
I have modified the patch by your idea.
Please check.
Thanks,
Pan
Hello Pan,
Thanks for updating this latest patch -- nice work!
I have made a few changes:
(1) a few name changes for clarity's sake,
(2) moved the new task_table members to the end of the structure
so as to not break extension modules that use it,
(3) fixed the malloc() memory leak in get_task_mem_usage(),
(4) fixed the access of the tgid in store_context(), which needs to be
accessed and stored as a pid_t and not a ulong,
(5) added the "help -T" output to show the new fields in the task_table,
(6) and probably some other stuff that I've forgotten...
But for the most part I have left things functionally the same.
I've attached the end result, and will continue testing it tomorrow.
Thanks,
Dave
4:48 p.m.
New subject: improve ps performance
Hello Pan,
I've updated the patch I attached yesterday with a change that
caches the most-recent tgid search result. From ~70% to ~90% of
the time, either the last tgid entry or the very next one in the
tgid_array is the one being searched for, so it's not necessary
to call bsearch() every time. "help -t" will show the cache-hit
statistics.
Thanks,
Dave
3:44 a.m.
New subject: improve ps performance
On 09/11/2014 04:48 AM, Dave Anderson wrote:
Hello Pan,
I've updated the patch I attached yesterday with a change that
caches the most-recent tgid search result. From ~70% to ~90% of
the time, either the last tgid entry or the very next one in the
tgid_array is the one being searched for, so it's not necessary
to call bsearch() every time.
yes, you are right. I have not considered the problem
which
calling bsearch() every time.
thank you very much.
"help -t" will show the cache-hit
statistics.
Thanks,
Dave
3:15 p.m.
New subject: improve ps performance
----- Original Message -----
Hello Pan,
I've updated the patch I attached yesterday with a change that
caches the most-recent tgid search result. From ~70% to ~90% of
the time, either the last tgid entry or the very next one in the
tgid_array is the one being searched for, so it's not necessary
to call bsearch() every time. "help -t" will show the cache-hit
statistics.
Thanks,
Dave
Hello Pan,
This patch as written needs to be made less restrictive for use
on a live system.
When running on a live system that has many tasks constantly
forking/exec'ing, the "ps" command may occasionally fail like so:
crash> ps
PID PPID CPU TASK ST %MEM VSZ RSS COMM
0 0 0 ffffffff81c13440 RU 0.0 0 0 [swapper/0]
0 0 1 ffff88021282d330 RU 0.0 0 0 [swapper/1]
0 0 2 ffff88021282dac0 RU 0.0 0 0
[swapper/2]
0 0 3 ffff88021282e250 RU 0.0 0 0
[swapper/3]
1 0 1 ffff880212828000 IN 0.0 50140 3120 systemd
2 0 3 ffff880212828790 IN 0.0 0 0 [kthreadd]
... [ cut ] ...
7578 27670 0 ffff8801f45e3c80 DE 0.0 0 0 cc
7622 27668 1 ffff880210ee3c80 ZO 0.0 0 0 info
7629 27667 1 ffff8801075bd330 DE 0.0 0 0 rev
7631 27680 0 ffff8801075bf170 ZO 0.0 0 0 printenv
7635 27685 3 ffff880108bbe9e0 ZO 0.0 0 0 ypwhich
ps: bsearch for tgid failed: task: ffff880210ee6250 tgid: 7654
crash>
Without this patch, the search for the matching tgid would not generate
an error at all, but just quietly continue.
The problem is due to the task.tgid may change on a live system, or more
likely, the task itself may have been re-used.
I would like to fix it simply ignoring tgid bsearch failures on live systems,
and just use the RSS stats stored in the per-tgid mm_struct.
Does that work for you?
Dave
7:06 a.m.
New subject: improve ps performance
On 09/20/2014 03:15 AM, Dave Anderson wrote:
----- Original Message -----
> Hello Pan,
>
> I've updated the patch I attached yesterday with a change that
> caches the most-recent tgid search result. From ~70% to ~90% of
> the time, either the last tgid entry or the very next one in the
> tgid_array is the one being searched for, so it's not necessary
> to call bsearch() every time. "help -t" will show the cache-hit
> statistics.
>
> Thanks,
> Dave
Hello Pan,
This patch as written needs to be made less restrictive for use
on a live system.
When running on a live system that has many tasks constantly
forking/exec'ing, the "ps" command may occasionally fail like so:
crash> ps
PID PPID CPU TASK ST %MEM VSZ RSS COMM
0 0 0 ffffffff81c13440 RU 0.0 0 0 [swapper/0]
0 0 1 ffff88021282d330 RU 0.0 0 0 [swapper/1]
> 0 0 2 ffff88021282dac0 RU 0.0 0 0 [swapper/2]
0 0 3 ffff88021282e250 RU 0.0 0 0 [swapper/3]
1 0 1 ffff880212828000 IN 0.0 50140 3120 systemd
2 0 3 ffff880212828790 IN 0.0 0 0 [kthreadd]
... [ cut ] ...
7578 27670 0 ffff8801f45e3c80 DE 0.0 0 0 cc
7622 27668 1 ffff880210ee3c80 ZO 0.0 0 0 info
7629 27667 1 ffff8801075bd330 DE 0.0 0 0 rev
7631 27680 0 ffff8801075bf170 ZO 0.0 0 0 printenv
7635 27685 3 ffff880108bbe9e0 ZO 0.0 0 0 ypwhich
ps: bsearch for tgid failed: task: ffff880210ee6250 tgid: 7654
crash>
Without this patch, the search for the matching tgid would not generate
an error at all, but just quietly continue.
The problem is due to the task.tgid may change on a live system, or more
likely, the task itself may have been re-used.
I would like to fix it simply ignoring tgid bsearch failures on live systems,
and just use the RSS stats stored in the per-tgid mm_struct.
Does that work for you?
Dave
.
ok!
But I don't understand the meaning of "
fix it simply ignoring tgid bsearch failures on live systems,
and just use the RSS stats stored in the per-tgid mm_struct.
", if tgid may be changed, the tgid_array is useless on live systems.
And what is the "RSS stats stored in the per-tgid mm_struct" used for?
More clearly, please.
thanks,
Pan
2:57 p.m.
New subject: improve ps performance
----- Original Message -----
On 09/20/2014 03:15 AM, Dave Anderson wrote:
>
> ----- Original Message -----
>> Hello Pan,
>>
>> I've updated the patch I attached yesterday with a change that
>> caches the most-recent tgid search result. From ~70% to ~90% of
>> the time, either the last tgid entry or the very next one in the
>> tgid_array is the one being searched for, so it's not necessary
>> to call bsearch() every time. "help -t" will show the cache-hit
>> statistics.
>>
>> Thanks,
>> Dave
> Hello Pan,
>
> This patch as written needs to be made less restrictive for use
> on a live system.
>
> When running on a live system that has many tasks constantly
> forking/exec'ing, the "ps" command may occasionally fail like so:
>
> crash> ps
> PID PPID CPU TASK ST %MEM VSZ RSS COMM
> 0 0 0 ffffffff81c13440 RU 0.0 0 0
> [swapper/0]
> 0 0 1 ffff88021282d330 RU 0.0 0 0
> [swapper/1]
> > 0 0 2 ffff88021282dac0 RU 0.0 0 0
> > [swapper/2]
> 0 0 3 ffff88021282e250 RU 0.0 0 0
> [swapper/3]
> 1 0 1 ffff880212828000 IN 0.0 50140 3120 systemd
> 2 0 3 ffff880212828790 IN 0.0 0 0
> [kthreadd]
> ... [ cut ] ...
> 7578 27670 0 ffff8801f45e3c80 DE 0.0 0 0 cc
> 7622 27668 1 ffff880210ee3c80 ZO 0.0 0 0 info
> 7629 27667 1 ffff8801075bd330 DE 0.0 0 0 rev
> 7631 27680 0 ffff8801075bf170 ZO 0.0 0 0 printenv
> 7635 27685 3 ffff880108bbe9e0 ZO 0.0 0 0 ypwhich
> ps: bsearch for tgid failed: task: ffff880210ee6250 tgid: 7654
> crash>
>
> Without this patch, the search for the matching tgid would not generate
> an error at all, but just quietly continue.
>
> The problem is due to the task.tgid may change on a live system, or more
> likely, the task itself may have been re-used.
>
> I would like to fix it simply ignoring tgid bsearch failures on live
> systems,
> and just use the RSS stats stored in the per-tgid mm_struct.
>
> Does that work for you?
>
> Dave
>
>
> .
>
ok!
But I don't understand the meaning of "
fix it simply ignoring tgid bsearch failures on live systems,
and just use the RSS stats stored in the per-tgid mm_struct.
", if tgid may be changed, the tgid_array is useless on live systems.
Well, in this case, it may be true for a particular task if the task struct
had been re-used in between the time the arrays were created and the time
that the "ps" command gets around to reading and displaying its various
statistics. And so the command may read invalid data w/respect to that task.
But let's be clear -- that kind of behavior is, and always has been, an
unavoidable circumstance when running the crash utility on live systems, or
when looking at a "live" dump.
It's not just the "ps" command, but any command that displays data that
is subject to the "shifting sands" syndrome, where the kernel data is
constantly being modified while the crash command is running.
So the idea is to not just cancel the whole command with an error(FATAL...)
if such an anomoly occurs on a live system.
And what is the "RSS stats stored in the per-tgid
mm_struct" used for?
Sorry -- I meant to quietly skip the checking of the other tasks in the
task group, and simply use whatever is stored in the mm_struct pointed to
by the original task. Without your patch, if the tgid was not found, the
command would just continue. With your patch applied, it would be OK
do the error(FATAL) in the case of a static dumpfile. But in the case of
a live system (or live dump), it's not worth killing the command at that
point.
Clear?
Dave
More clearly, please.
thanks,
Pan
4:08 a.m.
New subject: improve ps performance
On 09/21/2014 02:57 AM, Dave Anderson wrote:
----- Original Message -----
> On 09/20/2014 03:15 AM, Dave Anderson wrote:
>> ----- Original Message -----
>>> Hello Pan,
>>>
>>> I've updated the patch I attached yesterday with a change that
>>> caches the most-recent tgid search result. From ~70% to ~90% of
>>> the time, either the last tgid entry or the very next one in the
>>> tgid_array is the one being searched for, so it's not necessary
>>> to call bsearch() every time. "help -t" will show the cache-hit
>>> statistics.
>>>
>>> Thanks,
>>> Dave
>> Hello Pan,
>>
>> This patch as written needs to be made less restrictive for use
>> on a live system.
>>
>> When running on a live system that has many tasks constantly
>> forking/exec'ing, the "ps" command may occasionally fail like so:
>>
>> crash> ps
>> PID PPID CPU TASK ST %MEM VSZ RSS COMM
>> 0 0 0 ffffffff81c13440 RU 0.0 0 0
>> [swapper/0]
>> 0 0 1 ffff88021282d330 RU 0.0 0 0
>> [swapper/1]
>> > 0 0 2 ffff88021282dac0 RU 0.0 0 0
>> > [swapper/2]
>> 0 0 3 ffff88021282e250 RU 0.0 0 0
>> [swapper/3]
>> 1 0 1 ffff880212828000 IN 0.0 50140 3120 systemd
>> 2 0 3 ffff880212828790 IN 0.0 0 0
>> [kthreadd]
>> ... [ cut ] ...
>> 7578 27670 0 ffff8801f45e3c80 DE 0.0 0 0 cc
>> 7622 27668 1 ffff880210ee3c80 ZO 0.0 0 0 info
>> 7629 27667 1 ffff8801075bd330 DE 0.0 0 0 rev
>> 7631 27680 0 ffff8801075bf170 ZO 0.0 0 0 printenv
>> 7635 27685 3 ffff880108bbe9e0 ZO 0.0 0 0 ypwhich
>> ps: bsearch for tgid failed: task: ffff880210ee6250 tgid: 7654
>> crash>
>>
>> Without this patch, the search for the matching tgid would not generate
>> an error at all, but just quietly continue.
>>
>> The problem is due to the task.tgid may change on a live system, or more
>> likely, the task itself may have been re-used.
>>
>> I would like to fix it simply ignoring tgid bsearch failures on live
>> systems,
>> and just use the RSS stats stored in the per-tgid mm_struct.
>>
>> Does that work for you?
>>
>> Dave
>>
>>
>> .
>>
> ok!
> But I don't understand the meaning of "
>
> fix it simply ignoring tgid bsearch failures on live systems,
> and just use the RSS stats stored in the per-tgid mm_struct.
>
> ", if tgid may be changed, the tgid_array is useless on live systems.
Well, in this case, it may be true for a particular task if the task struct
had been re-used in between the time the arrays were created and the time
that the "ps" command gets around to reading and displaying its various
statistics. And so the command may read invalid data w/respect to that task.
But let's be clear -- that kind of behavior is, and always has been, an
unavoidable circumstance when running the crash utility on live systems, or
when looking at a "live" dump.
It's not just the "ps" command, but any command that displays data that
is subject to the "shifting sands" syndrome, where the kernel data is
constantly being modified while the crash command is running.
So the idea is to not just cancel the whole command with an error(FATAL...)
if such an anomoly occurs on a live system.
> And what is the "RSS stats stored in the per-tgid mm_struct" used for?
Sorry -- I meant to quietly skip the checking of the other tasks in the
task group, and simply use whatever is stored in the mm_struct pointed to
by the original task. Without your patch, if the tgid was not found, the
command would just continue. With your patch applied, it would be OK
do the error(FATAL) in the case of a static dumpfile. But in the case of
a live system (or live dump), it's not worth killing the command at that
point.
Clear?
Dave
> More clearly, please.
> thanks,
> Pan
>
.
Oh, Ok.
Can I do it like this patch which just ignore tgid besarch failures?
4:34 p.m.
New subject: improve ps performance
----- Original Message -----
... [ cut ] ...
Oh, Ok.
Can I do it like this patch which just ignore tgid besarch failures?
Sure, that works.
I was going to suggest keeping the error message for non-live dumpfiles,
but since it's impossible for a bsearch failure in that case, and because
this update maintains behavior compatibility with pre-crash-7.0.8, I agree
with you that it's just not worth having it.
The patch is queued for crash-7.0.9:
https://github.com/crash-utility/crash/commit/a3a441aeabd6c5c3c86b4793a28...
Thanks,
Dave
3762
days inactive
3776
days old
devel@lists.crash-utility.osci.io
9 comments
2 participants
participants (2)
-
Dave Anderson -
panfy.fnst