On 1/11/19 7:25 PM, Jerome Glisse wrote: [...] >>>> Why is it that page lock cannot be used for gup fast, btw? >>> >>> Well it can not happen within the preempt disable section. But after >>> as a post pass before GUP_fast return and after reenabling preempt then >>> it is fine like it would be for regular GUP. But locking page for GUP >>> is also likely to slow down some workload (with direct-IO). >>> >> >> Right, and so to crux of the matter: taking an uncontended page lock involves >> pretty much the same set of operations that your approach does. (If gup ends >> up >> contended with the page lock for other reasons than these paths, that seems >> surprising.) I'd expect very similar performance. >> >> But the page lock approach leads to really dramatically simpler code (and >> code >> reviews, let's not forget). Any objection to my going that direction, and >> keeping >> this idea as a Plan B? I think the next step will be, once again, to gather >> some >> performance metrics, so maybe that will help us decide. > > They are already work load that suffer from the page lock so adding more > code that need it will only worsen those situations. I guess i will do a > patchset with my solution as it is definitly lighter weight that having to > take the page lock. >
Hi Jerome, I expect that you're right, and in any case, having you code up the new synchronization parts is probably a smart idea--you understand it best. To avoid duplicating work, may I propose these steps: 1. I'll post a new RFC, using your mapcount idea, but with a minor variation: using the page lock to synchronize gup() and page_mkclean(). a) I'll also include a github path that has enough gup callsite conversions done, to allow performance testing. b) And also, you and others have provided a lot of information that I want to turn into nice neat comments and documentation. 2. Then your proposed synchronization system would only need to replace probably one or two of the patches, instead of duplicating the whole patchset. I dread having two large, overlapping patchsets competing, and hope we can avoid that mess. 3. We can run performance tests on both approaches, hopefully finding some test cases that will highlight whether page lock is a noticeable problem here. Or, the other thing that could happen is someone will jump in here and NAK anything involving the page lock, based on long experience, and we'll just go straight to your scheme anyway. I'm sorta expecting that any minute now. :) thanks, -- John Hubbard NVIDIA
