linux-kernel - RE: [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <93ac85090ffa4313a7dc87a38d1f6aa1@hisilicon.com>
Date:   Sun, 13 Dec 2020 21:27:19 +0000
From:   "Song Bao Hua (Barry Song)" <song.bao.hua@...ilicon.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Christoph Hellwig <hch@....de>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Robin Murphy <robin.murphy@....com>
CC:     "open list:AMD IOMMU (AMD-VI)" <iommu@...ts.linux-foundation.org>,
        "Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] dma-mapping: add unlikely hint for error path in
 dma_mapping_error



> -----Original Message-----
> From: Heiner Kallweit [mailto:hkallweit1@...il.com]
> Sent: Monday, December 14, 2020 5:33 AM
> To: Christoph Hellwig <hch@....de>; Marek Szyprowski
> <m.szyprowski@...sung.com>; Robin Murphy <robin.murphy@....com>; Song Bao Hua
> (Barry Song) <song.bao.hua@...ilicon.com>
> Cc: open list:AMD IOMMU (AMD-VI) <iommu@...ts.linux-foundation.org>; Linux
> Kernel Mailing List <linux-kernel@...r.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@...il.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