On 10/22/19 6:31 PM, Hugh Dickins wrote:
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.)
It might be worth fixing the unbalance since mlock depends on this flag
too. There should be a little bit overhead when handling PTE rmap remove
since we have to iterate every subpage to check if _mapcount is same
with compound_mapcount or not in order to clear DoubleMap flag. It is
easy to handle this when the last PMD map is gone.
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).
Thanks.
Hugh
