From: Ka F. L. <kaf...@ya...> - 2003-06-20 04:55:06
|
It is my 2nd try on thread profiling and it is for 2.5. The attached do_fork_notify() is called in do_fork(). Any comment? Regards, KaFai static do_fork_notify(struct notifier_block * self, unsigned long val, void *data) { struct task_struct * p = (struct task_struct *)data; struct mm_struct * mm; unsigned long cookie; add_event_entry(ESCAPE_CODE); add_event_entry(DO_FORK_CODE); add_event_entry(p->tgid); add_event_entry(p->pid); mm = take_task_mm(p); cookie = get_exec_dcookie(mm); release_mm(mm); add_event_entry(cookie); wake_up_buffer_waiter(); return 0; } __________________________________ Do you Yahoo!? SBC Yahoo! DSL - Now only $29.95 per month! http://sbc.yahoo.com |
From: John L. <le...@mo...> - 2003-06-20 14:43:46
|
On Thu, Jun 19, 2003 at 09:55:06PM -0700, Ka Fai Lau wrote: > It is my 2nd try on thread profiling and it is for > 2.5. Hi Ka. > The attached do_fork_notify() is called in do_fork(). We absolutely do *not* need further kernel-side changes. We already have all the context switch information we need. In fact the changes you need to make now should be pretty simple. Open daemon/opd_image.c. Near the bottom you will find a set of functions which deal with a struct transient. Inside this struct is recorded the current task's task->pid and task->tgid. Now, when we log a sample with opd_put_sample, we will eventually open a sample file by calling into the opd_sample_files.c stuff. All you need to do is export this information straight into that code, then add MANGLE_TGID/PID flags, and pass the right values inside the mangle_values struct. I think it's best to do that by passing a struct transient directly to opd_open_sample_file. Of course, you should also add this as an option to opcontrol --separate. You can trivially do per-cpu separation at the same time. Please ask if you have any further questions regarding this. regards, john |
From: Ka F. L. <kaf...@ya...> - 2003-06-21 23:37:39
|
> > The attached do_fork_notify() is called in > do_fork(). > > We absolutely do *not* need further kernel-side > changes. We already have > all the context switch information we need. In fact > the changes you need > to make now should be pretty simple. Great!. will do. I was looking at an old kernel. I think I have to update my source more often KaFai __________________________________ Do you Yahoo!? SBC Yahoo! DSL - Now only $29.95 per month! http://sbc.yahoo.com |
From: Ka F. L. <kaf...@ya...> - 2003-08-26 05:58:50
Attachments:
opd_25_thread.patch
|
Attached file is the patch for daemon/ and utils/opcontrol. Comments? There is no support on opreport yet, any direction or throught? Regards, KaFai __________________________________ Do you Yahoo!? Yahoo! SiteBuilder - Free, easy-to-use web site design software http://sitebuilder.yahoo.com |
From: John L. <le...@mo...> - 2003-08-26 16:05:42
|
On Mon, Aug 25, 2003 at 10:58:34PM -0700, Ka Fai Lau wrote: > Attached file is the patch for daemon/ and > utils/opcontrol. Comments? Thanks for working on this, it looks like a step in the right direction. See comments below. > There is no support on opreport yet, any direction or > throught? Well, you can still specify an exact profile to show with "tgid:454" or whatever. But yes, there's more work needed on the merging and side-by-side reports code before it works satisfactorily. Let's focus on that after we've got the first part committed. Comments : diff -ur /root/oprofile/daemon/opd_image.c oprofile-thread/daemon/opd_image.c --- /root/oprofile/daemon/opd_image.c 2003-08-07 17:52:56.000000000 -0700 +++ oprofile-thread/daemon/opd_image.c 2003-08-25 22:17:57.000000000 -0700 @@ -32,6 +32,7 @@ extern uint op_nr_counters; extern int separate_lib_samples; extern int separate_kernel_samples; +extern int thread_profiling; First, I don't like that you have two thread profiling modes. I don't think it's that useful. We should just have one mode : opcontrol --separate=thread That will always fill in the exact tgid and thread id. And it should look like the others, i.e. "separate_thread_samples" etc. static void opd_init_image(struct opd_image * image, cookie_t cookie, - cookie_t app_cookie) + cookie_t app_cookie, pid_t pid, pid_t tgid) Things are starting to get a bit ugly here (and will be worse when we add CPU separation too). You mention this yourself later in the patch. Can we always use "tid" for the thread ID, and "tgid" for the thread group ID please ? -void opd_put_image_sample(struct opd_image * image, vma_t offset, int counter) +void opd_put_image_sample(struct opd_image * image, vma_t offset, int counter) Spurious whitespace change ? + if (thread_profiling == 1) { + if (image->tgid != tgid) + continue; + } else if(thread_profiling == 2) { Please be careful to use the spacing consistently. There should always be a space after an if keyword. + if(app_image) { Here too. + * TODO: handle kernel thread. i.e. app_image is NULL + * how about passing in the struct transients or even make global and refactor + * some members from struct opd_image to struct transients? Yes, refactoring is what's needed. One suggestion would be : struct opd_image_descr { pid_t tid; pid_t tgid; /* later on cpu_t cpu; */ cookie_t cookie; cookie_t app_cookie; }; That would be a sub-member of struct transient for filling in, and all the opd_whatever_image() would take a single argument struct opd_image_descr *. Does that make sense to you ? Would you consider making this change ? --- /root/oprofile/utils/opcontrol 2003-08-11 16:54:54.000000000 -0700 +++ oprofile-thread/utils/opcontrol 2003-08-21 22:00:25.000000000 -0700 @@ -95,6 +95,7 @@ --kernel-only=[0|1] profile only the kernel (2.4 version) --note-table-size number of notes in kernel notes buffer (2.4 version) -p/--separate=[none,library,kernel,all] separate samples as given + --thread-profiling=[0|1|2] thread profiling mode As said above, this should be a --separate=thread option IMHO. thanks, john |
From: Ka F. L. <kaf...@ya...> - 2003-08-27 05:55:53
|
> think it's that useful. We should just have one mode > : > opcontrol --separate=thread Will do > struct opd_image_descr { > pid_t tid; > pid_t tgid; > /* later on cpu_t cpu; */ > cookie_t cookie; > cookie_t app_cookie; > }; > > That would be a sub-member of struct transient for > filling in, and all > the opd_whatever_image() would take a single > argument struct > opd_image_descr *. Does that make sense to you ? > Would you consider > making this change ? Sure! I can make this change. Yes, I think it is a good idea to put all id info into "struct opd_image_descr". Actually, I was thinking to create a global variable of "struct transient" and all functions use it instead of passing the same arguement around. Let say we don't want a global transient and the following is what I have in mind. Comments? -- KaFai /* uniquely identify an image */ struct opd_image_descr { pid_t pid; pid_t tgid; cookie_t cookie; cookie_t app_cookie; }; /* app_name and app_image are removed */ struct opd_image { /* name of this image */ char * name; /* descriptor that uniquely identify this image */ struct opd_image_descr descr; /* hash table link */ struct list_head hash_list; /* opened sample files */ samples_odb_t sample_files[OP_MAX_COUNTERS]; /* time of last modification */ time_t mtime; /* kernel image or not */ int kernel; /* used when separate_kernel_samples != 0, a linked list of module */ struct list_head module_list; }; /* cookie and app_cooie are removed since they can be obtained from lib_image or app_image */ struct transient { char const * buffer; size_t remaining; struct opd_image * lib_image; /* it always points to the current image (including kernel image)*/ struct opd_image * app_image; int in_kernel; unsigned long cpu; pid_t tid; pid_t tgid; }; /* those are the changes on function declaration */ /* user space image functions */ static struct opd_image * opd_get_image(struct opd_image_descr const * descr); static struct opd_image * opd_add_image(struct opd_image_descr const * descr); static struct opd_image * opd_find_image(struct opd_image_descr const * descr); /* kernel image or module functions */ /* FIXME: "struct transient * trans" instead of app_image because kernel thread can have app_image==NULL and it will stop thread_profiling for kernel thread */ /* no const because it will assign the kernel iamge to trans->lib_image */ struct opd_image * opd_get_kernel_image(struct transient * trans, char const * name); static struct opd_image * opd_add_kernel_image(char const * name); struct opd_image * opd_find_kernel_image(struct transient const * trans, vma_t * eip); /* opd_sample_files.c */ /* since the struct opd_image doesn't have the app_name anymore, we should pass struct transient instead */ __________________________________ Do you Yahoo!? Yahoo! SiteBuilder - Free, easy-to-use web site design software http://sitebuilder.yahoo.com |
From: John L. <le...@mo...> - 2003-08-27 13:59:12
|
On Tue, Aug 26, 2003 at 10:55:52PM -0700, Ka Fai Lau wrote: > thinking to create a global variable of "struct transient" and all > functions use it instead of passing the same arguement around. I think this is better after looking around the source. We should pass struct transient around. Ideally there would be a better name for it though ... > /* cookie and app_cooie are removed since they can be obtained from lib_image or app_image */ Have you checked this with and without --separate=kernel/lib ? I think it should be OK, but ... regards john -- Khendon's Law: If the same point is made twice by the same person, the thread is over. |
From: John L. <le...@mo...> - 2003-09-14 22:26:04
|
On Mon, Aug 25, 2003 at 10:58:34PM -0700, Ka Fai Lau wrote: > Attached file is the patch for daemon/ and > utils/opcontrol. Comments? > > There is no support on opreport yet, any direction or > throught? FYI, work over this weekend has meant that opreport/opannotate should be able to handle thread-split profiles perfectly well now. So it's just a matter of the daemon/ changes now. How is the latest patch going ? :) regards john -- Khendon's Law: If the same point is made twice by the same person, the thread is over. |
From: Philippe E. <ph...@wa...> - 2003-09-17 02:49:37
|
Ka Fai Lau wrote: > Attached file is the patch for daemon/ and > utils/opcontrol. Comments? > > There is no support on opreport yet, any direction or > throught? I updated and applied your patch to current cvs, user space tools has been updated. I was happy to port it, except a thinko I did when updated the code it worked at the first try :) Regards, Phil |
From: Philippe E. <ph...@wa...> - 2003-09-17 03:01:39
|
Philippe Elie wrote: > Ka Fai Lau wrote: > >> Attached file is the patch for daemon/ and >> utils/opcontrol. Comments? >> >> There is no support on opreport yet, any direction or >> throught? > > > I updated and applied your patch to current cvs, user space > tools has been updated. > > I was happy to port it, except a thinko I did when updated > the code it worked at the first try :) I forget to say we intend to rework a lot of code in 2.6 daemon in the next days regards, Phil |
From: Ka F. L. <kaf...@ya...> - 2003-10-01 05:44:08
|
Sorry. The yahoo email has mess up the read and non-read email somehow....so I missed a lot. It took me longer than expected since a lot of things happened in my company last 2 months Actually, I have finished the rework or refactor last week and prepare to create a diff from the latest CVS. I found out that it was done already... Let me look around and see what else I can help. Is there any plan to improve the GUI? KaFai --- Philippe Elie <ph...@wa...> wrote: > Philippe Elie wrote: > > Ka Fai Lau wrote: > > > >> Attached file is the patch for daemon/ and > >> utils/opcontrol. Comments? > >> > >> There is no support on opreport yet, any direction or > >> throught? > > > > > > I updated and applied your patch to current cvs, user space > > tools has been updated. > > > > I was happy to port it, except a thinko I did when updated > > the code it worked at the first try :) > > I forget to say we intend to rework a lot of code in 2.6 > daemon in the next days > > regards, > Phil > > > > > > > > ------------------------------------------------------- > This sf.net email is sponsored by:ThinkGeek > Welcome to geek heaven. > http://thinkgeek.com/sf > _______________________________________________ > oprofile-list mailing list > opr...@li... > https://lists.sourceforge.net/lists/listinfo/oprofile-list __________________________________ Do you Yahoo!? The New Yahoo! Shopping - with improved product search http://shopping.yahoo.com |
From: John L. <le...@mo...> - 2003-10-01 11:43:43
|
On Tue, Sep 30, 2003 at 10:43:15PM -0700, Ka Fai Lau wrote: > Sorry. The yahoo email has mess up the read and non-read email > somehow....so I missed a lot. Ah :( > Actually, I have finished the rework or refactor last week and prepare to create a diff from the > latest CVS. I found out that it was done already... Yep, Phil took your patch and made some changes, and applied it. > Let me look around and see what else I can help. Is there any plan to > improve the GUI? What sort of improvements do you have in mind ? regards john -- Khendon's Law: If the same point is made twice by the same person, the thread is over. |
From: Ka F. L. <kaf...@ya...> - 2003-10-03 06:20:08
|
> What sort of improvements do you have in mind ? The current gui is for starting and stopping the oprofile. I am thinking to add analysis functions like opreport or is it already in? Together with the call graph function, it will be easier to use What do u think? KaFai __________________________________ Do you Yahoo!? The New Yahoo! Shopping - with improved product search http://shopping.yahoo.com |
From: Philippe E. <ph...@wa...> - 2003-10-03 14:25:09
|
Ka Fai Lau wrote: >>What sort of improvements do you have in mind ? > > The current gui is for starting and stopping the oprofile. I am thinking to add analysis > functions like opreport or is it already in? Together with the call graph function, it will be > easier to use > > What do u think? We had a sort of gui for analysis in old oprofile version and we remove it. There is missing part in base tools for call graph analysis namely only opgprof can handle call graph. Actually call graph can't handle arc between application and shared libs. regards, Phil |
From: Ka F. L. <kaf...@ya...> - 2003-10-03 17:53:21
|
> We had a sort of gui for analysis in old oprofile version and > we remove it. Why it is removed? because the structure of sample file is removed? > There is missing part in base tools for call > graph analysis namely only opgprof can handle call graph. so only opgprof can handle call graph data now? > Actually call graph can't handle arc between application and > shared libs. Do you mean it cannot handle a stack that is mixed with app and shared lib eip? how about kernel? can you explain a bit more. Regards, KaFai __________________________________ Do you Yahoo!? The New Yahoo! Shopping - with improved product search http://shopping.yahoo.com |
From: Philippe E. <ph...@wa...> - 2003-10-03 19:10:59
|
Ka Fai Lau wrote: >>We had a sort of gui for analysis in old oprofile version and >>we remove it. > > Why it is removed? because the structure of sample file is removed? It was no longer maintained and not enough powerfull. >>There is missing part in base tools for call >>graph analysis namely only opgprof can handle call graph. > > so only opgprof can handle call graph data now? yes. > > >>Actually call graph can't handle arc between application and >>shared libs. > > Do you mean it cannot handle a stack that is mixed with app and shared lib eip? how about kernel? > can you explain a bit more. daemon receive this information but it's a bit tricky to find a good way to store them (iirc John got an idea to solve this problem). regards, Phil |
From: John L. <le...@mo...> - 2003-10-03 16:38:22
|
On Thu, Oct 02, 2003 at 11:20:05PM -0700, Ka Fai Lau wrote: > > What sort of improvements do you have in mind ? > The current gui is for starting and stopping the oprofile. I am thinking to add analysis > functions like opreport or is it already in? Together with the call graph function, it will be > easier to use This is a *massive* project to do properly ... it would take a lot of work to get it to the sort of thing I want to see ... regards john -- Khendon's Law: If the same point is made twice by the same person, the thread is over. |