From: Philippe E. <ph...@cl...> - 2001-12-30 01:44:13
|
hi, TODO file: >> o prepare for rtc: >> - move apic init to module init what you means? Apic init are already marked as __init function. I've not looked implementation of init/exit section but lvpt_apic_restore() is marked __exit and is called from oprof_init() which is marked as __init. Is it safe ? >> - remove user-space proc type code - so op_start can check parameters only after probing the module because op_events need to know the proc type. It is not probably a problem but - op_help is usable as a standalone programm to see all the events so we need to keep the user space proc type code >> o determine where we need ctx_sw_on/off to prevent pre-emption + test Hummm, I'm probably unable to make that even if I understand what it means. >> o move .oprofile to name=value pairs to avoid conflicts on change. There is a "thing" for that in util/persistent.h and an example test in oprofile-tests/understanting/persistent-test.cpp, but I don't like the output format I use. I hate persistency of C++ object... phil |
From: John L. <le...@mo...> - 2001-12-30 13:42:24
|
On Sun, Dec 30, 2001 at 02:40:56AM +0100, Philippe Elie wrote: > TODO file: > > >> o prepare for rtc: > >> - move apic init to module init > > what you means? Apic init are already marked as __init function. we need to make the init part of the CPU type detection, i.e. so we can return CPU_NOAPIC and export that in a sysctl for the GUI etc. on module load. > I've not looked implementation of init/exit section but > lvpt_apic_restore() is marked __exit and is called from > oprof_init() which is marked as __init. Is it safe ? Theoretically no ... please remove the __exit (and any others like that) > >> - remove user-space proc type code > > - so op_start can check parameters only after probing the module > because op_events need to know the proc type. It is not probably > a problem but > > - op_help is usable as a standalone programm to see all the events so we > need to keep the user space proc type code ok. The point is user-space cannot determine if we have a good local APIC without help from the module. The user-space detect code can do a "best-guess" attempt if it can't find /proc/sys/dev/oprofile/cpu_type or whatever, but this is for op_help only. op_start and oprof_start should load the module first. > >> o determine where we need ctx_sw_on/off to prevent pre-emption + test > > Hummm, I'm probably unable to make that even if I understand what it > means. There might be some points in the code that would break a kernel with the pre-emption patch applied (look for Robert Love under http://kernelnewbies.org/patches/). Not sure though, I've been too lazy to even have a cursory look at our code for this. > >> o move .oprofile to name=value pairs to avoid conflicts on change. > > There is a "thing" for that in util/persistent.h and an example test in > oprofile-tests/understanting/persistent-test.cpp, but I don't like the > output format I use. I hate persistency of C++ object... I have to admit I've still not looked properly at any of the things you've been doing recently. I really should. Unfortunately I *have* totally goosed my distribution it seems - oprof_start still core dumps for me :/ regards john -- "We're standing there pounding a dead parrot on the counter, and the management response is to frantically swap in new counters to see if that fixes the problem." - Peter Gutmann |
From: Philippe E. <ph...@cl...> - 2001-12-30 21:16:39
|
hi, Dave, John: I prefer a confirmation: module/op_x86.c static int __init check_p6_ok(void) { if (cpu_type != CPU_PPRO && cpu_type != CPU_PII && cpu_type != CPU_PIII) return 1; return enable_apic(); } should add : && cpu_type != CPU_ATHLON) return 1; As the code is written if !defined CONFIG_X86_UP_APIC (see apics_need_setup() and apic_setup()) then athlon can not run the profiler ? ***** John, if we want to get cpu type from the module to check against valid IO apic it means than modules remains loaded even if the initialization code lose. Do you like this? I agree with taking cpu type from the module but I'm unsure we must allow the module remaining in memory even in case of error. I prefer than oprof_start/op_start just try to load module and consider as fatal to fail loading module. User would rely on dmesg to check what was bad. So this change increase code size in module and in user space (but make a more reliable way to detect cpu). Ok ? reagards, phil |
From: John L. <le...@mo...> - 2001-12-31 03:35:00
|
On Sun, Dec 30, 2001 at 10:13:54PM +0100, Philippe Elie wrote: > I prefer a confirmation: module/op_x86.c > > static int __init check_p6_ok(void) > { > if (cpu_type != CPU_PPRO && > cpu_type != CPU_PII && > cpu_type != CPU_PIII) > return 1; > > return enable_apic(); > } > > should add : > > && cpu_type != CPU_ATHLON) > return 1; > > As the code is written if !defined CONFIG_X86_UP_APIC (see apics_need_setup() > and apic_setup()) then athlon can not run the profiler ? Dave ? Looks odd to me - anyone ever tested athlon in this case ?? We should add acomment above this if() describing what it is checking - it is non-obvious. > John, if we want to get cpu type from the module to check against valid IO apic > it means than modules remains loaded even if the initialization code lose. Do > you like this? I do actually. > I agree with taking cpu type from the module but I'm unsure we must allow the > module remaining in memory even in case of error. I prefer than oprof_start/op_start > just try to load module and consider as fatal to fail loading module. User would rely > on dmesg to check what was bad. So this change increase code size in module > and in user space (but make a more reliable way to detect cpu). Ok ? I'm unconvinced. Ideally the module should only fail to /load/ when there is another problem (wrong kernel version etc.). After the RTC patches, there won't be any actual failure cases for oprofile (people trying silly things like building on an alpha won't get past configure anyhoo). regards john -- "We're standing there pounding a dead parrot on the counter, and the management response is to frantically swap in new counters to see if that fixes the problem." - Peter Gutmann |