[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <0812b7bf-bb66-4b0d-8615-964cb5181de9@intel.com>
Date: Tue, 13 Feb 2024 11:19:10 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Christoph Hellwig <hch@....de>
CC: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
<pabeni@...hat.com>, Marek Szyprowski <m.szyprowski@...sung.com>, "Robin
Murphy" <robin.murphy@....com>, Joerg Roedel <joro@...tes.org>, Will Deacon
<will@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael
J. Wysocki" <rafael@...nel.org>, Magnus Karlsson <magnus.karlsson@...el.com>,
Maciej Fijalkowski <maciej.fijalkowski@...el.com>, Alexander Duyck
<alexanderduyck@...com>, <bpf@...r.kernel.org>, <netdev@...r.kernel.org>,
<iommu@...ts.linux.dev>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v2 2/7] dma: avoid redundant calls for sync
operations
From: Christoph Hellwig <hch@....de>
Date: Tue, 13 Feb 2024 07:11:20 +0100
> On Mon, Feb 05, 2024 at 12:04:21PM +0100, Alexander Lobakin wrote:
>> Quite often, NIC devices do not need dma_sync operations on x86_64
>> at least.
>
> This is a fundamental property of the platform being DMA coherent,
> and devices / platforms not having addressing limitations or other
> need for bounce buffering (like all those whacky trusted platform
> schemes). Nothing NIC-specific here.
This sentence is from the original Eric's commit message, but I'll
reword it :D
>
>> In case some device doesn't work with the shortcut:
>> * include <linux/dma-map-ops.h> to the driver source;
>> * call dma_set_skip_sync(dev, false) at the beginning of the probe
>> callback. This will disable the shortcut and force DMA syncs.
>
> No, drivers should never include dma-map-ops.h. If we have a legit
> reason for drivers to ever call it it would have to move to
> dma-mapping.h. But I see now reason why there would be such a need.
> For now I'd suggest simply dropping this paragraph from the commit
> message.
That's why I didn't move it to dma-mapping.h -- in general, drivers
should not call it, so it would be a workaround. I added this paragraph
in v2 as a couple folks asked "what if some weird device will break with
this optimization". I can drop it anyway.
>
>> if (dma_map_direct(dev, ops))
>> + /*
>> + * dma_skip_sync could've been set to false on first SWIOTLB
>> + * buffer mapping, but @dma_addr is not necessary an SWIOTLB
>> + * buffer. In this case, fall back to more granular check.
>> + */
>> return dma_direct_need_sync(dev, dma_addr);
>> +
>
> Nit: with such a long block comment adding curly braces would make the
> code a bit more readable.
>
>> +#ifdef CONFIG_DMA_NEED_SYNC
>> +void dma_setup_skip_sync(struct device *dev)
>> +{
>> + const struct dma_map_ops *ops = get_dma_ops(dev);
>> + bool skip;
>> +
>> + if (dma_map_direct(dev, ops))
>> + /*
>> + * dma_skip_sync will be set to false on first SWIOTLB buffer
>> + * mapping, if any. During the device initialization, it's
>> + * enough to check only for DMA coherence.
>> + */
>> + skip = dev_is_dma_coherent(dev);
>> + else if (!ops->sync_single_for_device && !ops->sync_single_for_cpu)
>> + /*
>> + * Synchronization is not possible when none of DMA sync ops
>> + * is set. This check precedes the below one as it disables
>> + * the synchronization unconditionally.
>> + */
>> + skip = true;
>> + else if (ops->flags & DMA_F_CAN_SKIP_SYNC)
>> + /*
>> + * Assume that when ``DMA_F_CAN_SKIP_SYNC`` is advertised,
>> + * the conditions for synchronizing are the same as with
>> + * the direct DMA.
>> + */
>> + skip = dev_is_dma_coherent(dev);
>> + else
>> + skip = false;
>> +
>> + dma_set_skip_sync(dev, skip);
>
> I'd just assign directly to dev->dma_skip_sync instead of using a
> local variable and the dma_set_skip_sync call - we are under
> ifdef CONFIG_DMA_NEED_SYNC here and thus know is is available.
>
>> +static inline void swiotlb_disable_dma_skip_sync(struct device *dev)
>> +{
>> + /*
>> + * If dma_skip_sync was set, reset it to false on first SWIOTLB buffer
>> + * mapping/allocation to always sync SWIOTLB buffers.
>> + */
>> + if (unlikely(dma_skip_sync(dev)))
>> + dma_set_skip_sync(dev, false);
>> +}
>
> Nothing really swiotlb-specific here. Also the naming is a bit odd.
> Maybe have a dma_set_skip_sync helper without the bool to enable
> skipping, and a dma_clear_skip_sync that clear the flag. The optimization
> to first check the flag here could just move into that latter
> helper.
Sounds good!
Thanks,
Olek
Powered by blists - more mailing lists