lkml.org 
[lkml]   [2009]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [announce] Performance Counters for Linux, v6
Date
On Wednesday 21 January 2009, Ingo Molnar wrote:

> diff --git a/arch/powerpc/include/asm/systbl.h b/arch/powerpc/include/asm/systbl.h
> index 72353f6..4c8095f 100644
> --- a/arch/powerpc/include/asm/systbl.h
> +++ b/arch/powerpc/include/asm/systbl.h
> @@ -322,3 +322,4 @@ SYSCALL_SPU(epoll_create1)
> SYSCALL_SPU(dup3)
> SYSCALL_SPU(pipe2)
> SYSCALL(inotify_init1)
> +SYSCALL(perf_counter_open)

Is there a reason to forbid this on the SPU? It's probably not particularly
useful, but it shouldn't hurt.

> +
> +#ifdef CONFIG_PERF_COUNTERS
> +# include <asm/perf_counter.h>
> +#endif
> +
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/rculist.h>
> +#include <linux/rcupdate.h>
> +#include <linux/spinlock.h>

I guess all */perf_counter.h files should be exported, but then
the #ifdef won't work when the file is included from user space.
Also, some of the included headers are not exported.

> +/*
> + * Hardware event to monitor via a performance monitoring counter:
> + */
> +struct perf_counter_hw_event {
> + s64 type;
> +
> + u64 irq_period;
> + u32 record_type;
> +
> + u32 disabled : 1, /* off by default */
> + nmi : 1, /* NMI sampling */
> + raw : 1, /* raw event type */
> + inherit : 1, /* children inherit it */
> + pinned : 1, /* must always be on PMU */
> + exclusive : 1, /* only counter on PMU */
> +
> + __reserved_1 : 26;
> +
> + u64 __reserved_2;
> +};

When exported, the types should be __s64 etc instead of s64.

> +/*
> + * Ioctls that can be done on a perf counter fd:
> + */
> +#define PERF_COUNTER_IOC_ENABLE _IO('$', 0)
> +#define PERF_COUNTER_IOC_DISABLE _IO('$', 1)
> +
> +/*
> + * Kernel-internal data types:
> + */

The kernel internal parts of the header should be hidden in #ifdef
__KERNEL__.

> +
> +asmlinkage int sys_perf_counter_open(
> +
> + struct perf_counter_hw_event *hw_event_uptr __user,
> + pid_t pid,
> + int cpu,
> + int group_fd);

For the syscall, I'd suggest sys_perf_counter_fd or sys_perfcounterfd
to go along with signalfd, eventfd, etc. The only other _open syscall
we have is sys_mq_open(), which is different since it actually performs
an open() on a (hidden) file.

All system calls should return a 'long' value to make sure that the
errno handling works on all architectures.

> +static const struct file_operations perf_fops = {
> + .release = perf_release,
> + .read = perf_read,
> + .poll = perf_poll,
> + .unlocked_ioctl = perf_ioctl,
> + .compat_ioctl = perf_ioctl,
> +};

It feels inconsistent to combine a syscall for getting the fd with ioctl.
Assuming the interface is semantically right, I'd suggest using either
a chardev with more ioctls or more syscalls but not both:

asmlinkage long sys_perfcounter_fd(
struct perf_counter_hw_event *hw_event_uptr __user,
pid_t pid,
int cpu,
int group_fd);
asmlinkage long sys_perfcounter_setflags(int fd, u32 flags);

or

struct perf_counter_setup {
struct perf_counter_hw_event event;
__u64 pid;
__u32 cpu;
__u32 group_fd;
};
#define PERF_COUNTER_IOC_SETUP _IOW('$', 1, struct perf_counter_setup)
#define PERF_COUNTER_IOC_SETFLAGS _IOW('$', 2, __u32)
Arnd <><


\
 
 \ /
  Last update: 2009-02-21 01:51    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site