lkml.org 
[lkml]   [2008]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: perfmon3 interface overview
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
> <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.

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


\
 
 \ /
  Last update: 2008-10-07 06:05    [W:0.103 / U:36.908 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site