linux-kernel - Re: [PATCH] dma: direct: Optimize the code for the dma_direct

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] [day] [month] [year] [list]

Message-ID: <f688bf74-7d12-459c-b8b7-b85aaafc3785@arm.com>
Date: Tue, 4 Jun 2024 13:39:36 +0100
From: Robin Murphy <robin.murphy@....com>
To: kunyu <kunyu@...china.com>, hch@....de, m.szyprowski@...sung.com
Cc: iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dma: direct: Optimize the code for the dma_direct_free
 function

On 04/06/2024 9:41 am, kunyu wrote:
> The 'is_swiotlb-for-alloc' and 'dev_isdma_coherent' judgment functions
> need to be called multiple times, so they are adjusted to variable
> judgment, which can improve code conciseness.
> 
> Signed-off-by: kunyu <kunyu@...china.com>
> ---
>   kernel/dma/direct.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)

It's hardly concise to add *more* code than was there before... :/

Personally I don't think shaving a handful of characters off each 
invocation has any positive impact on readability in this case, while 
the extra visual indirection, and breaking consistency with the rest of 
this file, definitely has a negative one.

Also note that these "functions" are already just inline wrappers around 
a single variable dereference - for my arm64 build at least, this patch 
has no effect at all on the generated object code, since the compiler 
can still optimise out the locals (so at least it doesn't make things 
*worse* by forcing it to allocate a larger stack frame).

Thanks,
Robin.

> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 4d543b1e9d57..041e316ad4c0 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -315,23 +315,24 @@ void dma_direct_free(struct device *dev, size_t size,
>   		void *cpu_addr, dma_addr_t dma_addr, unsigned long attrs)
>   {
>   	unsigned int page_order = get_order(size);
> +	bool swiotlb_for_alloc = is_swiotlb_for_alloc(dev);
> +	bool is_dma_coherent = dev_is_dma_coherent(dev);
>   
>   	if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
> -	    !force_dma_unencrypted(dev) && !is_swiotlb_for_alloc(dev)) {
> +	    !force_dma_unencrypted(dev) && !swiotlb_for_alloc) {
>   		/* cpu_addr is a struct page cookie, not a kernel address */
>   		dma_free_contiguous(dev, cpu_addr, size);
>   		return;
>   	}
>   
>   	if (IS_ENABLED(CONFIG_ARCH_HAS_DMA_ALLOC) &&
> -	    !dev_is_dma_coherent(dev) &&
> -	    !is_swiotlb_for_alloc(dev)) {
> +	    !is_dma_coherent && !swiotlb_for_alloc) {
>   		arch_dma_free(dev, size, cpu_addr, dma_addr, attrs);
>   		return;
>   	}
>   
>   	if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL) &&
> -	    !dev_is_dma_coherent(dev)) {
> +	    !is_dma_coherent) {
>   		if (!dma_release_from_global_coherent(page_order, cpu_addr))
>   			WARN_ON_ONCE(1);
>   		return;