On 06/04/21 15:49, Sasha Levin wrote:
On Tue, Apr 06, 2021 at 08:09:26AM +0200, Paolo Bonzini wrote:
Whoa no, you have included basically a whole new feature, except for
the final patch that actually enables the feature. The whole new MMU
Right, we would usually grab dependencies rather than modifying the
patch. It means we diverge less with upstream, and custom backports tend
to be buggier than just grabbing dependencies.
In general I can't disagree. However, you *are* modifying at least
commit 048f49809c in your backport, so it's not clear where you draw the
line and why you didn't ask for help (more on this below).
Only the first five patches here are actual prerequisites for an easy
backport of the two commits that actually matter (a835429cda91, "KVM:
x86/mmu: Ensure TLBs are flushed when yielding during GFN range zap";
and 048f49809c52, "KVM: x86/mmu: Ensure TLBs are flushed for TDP MMU
during NX zapping", 2021-03-30). Everything after "KVM: x86/mmu: Yield
in TDU MMU iter even if no SPTES changed" could be dropped without
making it any harder to backport those two.
is still not meant to be used in production and development is still
happening as of 5.13.
Unrelated to this discussion, but how are folks supposed to know which
feature can and which feature can't be used in production? If it's a
released kernel, in theory anyone can pick up 5.12 and use it in
production.
It's not enabled by default and requires turning on a module parameter.
That also means that something like CKI will not notice if anything's
wrong with these patches. It also means that I could just shrug and
hope that no one ever runs this code in 5.10 and 5.11 (which is actually
the most likely case), but it is the process that is *just wrong*.
Were all these patches (82-97) included just to enable patch 98 ("KVM:
x86/mmu: Ensure TLBs are flushed for TDP MMU during NX zapping")? Same
for 105-120 in 5.11.
Yup. Is there anything wrong with those patches?
The big issue, and the one that you ignoredz every time we discuss this
topic, is that this particular subset of 17 has AFAIK never been tested
by anyone.
There's plenty of locking changes in here, one patch that you didn't
backport has this in its commit message:
This isn't technically a bug fix in the current code [...] but that
is all very, very subtle, and will break at the slightest sneeze,
meaning that the locking in 5.10 and 5.11 was also less robust to
changes elsewhere in the code.
Let's also talk about the process and the timing. I got the "failed to
apply" automated message last Friday and I was going to work on the
backport today since yesterday was a holiday here. I was *never* CCed
on a post of this backport for maintainers to review; you guys
*literally* took random subsets of patches from a feature that is new
and in active development, and hoped that they worked on a past release.
I could be happy because you just provided me with a perfect example of
why to use my employer's franken-kernel instead of upstream stable
kernels... ;) but this is not how a world-class operating system is
developed. Who cares if a VM breaks or even if my laptop panics; but
I'd seriously fear for my data if you applied the same attitude to XFS
or ext4fs.
For now, please drop all 17 patches from 5.10 and 5.11. I'll send a
tested backport as soon as possible.
Paolo
