Ignore specified signal when using perf by sdmaclea · Pull Request #12436 · dotnet/coreclr

sdmaclea · 2017-06-23T21:37:35Z

Add a do nothing signal handler to SIGRTMAX for profiling purposes. Allows synchronization point injection into the profile

Add a do nothing signal handler to SIGRTMAX for profiling purposes. Allows synchronization point in injection into the profile

sdmaclea · 2017-06-23T21:39:04Z

@janvorli PTAL I found this necessary/helpful for profiling more complicated benchmarks.

janvorli · 2017-06-26T09:21:20Z

+}
+
+
+void PAL_IgnoreProfileSignal(void)


@sdmaclea, could you please explain how the SIGRTMAX is used as a marker?

@janvorli perf record -g -e cycles:ppp -e signal:* will record instructions samples as the cycle counter expires as well as signals. If I want to inject a marker into a benchmark, I can use something like pkill -64 corerun. When I am profiling a server application, this allows me to sweep through various client configurations and add a marker when the stimulus changes. When I analyze the profile, I can use perf script | <filterscript> to analyze only the events between a specific set of marker. This can be fed into other flows i.e. perf script | <filterscript> | <flamegraph flatten script> | <flamegraph svg script>

For the filter script, I used an awk script like below to only print cycle events after the first SIGRTMAX event:

/.*signal:signal_deliver: sig=.*/ { trace = /=64/; } /sched:/ || /signal:/ { suppress=1 } /cycles:ppp/ { suppress=0 } { if(trace && !suppress) print }

That awk script was designed to work with perf record -g -e cycles:ppp -e sched:* -e signal:signal_deliver --filter "sig==64 || sig==2" It turns tracing on when it receives SIGRTMAX (64) and off when it receives SIGINT (2).

What if rather than having the SIGRTMAX hardcoded in the code, we would add a new config option (COMPlus_XXXXX) that would contain the number of signal that we want to ignore? There are no realtime signals on FreeBSD (yet?) or OSX and it seems that the signal used for this purpose doesn't necessarily have to be a realtime one. So such solution would be more universal and also explicit about whether to ignore the signal or not, which seems useful. I could also imagine apps that actually use the SIGRTMAX and host coreclr. Again, the explicit selection of the signal would be helpful in such case.

@janvorli Configurable would be good. It may be nice to allowCOMPlus_PerfSignalIgnoreMask could allow multiple bits to be set, but it would probably be limited to 64 bits (0-63). Or a comment describing recommended signals.

I think that sticking to one signal would be fine. Or can you already see a use case where multiple signals would be useful?

@janvorli I could only speculate that multiple signal might be useful.

Pushed implementation with only single signal.

janvorli · 2017-06-28T18:06:30Z


 #ifdef FEATURE_PERFMAP
 RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_PerfMapEnabled, W("PerfMapEnabled"), 0, "This flag is used on Linux to enable writing /tmp/perf-$pid.map. It is disabled by default", CLRConfig::REGUTIL_default)
+RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_PerfMapIgnoreSignal, W("PerfMapIgnoreSignal"), 0, "When perf map is enabled, this option will configure the specified signal to be accepeted and ignored as a marker in the perf logs.  It is disabled by default", CLRConfig::REGUTIL_default)


A nit - spelling: accepeted -> accepted. LGTM otherwise.

@janvorli Done++

adityamandaleeka · 2017-06-29T01:50:49Z


 #ifdef FEATURE_PERFMAP
 RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_PerfMapEnabled, W("PerfMapEnabled"), 0, "This flag is used on Linux to enable writing /tmp/perf-$pid.map. It is disabled by default", CLRConfig::REGUTIL_default)
+RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_PerfMapIgnoreSignal, W("PerfMapIgnoreSignal"), 0, "When perf map is enabled, this option will configure the specified signal to be accepted and ignored.  This allows it to be used as a marker in the perf logs.  It is disabled by default", CLRConfig::REGUTIL_default)


I don't have a strong object to this, but I'm wondering if this should be associated with the perf map stuff (as opposed to not being under FEATURE_PERFMAP and just being called COMPlus_IgnoreSignal or something). I understand that your use case is profiling-associated, but perhaps we can make this a more general concept.

Hmm..
I can leave it dependent on FEATURE_PERFMAP. I can enable it independent to the PerfMapEnabled, if t is is generally useful.

Perf has unique requirement on how the signal should be ignored. Normally, you would tell the kernel to ignore the signal by not delivering it. I had assumed this meant the config should be unique to perf, only needed to handle one signal, and was not generally useful..

Most common orthogonal use cases would be prefer the kernel ignoring a signal. This is significantly faster for the kernel as it doesn't need to do anything.

@adityamandaleeka @janvorli What do you think?

@sdmaclea I would prefer keeping it as you have it now.

adityamandaleeka · 2017-06-29T01:51:36Z

+void PAL_IgnoreProfileSignal(int signalNum)
+{
+#if !HAVE_MACH_EXCEPTIONS
+    // Add a signal handler which will ignore signals


typo: synchronize

adityamandaleeka · 2017-06-29T01:52:06Z

+
+        int signalNum = (int) CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_PerfMapIgnoreSignal);
+
+        if(signalNum > 0)


minor style nit: space after if

sdmaclea · 2017-07-05T14:57:27Z

@dotnet-bot test Windows_NT arm64 Cross Checked Build and Test

sdmaclea · 2017-07-05T14:58:46Z

@dotnet-bot test Windows_NT arm64 Checked

adityamandaleeka · 2017-07-07T23:14:00Z

@dotnet-bot test Ubuntu arm64 Cross Debug Build

janvorli · 2017-07-19T08:37:05Z

@dotnet-bot test Ubuntu arm64 Cross Debug Build

sdmaclea · 2017-07-19T15:09:25Z

@janvorli Ubuntu arm64 CI is broken per @jashook . @jashook @RussKeldorph Is there an ETA for fixing this? Should it be disabled/ignored?

sdmaclea · 2017-07-25T15:44:17Z

Please ignore the Ubuntu arm64 build error. This is a CI issue. It build locally. I have been using it without an issue for weeks.

Please merge

Ignore SIGRTMAX when using perf

c5db11c

Add a do nothing signal handler to SIGRTMAX for profiling purposes. Allows synchronization point in injection into the profile

dnfclas added the cla-already-signed label Jun 23, 2017

janvorli reviewed Jun 26, 2017

View reviewed changes

Add COMPlus_PerfMapIgnoreSignal

b49cba8

sdmaclea changed the title ~~Ignore SIGRTMAX when using perf~~ Ignore specified signal when using perf Jun 28, 2017

janvorli reviewed Jun 28, 2017

View reviewed changes

adityamandaleeka reviewed Jun 29, 2017

View reviewed changes

Fix nits per review

c0c2b55

sdmaclea force-pushed the PR-PAL_IgnoreProfileSignal branch from 00607bd to c0c2b55 Compare July 5, 2017 14:55

sdmaclea mentioned this pull request Jul 5, 2017

[Arm64/Unix] Use portable MP optimized new/alloc #12333

Merged

adityamandaleeka approved these changes Jul 7, 2017

View reviewed changes

adityamandaleeka mentioned this pull request Jul 7, 2017

Temporarily block core dump printing on arm64 #12682

Merged

janvorli merged commit 5d105ad into dotnet:master Jul 25, 2017

sdmaclea deleted the PR-PAL_IgnoreProfileSignal branch August 1, 2017 17:51

karelz modified the milestone: 2.1.0 Aug 28, 2017


		int signalNum = (int) CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_PerfMapIgnoreSignal);

		if(signalNum > 0)

		}


		void PAL_IgnoreProfileSignal(void)

Conversation

sdmaclea commented Jun 23, 2017

Uh oh!

sdmaclea commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdmaclea Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

sdmaclea commented Jul 5, 2017

Uh oh!

sdmaclea commented Jul 5, 2017

Uh oh!

adityamandaleeka commented Jul 7, 2017

Uh oh!

janvorli commented Jul 19, 2017

Uh oh!

sdmaclea commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdmaclea commented Jul 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sdmaclea Jun 26, 2017 •

edited

Loading

sdmaclea commented Jul 19, 2017 •

edited

Loading