diff mbox series

[1/5] udmabuf: cancel mmap page fault, direct map it

Message ID 20240801104512.4056860-2-link@vivo.com (mailing list archive)
State Not Applicable
Headers
Series udmbuf bug fix and some improvements |

Commit Message

Huan Yang Aug. 1, 2024, 10:45 a.m. UTC 
  The current udmabuf mmap uses a page fault mechanism to populate the vma.

However, the current udmabuf has already obtained and pinned the folio
upon completion of the creation.This means that the physical memory has
already been acquired, rather than being accessed dynamically. The
current page fault method only saves some page table memory.

As a result, the page fault mechanism has lost its purpose as a demanding
page. Due to the fact that page fault requires trapping into kernel mode
and filling in when accessing the corresponding virtual address in mmap,
this means that user mode access to virtual addresses needs to trap into
kernel mode.

Therefore, when creating a large size udmabuf, this represents a
considerable overhead.

Therefore, the current patch removes the page fault method of mmap and
instead fills it directly when mmap is triggered.

Signed-off-by: Huan Yang <link@vivo.com>
---
 drivers/dma-buf/udmabuf.c | 70 ++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 31 deletions(-)

Comments

Christian König Aug. 1, 2024, 10:50 a.m. UTC | #1 
Am 01.08.24 um 12:45 schrieb Huan Yang:
> The current udmabuf mmap uses a page fault mechanism to populate the vma.
>
> However, the current udmabuf has already obtained and pinned the folio
> upon completion of the creation.This means that the physical memory has
> already been acquired, rather than being accessed dynamically. The
> current page fault method only saves some page table memory.
>
> As a result, the page fault mechanism has lost its purpose as a demanding
> page. Due to the fact that page fault requires trapping into kernel mode
> and filling in when accessing the corresponding virtual address in mmap,
> this means that user mode access to virtual addresses needs to trap into
> kernel mode.
>
> Therefore, when creating a large size udmabuf, this represents a
> considerable overhead.
>
> Therefore, the current patch removes the page fault method of mmap and
> instead fills it directly when mmap is triggered.
>
> Signed-off-by: Huan Yang <link@vivo.com>
> ---
>   drivers/dma-buf/udmabuf.c | 70 ++++++++++++++++++++++-----------------
>   1 file changed, 39 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 047c3cd2ceff..d69aeada7367 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -38,36 +38,39 @@ struct udmabuf_folio {
>   	struct list_head list;
>   };
>   
> -static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
> -{
> -	struct vm_area_struct *vma = vmf->vma;
> -	struct udmabuf *ubuf = vma->vm_private_data;
> -	pgoff_t pgoff = vmf->pgoff;
> -	unsigned long pfn;
> -
> -	if (pgoff >= ubuf->pagecount)
> -		return VM_FAULT_SIGBUS;
> -
> -	pfn = folio_pfn(ubuf->folios[pgoff]);
> -	pfn += ubuf->offsets[pgoff] >> PAGE_SHIFT;
> -
> -	return vmf_insert_pfn(vma, vmf->address, pfn);
> -}
> -
> -static const struct vm_operations_struct udmabuf_vm_ops = {
> -	.fault = udmabuf_vm_fault,
> -};
> +static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
> +				     enum dma_data_direction direction);
>   
>   static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
>   {
>   	struct udmabuf *ubuf = buf->priv;
> +	struct sg_table *table = ubuf->sg;
> +	unsigned long addr = vma->vm_start;
> +	struct sg_page_iter piter;
> +	int ret;
>   
>   	if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
>   		return -EINVAL;
>   
> -	vma->vm_ops = &udmabuf_vm_ops;
> -	vma->vm_private_data = ubuf;
> -	vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
> +	if (!table) {
> +		table = get_sg_table(NULL, buf, 0);
> +		if (IS_ERR(table))
> +			return PTR_ERR(table);
> +		ubuf->sg = table;
> +	}
> +
> +	for_each_sgtable_page(table, &piter, vma->vm_pgoff) {

That might not work correctly. We intentionally remove the pages from 
the sgtable when it is shared between devices.

Additional to that the sgtable is *not* a page container, but rather a 
DMA address container. So that here is also a rather bad idea from the 
design side.

Regards,
Christian.

> +		struct page *page = sg_page_iter_page(&piter);
> +
> +		ret = remap_pfn_range(vma, addr, page_to_pfn(page), PAGE_SIZE,
> +				      vma->vm_page_prot);
> +		if (ret)
> +			return ret;
> +		addr += PAGE_SIZE;
> +		if (addr >= vma->vm_end)
> +			return 0;
> +	}
> +
>   	return 0;
>   }
>   
> @@ -126,6 +129,10 @@ static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
>   		sg_set_folio(sgl, ubuf->folios[i], PAGE_SIZE,
>   			     ubuf->offsets[i]);
>   
> +	// if dev is NULL, no need to sync.
> +	if (!dev)
> +		return sg;
> +
>   	ret = dma_map_sgtable(dev, sg, direction, 0);
>   	if (ret < 0)
>   		goto err_map;
> @@ -206,20 +213,21 @@ static int begin_cpu_udmabuf(struct dma_buf *buf,
>   {
>   	struct udmabuf *ubuf = buf->priv;
>   	struct device *dev = ubuf->device->this_device;
> -	int ret = 0;
> +	struct sg_table *sg;
>   
> -	if (!ubuf->sg) {
> -		ubuf->sg = get_sg_table(dev, buf, direction);
> -		if (IS_ERR(ubuf->sg)) {
> -			ret = PTR_ERR(ubuf->sg);
> -			ubuf->sg = NULL;
> -		}
> -	} else {
> +	if (ubuf->sg) {
>   		dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents,
>   				    direction);
> +		return 0;
>   	}
>   
> -	return ret;
> +	sg = get_sg_table(dev, buf, direction);
> +	if (IS_ERR(sg))
> +		return PTR_ERR(sg);
> +
> +	ubuf->sg = sg;
> +
> +	return 0;
>   }
>   
>   static int end_cpu_udmabuf(struct dma_buf *buf,
Huan Yang Aug. 1, 2024, 11:08 a.m. UTC | #2 
在 2024/8/1 18:50, Christian König 写道:
> Am 01.08.24 um 12:45 schrieb Huan Yang:
>> The current udmabuf mmap uses a page fault mechanism to populate the 
>> vma.
>>
>> However, the current udmabuf has already obtained and pinned the folio
>> upon completion of the creation.This means that the physical memory has
>> already been acquired, rather than being accessed dynamically. The
>> current page fault method only saves some page table memory.
>>
>> As a result, the page fault mechanism has lost its purpose as a 
>> demanding
>> page. Due to the fact that page fault requires trapping into kernel mode
>> and filling in when accessing the corresponding virtual address in mmap,
>> this means that user mode access to virtual addresses needs to trap into
>> kernel mode.
>>
>> Therefore, when creating a large size udmabuf, this represents a
>> considerable overhead.
>>
>> Therefore, the current patch removes the page fault method of mmap and
>> instead fills it directly when mmap is triggered.
>>
>> Signed-off-by: Huan Yang <link@vivo.com>
>> ---
>>   drivers/dma-buf/udmabuf.c | 70 ++++++++++++++++++++++-----------------
>>   1 file changed, 39 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
>> index 047c3cd2ceff..d69aeada7367 100644
>> --- a/drivers/dma-buf/udmabuf.c
>> +++ b/drivers/dma-buf/udmabuf.c
>> @@ -38,36 +38,39 @@ struct udmabuf_folio {
>>       struct list_head list;
>>   };
>>   -static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
>> -{
>> -    struct vm_area_struct *vma = vmf->vma;
>> -    struct udmabuf *ubuf = vma->vm_private_data;
>> -    pgoff_t pgoff = vmf->pgoff;
>> -    unsigned long pfn;
>> -
>> -    if (pgoff >= ubuf->pagecount)
>> -        return VM_FAULT_SIGBUS;
>> -
>> -    pfn = folio_pfn(ubuf->folios[pgoff]);
>> -    pfn += ubuf->offsets[pgoff] >> PAGE_SHIFT;
>> -
>> -    return vmf_insert_pfn(vma, vmf->address, pfn);
>> -}
>> -
>> -static const struct vm_operations_struct udmabuf_vm_ops = {
>> -    .fault = udmabuf_vm_fault,
>> -};
>> +static struct sg_table *get_sg_table(struct device *dev, struct 
>> dma_buf *buf,
>> +                     enum dma_data_direction direction);
>>     static int mmap_udmabuf(struct dma_buf *buf, struct 
>> vm_area_struct *vma)
>>   {
>>       struct udmabuf *ubuf = buf->priv;
>> +    struct sg_table *table = ubuf->sg;
>> +    unsigned long addr = vma->vm_start;
>> +    struct sg_page_iter piter;
>> +    int ret;
>>         if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
>>           return -EINVAL;
>>   -    vma->vm_ops = &udmabuf_vm_ops;
>> -    vma->vm_private_data = ubuf;
>> -    vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
>> +    if (!table) {
>> +        table = get_sg_table(NULL, buf, 0);
>> +        if (IS_ERR(table))
>> +            return PTR_ERR(table);
>> +        ubuf->sg = table;
>> +    }
>> +
>> +    for_each_sgtable_page(table, &piter, vma->vm_pgoff) {
>
> That might not work correctly. We intentionally remove the pages from 
> the sgtable when it is shared between devices.
>
> Additional to that the sgtable is *not* a page container, but rather a 
> DMA address container. So that here is also a rather bad idea from the 
> design side.

Sorry for that and patch 1 3 4's ops. I was not aware of this before. 
All idea to do this in mmap/vmap is just like system_heap and any other 
heaps that I learned.

But well to learn it.

BTW, sgtable is a wrong idea to maintain page, maybe we need to both 
setup page's array(order 0 page), and folio's array(only the folio, use 
for unpin)?

Or else, mapping page to vm_off and vma solely through folio's array is 
quite challenging.
Moreover, even in this way, the memory overhead is smaller than the 
current unpin list.

Thanks for your correct.:)

>
> Regards,
> Christian.
>
>> +        struct page *page = sg_page_iter_page(&piter);
>> +
>> +        ret = remap_pfn_range(vma, addr, page_to_pfn(page), PAGE_SIZE,
>> +                      vma->vm_page_prot);
>> +        if (ret)
>> +            return ret;
>> +        addr += PAGE_SIZE;
>> +        if (addr >= vma->vm_end)
>> +            return 0;
>> +    }
>> +
>>       return 0;
>>   }
>>   @@ -126,6 +129,10 @@ static struct sg_table *get_sg_table(struct 
>> device *dev, struct dma_buf *buf,
>>           sg_set_folio(sgl, ubuf->folios[i], PAGE_SIZE,
>>                    ubuf->offsets[i]);
>>   +    // if dev is NULL, no need to sync.
>> +    if (!dev)
>> +        return sg;
>> +
>>       ret = dma_map_sgtable(dev, sg, direction, 0);
>>       if (ret < 0)
>>           goto err_map;
>> @@ -206,20 +213,21 @@ static int begin_cpu_udmabuf(struct dma_buf *buf,
>>   {
>>       struct udmabuf *ubuf = buf->priv;
>>       struct device *dev = ubuf->device->this_device;
>> -    int ret = 0;
>> +    struct sg_table *sg;
>>   -    if (!ubuf->sg) {
>> -        ubuf->sg = get_sg_table(dev, buf, direction);
>> -        if (IS_ERR(ubuf->sg)) {
>> -            ret = PTR_ERR(ubuf->sg);
>> -            ubuf->sg = NULL;
>> -        }
>> -    } else {
>> +    if (ubuf->sg) {
>>           dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents,
>>                       direction);
>> +        return 0;
>>       }
>>   -    return ret;
>> +    sg = get_sg_table(dev, buf, direction);
>> +    if (IS_ERR(sg))
>> +        return PTR_ERR(sg);
>> +
>> +    ubuf->sg = sg;
>> +
>> +    return 0;
>>   }
>>     static int end_cpu_udmabuf(struct dma_buf *buf,
>
diff mbox series

Patch

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 047c3cd2ceff..d69aeada7367 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -38,36 +38,39 @@  struct udmabuf_folio {
 	struct list_head list;
 };
 
-static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
-{
-	struct vm_area_struct *vma = vmf->vma;
-	struct udmabuf *ubuf = vma->vm_private_data;
-	pgoff_t pgoff = vmf->pgoff;
-	unsigned long pfn;
-
-	if (pgoff >= ubuf->pagecount)
-		return VM_FAULT_SIGBUS;
-
-	pfn = folio_pfn(ubuf->folios[pgoff]);
-	pfn += ubuf->offsets[pgoff] >> PAGE_SHIFT;
-
-	return vmf_insert_pfn(vma, vmf->address, pfn);
-}
-
-static const struct vm_operations_struct udmabuf_vm_ops = {
-	.fault = udmabuf_vm_fault,
-};
+static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
+				     enum dma_data_direction direction);
 
 static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
 {
 	struct udmabuf *ubuf = buf->priv;
+	struct sg_table *table = ubuf->sg;
+	unsigned long addr = vma->vm_start;
+	struct sg_page_iter piter;
+	int ret;
 
 	if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
 		return -EINVAL;
 
-	vma->vm_ops = &udmabuf_vm_ops;
-	vma->vm_private_data = ubuf;
-	vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
+	if (!table) {
+		table = get_sg_table(NULL, buf, 0);
+		if (IS_ERR(table))
+			return PTR_ERR(table);
+		ubuf->sg = table;
+	}
+
+	for_each_sgtable_page(table, &piter, vma->vm_pgoff) {
+		struct page *page = sg_page_iter_page(&piter);
+
+		ret = remap_pfn_range(vma, addr, page_to_pfn(page), PAGE_SIZE,
+				      vma->vm_page_prot);
+		if (ret)
+			return ret;
+		addr += PAGE_SIZE;
+		if (addr >= vma->vm_end)
+			return 0;
+	}
+
 	return 0;
 }
 
@@ -126,6 +129,10 @@  static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
 		sg_set_folio(sgl, ubuf->folios[i], PAGE_SIZE,
 			     ubuf->offsets[i]);
 
+	// if dev is NULL, no need to sync.
+	if (!dev)
+		return sg;
+
 	ret = dma_map_sgtable(dev, sg, direction, 0);
 	if (ret < 0)
 		goto err_map;
@@ -206,20 +213,21 @@  static int begin_cpu_udmabuf(struct dma_buf *buf,
 {
 	struct udmabuf *ubuf = buf->priv;
 	struct device *dev = ubuf->device->this_device;
-	int ret = 0;
+	struct sg_table *sg;
 
-	if (!ubuf->sg) {
-		ubuf->sg = get_sg_table(dev, buf, direction);
-		if (IS_ERR(ubuf->sg)) {
-			ret = PTR_ERR(ubuf->sg);
-			ubuf->sg = NULL;
-		}
-	} else {
+	if (ubuf->sg) {
 		dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents,
 				    direction);
+		return 0;
 	}
 
-	return ret;
+	sg = get_sg_table(dev, buf, direction);
+	if (IS_ERR(sg))
+		return PTR_ERR(sg);
+
+	ubuf->sg = sg;
+
+	return 0;
 }
 
 static int end_cpu_udmabuf(struct dma_buf *buf,