On Sat, Dec 18, 2021 at 11:21 AM Linus Torvalds torvalds@linux-foundation.org wrote:
To recap: (1) is important, and page_count() is the only thing that guarantees "you get full access to a page only when it's *obviously* exclusively yours". (2) is NOT important, but could be a performance issue, but we have real data from the past year that it isn't. (3) is important, and has a really spectacularly simple conceptual fix with quite simple code too.
In contrast, with the "mapcount" games you can't even explain why they should work, and the patches I see are actively buggy because everything is so subtle.
So to challenge you, please explain exactly how mapcount works to solve (1) and (3), and how it incidentally guarantees that (2) doesn't happen.
And that really involves explaining the actual code too. I can explain the high-level concepts in literally a couple of sentences.
For (1), "the page_count()==1 guarantees you are the only owner, so a COW event can re-use the page" really explains it. And the code is pretty simple too. There's nothing subtle about "goto copy" when pagecount is not 1. And even the locking is simple: "we hold the page table lock, we found a page, it has only one ref to it, we own it"
Our VM is *incredibly* complicated. There really are serious advantages to having simple rules in place.
And for (2), the simple rule is "yeah, we can cause spurious cow events". That's not only simple to explain, it's simple to code for. Suddenly you don't need to worry. "Copying the page is always safe". That's a really really powerful statement.
Now, admittedly (3) is the one that ends up being more complicated, but the *concept* sure is simple. "If you don't want to COW this page, then don't mark it for COW".
The *code* for (3) is admittedly a bit more complicated. The "don't mark it for COW" is simple to say, but we do have that fairly odd locking thing with fork() doing a seqcount_write_begin/end, and then GIP does the read-seqcount thing with retry. So it's a bit unusual, and I don't think we have that particular pattern anywhere else, but it's one well-defined lock and while unusual it's not *complicated* as far as kernel locking rules go. It's unusual and perhaps not trivial, but in the end those seqcount code sequences are maybe 10 lines total, and they don't interact with anything else.
And yes, the "don't mark it for COW" means that write-protecting something is special, mainly because we sadly do not have extra bits in the page tables. It would be *really* easy if we could just hide this "don't COW this page" in the page table. Truly trivial. We don't, because of portability across different architectures ;(
So I'll freely give you that my (3) is somewhat painful, but it's painful with a really simple concept.
And the places that get (3) wrong are generally places that nobody has been able to care about. I didn't realize the problem with creating a swap page after the fact for a while, so that commit feb889fb40fa ("mm: don't put pinned pages into the swap cache") came later, but it's literally a very simple two-liner.
The commit message for commit feb889fb40fa may be worth reading. It very much explains the spirit of the thing, and is much longer than the trivial patch itself.
Simple and clear concepts matter. Code gets complicated even then, but complex code with complex concepts is a bad combination.
Linus