Messages in this thread | | | Subject | Re: [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error | From | Heiner Kallweit <> | Date | Sun, 13 Dec 2020 22:23:29 +0100 |
| |
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 >
| |