Bindings to linux perf_event header. by maxhaton · Pull Request #3160 · dlang/druntime

maxhaton · 2020-07-14T01:06:39Z

I am working towards a uarch-level benchmarking and tracing library for phobos, so: these are bindings to perf_event.h . eBPF coming soon.

The system call perf_event_open is not included by glibc so I have included a function to call it that includes system call numbers for X86, amd64 and both varieties of ARM. The bitfields were done by including Phobos bitmanips manually - there could potentially be namespace collisions due to the extern(C) but I chose to keep it simple for now.

A simple example (Converted almost verbatim from the manpage) works fine but I am looking for a more thorough test suite (or at least one that covers most of the functions without converting hacky "kernel C" into D by hand).

import core.sys.linux.perf_event;
import core.stdc.stdio;
import core.stdc.string;
import core.stdc.stdlib;
import std.stdio : writeln;
import core.sys.posix.sys.ioctl;
import core.sys.posix.unistd;
import core.sys.linux.unistd;
extern(C)
int main(int argc, char** argv)
{
    perf_event_attr pe;
    long count;
    int fd;

    memset(&pe, 0, perf_event_attr.sizeof);
    pe.type = perf_type_id.PERF_TYPE_HARDWARE;
    pe.size = perf_event_attr.sizeof;
    pe.config = perf_hw_id.PERF_COUNT_HW_INSTRUCTIONS;
    pe.disabled = 1;
    pe.exclude_kernel = 1;
    pe.exclude_hv = 1;

    fd = cast(int) perf_event_open(&pe, 0, -1, -1, 0);
    if (fd == -1)
    {
        fprintf(stderr, "Error opening leader %llx\n", pe.config);
        exit(EXIT_FAILURE);
    }

    ioctl(fd, PERF_EVENT_IOC_RESET, 0);
    ioctl(fd, PERF_EVENT_IOC_ENABLE, 0);

    puts("Measuring instruction count for this puts");

    ioctl(fd, PERF_EVENT_IOC_DISABLE, 0);
    read(fd, &count, long.sizeof);
    
    printf("Used %lld instructions\n", count);

    close(fd);
    return 0;
}

dlang-bot · 2020-07-14T01:06:41Z

Thanks for your pull request, @maxhaton!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#3160"

WalterBright · 2020-07-14T09:28:04Z

I'm seeing the same buildcite/druntime ldc-developers/ldc failure in other PRs. What broke it?

maxhaton · 2020-07-14T17:52:59Z

@WalterBright as far as I can see it's a style issue in the comments from the C header. I'll push something to fix it soon - if I can't fix it easily I'll just strip them given that the ddoc for the sys.linux packages isn't generated anyway as far as I can see.

maxhaton · 2020-07-14T18:07:29Z

I removed the comments, passes the checks on my machine now.

Geod24

There's a lot of weird things here, and it doesn't seem so in line with how other C bindings are handled.

Geod24 · 2020-07-15T00:09:12Z

src/core/sys/linux/perf_event.d

+*
+* Converted/Bodged from linux userspace header
+*
+* Copyright: Max Haughton 2020


It would be better if the copyright was assigned to the foundation instead of yourself.

Geod24 · 2020-07-15T00:10:58Z

src/core/sys/linux/perf_event.d

+/*
+* D header file for perf_event_open system call.
+*
+* Converted/Bodged from linux userspace header


Having "bodged" in official documentation doesn't really inspire confidence.

Ha. I'll get rid of that tomorrow. Although the docs aren't generated for this folder if I'm not mistaken.

Geod24 · 2020-07-15T00:11:08Z

src/core/sys/linux/perf_event.d

+* Converted/Bodged from linux userspace header
+*
+* Copyright: Max Haughton 2020
+* License: <a href="http://www.boost.org/LICENSE_1_0.txt">Boost License 1.0</a>.


No HTML please, use macros

Geod24 · 2020-07-15T00:11:55Z

src/core/sys/linux/perf_event.d

+else version (ARM64)
+{
+    enum __NR_perf_event_open = 241;
+}


else static assert(0, "Platform not supported");

Geod24 · 2020-07-15T04:37:45Z

src/core/sys/linux/perf_event.d

+{
+    enum __NR_perf_event_open = 241;
+}
+extern (C) extern long syscall(long __sysno, ...);


I'm pretty sure we have a binding for this

There is not. There are a few PRs but none of them made it in #2820

Geod24 · 2020-07-15T04:39:03Z

src/core/sys/linux/perf_event.d

+    private ulong _disabled_inherit_pinned_exclusive_exclude_user_exclude_kernel_exclude_hv_exclude_idle_mmap_comm_freq_inherit_stat_enable_on_exec_task_watermark_precise_ip_mmap_data_sample_id_all_exclude_host_exclude_guest_exclude_callchain_kernel_exclude_callchain_user_mmap2_comm_exec_use_clockid_context_switch_write_backward_namespaces___reserved_1;
+    @property ulong disabled() @safe pure nothrow @nogc const
+    {
+        auto result = (_disabled_inherit_pinned_exclusive_exclude_user_exclude_kernel_exclude_hv_exclude_idle_mmap_comm_freq_inherit_stat_enable_on_exec_task_watermark_precise_ip_mmap_data_sample_id_all_exclude_host_exclude_guest_exclude_callchain_kernel_exclude_callchain_user_mmap2_comm_exec_use_clockid_context_switch_write_backward_namespaces___reserved_1 & 1U) >> 0U;
+        return cast(ulong) result;
+    }


It's a mixin from the Phobos bitmanip template. It's not a public interface so I didn't bother renaming it.

Geod24 · 2020-07-15T04:39:10Z

src/core/sys/linux/perf_event.d

+
+struct perf_event_attr
+{
+


Extra whitespace

Geod24 · 2020-07-15T04:40:42Z

src/core/sys/linux/perf_event.d

+
+    enum ulong mispred_min = cast(ulong) 0U;
+    enum ulong mispred_max = cast(ulong) 1U;
+    @property ulong predicted() @safe pure nothrow @nogc const


Since the whole thing is already nothrow @nogc you don't need those

Geod24 · 2020-07-15T04:41:43Z

Removing the comment is not really a solution IMO, they add a lot of value. What was the failure ?

maxhaton · 2020-07-15T04:55:54Z

@Geod24 it was a whitespace check in the CI. The comments removal was easier than manually going through several hundred lines of comments - the ABI of perf_event is completely stable at this point so the documentation is in the manpages/online.

wilzbach · 2020-07-15T07:46:44Z

I'm sorry, but how is removing trailing whitespace going to take more than ten seconds?

atilaneves · 2020-07-15T10:16:58Z

@Geod24 it was a whitespace check in the CI. The comments removal was easier than manually going through several hundred lines of comments - the ABI of perf_event is completely stable at this point so the documentation is in the manpages/online.

Open in Emacs, M-x delete-trailing-whitespace, done.

ghost · 2020-07-15T10:26:01Z

src/core/sys/linux/perf_event.d

+
+    ulong sample_type;
+    ulong read_format;
+    private ulong _disabled_inherit_pinned_exclusive_exclude_user_exclude_kernel_exclude_hv_exclude_idle_mmap_comm_freq_inherit_stat_enable_on_exec_task_watermark_precise_ip_mmap_data_sample_id_all_exclude_host_exclude_guest_exclude_callchain_kernel_exclude_callchain_user_mmap2_comm_exec_use_clockid_context_switch_write_backward_namespaces___reserved_1;


seriously ? It is private so the mangling does not matter. Reduce the size of this identifier, please.

eventually you can keep the long stuff in a comment, as it is certainly a very very self documenting name.

maxhaton · 2020-07-15T16:42:48Z

Fixed it. It wasn't trailing whitespace, it was tabs vs spaces but somehow only in the comments. I was under the impression that it was all spaces, but apparently not - it is now.

maxhaton · 2020-07-15T19:31:52Z

@Geod24 everything passes now, feedback is incorporated, anything else?

src/core/sys/linux/perf_event.d

n8sh · 2020-07-16T19:55:09Z

Except for in the definition of perf_callchain_context it looks like every cast(ulong) is unnecessary.

RazvanN7

@maxhaton Please rebase and let's get this in.

maxhaton · 2021-08-24T16:49:34Z

@maxhaton Please rebase and let's get this in.

@RazvanN7 Blast from the past, but will do

Geod24 · 2021-08-25T09:22:40Z

Damn I thought this was merged a while ago.

RazvanN7 · 2021-08-26T14:03:47Z

@maxhaton Ping me when it's ready.

…_event_open is not bound by glibc so I have included a helper to call this through syscall (in linux unistd) which is in glibc. The bitfields were done via the phobos library (manual mixin inclusion)

…d), the comments are now back with working indentation

Version specifier was incorrect for 64 bit arm Co-authored-by: Nathan Sashihara <21227491+n8sh@users.noreply.github.com>

… does...

maxhaton · 2021-08-26T22:03:41Z

@RazvanN7 Rebased and did a once over documenting everything I could spot. This API should be pretty stable, but I would like to get a test or two in another PR at some point. I have some eBPF bindings somewhere as well, they could go in.

RazvanN7 · 2021-08-27T06:38:03Z

Thanks, @maxhaton !

ibuclaw · 2021-10-01T17:56:31Z

src/core/sys/linux/perf_event.d

+version (X86_64)
+{
+        enum __NR_perf_event_open = 298;
+}
+else version (X86)
+{
+        enum __NR_perf_event_open = 336;
+}
+else version (ARM)
+{
+        enum __NR_perf_event_open = 364;
+}
+else version (AArch64)
+{
+        enum __NR_perf_event_open = 241;
+}
+else
+{
+        static assert(0, "Architecture not supported");
+}


We should really be rejecting PRs that don't cover all primary supported linux platforms. It's annoying to be running through some finishing line bootstrap tests, and I can't even build druntime anymore on PPC.

Well that's why we need to extend the CI here to include you.

ibuclaw · 2021-10-01T18:11:56Z

src/core/sys/linux/perf_event.d

+        //     ulong, "__reserved_1", 35));
+        private ulong perf_event_attr_bitmanip;
+        ///
+        @property ulong disabled() @safe pure nothrow @nogc const


There's functions here and no unittests? How can we vet that this actually works?

maxhaton · 2021-10-01T19:51:36Z

If you can find a platform with perf_event turned on I can get a testsuite going but I'm guessing paranoid is set to the NO_WAY_JOSE setting.

…

On Fri, 1 Oct 2021, 19:12 Iain Buclaw, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/core/sys/linux/perf_event.d <#3160 (comment)>: > + // ulong, "mmap_data", 1, + // ulong, "sample_id_all", 1, + // ulong, "exclude_host", 1, + // ulong, "exclude_guest", 1, + // ulong, "exclude_callchain_kernel", 1, + // ulong, "exclude_callchain_user", 1, + // ulong, "mmap2", 1, + // ulong, "comm_exec", 1, + // ulong, "use_clockid", 1, + // ulong, "context_switch", 1, + // ulong, "write_backward", 1, + // ulong, "namespaces", 1, + // ulong, "__reserved_1", 35)); + private ulong perf_event_attr_bitmanip; + /// + @Property ulong disabled() @safe pure nothrow @nogc const There's functions here and no unittests? How can we vet that this actually works? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3160 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLI75DILDX2F4CQRJYLRPDUEX2XRANCNFSM4OZAS2GQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

maxhaton · 2021-10-01T19:52:33Z

I completely forgot I did that, my bad, do we have syscall tables I can rely upon at build time?

…

On Fri, 1 Oct 2021, 18:56 Iain Buclaw, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/core/sys/linux/perf_event.d <#3160 (comment)>: > +version (X86_64) +{ + enum __NR_perf_event_open = 298; +} +else version (X86) +{ + enum __NR_perf_event_open = 336; +} +else version (ARM) +{ + enum __NR_perf_event_open = 364; +} +else version (AArch64) +{ + enum __NR_perf_event_open = 241; +} +else +{ + static assert(0, "Architecture not supported"); +} We should really be rejecting PRs that don't cover all primary supported linux platforms. It's annoying to be running through some finishing line bootstrap tests, and I can't even build druntime anymore on PPC. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3160 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLI75HK7VSXFULIIVPGOADUEXY5XANCNFSM4OZAS2GQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

ibuclaw · 2021-10-01T22:57:53Z

I completely forgot I did that, my bad, do we have syscall tables I can rely upon at build time?

In the version (X86_64) path, D_X32 (-mx32) adds the magic number 0x40000000 to 298.

For all others (HPPA, MIPS, PPC, RISCV, S390, SPARC), you can grab them from musl or glibc sources.

ibuclaw · 2021-11-10T09:40:30Z

src/core/sys/linux/perf_event.d

+
+                struct
+                {
+                        import std.bitmanip : bitfields;


$#^*% !!! ???

maxhaton requested review from Burgos, MartinNowak, andralex and wilzbach as code owners July 14, 2020 01:06

Geod24 reviewed Jul 15, 2020

View reviewed changes

ghost reviewed Jul 15, 2020

View reviewed changes

n8sh reviewed Jul 16, 2020

View reviewed changes

src/core/sys/linux/perf_event.d Outdated Show resolved Hide resolved

dlang-bot added the stalled label May 27, 2021

RazvanN7 approved these changes Aug 24, 2021

View reviewed changes

dlang-bot removed the stalled label Aug 25, 2021

maxhaton and others added 6 commits August 26, 2021 19:24

Added bindings to the linux perf_event.h header. The system call perf…

cfff622

…_event_open is not bound by glibc so I have included a helper to call this through syscall (in linux unistd) which is in glibc. The bitfields were done via the phobos library (manual mixin inclusion)

Fixed the issue with comments (by removing original C comments)

aeccef4

Fixed the header, gave the bitfield internals better names (*_bitfiel…

d614807

…d), the comments are now back with working indentation

Style fix - trailing space fell through the crack

484442a

Update src/core/sys/linux/perf_event.d

9509a1a

Version specifier was incorrect for 64 bit arm Co-authored-by: Nathan Sashihara <21227491+n8sh@users.noreply.github.com>

Documented many symbols

e7fccbf

maxhaton force-pushed the performance_perf_event branch from c6eafa6 to e7fccbf Compare August 26, 2021 21:58

Rectified some trailing whitespace. What vital work the style checker…

dc08d27

… does...

RazvanN7 approved these changes Aug 27, 2021

View reviewed changes

RazvanN7 merged commit e6caaab into dlang:master Aug 27, 2021

ibuclaw reviewed Oct 1, 2021

View reviewed changes

ibuclaw mentioned this pull request Oct 4, 2021

core.sys.linux.perf_event: Add missing D runtime linux ports #3578

Merged

ibuclaw reviewed Nov 10, 2021

View reviewed changes

Uh oh!

Conversation

maxhaton commented Jul 14, 2020

Uh oh!

dlang-bot commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

WalterBright commented Jul 14, 2020

Uh oh!

maxhaton commented Jul 14, 2020

Uh oh!

maxhaton commented Jul 14, 2020

Uh oh!

Geod24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Jul 15, 2020

Uh oh!

maxhaton commented Jul 15, 2020

Uh oh!

wilzbach commented Jul 15, 2020

Uh oh!

atilaneves commented Jul 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxhaton commented Jul 15, 2020

Uh oh!

maxhaton commented Jul 15, 2020

Uh oh!

Uh oh!

n8sh commented Jul 16, 2020

Uh oh!

RazvanN7 left a comment

Choose a reason for hiding this comment

Uh oh!

maxhaton commented Aug 24, 2021

Uh oh!

Geod24 commented Aug 25, 2021

Uh oh!

RazvanN7 commented Aug 26, 2021

Uh oh!

maxhaton commented Aug 26, 2021

Uh oh!

RazvanN7 commented Aug 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxhaton commented Oct 1, 2021 via email

Uh oh!

dlang-bot commented Jul 14, 2020 •

edited

Loading