RE: [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Heiner Kallweit [mailto:hkallweit1@xxxxxxxxx]
> Sent: Monday, December 14, 2020 5:33 AM
> To: Christoph Hellwig <hch@xxxxxx>; Marek Szyprowski
> <m.szyprowski@xxxxxxxxxxx>; Robin Murphy <robin.murphy@xxxxxxx>; Song Bao Hua
> (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>
> Cc: open list:AMD IOMMU (AMD-VI) <iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx>; Linux
> Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>
> Subject: [PATCH v2] dma-mapping: add unlikely hint for error path in
> dma_mapping_error
> 
> Zillions of drivers use the unlikely() hint when checking the result of
> dma_mapping_error(). This is an inline function anyway, so we can move
> the hint into this function and remove it from drivers.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>

not sure if this is really necessary. It seems the original code
is more readable. Readers can more easily understand we are
predicting the branch based on the return value of
dma_mapping_error().

Anyway, I don't object to this one. if other people like it, I am
also ok with it.

> ---
> v2:
> Split the big patch into the change for dma-mapping.h and follow-up
> patches per subsystem that will go through the trees of the respective
> maintainers.
> ---
>  include/linux/dma-mapping.h | 2 +-
>  kernel/dma/map_benchmark.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 2e49996a8..6177e20b5 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -95,7 +95,7 @@ static inline int dma_mapping_error(struct device *dev,
> dma_addr_t dma_addr)
>  {
>  	debug_dma_mapping_error(dev, dma_addr);
> 
> -	if (dma_addr == DMA_MAPPING_ERROR)
> +	if (unlikely(dma_addr == DMA_MAPPING_ERROR))
>  		return -ENOMEM;
>  	return 0;
>  }
> diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
> index b1496e744..901420a5d 100644
> --- a/kernel/dma/map_benchmark.c
> +++ b/kernel/dma/map_benchmark.c
> @@ -78,7 +78,7 @@ static int map_benchmark_thread(void *data)
> 
>  		map_stime = ktime_get();
>  		dma_addr = dma_map_single(map->dev, buf, PAGE_SIZE, map->dir);
> -		if (unlikely(dma_mapping_error(map->dev, dma_addr))) {
> +		if (dma_mapping_error(map->dev, dma_addr)) {
>  			pr_err("dma_map_single failed on %s\n",
>  				dev_name(map->dev));
>  			ret = -ENOMEM;
> --
> 2.29.2

Thanks
Barry