Remove exclude guest flag from perf event attrs.

[Creatone]

From https://man7.org/linux/man-pages/man2/perf_event_open.2.html :

exclude_guest (since Linux 3.2)
              When conducting measurements that include processes running VM
              instances (i.e., have executed a KVM_RUN ioctl(2)), do not
              measure events happening inside guest instances.  This is only
              meaningful outside the guests; this setting does not change
              counts gathered inside of a guest.  Currently, this function‐
              ality is x86 only.

Depend on the kernel version, perf_event_open can throw invalid_argument for uncore events with this flag set.
For core events it's also unnecessary.

Signed-off-by: Paweł Szulik pawel.szulik@intel.com

[k8s-ci-robot]

Hi @Creatone. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dashpole]

/ok-to-test
cc @katarzyna-z @iwankgb

[iwankgb]

Can we make it configurable? We did it for a reason in WCA, I guess.

[Creatone]

Before posting this PR, I had a discussion with @ppalucki and he doesn't know why this parameter was added.
Someone else was the author of this part of code, @gryf can you explain why this parameter is needed?

[iwankgb]

What is this flag responsible for?

[Creatone]

To not measure events inside VM.

Why omit VM when we depend on measures from cgroups?

[gryf]

I don't remember the exact reason, why we exclude VMs from measurement in WCA. Perhaps it has something to do with specific environment we planned to measure.

[ppalucki]

IMHO there is no reason to omit VM measurements by default at all - I strongly recommend removing this flag because it will result in misleading results for virtual environments - we're going to remove that from WCA as well.

[gryf]

The only reason I could think of is to actually prevent to count anything from hypervisor, perhaps to detect a process, which could grab the resources VM wants, thus performance of VM is affected. I'd opt for @iwankgb proposal to make it configurable.

[iwankgb]

@ppalucki @Creatone configurable with default value to include hypervisor?

[Creatone]

Sure, but where keep value? Another flag or in a perf config file?

[katarzyna-z]

I think that we can extend config file.

[Creatone]

/retest

[katarzyna-z]

lgtm

[katarzyna-z]

@iwankgb What do you think about proposed form of configuration?

[iwankgb]

Can we use counters that take hypervisor into consideration and those that do not in a single group?

[Creatone]

We assumed it would be possible when event grouping would be available ( #2578 ).

[dashpole]

Sorry for the slow review. Based on the discussion, i'm a little concerned we don't have real use-cases for the additional configuration. It sounds like someone could use this in theory, but would anyone actually set this in practice?

If we are unsure if it is needed, we should err on the side of omitting it for now, and doing what will work for most. We can always add it in later if there is a real need.
If we do think the configuration is justified, i'd like to see a bit more documentation explaining when someone would want to set it. It sounds like there may be a small set of use-cases, so we could potentially enumerate them.

[Creatone]

I agree with you.
If this parameter won't be configurable now, I'll delete it to prevent invalid_argument errors on some configurations.

[iwankgb]

@bobbypage your involvement here would be more than appreciated.

[Creatone]

@bobbypage thanks to this PR, perf events would be accurate for VMs. Could you check this PR?

[dashpole]

lgtm

[dashpole]

Sorry, didn't see the most recent commit removing the flag. This looks good now.