* [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error
@ 2020-12-13 16:32 Heiner Kallweit
2020-12-13 21:27 ` Song Bao Hua (Barry Song)
2020-12-14 13:01 ` Robin Murphy
0 siblings, 2 replies; 5+ messages in thread
From: Heiner Kallweit @ 2020-12-13 16:32 UTC (permalink / raw)
To: Christoph Hellwig, Marek Szyprowski, Robin Murphy, Barry Song
Cc: open list:AMD IOMMU (AMD-VI), Linux Kernel Mailing List
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error
2020-12-13 21:27 ` Song Bao Hua (Barry Song)
@ 2020-12-13 21:23 ` Heiner Kallweit
0 siblings, 0 replies; 5+ messages in thread
From: Heiner Kallweit @ 2020-12-13 21:23 UTC (permalink / raw)
To: Song Bao Hua (Barry Song),
Christoph Hellwig, Marek Szyprowski, Robin Murphy
Cc: open list:AMD IOMMU (AMD-VI), Linux Kernel Mailing List
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error
2020-12-13 16:32 [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error Heiner Kallweit
@ 2020-12-13 21:27 ` Song Bao Hua (Barry Song)
2020-12-13 21:23 ` Heiner Kallweit
2020-12-14 13:01 ` Robin Murphy
1 sibling, 1 reply; 5+ messages in thread
From: Song Bao Hua (Barry Song) @ 2020-12-13 21:27 UTC (permalink / raw)
To: Heiner Kallweit, Christoph Hellwig, Marek Szyprowski, Robin Murphy
Cc: open list:AMD IOMMU (AMD-VI), Linux Kernel Mailing List
> -----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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error
2020-12-13 16:32 [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error Heiner Kallweit
2020-12-13 21:27 ` Song Bao Hua (Barry Song)
@ 2020-12-14 13:01 ` Robin Murphy
2021-01-08 16:45 ` Heiner Kallweit
1 sibling, 1 reply; 5+ messages in thread
From: Robin Murphy @ 2020-12-14 13:01 UTC (permalink / raw)
To: Heiner Kallweit, Christoph Hellwig, Marek Szyprowski, Barry Song
Cc: open list:AMD IOMMU (AMD-VI), Linux Kernel Mailing List
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;
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error
2020-12-14 13:01 ` Robin Murphy
@ 2021-01-08 16:45 ` Heiner Kallweit
0 siblings, 0 replies; 5+ messages in thread
From: Heiner Kallweit @ 2021-01-08 16:45 UTC (permalink / raw)
To: Robin Murphy, Christoph Hellwig, Marek Szyprowski, Barry Song
Cc: open list:AMD IOMMU (AMD-VI), Linux Kernel Mailing List
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?
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-08 16:45 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-13 16:32 [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error Heiner Kallweit
2020-12-13 21:27 ` Song Bao Hua (Barry Song)
2020-12-13 21:23 ` Heiner Kallweit
2020-12-14 13:01 ` Robin Murphy
2021-01-08 16:45 ` Heiner Kallweit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).