Re: [PATCH v4 5/5] udmabuf: remove udmabuf_folio

[Huan Yang <link@vivo.com>]

在 2024/8/29 14:47, Kasireddy, Vivek 写道:
> Hi Huan,
>
>> Subject: [PATCH v4 5/5] udmabuf: remove udmabuf_folio
>>
>> Currently, udmabuf handles folio by creating an unpin list to record
>> each folio obtained from the list and unpinning them when released. To
>> maintain this approach, many data structures have been established.
>>
>> However, maintaining this type of data structure requires a significant
>> amount of memory and traversing the list is a substantial overhead,
>> which is not friendly to the CPU cache.
>>
>> We actually don't need to use unpin_list to unpin during release.
>> Instead, just use a folios array record each folio is ok.
>> Compare udmabuf_folio 24 byte, folio array is 8 byte. Even if array need
>> to be pgcnt*8, may waste some memory when use large folio.
>> The access of array is faster than list, also, if 4K, array can also
>> save memory than list.
>>
>> Signed-off-by: Huan Yang <link@vivo.com>
>> ---
>>   drivers/dma-buf/udmabuf.c | 80 ++++++++++++++++++---------------------
>>   1 file changed, 37 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
>> index eb55bb4a5fcc..a45cec1f82b3 100644
>> --- a/drivers/dma-buf/udmabuf.c
>> +++ b/drivers/dma-buf/udmabuf.c
>> @@ -27,15 +27,21 @@ MODULE_PARM_DESC(size_limit_mb, "Max size of a
>> dmabuf, in megabytes. Default is
>>   struct udmabuf {
>>   	pgoff_t pagecount;
>>   	struct folio **folios;
>> +
>> +	/**
>> +	 * Unlike folios, pinned_folios is only used for unpin.
>> +	 * So, nr_pinned is not the same to pagecount, the pinned_folios
>> +	 * only set each folio which already pinned when udmabuf_create.
>> +	 * Note that, since a folio may be pinned multiple times, each folio
>> +	 * can be added to pinned_folios multiple times, depending on how
>> many
>> +	 * times the folio has been pinned when create.
>> +	 */
>> +	pgoff_t nr_pinned;
>> +	struct folio **pinned_folios;
>> +
>>   	struct sg_table *sg;
>>   	struct miscdevice *device;
>>   	pgoff_t *offsets;
>> -	struct list_head unpin_list;
>> -};
>> -
>> -struct udmabuf_folio {
>> -	struct folio *folio;
>> -	struct list_head list;
>>   };
>>
>>   static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
>> @@ -189,32 +195,12 @@ static void unmap_udmabuf(struct
>> dma_buf_attachment *at,
>>   	return put_sg_table(at->dev, sg, direction);
>>   }
>>
>> -static void unpin_all_folios(struct list_head *unpin_list)
>> -{
>> -	struct udmabuf_folio *ubuf_folio;
>> -
>> -	while (!list_empty(unpin_list)) {
>> -		ubuf_folio = list_first_entry(unpin_list,
>> -					      struct udmabuf_folio, list);
>> -		unpin_folio(ubuf_folio->folio);
>> -
>> -		list_del(&ubuf_folio->list);
>> -		kfree(ubuf_folio);
>> -	}
>> -}
>> -
>> -static int add_to_unpin_list(struct list_head *unpin_list,
>> -			     struct folio *folio)
>> +static __always_inline void unpin_all_folios(struct udmabuf *ubuf)
>>   {
>> -	struct udmabuf_folio *ubuf_folio;
>> -
>> -	ubuf_folio = kzalloc(sizeof(*ubuf_folio), GFP_KERNEL);
>> -	if (!ubuf_folio)
>> -		return -ENOMEM;
>> +	pgoff_t i;
>>
>> -	ubuf_folio->folio = folio;
>> -	list_add_tail(&ubuf_folio->list, unpin_list);
>> -	return 0;
>> +	for (i = 0; i < ubuf->nr_pinned; ++i)
>> +		unpin_folio(ubuf->pinned_folios[i]);
>>   }
>>
>>   static void release_udmabuf(struct dma_buf *buf)
>> @@ -225,7 +211,8 @@ static void release_udmabuf(struct dma_buf *buf)
>>   	if (ubuf->sg)
>>   		put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);
>>
>> -	unpin_all_folios(&ubuf->unpin_list);
>> +	unpin_all_folios(ubuf);
>> +	kvfree(ubuf->pinned_folios);
> Move the kvfree() of pinned_folios into unpin_all_folios().
>
>>   	kvfree(ubuf->offsets);
>>   	kvfree(ubuf->folios);
>>   	kfree(ubuf);
>> @@ -326,9 +313,9 @@ static int __udmabuf_pin_list_folios(struct
>> udmabuf_create_item *item,
>>   				     struct folio **folios)
>>   {
>>   	struct file *memfd = NULL;
>> -	pgoff_t pgoff, ipgcnt, upgcnt = ubuf->pagecount;
>> +	pgoff_t pgoff, ipgcnt, upgcnt, nr_pinned;
>>   	u32 cur_folio, cur_pgcnt;
>> -	struct folio **ubuf_folios;
>> +	struct folio **ubuf_folios, **pinned_folios;
>>   	pgoff_t *ubuf_offsets;
>>   	long nr_folios;
>>   	loff_t end, start;
>> @@ -351,22 +338,21 @@ static int __udmabuf_pin_list_folios(struct
>> udmabuf_create_item *item,
>>   	}
>>
>>   	cur_pgcnt = 0;
>> +	nr_pinned = ubuf->nr_pinned;
>> +	upgcnt = ubuf->pagecount;
>>   	ubuf_folios = ubuf->folios;
>>   	ubuf_offsets = ubuf->offsets;
>> +	pinned_folios = ubuf->pinned_folios;
>>
>>   	for (cur_folio = 0; cur_folio < nr_folios; ++cur_folio) {
>>   		pgoff_t subpgoff = pgoff;
>>   		long fsize = folio_size(folios[cur_folio]);
>>
>> -		ret = add_to_unpin_list(&ubuf->unpin_list, folios[cur_folio]);
>> -		if (ret < 0) {
>> -			kfree(folios);
>> -			goto err;
>> -		}
>> +		pinned_folios[nr_pinned++] = folios[cur_folio];
>>
>>   		for (; subpgoff < fsize; subpgoff += PAGE_SIZE) {
>> -			ubuf->folios[upgcnt] = folios[cur_folio];
>> -			ubuf->offsets[upgcnt] = subpgoff;
>> +			ubuf_folios[upgcnt] = folios[cur_folio];
>> +			ubuf_offsets[upgcnt] = subpgoff;
> These are unrelated changes that should belong to the previous patch. I
> suggest moving this patch before the codestyle cleanup patch.
Yes, sorry for this.
>
> Thanks,
> Vivek
>
>>   			++upgcnt;
>>
>>   			if (++cur_pgcnt >= ipgcnt)
>> @@ -381,12 +367,12 @@ static int __udmabuf_pin_list_folios(struct
>> udmabuf_create_item *item,
>>   	}
>>   end:
>>   	ubuf->pagecount = upgcnt;
>> +	ubuf->nr_pinned = nr_pinned;
>>   	fput(memfd);
>>
>>   	return 0;
>>
>>   err:
>> -	ubuf->pagecount = upgcnt;
>>   	if (memfd)
>>   		fput(memfd);
>>
>> @@ -407,7 +393,6 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>   	if (!ubuf)
>>   		return -ENOMEM;
>>
>> -	INIT_LIST_HEAD(&ubuf->unpin_list);
>>   	pglimit = (size_limit_mb * 1024 * 1024) >> PAGE_SHIFT;
>>   	for (i = 0; i < head->count; i++) {
>>   		pgoff_t itempgcnt;
>> @@ -442,6 +427,14 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>   		goto err;
>>   	}
>>
>> +	ubuf->pinned_folios = kvmalloc_array(pgcnt,
>> +					     sizeof(*ubuf->pinned_folios),
>> +					     GFP_KERNEL);
>> +	if (!ubuf->pinned_folios) {
>> +		ret = -ENOMEM;
>> +		goto err;
>> +	}
>> +
>>   	folios = kvmalloc_array(max_ipgcnt, sizeof(*folios), GFP_KERNEL);
>>   	if (!folios) {
>>   		ret = -ENOMEM;
>> @@ -463,8 +456,9 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>   	return ret;
>>
>>   err:
>> -	unpin_all_folios(&ubuf->unpin_list);
>> +	unpin_all_folios(ubuf);
>>   	kvfree(folios);
>> +	kvfree(ubuf->pinned_folios);
>>   	kvfree(ubuf->offsets);
>>   	kvfree(ubuf->folios);
>>   	kfree(ubuf);
>> --
>> 2.45.2