perf icon indicating copy to clipboard operation
perf copied to clipboard

(*perf.Group).Options are not respected after g.Add()

Open pwaller opened this issue 6 years ago • 3 comments

In https://github.com/acln0/perf/pull/4#issuecomment-511237523 I commented that I was getting perf: failed to open event leader: perf_event_open: permission denied with perf.perf_event_paranoid=2.

Stepping through perf_event_open with a kernel debugger I isolated the cause. The ExcludeKernel option isn't being passed through.

Here's the code I wrote where I expected the option to be set:

	var g perf.Group
	g.CountFormat = perf.CountFormat{
		Running: true,
		ID:      true,
	}

	g.Options.ExcludeKernel = true
	g.Options.ExcludeHypervisor = true
	
	g.Add(perf.Instructions, perf.CPUCycles)

Indeed, if I look at the leader attributes being used:

https://github.com/acln0/perf/blob/e7587bd386de337d7053aa87cdf522ac62571775/group.go#L73-L75

... I find that g.Options aren't being respected.

pwaller avatar Jul 19 '19 11:07 pwaller

Apologies, I've just realised that the code I presented above was not the code that I was running.

In my erroneous code, I was doing this:

	g.Add(perf.Instructions, perf.CPUCycles)

	g.Options.ExcludeKernel = true
	g.Options.ExcludeHypervisor = true

In the issue body, the code actually works, so they are being passed through.

I didn't realise that the options need to be set before add is called. I wonder if there is a way to make this less error prone or catch this kind of user mistake.

pwaller avatar Jul 19 '19 11:07 pwaller

Good catch.

https://godoc.org/acln.ro/perf#Group.Add says:

For each Configurator, a new *Attr is created, the group-specific settings are applied, then Configure is called on the *Attr to produce the final event attributes.

This may not be the best design indeed. The current design makes doing another pass of applying settings just before registering the event tricky. Perhaps Add should defer applying settings until the call to Open.

acln0 avatar Jul 19 '19 12:07 acln0

I'm going to fix this as soon as I have a little time to review and merge #4 also. Thanks.

acln0 avatar Jul 19 '19 12:07 acln0