----- 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
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
