Re: [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error
Am 13.12.2020 um 22:27 schrieb Song Bao Hua (Barry Song):
>
>
>> -Original Message-
>> From: Heiner Kallweit [mailto:hkallwe...@gmail.com]
>> Sent: Monday, December 14, 2020 5:33 AM
>> To: Christoph Hellwig ; Marek Szyprowski
>> ; Robin Murphy ; Song Bao Hua
>> (Barry Song)
>> Cc: open list:AMD IOMMU (AMD-VI) ; Linux
>> Kernel Mailing List
>> 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
>
> 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
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error
> -Original Message-
> From: Heiner Kallweit [mailto:hkallwe...@gmail.com]
> Sent: Monday, December 14, 2020 5:33 AM
> To: Christoph Hellwig ; Marek Szyprowski
> ; Robin Murphy ; Song Bao Hua
> (Barry Song)
> Cc: open list:AMD IOMMU (AMD-VI) ; Linux
> Kernel Mailing List
> 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
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[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
---
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
