Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 15 Nov 2022 22:53:14 -0800
From:   Christoph Hellwig <hch@infradead.org>
To:     Eric Dumazet <edumazet@google.com>
Cc:     Joerg Roedel <joro@8bytes.org>,
        Robin Murphy <robin.murphy@arm.com>,
        Will Deacon <will@kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        netdev@vger.kernel.org, Eric Dumazet <eric.dumazet@gmail.com>,
        iommu@lists.linux.dev
Subject: Re: [PATCH v2 -next] iommu/dma: avoid expensive indirect calls for
 sync operations
Message-ID: <Y3SI2vMf58/WZDwS@infradead.org>
References: <20221115182841.2640176-1-edumazet@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221115182841.2640176-1-edumazet@google.com>
Precedence: bulk

As Robing pointed out this really is mostly a dma-mapping layer
patch and the subject should reflect that.

> +		if (!IS_ENABLED(CONFIG_DMA_API_DEBUG) && dev_is_dma_coherent(dev))
> +			dev->skip_dma_sync = true;

I don't think CONFIG_DMA_API_DEBUG should come into play here.  It
is independent from the low-level sync calls.  That'll need a bit
of restructuring later on to only skil the sync calls and not the
debug_dma_* calls, but I think that is worth it to keep the concept
clean.
In fact that might lead to just checking the skip_dma_sync flag in
a wrapper in dma-mapping.h, avoiding the function call entirely
for the fast path, at the downside of increasing code size by
adding an extra branch in the callers, but I think that is ok.

I think we should also apply the skip_dma_sync to dma-direct while
we're it.  While dma-direct already avoids the indirect calls we can
shave off a few more cycles with this infrastructure, so why skip that?

> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -647,6 +647,7 @@ struct device {
>      defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
>  	bool			dma_coherent:1;
>  #endif
> +	bool			skip_dma_sync:1;

This also needs a blurb in the kerneldoc comment describing struct
device.  Please make it very clear in the comment that the flag is
only for internal use in the DMA mapping subsystem and not for use
by drives.

> +static inline bool dev_skip_dma_sync(struct device *dev)
> +{
> +	return dev->skip_dma_sync;
> +}

I'd drop this wrapper and just check the flag directly.

> +	if (unlikely(dev->skip_dma_sync))
> +		dev->skip_dma_sync = false;

Please add a comment here.


Btw, one thing I had in my mind for a while is to do direct calls to
dma-iommu from the core mapping code just like we for dma-direct.
Would that be useful for your networking loads, or are the actual
mapping calls rare enough to not matter?