From: Maynard J. <may...@us...> - 2011-10-20 16:28:39
|
Andreas Krebbel wrote: > On 10/19/2011 09:44 PM, Maynard Johnson wrote: >> This patch seems way more intrusive than it needs to be. Please look at how >> support for other new processor types have been added to oprofile -- for >> example, the recently contributed tile<x> support. When contributing new >> processor support for oprofile, we expect corresponding oprofile kernel code to >> fit the model that oprofile userspace needs. A lot of the changes I see below >> are the result of your oprofile kernel module not communicating with oprofile >> userspace in the standard form. > > When the kernel part has been developed the target was to keep it working with existing > user space. By using the timer mode sampling interface the usage of hardware sampling is > fully transparent (actually too transparent) and does not depend on a specific OProfile > user space version. That design decision looked pretty reasonable to me until I inherited > the task to adjust the user space for better usability :( I agree that using hardware > events probably would have allowed for a smoother integration. The user space here > suffers a lot from the design decision made for the kernel part. > > Unfortunately I don't have the impression the kernel guys are willing to touch the > OProfile hardware sampling support again. There seems to be much higher demand to support > perf instead and we will most likely rather go that direction. > > So I'm somehow stuck with the interface I have. The patch therefore needs a lot of > special cases for s390 in order to make everything look like expected. Andreas, OK, I think I over-stated the case that your userspace-to-kernel communication was non-standard. I don't think it's a big issue that you write to a unique file (hwsampler) in oprofilefs to enable/disable hardware sampling. I'm quite confident that if you create the events/s390/events file with the two pseudo events (one for h/w sampling and one for time-based sampling), you can avoid a lot of the special case code, the expansion of 'struct cpu_descr', and the new opcontrol options. Instead, use CPU_TYPE (and, when necessary) the chosen event to to execute unique behavior. > >> Also, is there a reason why the existing 'buffer-size' option cannot be used >> instead of the new 's390hwsampbufsize' option? > > buffer-size and s390hwsampbufsize have different semantics for timer base sampling and > hardware sampling. For hardware sampling this value is the number of 2MB areas which > apart from samples also contain control data structures. So they cannot easily be mapped > to each other. Can't we document that in the man page for opcontrol and then handle the difference in the code (again, using CPU_TYPE and chosen event to select proper behavior)? I *really* don't want to add new options if we can somehow avoid it. > >> One final comment . . . There are a number of superflous changed lines that are >> only whitespace changes. Please avoid that. > > The only thing I can see are some empty lines I've intentionally added to separate code > snippets. I've tried to match the already used coding style regarding this. But the > number can certainly be reduced. See the changes to is_non_cell_ppc64_variant() . . . all lines are shifted right. Feel free to ping me on IRC if you have any questions. -Maynard > > Bye, > > -Andreas- |