Re: [RESEND PATCH v4 2/4] dma-mapping: benchmark: modify the framework to adapt to more map modes

[Jonathan Cameron]

On Sat, 14 Jun 2025 22:34:52 +0800
Qinxin Xia <xiaqinxin@xxxxxxxxxx> wrote:

> In some service scenarios, the performance of dma_map_sg needs to be
> tested to support different map modes for benchmarks. This patch adjusts
> the DMA map benchmark framework to make the DMA map benchmark framework
> more flexible and adaptable to other mapping modes in the future.
> By abstracting the framework into four interfaces:prepare, unprepare,
> do_map, and do_unmap.The new map schema can be introduced more easily
> without major modifications to the existing code structure.
> 
> Reviewed-by: Barry Song <baohua@xxxxxxxxxx>
> Signed-off-by: Qinxin Xia <xiaqinxin@xxxxxxxxxx>

There is what looks like an accidental change in behavior for loops
after the first one.  I think the cache lines will end up clean so
any flush will be just dropping them.  Prior to this patch they
were probably dirty.

Jonathan

>  #endif /* _KERNEL_DMA_BENCHMARK_H */
> diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
> index cc19a3efea89..05f85cf00c35 100644
> --- a/kernel/dma/map_benchmark.c
> +++ b/kernel/dma/map_benchmark.c
> @@ -5,6 +5,7 @@

> +static void *dma_single_map_benchmark_prepare(struct map_benchmark_data *map)
> +{
> +	struct dma_single_map_param *params __free(kfree) = kzalloc(sizeof(*params),
> +								    GFP_KERNEL);
Trivial: I'd split this slightly differently.

	struct dma_single_map_param *params __free(kfree) =
		kzalloc(sizeof(*params), GFP_KERNEL);


> +}

> +
> +static int dma_single_map_benchmark_do_map(void *mparam)
> +{
> +	struct dma_single_map_param *params = mparam;
> +	int ret = 0;
> +
> +	params->addr = dma_map_single(params->dev, params->xbuf,
> +				      params->npages * PAGE_SIZE, params->dma_dir);
> +	if (unlikely(dma_mapping_error(params->dev, params->addr))) {
> +		pr_err("dma_map_single failed on %s\n", dev_name(params->dev));

dev_err() seems more appropriate than passing in the dev to a pr_err.

> +		ret = -ENOMEM;
		return -ENOMEM;
Or better still don't assume the error return of dma_mapping_error()
(even though it is currently only -ENOMEM)

> +	}
> +
	return 0;


would be neater and avoid need for the local variable.
If you add stuff here later in the series then fine to ignore this comment.


> +	return ret;
> +}

>  static int map_benchmark_thread(void *data)
>  {
> -	void *buf;
> -	dma_addr_t dma_addr;
>  	struct map_benchmark_data *map = data;
> -	int npages = map->bparam.granule;
> -	u64 size = npages * PAGE_SIZE;
> +	__u8 map_mode = map->bparam.map_mode;
>  	int ret = 0;
>  
> -	buf = alloc_pages_exact(size, GFP_KERNEL);
> -	if (!buf)
> +	struct map_benchmark_ops *mb_ops = dma_map_benchmark_ops[map_mode];
> +	void *mparam = mb_ops->prepare(map);
> +
> +	if (!mparam)
>  		return -ENOMEM;
>  
>  	while (!kthread_should_stop())  {
> @@ -49,23 +132,10 @@ static int map_benchmark_thread(void *data)
>  		ktime_t map_stime, map_etime, unmap_stime, unmap_etime;
>  		ktime_t map_delta, unmap_delta;
>  
> -		/*
> -		 * for a non-coherent device, if we don't stain them in the
> -		 * cache, this will give an underestimate of the real-world
> -		 * overhead of BIDIRECTIONAL or TO_DEVICE mappings;
> -		 * 66 means evertything goes well! 66 is lucky.
> -		 */
> -		if (map->dir != DMA_FROM_DEVICE)
> -			memset(buf, 0x66, size);

This seems to change the behavior form memset every time to only once
in the prepare call above.  If that has no affect on what is being benchmarked,
then add a comment on it to the patch description.


> -
>  		map_stime = ktime_get();
> -		dma_addr = dma_map_single(map->dev, buf, size, map->dir);
> -		if (unlikely(dma_mapping_error(map->dev, dma_addr))) {
> -			pr_err("dma_map_single failed on %s\n",
> -				dev_name(map->dev));
> -			ret = -ENOMEM;
> +		ret = mb_ops->do_map(mparam);
> +		if (ret)
>  			goto out;
> -		}
>  		map_etime = ktime_get();
>  		map_delta = ktime_sub(map_etime, map_stime);
>  
> @@ -73,7 +143,8 @@ static int map_benchmark_thread(void *data)
>  		ndelay(map->bparam.dma_trans_ns);
>  
>  		unmap_stime = ktime_get();
> -		dma_unmap_single(map->dev, dma_addr, size, map->dir);
> +		mb_ops->do_unmap(mparam);
> +
>  		unmap_etime = ktime_get();
>  		unmap_delta = ktime_sub(unmap_etime, unmap_stime);
>  
> @@ -108,7 +179,7 @@ static int map_benchmark_thread(void *data)
>  	}
>  
>  out:
> -	free_pages_exact(buf, size);
> +	mb_ops->unprepare(mparam);
>  	return ret;
>  }