On 10/13/21 20:43, Jason Gunthorpe wrote:
On Wed, Oct 13, 2021 at 08:18:08PM +0100, Joao Martins wrote:
quoted
On 10/13/21 18:41, Jason Gunthorpe wrote:
quoted
On Mon, Oct 11, 2021 at 04:53:29PM +0100, Joao Martins wrote:
quoted
On 10/8/21 12:54, Jason Gunthorpe wrote:
quoted
quoted
The only optimization that might work here is to grab the head, then
compute the extent of tail pages and amalgamate them. Holding a ref on
the head also secures the tails.
How about pmd_page(orig) / pud_page(orig) like what the rest of hugetlb/thp
checks do? i.e. we would pass pmd_page(orig)/pud_page(orig) to __gup_device_huge()
as an added @head argument. While keeping the same structure of counting tail pages
between @addr .. @end if we have a head page.
The right logic is what everything else does:
page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
refs = record_subpages(page, addr, end, pages + *nr);
head = try_grab_compound_head(pud_page(orig), refs, flags);
If you can use this, or not, depends entirely on answering the
question of why does __gup_device_huge() exist at all.
So for device-dax it seems to be an untackled oversight[*], probably
inherited from when fsdax devmap was introduced. What I don't know
is the other devmap users :(
devmap generic infrastructure waits until all page refcounts go to
zero, and it should wait until any fast GUP is serialized as part of
the TLB shootdown - otherwise it is leaking access to the memory it
controls beyond it's shutdown
So, I don't think the different devmap users can break this?
maybe fsdax may not honor that after removing the pgmap ref count
as we talked earlier in the thread without the memory failures stuff.
But my point earlier on "oversight" was about the fact that we describe
PMD/PUDs with base pages. And hence why we can't do this:
page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
refs = record_subpages(page, addr, end, pages + *nr);
head = try_grab_compound_head(pud_page(orig), refs, flags);
... For devmap.
quoted
quoted
This I don't fully know:
1) As discussed quite a few times now, the entire get_dev_pagemap
stuff looks usless and should just be deleted. If you care about
optimizing this I would persue doing that as it will give the
biggest single win.
I am not questioning the well-deserved improvement -- but from a pure
optimization perspective the get_dev_pagemap() cost is not
visible and quite negligeble.
You are doing large enough GUPs then that the expensive xarray seach
is small compared to the rest?
The tests I showed are on 16G and 128G (I usually test with > 1Tb).
But we are comparing 1 pgmap lookup, and 512 refcount atomic updates for
a PMD sized pin (2M).
It depends on what you think small is. Maybe for a 4K-16K of memory pinned,
probably the pgmap lookup is more expensive.
A couple months ago I remember measuring xarray lookups and that would be
around ~150ns per lookup with few entries excluding the ref update. I can
be pedantic and give you a more concrete measurement for get_dev_pagemap()
but quite honestly don't think that it will be close with we pin a hugepage.
But this is orthogonal to the pgmap ref existence, I was merely commenting on
the performance aspect.
quoted
quoted
2) It breaks up the PUD/PMD into tail pages and scans them all
Why? Can devmap combine multiple compound_head's into the same PTE?
I am not aware of any other usage of compound pages for devmap struct pages
than this series. At least I haven't seen device-dax or fsdax using
this.
Let me ask this question differently, is this assertion OK?
I understood the question. I was more trying to learn from you on HMM/P2PDMA
use-cases as you sounded like compound pages exist elsewhere in devmap :)
quoted hunk ↗ jump to hunk
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -808,8 +808,13 @@ static void insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,
}
entry = pmd_mkhuge(pfn_t_pmd(pfn, prot));
- if (pfn_t_devmap(pfn))
+ if (pfn_t_devmap(pfn)) {
+ struct page *pfn_to_page(pfn);
+
+ WARN_ON(compound_head(page) != page);
+ WARN_ON(compound_order(page) != PMD_SHIFT);
entry = pmd_mkdevmap(entry);
+ }
if (write) {
entry = pmd_mkyoung(pmd_mkdirty(entry));
entry = maybe_pmd_mkwrite(entry, vma);
(and same for insert_pfn_pud)
You said it is OK for device/dax/device.c?
Yes, correct. (I also verified with similar snippet above to be
extra-pedantic)
I would like to emphasize that it is only OK *after this series*.
Without this series there is neither compound order or head, nor any
compound page whatsoever.
And not for fs/dax.c?
IIUC, Correct. fs/dax code is a little fuzzy on the sector to PFN
translation but still:
There's no compound pages in fsdax (neither do I add them). So
compound_order() doesn't exist (hence order 0) and there's no head
which also means that compound_head(@page) returns @page (which is
a tad misleading on being a head definition as a PMD subpage would
not return @page).
quoted
Unless HMM does this stuff, or some sort of devmap page migration? P2PDMA
doesn't seem to be (yet?) caught by any of the GUP path at least before
Logan's series lands. Or am I misunderstanding things here?
Of the places that call the insert_pfn_pmd/pud call chains I only see
device/dax/device.c and fs/dax.c as being linked to devmap. So other
devmap users don't use this stuff.
quoted
I was changing __gup_device_huge() with similar to the above, but yeah
it follows that algorithm as inside your do { } while() (thanks!). I can
turn __gup_device_huge() into another (renamed to like try_grab_pages())
helper and replace the callsites of gup_huge_{pud,pmd} for the THP/hugetlbfs
equivalent handling.
I suppose it should be some #define because the (pmd_val != orig) logic
is not sharable
I wasn't going to have that pmd_val() in there. Just this refcount part:
page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
refs = record_subpages(page, addr, end, pages + *nr);
head = try_grab_compound_head(pud_page(orig), refs, flags);
and __gup_device_huge() melded in.
But, yes, a general call that the walker should make at any level to
record a pfn -> npages range efficiently.
quoted
I think the right answer is "depends on the devmap" type. device-dax with
PMD/PUDs (i.e. 2m pagesize PMEM or 1G pagesize pmem) works with the same
rules as hugetlbfs. fsdax not so much (as you say above) but it would
follow up changes to perhaps adhere to similar scheme (not exactly sure
how do deal with holes). HMM I am not sure what the rules are there.
P2PDMA seems not applicable?
I would say, not "depends on the devmap", but what are the rules for
calling insert_pfn_pmd in the first place.
I am gonna translate that into HMM and P2PDMA so far aren't affected as you
also said earlier above. Even after Logan's P2PDMA NVME series it appears is
always PTEs. So fsdax and device-dax are the only pmd_devmap users we
should care, and for pud_devmap() only device-dax.
If users are allowed the create pmds that span many compound_head's
then we need logic as I showed. Otherwise we do not.
/me nods.
And I would document this relationship in the GUP side "This do/while
is required because insert_pfn_pmd/pud() is used with compound pages
smaller than the PUD/PMD size" so it isn't so confused with just
"devmap"
Also, it's not that PMDs span compound heads, it's that PMDs/PUDs use
just base pages. Compound pages/head in devmap are only introduced by
series and for device-dax only.