On Tue, 22 Oct 2019, Yang Shi wrote: > On 10/22/19 3:27 PM, Hugh Dickins wrote: > > > > I completely agree that the current PageTransCompoundMap() is wrong. > > > > A fix for that is one of many patches I've not yet got to upstreaming. > > Comparing yours and mine, I'm worried by your use of PageDoubleMap(), > > because really that's a flag for anon THP, and not properly supported > > on shmem (or now I suppose file) THP - I forget the details, is it > > that it sometimes gets set, but never cleared? Generally, we just > > don't refer to PageDoubleMap() on shmem THPs (but there may be > > exceptions: sorting out the THP mapcount maze, and eliminating > > PageDoubleMap(), is one of my long-held ambitions, not yet reached). > > > > Here's the patch I've been carrying, but it's from earlier, so I > > should warn that I've done no more than build-testing it on 5.4, > > and I'm too far away from these issues at the moment to be able to > > make a good judgement or argue for it - I hope you and others can > > decide which patch is the better. I should also add that we're > > barely using PageTransCompoundMap() at all: at best it can only > > give a heuristic guess as to whether the page is pmd-mapped in > > any particular case, and we preferred to take forward the KVM > > patches we posted back in April 2016, plumbing hva down to where > > it's needed - though of course those are somewhat different now. > > Thanks for catching this. I was definitely thinking about using > compount_mapcount instead of DoubleMap flag when I was working the patch. I > just simply thought it would change less file by using DoubleMap flag but I > didn't notice it was kind of unbalanced for file THP. > > With the unbalanced DoubleMap flag, it sounds better to use > compound_mapcount. Yes: no doubt PageDoubleMap could be fixed on shmem+file, but I have no interest in doing that, because it's just unnecessary overhead for them. (They have their own overhead, of subpage mapcounting for pmd: which is something to eliminate and unify with anon when I get around to it.) > > Thanks for sharing your patch, I'm going to rework v2 by using > compound_mapcount. Do you mind I might steal your patch? Please do! One less for me to worry about, thanks. > > I'm supposed we'd better fix this bug regardless of whether you would like to > move forward your KVM patches. Absolutely. There remain a few other uses of PageTransCompoundMap anyway, and I really wanted this outright mm fix to go in before re-submitting AndresLC's KVM patch (I'll ask a KVM-savvy colleague to take that over, Cc'ing you, once the mm end is correct). Hugh
