On Tue, Aug 31, 2021 at 01:34:04PM +0100, Joao Martins wrote:
On 8/30/21 2:07 PM, Jason Gunthorpe wrote:
quoted
On Fri, Aug 27, 2021 at 07:34:54PM +0100, Joao Martins wrote:
quoted
On 8/27/21 5:25 PM, Jason Gunthorpe wrote:
quoted
On Fri, Aug 27, 2021 at 03:58:13PM +0100, Joao Martins wrote:
quoted
#if defined(CONFIG_ARCH_HAS_PTE_DEVMAP) && defined(CONFIG_TRANSPARENT_HUGEPAGE)
static int __gup_device_huge(unsigned long pfn, unsigned long addr,
unsigned long end, unsigned int flags,
struct page **pages, int *nr)
{
- int nr_start = *nr;
+ int refs, nr_start = *nr;
struct dev_pagemap *pgmap = NULL;
int ret = 1;
do {
- struct page *page = pfn_to_page(pfn);
+ struct page *head, *page = pfn_to_page(pfn);
+ unsigned long next = addr + PAGE_SIZE;
pgmap = get_dev_pagemap(pfn, pgmap);
if (unlikely(!pgmap)) {@@ -2252,16 +2265,25 @@ static int __gup_device_huge(unsigned long pfn, unsigned long addr,
ret = 0;
break;
}
- SetPageReferenced(page);
- pages[*nr] = page;
- if (unlikely(!try_grab_page(page, flags))) {
- undo_dev_pagemap(nr, nr_start, flags, pages);
+
+ head = compound_head(page);
+ /* @end is assumed to be limited at most one compound page */
+ if (PageHead(head))
+ next = end;
+ refs = record_subpages(page, addr, next, pages + *nr);
+
+ SetPageReferenced(head);
+ if (unlikely(!try_grab_compound_head(head, refs, flags))) {
+ if (PageHead(head))
+ ClearPageReferenced(head);
+ else
+ undo_dev_pagemap(nr, nr_start, flags, pages);
ret = 0;
break;
Why is this special cased for devmap?
Shouldn't everything processing pud/pmds/etc use the same basic loop
that is similar in idea to the 'for_each_compound_head' scheme in
unpin_user_pages_dirty_lock()?
Doesn't that work for all the special page type cases here?
We are iterating over PFNs to create an array of base pages (regardless of page table
type), rather than iterating over an array of pages to work on.
That is part of it, yes, but the slow bit here is to minimally find
the head pages and do the atomics on them, much like the
unpin_user_pages_dirty_lock()
I would think this should be designed similar to how things work on
the unpin side.
I don't think it's the same thing. The bit you say 'minimally find the
head pages' carries a visible overhead in unpin_user_pages() as we are
checking each of the pages belongs to the same head page -- because you
can pass an arbritary set of pages. This does have a cost which is not
in gup-fast right now AIUI. Whereas in our gup-fast 'handler' you
already know that you are processing a contiguous chunk of pages.
If anything, we are closer to unpin_user_page_range*()
than unpin_user_pages().
Yes, that is what I mean, it is very similar to the range case as we
don't even know that a single compound spans a pud/pmd. So you end up
doing the same loop to find the compound boundaries.
Under GUP slow we can also aggregate multiple page table entires, eg a
split huge page could be procesed as a single 2M range operation even
if it is broken to 4K PTEs.
Switching to similar iteration logic to unpin would look something like
this (still untested):
for_each_compound_range(index, &page, npages, head, refs) {
pgmap = get_dev_pagemap(pfn + *nr, pgmap);
I recall talking to DanW about this and we agreed it was unnecessary
here to hold the pgmap and should be deleted.
Jason