On 18.12.21 20:52, Linus Torvalds wrote:
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.
Oh, there is no need for additional challenges, I've been challenged with this problem for way too long already ;)
And again, I appreciate this discussion and your feedback. I want to get all of this fixed ASAP, even if it's not going to be the way I propose as I raised. Any input is appreciated (as long as people don't scream at me).
To get to your point: I thought about your remarks with the swapcount and it makes sense. The mapcount itself is not expressive enough to catch macpount == 1 vs mapcount > 1.
What *would* work to have precise COW I think is having the active and inactive count instead of just the active (mapcount) part, whereby:
active: page tables mapping this page -> mapcount inactive: page tables referencing this page via swap/migration entry
An alternative would be to just know that there are inactive users. We'd have to read both values atomically in one shot.
There would be ways to store that information in the _mapcount but it certainly adds a lot of complexity, and ...
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.
... you have a point here.
Having that said, I hope we can agree that the "page_count" is not the perfect solution. I hope we can at least tweak it for now to get rid of 3) Wrong COW.
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.
Thanks for admitting that!
I might have had an idea yesterday on how to fix most of the issues without relying on the mapcount, doing it similar (but slightly different) as you propose here. Let's call it a mixture of the unsharing approach and your approach. I cannot promise anything, so ...
... I'll go playing with it and share some details ASAP. At least it sounds comparatively simple in my head.
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.
Just to give you my perspective:
Personally I don't care too much about 2). The only reason why I somehow care about "Unnecessary COW" are * Challenging for hugetlb use as I explained. We might still want to use the mapcount there. * It's mostly a symptom of our eventually too simple COW logic that effectively leads to 3).
While I do care about 1) (Missed CoW) for our customers, I *especially* care about 3) (Wrong Cow) simply because silent memory corruptions in user space are not acceptable.
As you say, fixing 1) the "page_count" way might be easy, at least for THP.
Simple example: Have swapping enabled and register a fixed io_uring buffer at the wrong time. Fixed io_uring buffers are no a commodity feature for unprivileged users space ...
So that's why I so deeply care about all of this.