Re: perfmon3 interface overview

["stephane eranian" <eranian@googlemail.com>]

David,

On Sun, Oct 5, 2008 at 7:53 AM, David Gibson
<david@gibson.dropbear.id.au> 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.




>>  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.

>>  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.

> [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 */
    int pfm_control_sets(int fd, uint64_t flags, void *sets, size_t sz);

Does this look reasonable?