Received-SPF: pass (google.com: domain of linux-kernel+bounces-174452-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <b4632761-3ec6-4070-a60e-b74c1bfdd579@intel.com>
Date: Thu, 9 May 2024 13:44:37 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 2/7] dma: avoid redundant calls for sync operations
To: Marek Szyprowski <m.szyprowski@samsung.com>, Christoph Hellwig
	<hch@lst.de>
CC: Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>, Joerg Roedel <joro@8bytes.org>, "Will
 Deacon" <will@kernel.org>, "Rafael J. Wysocki" <rafael@kernel.org>, "Magnus
 Karlsson" <magnus.karlsson@intel.com>,
	<nex.sw.ncis.osdt.itp.upstreaming@intel.com>, <bpf@vger.kernel.org>,
	<netdev@vger.kernel.org>, <iommu@lists.linux.dev>,
	<linux-kernel@vger.kernel.org>
References: <20240507112026.1803778-1-aleksander.lobakin@intel.com>
 <CGME20240507112115eucas1p117bc01652d4cdbe810de841830227f47@eucas1p1.samsung.com>
 <20240507112026.1803778-3-aleksander.lobakin@intel.com>
 <46160534-5003-4809-a408-6b3a3f4921e9@samsung.com>
From: Alexander Lobakin <aleksander.lobakin@intel.com>
Content-Language: en-US
In-Reply-To: <46160534-5003-4809-a408-6b3a3f4921e9@samsung.com>
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?dGNvTFd5Mkl5S3NuKzZPNkhiaDNteU52VkF3TStYZlhHcVZ2V01hMlpib1dU?=
 =?utf-8?B?T2REVU1SMUVwVWpETThrUmU2Nm9MWStXWm00bjlwaVhPbTE0Y2F2YWhYYWVJ?=
 =?utf-8?B?N3NmcWhmNmxFZlp6YnN2MHFHZURyd1lITWFzMWxXc0RtM0R1S25HQzlXVHZv?=
 =?utf-8?B?bGpjUVFkTk5Vc2tmR1F0YTFkT0tlbzZlblBQaytRUmlUSzVmUk12Z3NwVGpY?=
 =?utf-8?B?aGpDRUF1dHB3UXlWUjd6R2tVL0tEdzJQK2hTY29Fdm0zaXM5MlNiRWg5aVlZ?=
 =?utf-8?B?cFZNWmhzbWxtZ2hydlVmUlo3VkxVeFhtZFpuOHdDZisyTWMvNm1iS1FSZkdp?=
 =?utf-8?B?aTdQWk5iY0tleExIUmlxNTJZKzN0OWJUZVdsczVSN2dVUm0wRnM3SGE1Yzdw?=
 =?utf-8?B?TXpocE5wNEVnK2trTkVjZVNUMnVpckkwMnhoc2lMWU1GNzRuNzF0eUIrYVg1?=
 =?utf-8?B?VHcyNW5pUEs5MTlpM0M5SmY1UytaVUk5NGZVQ3dVV2JzdStXUXdhQnVpSnJ1?=
 =?utf-8?B?ZG56bjFZY1dFNDljSGRpeWlTeGRYS1Y3OTNsRTBmZDdnaUJhM1UwTTNFY2Ux?=
 =?utf-8?B?eTJ0OUxBclM2UFVvZEJkVi9rOHJBSjgycE9zS1pXalZsTzJHWnc3Ym1qN2Q1?=
 =?utf-8?B?UmxVd09iTjRjbVNWUEp4YUdkVUc3OVVQRnVQaUFwdGhndTNWWWZHV3I3UXg5?=
 =?utf-8?B?V1E3d2pVOVl5OVprNlhTekluZ2NSZHU0V21YM3RiZXoxZVN0bzI4RnF3N2d2?=
 =?utf-8?B?bnFoM252bS9HRU9lK2k4ZkQ1NTBKbDd4cEcvU1BBeXhhSWk4SzZUL0dPOURQ?=
 =?utf-8?B?MmExOWo1dEpNTWNTOW0yVlpRTGRtTFlkSTlIa1pUdUJPYnJZeU5vT0VSMGds?=
 =?utf-8?B?TkdDSWxadUhBKzlycHgyZFo5aEQydGhpU2ZUdkYwUko2cjVkVEtRclRIbk83?=
 =?utf-8?B?YlhMWG9ZYTZRbDloQmxSUHg0YWFLWW13R1hxNmFSbDA0cjRqVWg2MzRzU0o1?=
 =?utf-8?B?c0JUbVovZGNVWldmNi9JREdiQ2pKbXM3VXhMS0FPcHZQcTdRTkJWYVRUelEx?=
 =?utf-8?B?SHE4enNwWm55WEg1cHZpaFlDYzBtVEJ6RzQ4NkRTTC9PbUt3WVAxazZaOGZ4?=
 =?utf-8?B?cDliMFJxWXhvSUtsMHlFaG5kOGtDajJMZnNaNjlwc2t2ZC9GRk1BWnBERGVK?=
 =?utf-8?B?M1V4RkhFcHR5bzRjMzFHYmxIQ3BiN2xVb1pIRVcvSWswcTJlcTg4Skpmd1NB?=
 =?utf-8?B?SFBqZmkyQ21HcXRZRFg1TnpORDFld3J6RFFsZGpvcWRUcXZEYXJzayt6MHk1?=
 =?utf-8?B?QW9nVVpYSXdOVnFQcFBJSG0xWkE1TFQ3SEZPbUQvSnZBL0VYT3VZcUlkdGc2?=
 =?utf-8?B?RGFHWmlFU2hCZXFxZXVNWWJxcGpBaDVFNXp1QTJSU0Y1K2UyeWo0UDFzQzZj?=
 =?utf-8?B?MGcxQW5DVEJ5aFpvbGwzanduUXYzUzgrOXMzNGt0WVI1ejZXQjdJSitqcEV4?=
 =?utf-8?B?TkxZNkVlNjh3bWRDblVrMDFnb0FwS0YzcmFvUjBubXNTMGZiaTc2MklCcld4?=
 =?utf-8?B?M3IrMFRYWS9lQ05BM1JZUGVuQ2l3SWhxdFhJYk1jZlVkQlVTekhXeDFMbCtC?=
 =?utf-8?B?ZDZDdDVoamlDMERTRFgremQ0aHFEVS9STUI2K1FQZ3k2WmEvUjBwVjVZbVM0?=
 =?utf-8?B?S2tPLy9qUzN4V080N3RtdEtHWjRjc24rTEF3SUltWTAvY1dBSENoZnZRRFpm?=
 =?utf-8?B?UVI3c09paHVLSU9KUmtpc0txVU9sTjdBRGdDL05DcEE4UEI4cU1TNFpERGlT?=
 =?utf-8?B?U0xlMTIraklkdmF2Qit6UitlaGYrYXBncHNQNFdFNUh6T2xhUGQyTGhaSmor?=
 =?utf-8?B?MnZuaWV3V1YrWHJFa1YvSmltWFJrVlFDc1E4Z091L2lDQzlRbnBKeGUyK1lY?=
 =?utf-8?B?SmwzVTVxVHBjQW01V2R0REc2YVhxU0p0ZWRvUW11dGtXSVZ3dVAvcXEza1Jp?=
 =?utf-8?B?bGluZmxsNTJTaVZ5VDJ0b1RXNEJtQTJacjB6SnNUYlZrK0ZmUTA3WE5zZHJZ?=
 =?utf-8?B?aDdtZVZhZFBsYUQzanZaZkdOOHoyNzJxbWpQbVV0UzNMTDVLdGdkS09yTnhJ?=
 =?utf-8?B?cTJXR1FGajdoRStiYlVuRDE0bSttZFRVM2pzZlQ5eXNEZXVYRVJJeldBQ3Q2?=
 =?utf-8?B?T2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7a6d5e46-e30a-4d49-30cd-08dc701d7b37
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 May 2024 11:45:10.5505
 (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: CIkn3X3eG7BruJdiphVxLjIrR7mYm2RnFvrjAtqFZWWTFQPSZmv5q5X26+yg8npDz4G9p/90A318pVfRaNZZAGfWgvYOpq6vYFgUHNWNtTU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7848
X-OriginatorOrg: intel.com

From: Marek Szyprowski <m.szyprowski@samsung.com>
Date: Thu, 9 May 2024 13:41:16 +0200

> Dear All,
> 
> On 07.05.2024 13:20, Alexander Lobakin wrote:
>> Quite often, devices do not need dma_sync operations on x86_64 at least.
>> Indeed, when dev_is_dma_coherent(dev) is true and
>> dev_use_swiotlb(dev) is false, iommu_dma_sync_single_for_cpu()
>> and friends do nothing.
>>
>> However, indirectly calling them when CONFIG_RETPOLINE=y consumes about
>> 10% of cycles on a cpu receiving packets from softirq at ~100Gbit rate.
>> Even if/when CONFIG_RETPOLINE is not set, there is a cost of about 3%.
>>
>> Add dev->need_dma_sync boolean and turn it off during the device
>> initialization (dma_set_mask()) depending on the setup:
>> dev_is_dma_coherent() for the direct DMA, !(sync_single_for_device ||
>> sync_single_for_cpu) or the new dma_map_ops flag, %DMA_F_CAN_SKIP_SYNC,
>> advertised for non-NULL DMA ops.
>> Then later, if/when swiotlb is used for the first time, the flag
>> is reset back to on, from swiotlb_tbl_map_single().
>>
>> On iavf, the UDP trafficgen with XDP_DROP in skb mode test shows
>> +3-5% increase for direct DMA.
>>
>> Suggested-by: Christoph Hellwig <hch@lst.de> # direct DMA shortcut
>> Co-developed-by: Eric Dumazet <edumazet@google.com>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>> ---
>>   include/linux/device.h      |  4 +++
>>   include/linux/dma-map-ops.h | 12 ++++++++
>>   include/linux/dma-mapping.h | 53 +++++++++++++++++++++++++++++++----
>>   kernel/dma/mapping.c        | 55 +++++++++++++++++++++++++++++--------
>>   kernel/dma/swiotlb.c        |  6 ++++
>>   5 files changed, 113 insertions(+), 17 deletions(-)
> 
> 
> This patch landed in today's linux-next as commit f406c8e4b770 ("dma: 
> avoid redundant calls for sync operations"). Unfortunately I found that 
> it breaks some of the ARM 32bit boards by forcing skipping DMA sync 
> operations on non-coherent systems. This happens because this patch 
> hooks dma_need_sync=true initialization into set_dma_mask(), but 
> set_dma_mask() is not called from all device drivers, especially from 
> those which operates properly with the default 32bit dma mask (like most 
> of the platform devices created by the OF layer).
> 
> Frankly speaking I have no idea how this should be fixed. I expect that 
> there are lots of broken devices after this change, because I don't 
> remember that calling set_dma_mask() is mandatory for device drivers.
> 
> After adding dma_set_mask(dev, DMA_BIT_MASK(32)) to the drivers relevant 
> for my boards the issues are gone, but I'm not sure this is the right 
> approach...

If I remember correctly, *all* device drivers which use DMA *must* call
dma_set_*mask() on probe. That's why we added it there and didn't care.
Alternatively, if it really breaks a lot of drivers, we can set
dma_need_sync = true by default before the driver probing. I thought of
this, but the correct approach would be to call dma_set_*mask() from the
respective drivers.

> 
> 
>> ...
> 
> Best regards

Thanks,
Olek