On Wed, Aug 26, 2020 at 01:56:43PM +0000, Maoming (maoming, Cloud
Infrastructure Service Product Dept.) wrote:
> > + /*
> > + * Unlike THP, the splitting should not happen for hugetlb pages.
> > + * Since PG_reserved is not relevant for compound pages, and the pfn of
> > + * PAGE_SIZE page which in hugetlb pages is valid,
> > + * it is not necessary to check rsvd for hugetlb pages.
> > + * We do not need to alloc pages because of vaddr and we can finish all
> > + * work by a single operation to the head page.
> > + */
> > + atomic_add(contiguous_npage, compound_pincount_ptr(head));
> > + page_ref_add(head, contiguous_npage);
> > + mod_node_page_state(page_pgdat(head), NR_FOLL_PIN_ACQUIRED,
> > +contiguous_npage);
>
> I think I asked this question in v1, but I didn't get any answer... So I'm
> trying again...
>
> Could I ask why manual referencing of pages is done here rather than
> using
> pin_user_pages_remote() just like what we've done with
> vaddr_get_pfn(), and let
> try_grab_page() to do the page reference and accountings?
>
> I feel like this at least is against the FOLL_PIN workflow of gup, because
> those FOLL_PIN paths were bypassed, afaict.
>
>
> Hi,
> My apologies for not answering your question.
> As I understand, pin_user_pages_remote() might spend much time.
> Because all PAGE_SIZE-pages in a hugetlb page are pinned one by one in
> pin_user_pages_remote() and try_grab_page().
> So I think maybe we can use these simple code to do all work.
> Am I wrong? And is there something else we can use? For example
> :pin_user_pages_fast()
Yeah I can understand your concern, however so far it's not about the perf but
correctness. Documentation/core-api/pin_user_pages.rst tells us that we should
always use pin_user_page*() APIs to pin DMA pages (with FOLL_LONGTERM). That's
something we should follow for now, otherwise the major logic of either
FOLL_PIN or FULL_LONGTERM could be bypassed without being noticed.
I'm not sure whether the perf issue is a big one. So have you tried the pin
page APIs first and did some measurement? There is indeed a tight loop in
follow_hugetlb_page() however not sure how much it'll affect VFIO_IOMMU_MAP_DMA
in general. Even if we want to do something, it seems to be more suitable to
be done inside follow_hugetlb_page() rather than in vfio, imho.
Another comment is about the design of the whole patch - I think Alex commented
on that too on the awkwardness on appending the hugetlbfs logic to the end of
the existing logic. Considering that current logic of vfio_pin_pages_remote()
is "let's pin some pages as long as continuous", not sure whether we can make
it into:
vfio_pin_pages_remote()
{
if (PageHuge(first_page))
vfio_pin_pages_hugetlbfs();
else
vfio_pin_pages_normal();
}
The thing is, if the 1st page is normal page, then the follow-up pages
shouldn't normally be hugetlbfs pages so they won't be physically continuous.
Vice versa. In other words, each call to vfio_pin_pages_remote() should only
handle only one type of page after all. So maybe we can diverge them at the
beginning of the call directly.
--
Peter Xu
Thanks for your suggestions. I will fix it.
And I have another question.
In hugetlb_put_pfn(), I delete unpin_user_pages_dirty_lock() and use some
simple code to put hugetlb pages.
Is this right?
/*
* put pfns for a hugetlb page
* @start:the PAGE_SIZE-page we start to put,can be any page in this hugetlb
page
* @npage:the number of PAGE_SIZE-pages need to put
* @prot:IOMMU_READ/WRITE
*/
static int hugetlb_put_pfn(unsigned long start, unsigned int npage, int prot)
{
struct page *page;
struct page *head;
if (!npage || !pfn_valid(start))
return 0;
page = pfn_to_page(start);
if (!page || !PageHuge(page))
return 0;
head = compound_head(page);
/*
* The last page should be in this hugetlb page.
* The number of putting pages should be equal to the number
* of getting pages.So the hugepage pinned refcount and the normal
* page refcount can not be smaller than npage.
*/
if ((head != compound_head(pfn_to_page(start + npage - 1)))
|| (page_ref_count(head) < npage)
|| (compound_pincount(page) < npage))
return 0;
if ((prot & IOMMU_WRITE) && !PageDirty(page))
set_page_dirty_lock(page);
atomic_sub(npage, compound_pincount_ptr(head));
if (page_ref_sub_and_test(head, npage))
__put_page(head);
mod_node_page_state(page_pgdat(head), NR_FOLL_PIN_RELEASED, npage);
return 1;
}
