11:01 a.m.
Upon a quick examination of the latest patchset posted, this will
break pre-compiled extension modules that walk through the task list:
--- a/crash-7.0.7/defs.h
+++ b/crash-7.0.7/defs.h
@@ -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;
};
Because it is typically done like so:
struct task_context *tc;
tc = FIRST_CONTEXT();
for (i = 0; i < RUNNING_TASKS(); i++, tc++) {
...
Because it's an array, it's not safe to just move the new fields to the
end of the data structure (as is the case with the program_context struct).
Just checking the extension modules that are posted on the
http://people.redhat.com/anderson web site, it would break 3 of them:
$ grep RUNNING_TASKS *.c
ksm.c: for (i = 0; i < RUNNING_TASKS(); i++, tc++) {
swap_usage.c: for (i = 0; i < RUNNING_TASKS(); i++, tc++) {
trace.c: for (i = 0; i < RUNNING_TASKS(); i++)
trace.c: for (i = 0; i < RUNNING_TASKS(); i++)
$
I haven't studied how this patchset works, but I wonder whether this
could be avoided?
It's not a set-in-stone rule that these kind of changes can never be
made. But on the other hand, I don't believe that structure has ever
been changed -- because anything about a task can be determined
directly or indirectly from its contents.
Dave
----- Original Message -----
--
Crash-utility mailing list
Crash-utility(a)redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
3:11 p.m.
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
3:23 p.m.
----- Original Message -----
Unfortunately I can't see a obvious way to remove the new
tc->tgid_context_index field...
Perhaps something like this...
Coming into get_task_mem_usage() function, you could get the
tgid of the incoming task by calling task_tgid().
Since the tgid_task_context[] array is sorted by tgid number,
perhaps a binary search of the array could quickly find
the associated entry (or one of the entries if multi-threaded)?
That shouldn't be a very time-consuming process.
Dave
3585
days inactive
3596
days old
devel@lists.crash-utility.osci.io
3 comments
2 participants
participants (2)
-
Dave Anderson -
panfy.fnst