On Sun, Oct 05, 2008 at 11:23:15PM +0200, stephane eranian wrote:
> David,
>
> On Sun, Oct 5, 2008 at 7:53 AM, David Gibson
> <[EMAIL PROTECTED]> wrote:
> > [snip]
> >> 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);
> >
> > Uh.. maybe.. there are actually several possible variants all of which
> > would meet the general idea I had in mind.
> >
> >> I have already introduced a type flag (PFM_RWFL_PMD, PFM_RWFL_PMC).
> >> Why separate the type into its own parameter?
> >
> > Combining the type into the flags is certainly a possibility. I was
> > just a bit worried that if you eventually have enough actual flag
> > flags, in addition to the type values, you might start running out of
> > bits.
> >
> I see, so you were just decoupling to double the number of available bits.
> I guess we have a minimum of 32 bits. It seems overkill to support up to
> 32 'types'. We could force flags to uint64_t. But then we would have to do
> the same for all other syscalls to be consistent. Defining flags as long
> won't work because of 32-bit vs. 64-bit ABI compatibility issues.
I guess so. I guess there's also the possibility that you might later
have some flags that can apply to many of the syscalls, and it would
be nice to have them in a standard bit position for all calls. That
might make bit allocation a bit interesting if you're squeezing type
into the same field.
My opinion is not really strong either way though.
> >> As for the freeform array, isn't that what people do not like because
> >> of void *
> >> and thus weak type checking?
> >
> > Yeah; this is an interesting tradeoff of flexibility versus
> > predictability. Actually, what I originally had in mind was
> > seperately passing both 'n' and a per-element size. That provides a
> > bit more defined structure to the void * - it must be an array of n
> > identical, fixed-size elements. The internal structure of each
> > element is defined only by type, but it is assumed to contain no
> > pointers to further chained structures (i.e. its safe for wrapper code
> > to do shallow copies of the array).
>
> Ok, that reminds me of the API of calloc(). It certainly forces you to
> think it terms of 'array of elements'. Yet it can be perverted very easily
> with the number of elements at 1.
That's true. Like I say, it's a tradeoff.
> >> I will look at switching to size instead of count. I think it does
> >> make sense.
> >
> > Yeah. As I said I was originally thinking of keeping the 'n'
> > parameter, and adding an element size parameter. But just having an
> > overall size is also a possibility - it gives less definition to the
> > internal structure but at least things can marshal or copy the whole
> > array parameter without having to know its internals.
> >
> Yes, that it true. It also simplifies the checking on size. With count
> we had to check for multiply overflow because internall we had to
> check the size anyway.
That's a nice extra bonus, yes.
> > [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.
> >
> > Sorry, I didn't express myself well. I realise it's an integer, and
> > didn't mean an actual NULL, but as you say a special integer value
> > with a function similar to NULL. -1 is the most obvious choice.
> >
> Yes, -1 would work.
>
> If I summarize our discussion. It seems we can define the API as follows:
>
> int pfm_create_session(int fd, uint64_t flags, pfarg_sinfo_t *sif,
> [ char *smpl_name, void *smpl_arg, size_t arg_size]);
> int pfm_read_pmrs(int fd, uint64_t flags, void *tab, size_t sz);
> int pfm_write_pmrs(int fd, uint64_t flags, void *tab, size_t sz);
> int pfm_attach_session(int fd, uint64_t flags, int target); /*
> attach, detach with target=-1 */
> int pfm_control_session(int fd, uint64_t flags); /* for start/stop */
Unless you have other functions in mind for this, I'd suggest a name
that's a little less generic here. pfm_set_running() maybe.
> int pfm_control_sets(int fd, uint64_t flags, void *sets, size_t
> sz);
Hrm. This now has identical signature to the {read,write}_pmrs. I
wonder if these could be sensibly combined. Maybe have
pfm_get_info(), pfm_set_info() which can get/set control structures
for several different namespaces: PMCs PMDs and now event sets.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
-------------------------------------------------------------------------
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
