Received-SPF: pass (google.com: domain of linux-kernel+bounces-63332-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <0812b7bf-bb66-4b0d-8615-964cb5181de9@intel.com>
Date: Tue, 13 Feb 2024 11:19:10 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v2 2/7] dma: avoid redundant calls for sync
 operations
Content-Language: en-US
To: Christoph Hellwig <hch@lst.de>
CC: "David S. Miller" <davem@davemloft.net>, Eric Dumazet
	<edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>, Marek Szyprowski <m.szyprowski@samsung.com>, "Robin
 Murphy" <robin.murphy@arm.com>, Joerg Roedel <joro@8bytes.org>, Will Deacon
	<will@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael
 J. Wysocki" <rafael@kernel.org>, Magnus Karlsson <magnus.karlsson@intel.com>,
	Maciej Fijalkowski <maciej.fijalkowski@intel.com>, Alexander Duyck
	<alexanderduyck@fb.com>, <bpf@vger.kernel.org>, <netdev@vger.kernel.org>,
	<iommu@lists.linux.dev>, <linux-kernel@vger.kernel.org>
References: <20240205110426.764393-1-aleksander.lobakin@intel.com>
 <20240205110426.764393-3-aleksander.lobakin@intel.com>
 <20240213061120.GC22451@lst.de>
From: Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20240213061120.GC22451@lst.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UWdrK29Od0lCWDkveTRNRGczdzBGSGtmQjhDa0lNMERSdnRFVXBTaERERW5L?=
 =?utf-8?B?VXVHbWhGOFhiZ2pXd0ljMS9CblBJV2tHY1NsTC9VeTg1TUNLakxBZ3IvdHMx?=
 =?utf-8?B?eFF6OE90d0RYWHpTZUhOZXhKcFF5VnRDMlZKOExyb3hCNmpSZmVSTkFuaGFS?=
 =?utf-8?B?UEFmWGZtdTVnbk5Db3JkdzdKYXZZMm5PSkFTR055QVVXdktoSFpNWTNKZkl2?=
 =?utf-8?B?ZzAzbGxhMnY3T0FCcnBXZ2RtanRsbW9SSVM4Mk0xKzd0YWVGczNIazhCUjJK?=
 =?utf-8?B?SXZLRFJ4eDFSUEFIeWMrQ2FKQ2RwQ0p6bXRLbTI2czVJaXhKM2tOQnRvL2ZC?=
 =?utf-8?B?cDVuZi9Ma1dYZWtMTWNmZEFlcVZuUklUU1FHVzFLREdkSzQwamtGZGFQOFE4?=
 =?utf-8?B?a2d5UEc2VnlVSE40R3hMK09lWHc2ZHpQUUwxQXkwZjJ6bkJEVDBLdkRPMG94?=
 =?utf-8?B?REh4SWtlOGpYQm1GVlpEU29XTHNLR3J1RWltdmNhd1BrVElPMk9IM0lJaU00?=
 =?utf-8?B?UWFnY3NmaVZVUWdLR3J5a2k0R3JzanFiUTNmSmkxWWxRenZUZHhBM0QyY0pq?=
 =?utf-8?B?MTRxcjdZTEVTaCszcUpzMzRKOVBpM2NSb1l5L1dPMjJ6NzNmM2F3Q0V5V2hG?=
 =?utf-8?B?ZytCMzJtN0UvaWUwQ20xNU1SekpidXVVeDZrZUVpZHByYk5lZ3lkcGYxczgr?=
 =?utf-8?B?dUVBMEVzU2pqcHhPSFUvWDdMb0tCNHZYSkN0S0tQNXYvazd2RDluYmdyRTNn?=
 =?utf-8?B?RWE0ck5tZmhlekcrNEZwR010K2JOcFlHb0lPOUlWTUJEWXFnVDNPS094OXJY?=
 =?utf-8?B?Y0hXTVNDaVZJQVJMVlp2dzNPOGlFK2FkVjRhVHY5MzlSK1RKNFI3aHoyMWpI?=
 =?utf-8?B?N21kMno1RUR0Zk9sWDRGclFIT214OUZuQkZDZFNpR1IvbG9CZWNVZlNjNmV3?=
 =?utf-8?B?SG9QZURKdHcrMlFJRXpWdm9SZUMwK2VKZlZONTZDLytCTEt4bUlhd1lHeGRU?=
 =?utf-8?B?c2paYndCMEJFcDNpMElEZDZJZENBV2trVVhRSUFyaE9HTDEwREhZZ3ZWemtR?=
 =?utf-8?B?TXQvWGkwTHFpTmc2cURQY3Bqc2NEdUFteFNzbjBRalFWYm1CT1dUNm5sYm5G?=
 =?utf-8?B?YnRpRU9HS0hQMDhGOGJrWmJhSlYzSE95TTJ1M3NwWmpWSFdXWkdmb2xwempC?=
 =?utf-8?B?Uk95VGhuZFlqa2NuS3pVc3E3VDJvLzZqVXFvenRWWDRmZWFKRFV0WnFQUVZF?=
 =?utf-8?B?ZERiK0t0VHV3OG5CQU9Qb1RXVmlFTTNZd1k3OThEb0FNeVplRmNhamdWVWdC?=
 =?utf-8?B?ZVlzWDJFMUJBU1hDOWNrQTJLSHFGMzVKdUwyMGJBZkZBNUxUUFZSSmZyZkpa?=
 =?utf-8?B?dWFJVi9RZThEOU5XMzgrMXFaYmdDalBPMi8xWWZseHpkMG1CWGNEMGNPNCtv?=
 =?utf-8?B?cXJDaGpIQlA4TXBRbnA5UHdXK1N0NlpYd3VIbnZwNktBclJhTlh0VzB6ZWZ6?=
 =?utf-8?B?MTdrRzZmbFkyUHVaL2tudithRHlhU3l6Yi92MG02Rk5pMS9ITlVhQ3I5YnlS?=
 =?utf-8?B?QjYwdmZ5WVNGZWo5WFhqZTU1T2ZDU0N1SkYwMUdDSUVEelJYMHNvRENUaGZj?=
 =?utf-8?B?Ym96K3Byd09IRUx2QzEyOFViUG1rZlNxbzdFOUZIU01vUEhoMFUwNDNOdWc0?=
 =?utf-8?B?RG02VjZucXMwelZHbHpDODM5UGlxcUw1VzNkSk9sekh3V3BuaWtXdUxZT2tQ?=
 =?utf-8?B?bFRLL1B1eWtqNWorbGpJc0h3V3hNRWVvdzJoVm91WmFJVDNlNHJPQXlCd2g1?=
 =?utf-8?B?djRKKy9weHYwSTR1cjVIbTFMemJ0WTR4bWNhNUpDTWhiSDcvMy92STBRY0hE?=
 =?utf-8?B?eXJnaG1DN3BBNEhNTjZ3bDBOejJlMk1qcXVUL05IcStTaDNteENqajJiU3pq?=
 =?utf-8?B?S1hnSW9paGM1M2ZJbXVqbC9WcGZPczk0NUlvQnNIY2xlOGxQMWlpYXJaSGpN?=
 =?utf-8?B?Rm15dExJUFZSWVN2KzBWSXhrZFFmbFNmMEJDaGorYUpXbGE1RFo5TU1ZZXQ3?=
 =?utf-8?B?ZGhDMXoyK2N5L0x4aEMxdmRFNTYvZG83Y1lVR1lwMW9GekExZ3RvbEhNcHN0?=
 =?utf-8?B?all3VDcyL1JpUm5FMVp1VFZ0MmFrMHhRandNMWtFeUR6VGFUT1g4Y2t3K0Nw?=
 =?utf-8?B?SGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3c8fcb46-efa8-4c0d-5cc1-08dc2c7d6204
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Feb 2024 10:20:20.9197
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: nNGbyWctGP+YMnfzY1O691QPouS62RBVI+OU8EHW96edIQTalLm4pV5vRqrt13n4mue82MitC7eYiVWOjVtu4Iav+AbjiE+srkfwPP+DLhU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7457
X-OriginatorOrg: intel.com

From: Christoph Hellwig <hch@lst.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