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

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

[Heiner Kallweit]

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@gmail.com>
---
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

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@gmail.com]
> Sent: Monday, December 14, 2020 5:33 AM
> To: Christoph Hellwig <hch@lst.de>; Marek Szyprowski
> <m.szyprowski@samsung.com>; Robin Murphy <robin.murphy@arm.com>; Song Bao Hua
> (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: open list:AMD IOMMU (AMD-VI) <iommu@lists.linux-foundation.org>; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>
> 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@gmail.com>

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

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

[Heiner Kallweit]

Am 13.12.2020 um 22:27 schrieb Song Bao Hua (Barry Song):
> 
> 
>> -----Original Message-----
>> From: Heiner Kallweit [mailto:hkallweit1@gmail.com]
>> Sent: Monday, December 14, 2020 5:33 AM
>> To: Christoph Hellwig <hch@lst.de>; Marek Szyprowski
>> <m.szyprowski@samsung.com>; Robin Murphy <robin.murphy@arm.com>; Song Bao Hua
>> (Barry Song) <song.bao.hua@hisilicon.com>
>> Cc: open list:AMD IOMMU (AMD-VI) <iommu@lists.linux-foundation.org>; Linux
>> Kernel Mailing List <linux-kernel@vger.kernel.org>
>> 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@gmail.com>
> 
> 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().
> 
I basically see two points promoting the proposed change:
1. Driver authors shouldn't have to think (as far as possible) about
   whether a branch prediction hint could make sense for a standard
   core API call. I saw quite some past discussions about when
   something is unlikely enough so that an unlikely() makes sense.
   If the core can hide some more complexity from drivers, then
   I think it's a good thing.
2. If we ever want or have to change the use of unlikely with
   dma_mapping_error(), then we have to do it in just one place.

> 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
> 

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

[Robin Murphy]

On 2020-12-13 16:32, Heiner Kallweit wrote:
> 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.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

FWIW I consider this case similar to the same hint in WARN_ON() and 
friends - it's a pretty severe condition that should never be expected 
to be hit in normal operation, so it's entirely logical for it to be 
implicitly unlikely. I struggle to imagine any case that would 
specifically *not* want that (or worse, want to hint it as likely). Some 
DMA API backends may spend considerable time trying as hard as possible 
to make a mapping work before eventually admitting defeat, so the idea 
of ever trying to optimise at the driver level for failure in hot paths 
just makes no sense.

Thanks,
Robin.

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> 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;
> 

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

[Heiner Kallweit]

On 14.12.2020 14:01, Robin Murphy wrote:
> On 2020-12-13 16:32, Heiner Kallweit wrote:
>> 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.
> 
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> 
> FWIW I consider this case similar to the same hint in WARN_ON() and friends - it's a pretty severe condition that should never be expected to be hit in normal operation, so it's entirely logical for it to be implicitly unlikely. I struggle to imagine any case that would specifically *not* want that (or worse, want to hint it as likely). Some DMA API backends may spend considerable time trying as hard as possible to make a mapping work before eventually admitting defeat, so the idea of ever trying to optimise at the driver level for failure in hot paths just makes no sense.
> 
> Thanks,
> Robin.
> 
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> 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;
>>

Is this patch going to make it to linux-next?