From: Heiner Kallweit <hkallweit1@gmail.com>
To: "Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com>,
Christoph Hellwig <hch@lst.de>,
Marek Szyprowski <m.szyprowski@samsung.com>,
Robin Murphy <robin.murphy@arm.com>
Cc: "open list:AMD IOMMU (AMD-VI)" <iommu@lists.linux-foundation.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error
Date: Sun, 13 Dec 2020 22:23:29 +0100 [thread overview]
Message-ID: <2faa40b9-e632-b9f3-6de0-471f0b7a6cae@gmail.com> (raw)
In-Reply-To: <93ac85090ffa4313a7dc87a38d1f6aa1@hisilicon.com>
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
>
next prev parent reply other threads:[~2020-12-13 22:25 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2020-12-14 13:01 ` Robin Murphy
2021-01-08 16:45 ` Heiner Kallweit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2faa40b9-e632-b9f3-6de0-471f0b7a6cae@gmail.com \
--to=hkallweit1@gmail.com \
--cc=hch@lst.de \
--cc=iommu@lists.linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=robin.murphy@arm.com \
--cc=song.bao.hua@hisilicon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).