Received-SPF: pass (google.com: domain of linux-kernel+bounces-174460-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <3dce41a3-e5a9-43e7-b918-ecb8d688ea1c@intel.com>
Date: Thu, 9 May 2024 13:59:41 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 2/7] dma: avoid redundant calls for sync operations
From: Alexander Lobakin <aleksander.lobakin@intel.com>
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>
 <b4632761-3ec6-4070-a60e-b74c1bfdd579@intel.com>
Content-Language: en-US
In-Reply-To: <b4632761-3ec6-4070-a60e-b74c1bfdd579@intel.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?a2RkNkJVbUlNcTFNUjZETS9oQ3gzMDBvTm1hRVpISXZjQlJDUmd0YTJNemY4?=
 =?utf-8?B?NnhPWTBvQ3J4WGMwMVYvUkduMlpHTlh0ODRReDdDampSYjNrUkk0dWlnaFBS?=
 =?utf-8?B?eDFsSHVXZ0V3SSt5bHBybTdGNjFrNG1PNXdlZWQrY3pZOGg4OFJDZnBocWt5?=
 =?utf-8?B?UXFMbE1CdW9CWGR2eHV4R3JINmczdkpFUnlTMUMvSW9YUHV3OTFwRVlDQ0FI?=
 =?utf-8?B?Um4yRS9XQzhXY0xPODE0WkIvT0JWVjBHdkZ2QW9BQ1hIUDR4eHBFMHE1Y3VB?=
 =?utf-8?B?NHYrTFo3aVFudUx6cWlwd2xicjdzQ1dnTi9pWitrbDVCVUt0UjJBRHNxSDhh?=
 =?utf-8?B?OFFteFdPMlRZeGMzc3lJdXM4WVo4YktYTnhaS1JtRGF3S2xZbzU1L2pFUDJz?=
 =?utf-8?B?SWFvczBFL2N5b25CeEhGUUtxU3NyVUwzSitoRWp6QVkzWk95VE81OG9hTlhI?=
 =?utf-8?B?VTM5aWk2cW9JMUhhRmN5UVpXZHluZXV3eEQ5Slo0Qm9wTnUvc1pFVFNibjBR?=
 =?utf-8?B?YVJSdEIxK3VtV1NYcWp6Wkd5Q21LSHJ6NzZzckJQVkFHYXBVY1h0U0ZRUUpt?=
 =?utf-8?B?elRpTzVwb3FybGF6Q1F4M3lPK2NKMWpEKzJROEZGNEdGQ0JRd3RaUE5jMEhh?=
 =?utf-8?B?YUNoeE54QWlhREpvK2t1SmxBeXdGVEFaOVFEZ1VxekhMa2tyUjRON0EzUXlJ?=
 =?utf-8?B?b1hSUStiQ1pvMHQ0ZXlETzdOTXpuT0pjdmIxZDVSVEJsNjA4azZZcDlvL0dn?=
 =?utf-8?B?cEtFOEw3K3YvaWo3c3dvNE1IbUxTbzZtNXJzdjF2NXZBMWM2elhUbTZiaHll?=
 =?utf-8?B?b1kwTGRwb0FTYlRIc3pYbTdGZnV5YzZUc0ZKMVgvMzRxT0ZIbW96U0xiTEF1?=
 =?utf-8?B?UGdjUXVGMG5uZEt6MXFzYlNYMjN2RFYzelpNZGJIUGNKVFY2SXgwbXZxc1VM?=
 =?utf-8?B?SFBBUkJhSVpoVUtVaThXWG9XNFlKUjFsek9oTmZoQmRBQlp6NEtHOWJ5eUtp?=
 =?utf-8?B?UDh3djBrMWV5b25aZCtEYWtlWWJxZnFUWG93VVJXSnh2MmwwMmVKSlRMaTky?=
 =?utf-8?B?NU5MQ2UvZTAzVVoxRU95UG1ueUFVMldtZmR1Yy8wdUtNck5IbmpYMTZKZHdp?=
 =?utf-8?B?ZWNkYndtenhFZGxFOHlleHFTTFVJcitzeFFpcHJjUWhIR3ZSa08zNnVOb1J4?=
 =?utf-8?B?bFdkN2YrcyszaHRtU3o1NzlpV2hMVXVJcDNucXRlTXM2RkZMdnRVRWp1VzRi?=
 =?utf-8?B?cmUvNVFMczBaaHB2OERHdW5mRGVZMmtrRFg5QVI3VFZ3MmloSmVvaUIxQWdN?=
 =?utf-8?B?SVFBTHhBNWw5L1JUc3dTTlNBSVR5ekpuYXNBbUxQcGJtQUpTd1V3cVlQNTA1?=
 =?utf-8?B?YlR3T0xVS3BIbHA0MklWbDM2ek9DYkVBS3VBTFdXL0pWclJxQ0ZLdjFxZUhF?=
 =?utf-8?B?S1h3dUhibldtTmN5cndGTC9HbkgxZnllc3dRTzFWcytNVGFJVEpWY2hSTlRD?=
 =?utf-8?B?REpISTM4RXo3QUlNaHo2Q2V2UFRjMTIwUy8vRDZsVHFmOHFZT2dvbVNMd1FX?=
 =?utf-8?B?UjJvWitJTkpod2l3S0liYzdDc1pkaWdGSFNZTTVENE5VZ3BaY1NPMmc0TXdS?=
 =?utf-8?B?RTJZb1p6cHFXL1NTcHpCWUw5bFRtaUllT3IzSjNOTkhPazNjTnZZaFlxSnBN?=
 =?utf-8?B?Z2NHZ1l1dGd5OUg3SSs2eXVORlFTOXJsYjZOUm9manUxVlU2V2lBeU50MHZ6?=
 =?utf-8?B?L205eWFZWlFYQnloc3lJQ1UrQVlKWDAybE1adVFmKzYwMVpObThhUXRvQlNH?=
 =?utf-8?B?QXord0NRQi8rd2Y5eXppeVcwTHNMb0RrQ1Q5cWY1MlFXZ0NTL05FYVdVVVVi?=
 =?utf-8?B?Y3JJbDVDNDFxS0JFQStaWTlud2Q5SitCamMrWUlHK1hpVXUrZE56Z3JHeVVo?=
 =?utf-8?B?YytqZ3o2b2NZMlYzNFdQc2JNbm9adzJnc3F6QVgrOEFnUG9CbmVXOWR6ZWEz?=
 =?utf-8?B?ekRpbWJpUE84NWVhMjRKZHZZOHlROXFOQVNzRC9qYncvT0NsQWFIWWp6RzhT?=
 =?utf-8?B?SmR4aTB3ZnYrTzVCU1JDS3pyMldkYjJCVUw5QUtEZ3hneHdaZFNPa3A2b3Rq?=
 =?utf-8?B?am40Vm0xVG9SbDV0djRoTEFsSFRja2ljcWRpNnUwU01kcTByZlk2NnVycXJ2?=
 =?utf-8?Q?nrZWdy/NFiqIsRBXWEhYB2o=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bf8ab2f7-0d32-4938-5970-08dc701f963f
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 May 2024 12:00:14.8262
 (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: Rp2SjCauwSVjT7+jiX3xVtWivn8rxc8DzY8shb5mXt7Y0US1KYl+kY15g5Tc2zBLHZJz0B5FuOQbi/gbyZNKPj0ZrDLoNREY2CeodTV4Qsg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4917
X-OriginatorOrg: intel.com

From: Alexander Lobakin <aleksander.lobakin@intel.com>
Date: Thu, 9 May 2024 13:44:37 +0200

> 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

Or invert the flag, so that false would mean "it needs sync" and it
would be the default if dma_*mask*() wasn't called.

Chris, what do you think?

> this, but the correct approach would be to call dma_set_*mask() from the
> respective drivers.
> 
>>
>>
>>> ...
>>
>> Best regards

Thanks,
Olek