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    [W:0.030 / U:29.516 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site