On 2021/5/18 21:36, Peter Zijlstra wrote:
On Tue, May 18, 2021 at 09:28:52PM +0800, Xu, Like wrote:
How would pebs && !intr be possible?
I don't think it's possible.
And yet you keep that 'intr||pebs' weirdness :/
Also; wouldn't this be more legible
when written like:
perf_overflow_handler_t ovf = kvm_perf_overflow;
...
if (intr)
ovf = kvm_perf_overflow_intr;
...
event = perf_event_create_kernel_counter(&attr, -1, current, ovf, pmc);
Please yell if you don't like this:
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index 711294babb97..a607f5a1b9cd 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -122,6 +122,8 @@ static void pmc_reprogram_counter(struct kvm_pmc *pmc,
u32 type,
.config = config,
};
bool pebs = test_bit(pmc->idx, (unsigned long *)&pmu->pebs_enable);
+ perf_overflow_handler_t ovf = (intr || pebs) ?
+ kvm_perf_overflow_intr : kvm_perf_overflow;
This, that's exactly the kind of code I wanted to get rid of. ?: has
it's place I suppose, but you're creating dense ugly code for no reason.
perf_overflow_handle_t ovf = kvm_perf_overflow;
if (intr)
ovf = kvm_perf_overflow_intr;
Is so much easier to read. And if you really worry about that pebs
thing; you can add:
WARN_ON_ONCE(pebs && !intr);
Thanks! Glad you could review my code.
As a new generation, we do appreciate your patient guidance on your taste
in code.
