Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 6/15/22 20:52, Sean Christopherson wrote:
On Thu, Jun 02, 2022, Like Xu wrote:
I actually agree and understand the situation of maintainers/reviewers.
No one wants to maintain flawed code, especially in this community
where the majority of previous contributors disappeared after the code
was merged in. The existing heavy maintenance burden is already visible.

I don't think this is true. I think it's relatively rare for contributors to disappear.

Thus we may have a maintainer/reviewers scalability issue. Due to missing
trust, competence or mastery of rules, most of the patches sent to the list
have no one to point out their flaws.

Then write tests and run the ones that already exist.  Relying purely on reviewers
to detect flaws does not and cannot scale.  I agree that we currently have a
scalability issue, but I have different views on how to improve things.

I have privately received many complaints about the indifference of our
community, which is distressing.

You're welcome to expand on these complaints. But I suspect that a lot of these would come from people that have been told "review other people's work", "write tests" and/or "you submitted a broken patch" before.

"Let's try to accept" is basically what I did for PEBS and LBR, both of which I merged basically out of guilt after a little-more-than-cursory review. It turns out that both of them were broken in ways that weren't subtle at all; and as a result, other work already queued to 5.19 had to be bumped to 5.20.

Honestly I should have complained and un-merged them right after seeing the msr.flat failure. Or perhaps I should have just said "write tests and then I'll consider the series", but I "tried to accept" and we can already see it was a failure.

Obviously, "try to accept" is not a 100% commitment and it will fail with high
probability, but such a stance (along with standard clarifications and requirements)
from reviewers and maintainers will make the contributors more concerned,
attract potential volunteers, and focus the efforts of our nominated reviewers.

If it "fails with high probability", all that happened was a waste of time for everyone involved. Including the submitter who has waited for weeks for a reviews only to be told "test X fails".

I completely agree on needing better transparency for the lifecycle of patches
going through the KVM tree.  First and foremost, there need to be formal, documented
rules for the "official" kvm/* branches, e.g. everything in kvm/queue passes ABC
tests, everything in kvm/next also passes XYZ tests.  That would also be a good
place to document expectations, how things works, etc...

Agreed. I think this is a more general problem with Linux development and I will propose this for maintainer summit.

But again, the relationship between contributors and maintainers should be of mutual benefit. Rules help contributors, but contributors should themselves behave and not throw broken patches at maintainers. And again, guess what the best way is to tell maintainers your patch is not broken? Include a test. It shows that you are paying attention.

I fully realize that writing tests is not glamorous, and that some of KVM's tooling
and infrastructure is lacking,

I wouldn't say lacking. Sure it's complicated, but between selftests and kvm-unit-tests the tools *are* there. selftests that allow you to test migration at an instruction boundary, for example, are not that hard to write and were very important for features such as nested state and AMX. They're not perfect, but they go a long way towards giving confidence in the code; and it's easier to catch weird ioctl policies from reviewing comprehensive tests than from reviewing the actual KVM code.

We're not talking of something like SEV or TDX here, we're talking about very boring MSR emulation and only slightly less boring PMU passthrough.

Paolo




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux