David, On Fri, Oct 3, 2008 at 8:17 AM, David Gibson <[EMAIL PROTECTED]> wrote: > > > The new API now has 8 system calls in its fully-featured > > version. Many data structures shared with user level have been > > abandoned in favor of explicit syscall parameters. Each syscall has > > Ah, excellent, big structures passed through an interface are often a > pain to deal with, so I'm glad these are going.
Thanks. > > > I, at least, like the changes that have been made here. I do have a > few suggestions for further simplifications along the same lines > though, see below. > > [snip] > > II) programming the registers > > > > With v2.81: > > int pfm_write_pmcs(int fd, pfarg_pmc_t *pmds, int n); > > int pfm_write_pmds(int fd, pfarg_pmd_t *pmcs, int n); > > int pfm_read_pmds(int fd, parg_pmd_t *pmds, int n); > > > > With v3.0: > > int pfm_write_pmrs(int fd, int flags, pfarg_pmr_t *pmrs, int n); > > int pfm_write_pmrs(int fd, int flags, pfarg_pmr_t *pmrs, int n, > > pfarg_pmd_attr_t *pmas); > > > > int pfm_read_pmrs(int fd, int flags, pfarg_pmr_t *pmrs, int n); > > int pfm_read_pmrs(int fd, int flags, parg_pmr_t *pmrs, int n, > > pfarg_pmd_attr_t *pmas); > > I would suggest adding 'type' and 'size' (int) parameters, and folding > the pmrs and pmas arrays into one freeform array. 'size' gives the > size in bytes of the array elements, 'type' selects the structure of > each array element. This means: > - if you ever have some sort of registers to access that are > neither PMCs nor PMDs, you can access those two, using a new type > value. > - if a new method of operation wants to supply different > information for PMCs or PMDs, you can define a new structure and type > value (one obvious example would be a minimal (num,value) only > structure for minimum latency on simple applications). > - an explicit size value means things can marshal / copy / > whatnot the parameters without having to know what size each type > implies - the code will stay valid when new types / structures are > defined. So you are suggesting something along the lines: int pfm_read_pmrs(int fd, int flags, int type, void *tab, size_t sz); int pfm_write_pmrs(int fd, int flags, int type, void *tab, size_t sz); I have already introduced a type flag (PFM_RWFL_PMD, PFM_RWFL_PMC). Why separate the type into its own parameter? As for the freeform array, isn't that what people do not like because of void * and thus weak type checking? I will look at switching to size instead of count. I think it does make sense. > > [snip] > > III) attaching and detaching > > > > With v2.81: > > int pfm_load_context(int fd, pfarg_load_t *load); > > int pfm_unload_context(int fd); > > > > With v3.0: > > int pfm_attach_session(int fd, int flags, int target); > > int pfm_detach_session(int fd, int flags); > > Couldn't you get rid of one more syscall here by making detach a > special case of attach with a special "null" value for target, or a > special flag? We could combine the two and use the flag field to indicate attach/detach. The target is not a pointer but an int. Some people suggested I use an unsigned long instead. In anycase, we could not use 0 to indicate "detach" because CPU0 is a valid choice for system-wide. Thus we would have to pick another value to mean "nothing", e.g, -1. > IV) starting and stopping > > With v2.81: > int pfm_start(int fd, pfarg_start_t *st); > int pfm_stop(int fd); > int pfm_restart(int fd); > > With v3.0: > int pfm_start_session(int fd, int flags); > int pfm_stop_session(int fd, int flags); > Likewise, couldn't you cut this down by one more syscall by making it > int pfm_set_session_state(int fd, int flags); > and having a 'RUNNING' flag, which selects start or stop behaviour? That one we can certainly do. That's a good idea. Thanks for your feedback, keep it coming. ------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ _______________________________________________ perfmon2-devel mailing list perfmon2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
