On Fri, Mar 19, 2010 at 08:29:30AM -0500, Christoph Lameter wrote: > On Fri, 19 Mar 2010, Andrea Arcangeli wrote: > > > There is no change at all to pte management related to this. Please > > stick to facts. > > Look at the patches. They add synchronization to pte operations. The whole point is that it's fundamentally unavoidable and not specific to any of split_huge_page, compound_lock or get_page/put_page! Yes I've to sometime take the page_table_lock (only when pmd is huge, and that is a lockless check, so it's just 1 more branch from a cacheline guaranteed guaranteed already in l1 hot cache in turn definitely unmeasurable in microbenchmarks too) but even if we remove split_huge_page completely you will still have to take a lock, and it can't be the PT lock. The PT lock is indexed on the pte, and the pte doesn't exist if the pmd is huge! It's not new locking at all, it's just that when the pmd is huge we've to serialize the thread concurrent modifications to it, with something less granular than the PT lock because it doesn't exist for huge pmd... Yes sure, we can later add a PMD lock that works like the PT lock does today (with PT lock I mean the pte_offset_map_lock). When there is no pte that lock doesn't exist, that's all about it. It's misleading at the very least, to blame on split_huge_page for the fact I had to make the pte mandling any more complex. The very same page_table_lock and pmd_huge checks were there, well before I had to modify put_page and add the compound_lock. Initially gup was calling split_huge_page, so there was no need of compound_lock, but we need O_DIRECT with virtualization, O_DIRECT is an absolute must and we can't split pages across it. With mmu notifier users (like KVM page fault), I could trivially avoid having to introduce compound_lock while still preventing them having to call split_huge_page in gup, by adding a gup_foll(foll_flags) without FOLL_GET set, which would avoid them wasting time taking refcounts on any page. With mmu notifier it's a total waste to pin pages (well xpmem does some trick to try to avoid flushing secondary ptes and secondary tlbs synchronously but that has other issues and it's not what KVM and GRU are doing, so KVM and GRU would definitely get a microoptimization from a gup_variant that won't get_page at all!). In addition to avoiding the need of compound_lock. Problem with O_DIRECT is that I couldn't use mmu notifier to prevent it to take the pin on the page, because there is no way to interrupt DMA synchronously before mmu_notifier_invalidate_* returns... So I had to add compound_lock and keep gup API backwards compatible and have the proper serialization happen _only_ for PageTail inside put_page. > > What you're commenting is the "compound_lock" and the change to two > > functions only: get_page and put_page (put_page has to serialize > > against split_huge_page_refcount and it uses the "compound_lock" > > per-head-page bit spinlock to achieve it), and this change only > > applies to TailPages and won't add any overhead to the fast path for > > non-compound pages and no change for PageHead pages either. > > These are bloating inline VM primitives and add new twists on > syncronization of pages. They add useless complexity at a very fundamental > layer of the VM. Useless? I think I already explained why compound_lock is _useful_ and what we gain from it (avoid breaking every gup user out there and/or avoid removing split_huge_page in turn avoiding breaking every piece of VM code that walks pagetables out there, in turn making transparent hugepage self contained and possible to introduce ""_gradually_ starting from anonymous memory"" like you said was good idea at the end of in previous email). > > The only thing that tries to run get_page or put_page on a TailPage is > > gup and gup_fast. Nothing else. And it's mostly needed to avoid > > altering gup_fast/gup API while still preventing to split hugepages > > across GUP. The reason why I went to this extra-length to be backwards > > compatible with gup/gup-fast is to avoid the patch to escalate from > > <40 patches to maybe >100 or more. > > What is wrong with gup and gup_fast? They mimick the traversal of the page > tables by the MMU. If you update in the right sequence then there wont be > an issue. gup and gup_fast (if we don't split_huge_page during the follow_page pagtable walk which is what I did initially to start but it's unacceptable as it makes O_DIRECT with -drive cache=off split guest NPT hugepages on the I/O memory source/destination) will prevents split_huge_page to be able to adjust the refcount of the subpages, unless we serialize put_page against split_huge_page and we keep track of the individual gup refcounts on the subpages. Which is what the compound_lock and the get_page/put_page changes achieve in a self contained manner without spreading all over the drivers and the VM. > > It's troublesome enough already to merge this right now in this > > non-intrusive <40 patches fully backwards compatible form, you got to > > think how I could merge this if I went ahead and break everything in > > gup. > > Its pretty bold to call this patchset non-intrusive. Not sure why you > think you have to break gup. Certainly works fine for page migration. page migration won't convert a compound page to a not compound page in the page structure in place. this is not page migration, this is about converting a page structure from PageCompound to not-page compound. It's all trivial on the pagetable side, what is not trivial is the refcounting created by GUP. The gup caller, at any later time will call put_page on a random subpage, so we've to adjust the refcount for subpages inside __split_huge_page_refcount, depending on which tailpage was returned by gup. > This implies that the current page migration approaches are broken? The > simple solution here is not to convert the page if there is a unresolved > reference. You are only in this bind because you insists on an "atomic" > conversion between 2M and 4k pages instead of using the existing code that > tracks down references to pages etc. Migration will bail out if gup is running. split_huge_page basic design is that it can't fail. So it can't bail out. And we don't want to call split_huge_page during follow_page before gup returns the page. That would also solve it, I did it initially but it's unacceptable to split hugepages across GUP. > Then why introdoce the crap in the first place! Get rid of your > fixation on atomic splitting of huge pages. It's not fixation, it's about trying to merge <40 patches compounding to 5000 lines of code, and being able to verify their correctness for the reviewers. And later expand in more places, starting from remap and mprotect. While already getting total benefit in the workload that benefits most from hugepages! If you can't see the _huge_ value of it, it's pointless to keep discussing this. I guess if it was you the 2.0 kernel maintainer, lock_kernel would have never happened... or maybe you would have designed a CPU back in 1995 to run at 2ghz on 45nm technology. Developments of technology happens _gradually_. Pretending an huge step forward that may take years before it can go productive because it requires so much manpower to audit and verify its correctness and nail down the bugs and bisect where the crash comes from, is unrealistic and it's not how things happens here. Sure if you've to build a bridge you are forced to go to the final stage immediately because it can't be changed ever again. But for us it's different and not taking advantage of being able to edit the code, and using the same engineering approach you would take while building a bridge, looks silly to me. > > But while this suggestion of yours totally misses the point of why > > split_huge_page, that this to avoid sending a patchset that is hugely > > bigger in size and much harder to audit and merge without much risk, > > than what I sent. > > Messing around with the basic refcounting and VM primitives is no risk? It's definitely zero risk if compared to what you're proposing. > > > Transparent huge page support better be introduced gradually starting f.e. > > > with the support of 2M pages for anonymous pages. > > > > "introduced gradually starting with the support of 2M pages for > > anonymous pages" is exactly what my patch does. What you're suggesting > > in the previous part of the email is the opposite! > > That support is possible without refcounts on tail pages and all the other > syncronization twiddles. It's possible only if gup calls split_huge_page or if we change the gup api (i.e. if we change every single put_page that releases a pin taken by gup). Just identifying the put_page will be a tremendous error-prone pain. It's real pity that we didn't: #define put_gup_page(page) put_page(page) and require the gup users to use that instead of raw put_page. That would have allowed to reasonably identify the gup users. In fact I considered trying to find all put_page that released a gup pin initially, try that yourself... I recommend to start with direct-io.c. They tried it to fix the race between fork and O_DIRECT by identifying the put_page pin release points and that resulted in crashes and no patch has been merged so far to fix it. I think that is a race that is much better fixed in the core, as the locking is more scalar in the core. I still value a lot to identify all put_page that releases gup pins, so that shall happen regardless of how we eventually fix the race between fork and O_DIRECT, I'm saying it here just to proof how not trivial and error prone that task is. Not remotely comparable to the few liner change to get_page/put_page. > > Handling hugepages natively everywhere and removing both compound_lock > > and split_huge_page would then require the swapcache to handle 2M > > pages. swapcache is sharing 100% of pagecache code so then pagecache > > would need to handle 2M pages natively too. Hence the moment we remove > > the split_huge_page and the moment we require swapcache to handle 2M > > natively without splitting the hugepage first like my patch does, the > > whole thing escalates way beyond anonymous pages, like you seem to > > agree that it's a good idea to start with. > > You can convert a 2M page to 4k pages without messing up the > basic refcounting and synchronization by following the way things are done > in other parts of the kernel. No other place of the kernel does anything remotely comparable to split_huge_page. > As far as I can tell there is no need for large scale patches as you > suggest. In fact it seems that the patches would be much smaller if > you would use the existing code that deals with page movement. Have a look > at Mel's defragmentation patches? defrag, migration they all can fail, split_huge_page cannot. The very simple reason split_huge_page cannot fail is this: if I have to do anything more than a one liner to make mremap, mprotect and all the rest, then I prefer to take your non-practical more risky design. The moment you have to alter some very inner pagetable walking function to handle a split_huge_page error return, you'll already have to recraft the code in a big enough way, that you better make it hugepage aware. Making it hugepage aware is like 10 times more difficult and error prone and hard to test, than handling a split_huge_page error retval, but still in 10 files fixed for the error retval, will be worth 1 file converted not to call split_huge_page at all. That explains very clearly my decision to make split_huge_page not fail, and make sure all next efforts will be spent in removing split_huge_page and not in handling an error retval for a function that shouldn't have been called in the first place! -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxxx For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>