RE: [PATCH] vfio dma_map/unmap: optimized for hugetlbfs pages

[Zhoujian (jay)]

Thanks for taking a close look at the code, Alex.
We'll check them one by one ASAP.

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
> Sent: Tuesday, July 21, 2020 6:46 AM
> To: Zhoujian (jay) <jianjay.zhou@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; cohuck@xxxxxxxxxx;
> Maoming (maoming, Cloud Infrastructure Service Product Dept.)
> <maoming.maoming@xxxxxxxxxx>; Huangweidong (C)
> <weidong.huang@xxxxxxxxxx>; Peter Xu <peterx@xxxxxxxxxx>; Andrea
> Arcangeli <aarcange@xxxxxxxxxx>
> Subject: Re: [PATCH] vfio dma_map/unmap: optimized for hugetlbfs pages
> 
> On Mon, 20 Jul 2020 16:29:47 +0800
> Jay Zhou <jianjay.zhou@xxxxxxxxxx> wrote:
> 
> > From: Ming Mao <maoming.maoming@xxxxxxxxxx>
> >
> > Hi all,
> > I'm working on starting lots of big size Virtual Machines(memory:
> > >128GB) with VFIO-devices. And I encounter a problem that is the
> > waiting time of starting all Virtual Machines is too long. I analyze
> > the startup log and find that the time of pinning/unpinning pages
> > could be reduced.
> >
> > In the original process, to make sure the pages are contiguous, we
> > have to check all pages one by one. I think maybe we can use hugetlbfs
> > pages which can skip this step.
> > So I create a patch to do this.
> > According to my test, the result of this patch is pretty well.
> >
> > Virtual Machine: 50G memory, 32 CPU, 1 VFIO-device, 1G hugetlbfs page
> >         original   after optimization
> > pin time   700ms          0.1ms
> >
> > I Suppose that:
> > 1)the hugetlbfs page should not be split 2)PG_reserved is not relevant
> > for hugetlbfs pages 3)we can delete the for loops and use some
> > operations (such as atomic_add,page_ref_add) instead
> >
> > please correct me if I am wrong.
> >
> > Thanks.
> >
> > Signed-off-by: Ming Mao <maoming.maoming@xxxxxxxxxx>
> > ---
> >  drivers/vfio/vfio_iommu_type1.c | 236 ++++++++++++++++++++++++++++++--
> >  include/linux/vfio.h            |  20 +++
> >  2 files changed, 246 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > b/drivers/vfio/vfio_iommu_type1.c index 5e556ac91..42e25752e 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -415,6 +415,46 @@ static int put_pfn(unsigned long pfn, int prot)
> >  	return 0;
> >  }
> >
> > +/*
> > + * put pfns for a hugetlbfs page
> > + * @start:the 4KB-page we start to put,can be any page in this
> > +hugetlbfs page
> > + * @npage:the number of 4KB-pages need to put
> 
> This code supports systems where PAGE_SIZE is not 4KB.
> 
> > + * @prot:IOMMU_READ/WRITE
> > + */
> > +static int hugetlb_put_pfn(unsigned long start, unsigned int npage,
> > +int prot) {
> > +	struct page *page = NULL;
> > +	struct page *head = NULL;
> 
> Unnecessary initialization.
> 
> > +
> > +	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 hugetlbfs 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;
> > +}
> > +
> >  static int follow_fault_pfn(struct vm_area_struct *vma, struct mm_struct
> *mm,
> >  			    unsigned long vaddr, unsigned long *pfn,
> >  			    bool write_fault)
> > @@ -479,6 +519,90 @@ static int vaddr_get_pfn(struct mm_struct *mm,
> unsigned long vaddr,
> >  	return ret;
> >  }
> >
> > +struct vfio_hupetlbpage_info vfio_hugetlbpage_info[HUGE_MAX_HSTATE] = {
> > +	{vfio_hugetlbpage_2M, PMD_SIZE, ~((1ULL << HPAGE_PMD_SHIFT) - 1)},
> > +	{vfio_hugetlbpage_1G, PUD_SIZE, ~((1ULL << HPAGE_PUD_SHIFT) - 1)},
> 
> Other architectures support more huge page sizes, also 0-day identified these
> #defines don't exist when THP is not configured.  But why couldn't we figure out
> all of these form the compound_order()?  order == shift, size = PAGE_SIZE <<
> order.
> 
> > +};
> > +
> > +static bool is_hugetlbpage(unsigned long pfn, enum
> > +vfio_hugetlbpage_type *type) {
> > +	struct page *page = NULL;
> 
> Unnecessary initialization.
> 
> > +
> > +	if (!pfn_valid(pfn) || !type)
> > +		return false;
> > +
> > +	page = pfn_to_page(pfn);
> > +	/* only check for hugetlbfs pages */
> > +	if (!page || !PageHuge(page))
> > +		return false;
> > +
> > +	switch (compound_order(compound_head(page))) {
> > +	case PMD_ORDER:
> > +		*type = vfio_hugetlbpage_2M;
> > +		break;
> > +	case PUD_ORDER:
> > +		*type = vfio_hugetlbpage_1G;
> > +		break;
> > +	default:
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +/* Is the addr in the last page in hugetlbfs pages? */ static bool
> > +hugetlb_is_last_page(unsigned long addr, enum vfio_hugetlbpage_type
> > +type) {
> > +	unsigned int num = 0;
> 
> Unnecessary initialization, and in fact unnecessary variable altogether.
> ie.
> 
> 	return hugetlb_get_resdual_pages(addr & ~(PAGE_SIZE - 1), type) == 1;
> 
> 
> > +
> > +	num = hugetlb_get_resdual_pages(addr & ~(PAGE_SIZE - 1), type);
> 
> residual?
> 
> > +
> > +	if (num == 1)
> > +		return true;
> > +	else
> > +		return false;
> > +}
> > +
> > +static bool hugetlb_page_is_pinned(struct vfio_dma *dma,
> > +				unsigned long start,
> > +				unsigned long npages)
> > +{
> > +	struct vfio_pfn *vpfn = NULL;
> 
> Unnecessary initialization.
> 
> > +	struct rb_node *node = rb_first(&dma->pfn_list);
> > +	unsigned long end = start + npages - 1;
> > +
> > +	for (; node; node = rb_next(node)) {
> > +		vpfn = rb_entry(node, struct vfio_pfn, node);
> > +
> > +		if ((vpfn->pfn >= start) && (vpfn->pfn <= end))
> > +			return true;
> 
> This function could be named better, it suggests the hugetlbfs page is pinned, but
> really we're only looking for any pfn_list pinnings overlapping the pfn range.
> 
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +static unsigned int hugetlb_get_contiguous_pages_num(struct vfio_dma
> *dma,
> > +						unsigned long pfn,
> > +						unsigned long resdual_npage,
> > +						unsigned long max_npage)
> > +{
> > +	unsigned int num = 0;
> 
> Unnecessary initialization
> 
> > +
> > +	if (!dma)
> > +		return 0;
> > +
> > +	num = resdual_npage < max_npage ? resdual_npage : max_npage;
> 
> min(resdual_npage, max_npage)
> 
> 
> > +	/*
> > +	 * If there is only one page, it is no need to optimize them.
> 
> s/no need/not necessary/
> 
> > +	 * Maybe some pages have been pinned and inserted into dma->pfn_list by
> others.
> > +	 * In this case, we just goto the slow path simply.
> > +	 */
> > +	if ((num < 2) || hugetlb_page_is_pinned(dma, pfn, num))
> > +		return 0;
> 
> Why does having pinnings in the pfn_list disqualify it from hugetlbfs pinning?
> 
> Testing for the last page here is redundant to the pinning path, should it only be
> done here?  Can num be zero?
> 
> Maybe better to return -errno for this and above zero conditions rather than
> return a value that doesn't reflect the purpose of the function?
> 
> > +
> > +	return num;
> > +}
> > +
> >  /*
> >   * Attempt to pin pages.  We really don't want to track all the pfns and
> >   * the iommu can only map chunks of consecutive pfns anyway, so get
> > the @@ -492,6 +616,7 @@ static long vfio_pin_pages_remote(struct vfio_dma
> *dma, unsigned long vaddr,
> >  	long ret, pinned = 0, lock_acct = 0;
> >  	bool rsvd;
> >  	dma_addr_t iova = vaddr - dma->vaddr + dma->iova;
> > +	enum vfio_hugetlbpage_type type;
> >
> >  	/* This code path is only user initiated */
> >  	if (!current->mm)
> > @@ -521,6 +646,55 @@ static long vfio_pin_pages_remote(struct vfio_dma
> *dma, unsigned long vaddr,
> >  	if (unlikely(disable_hugepages))
> >  		goto out;
> >
> > +	/*
> > +	 * It is no need to get pages one by one for hugetlbfs pages.
> 
> s/no need/not necessary/
> 
> > +	 * 4KB-pages in hugetlbfs pages are contiguous.
> > +	 * But if the vaddr is in the last 4KB-page, we just goto the slow path.
> 
> 
> s/4KB-/PAGE_SIZE /
> 
> Please explain the significance of vaddr being in the last PAGE_SIZE page of a
> hugetlbfs page.  Is it simply that we should take the slow path for mapping a
> single page before the end of the hugetlbfs page?
> Is this optimization worthwhile?  Isn't it more consistent to handle all mappings
> over hugetlbfs pages the same?  Should we be operating on vaddr here for the
> hugetlbfs page alignment or base_pfn?
> 
> > +	 */
> > +	if (is_hugetlbpage(*pfn_base, &type) && !hugetlb_is_last_page(vaddr,
> type)) {
> > +		unsigned long hugetlb_resdual_npage = 0;
> > +		unsigned long contiguous_npage = 0;
> > +		struct page *head = NULL;
> > +
> > +		hugetlb_resdual_npage =
> > +			hugetlb_get_resdual_pages((vaddr + PAGE_SIZE) & ~(PAGE_SIZE -
> 1),
> > +type);
> 
> ~(PAGE_SIZE - 1) is PAGE_MASK, but that whole operation looks like a
> PAGE_ALIGN(vaddr)
> 
> This is trying to get the number of pages after this page to the end of the
> hugetlbfs page, right?  Rather than the hugetlb_is_last_page() above, shouldn't
> we have recorded the number of pages there and used math to get this value?
> 
> > +		/*
> > +		 * Maybe the hugetlb_resdual_npage is invalid.
> > +		 * For example, hugetlb_resdual_npage > (npage - 1) or
> > +		 * some pages of this hugetlbfs page have been pinned.
> > +		 */
> > +		contiguous_npage = hugetlb_get_contiguous_pages_num(dma,
> *pfn_base + 1,
> > +						hugetlb_resdual_npage, npage - 1);
> > +		if (!contiguous_npage)
> > +			goto slow_path;
> > +
> > +		/*
> > +		 * Unlike THP, the splitting should not happen for hugetlbfs pages.
> > +		 * Since PG_reserved is not relevant for compound pages, and the pfn
> of
> > +		 * 4KB-page which in hugetlbfs pages is valid,
> 
> s/4KB/PAGE_SIZE/
> 
> > +		 * it is no need to check rsvd for hugetlbfs pages.
> 
> s/no need/not necessary/
> 
> > +		 */
> > +		if (!dma->lock_cap &&
> > +		    current->mm->locked_vm + lock_acct + contiguous_npage > limit)
> {
> > +			pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n",
> > +				 __func__, limit << PAGE_SHIFT);
> > +			ret = -ENOMEM;
> > +			goto unpin_out;
> > +		}
> > +		/*
> > +		 * We got a hugetlbfs page using vaddr_get_pfn alreadly.
> > +		 * In this case,we do not need to alloc pages and we can finish all
> > +		 * work by a single operation to the head page.
> > +		 */
> > +		lock_acct += contiguous_npage;
> > +		head = compound_head(pfn_to_page(*pfn_base));
> > +		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);
> > +		pinned += contiguous_npage;
> > +		goto out;
> 
> I'm hoping Peter or Andrea understand this, but I think we still have pfn_base
> pinned separately and I don't see that we've done an unpin anywhere, so are we
> leaking the pin of the first page??
> 
> > +	}
> > +slow_path:
> >  	/* Lock all the consecutive pages from pfn_base */
> >  	for (vaddr += PAGE_SIZE, iova += PAGE_SIZE; pinned < npage;
> >  	     pinned++, vaddr += PAGE_SIZE, iova += PAGE_SIZE) { @@ -569,7
> > +743,30 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma,
> > dma_addr_t iova,  {
> >  	long unlocked = 0, locked = 0;
> >  	long i;
> > +	enum vfio_hugetlbpage_type type;
> > +
> > +	if (is_hugetlbpage(pfn, &type)) {
> > +		unsigned long hugetlb_resdual_npage = 0;
> > +		unsigned long contiguous_npage = 0;
> 
> Unnecessary initialization...
> 
> >
> > +		hugetlb_resdual_npage = hugetlb_get_resdual_pages(iova &
> > +~(PAGE_SIZE - 1), type);
> 
> PAGE_MASK
> 
> Like above, is it pfn or iova that we should be using when looking at hugetlbfs
> alignment?
> 
> > +		contiguous_npage = hugetlb_get_contiguous_pages_num(dma, pfn,
> > +						hugetlb_resdual_npage, npage);
> > +		/*
> > +		 * There is not enough contiguous pages or this hugetlbfs page
> > +		 * has been pinned.
> > +		 * Let's try the slow path.
> > +		 */
> > +		if (!contiguous_npage)
> > +			goto slow_path;
> > +
> > +		/* try the slow path if failed */
> > +		if (hugetlb_put_pfn(pfn, contiguous_npage, dma->prot)) {
> > +			unlocked = contiguous_npage;
> > +			goto out;
> > +		}
> 
> Should probably break the pin path into a separate get_pfn function for
> symmetry.
> 
> 
> > +	}
> > +slow_path:
> >  	for (i = 0; i < npage; i++, iova += PAGE_SIZE) {
> >  		if (put_pfn(pfn++, dma->prot)) {
> >  			unlocked++;
> > @@ -578,6 +775,7 @@ static long vfio_unpin_pages_remote(struct vfio_dma
> *dma, dma_addr_t iova,
> >  		}
> >  	}
> >
> > +out:
> >  	if (do_accounting)
> >  		vfio_lock_acct(dma, locked - unlocked, true);
> >
> > @@ -867,6 +1065,7 @@ static long vfio_unmap_unpin(struct vfio_iommu
> *iommu, struct vfio_dma *dma,
> >  	struct iommu_iotlb_gather iotlb_gather;
> >  	int unmapped_region_cnt = 0;
> >  	long unlocked = 0;
> > +	enum vfio_hugetlbpage_type type;
> >
> >  	if (!dma->size)
> >  		return 0;
> > @@ -900,16 +1099,33 @@ static long vfio_unmap_unpin(struct vfio_iommu
> *iommu, struct vfio_dma *dma,
> >  			continue;
> >  		}
> >
> > -		/*
> > -		 * To optimize for fewer iommu_unmap() calls, each of which
> > -		 * may require hardware cache flushing, try to find the
> > -		 * largest contiguous physical memory chunk to unmap.
> > -		 */
> > -		for (len = PAGE_SIZE;
> > -		     !domain->fgsp && iova + len < end; len += PAGE_SIZE) {
> > -			next = iommu_iova_to_phys(domain->domain, iova + len);
> > -			if (next != phys + len)
> > -				break;
> > +		if (is_hugetlbpage((phys >> PAGE_SHIFT), &type)
> > +		    && (!domain->fgsp)) {
> 
> Reverse the order of these tests.
> 
> > +			unsigned long hugetlb_resdual_npage = 0;
> > +			unsigned long contiguous_npage = 0;
> 
> 
> Unnecessary...
> 
> > +			hugetlb_resdual_npage =
> > +				hugetlb_get_resdual_pages(iova & ~(PAGE_SIZE - 1), type);
> 
> PAGE_MASK
> 
> > +			/*
> > +			 * The number of contiguous page can not be larger than
> dma->size
> > +			 * which is the number of pages pinned.
> > +			 */
> > +			contiguous_npage = ((dma->size >> PAGE_SHIFT) >
> hugetlb_resdual_npage) ?
> > +				hugetlb_resdual_npage : (dma->size >> PAGE_SHIFT);
> 
> min()
> 
> > +
> > +			len = contiguous_npage * PAGE_SIZE;
> > +		} else {
> > +			/*
> > +			 * To optimize for fewer iommu_unmap() calls, each of which
> > +			 * may require hardware cache flushing, try to find the
> > +			 * largest contiguous physical memory chunk to unmap.
> > +			 */
> > +			for (len = PAGE_SIZE;
> > +			     !domain->fgsp && iova + len < end; len += PAGE_SIZE) {
> > +				next = iommu_iova_to_phys(domain->domain, iova + len);
> > +				if (next != phys + len)
> > +					break;
> > +			}
> >  		}
> >
> >  		/*
> > diff --git a/include/linux/vfio.h b/include/linux/vfio.h index
> > 38d3c6a8d..91ef2058f 100644
> > --- a/include/linux/vfio.h
> > +++ b/include/linux/vfio.h
> > @@ -214,4 +214,24 @@ extern int vfio_virqfd_enable(void *opaque,
> >  			      void *data, struct virqfd **pvirqfd, int fd);  extern void
> > vfio_virqfd_disable(struct virqfd **pvirqfd);
> >
> > +enum vfio_hugetlbpage_type {
> > +	vfio_hugetlbpage_2M,
> > +	vfio_hugetlbpage_1G,
> > +};
> > +
> > +struct vfio_hupetlbpage_info {
> > +	enum vfio_hugetlbpage_type type;
> 
> The enum within the structure serves no purpose.
> 
> > +	unsigned long size;
> > +	unsigned long mask;
> > +};
> > +
> > +#define PMD_ORDER 9
> > +#define PUD_ORDER 18
> 
> Architecture specific.
> 
> > +/*
> > + * get the number of resdual 4KB-pages in a hugetlbfs page
> > + * (including the page which pointed by this address)
> 
> s/4KB/PAGE_SIZE/
> 
> > + */
> > +#define hugetlb_get_resdual_pages(address, type)				\
> > +		((vfio_hugetlbpage_info[type].size				\
> > +		- (address & ~vfio_hugetlbpage_info[type].mask)) >> PAGE_SHIFT)
> >  #endif /* VFIO_H */