Received-SPF: pass (google.com: domain of linux-kernel+bounces-42926-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <6059bf0c-cfe6-41dd-8672-584c9c13b902@intel.com>
Date: Mon, 29 Jan 2024 15:36:35 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next 2/7] dma: avoid expensive redundant calls for
 sync operations
Content-Language: en-US
To: =?UTF-8?B?UGV0ciBUZXNhxZnDrWs=?= <petr@tesarici.cz>, Robin Murphy
	<robin.murphy@arm.com>
CC: "David S. Miller" <davem@davemloft.net>, Eric Dumazet
	<edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>, Christoph Hellwig <hch@lst.de>, Marek Szyprowski
	<m.szyprowski@samsung.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: <20240126135456.704351-1-aleksander.lobakin@intel.com>
 <20240126135456.704351-3-aleksander.lobakin@intel.com>
 <0f6f550c-3eee-46dc-8c42-baceaa237610@arm.com>
 <7ff3cf5d-b3ff-4b52-9031-30a1cb71c0c9@intel.com>
 <0cf72c00-21d9-4f1a-be14-80336da5dff4@arm.com>
 <20240126194819.147cb4e2@meshulam.tesarici.cz>
From: Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20240126194819.147cb4e2@meshulam.tesarici.cz>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SHdVbjVHMGFVaGpYM2ZKSGdjQkp4YklHaHp2ajJLeVlld0piZmFGcGsya1Av?=
 =?utf-8?B?VGRoTUhvMThLSWlMR2t4TmhIOHQrcDZEeWRMZ21YQmFRcFMrc3V2QUVYVE1u?=
 =?utf-8?B?c2xUaXJQSDNCOGlXbnBFNzIyUytsdkZ1RWx5Zkdob25Gc1NpWUMrU01RWnBt?=
 =?utf-8?B?QWpqQ0k3TWF2aWx4b0RZYWRnTEFWS0o0VFRvekZwakZaZUNNNUdBOXd2Q2M2?=
 =?utf-8?B?ck5NMkNjWm1OOCtxOUZ4NTlpeENPeUQ1ZEQwMnVxYjREMU9JUmVHU25QRmlK?=
 =?utf-8?B?TFhkaTFSNEdzUXZNZE1OT2VUUWJxVW8yOWpJalVGRmYwL29abzRQRWRHUTll?=
 =?utf-8?B?MjR5WnlvTDNwTHhrMktVN0J4NkhKQ0xScXd2ZVlLNWlRWi9HTzN5M29oRlVB?=
 =?utf-8?B?aVh5L3BuTVpMejhqaENsTHBKVXBXUjhyUytaMVdueS90eXc4U2NYTjYvMEVN?=
 =?utf-8?B?b2s0eEJJQlpIUklCbk1xeTA5QXJSck1ISlYyenhkV1JGRGs0VU40d3BWYlRu?=
 =?utf-8?B?Wmx1dlY2QzZiUW85bXF2a3NFVk1aQU43cTZTRHg3d0U0N1NuT3VLZTBxM2tB?=
 =?utf-8?B?bnRlaXFiYWJHMXJZV1ZoU252V2p5YzA1SzNRVTI1RWxiYjk1U1M3Q1I3eHYw?=
 =?utf-8?B?TThrcFJ5ejh2alFralc5c2VuV3ZSTTNERWRvUWNkZWhsM1ZCeTNIcFpvalg2?=
 =?utf-8?B?RlNDSlQ0QzJNOWxrNDdVR3EwZlhMT1AvS3J5S2NuUVNBaFROeEVzNE5KOEdv?=
 =?utf-8?B?TlNyVWUvOFhuU3Z3V0liQ1dBZ1k5Ti9ORk9OckFFSW9lZythdEV2N3NReWI5?=
 =?utf-8?B?SFIzUnZVdlhHTGN3WG9tU2lwS3YwWUltbHpyTWVsOGNqclNXbTkxYUUwc3dv?=
 =?utf-8?B?czlRQ1lPa1puSGpJNlpZWit0STViYjVmaFNVOWQ1NSthQjExTHQxbVJ5dk1M?=
 =?utf-8?B?clZobGJlbjhEYnpjRXgyN05PZFNXUFZodXRZMW1ETlozdkJaYmg4SDZrd05L?=
 =?utf-8?B?L21vTXdXNFVlNkhMdmZvT0dZcjBtYnRlZGZMaFA4cTRrRzBqYnVwT0psWHRl?=
 =?utf-8?B?SWlNQVR3akQ1TGo4K0UxakJYRmREYXVoWVNoeEQvMnl5VFRKZUFFSHpyNStp?=
 =?utf-8?B?THQwcjdUUUoreWtEZ244WGRVU3VXOUF4UldKL0gzdCtjV0hkVk5oRlVvS1V5?=
 =?utf-8?B?LzRURU1VNTJnMjFXUytxVGlycVVGUTlJVmJVaFcyb2lNVUdNVktmclJaQkRj?=
 =?utf-8?B?a2IzRVo0dWVuQXI1SmU5YkFvaVFqSHRYbHp2U29qNjBNaWJGYXkxT281RlhJ?=
 =?utf-8?B?TFZUeUVBZE56VXBwZSthRlY5NVR5bmRSTm1ZK1hLNHE2MmY0TjhVWml0ZXNr?=
 =?utf-8?B?NDVSWXJuYjZSQ2kvZEtOM3FjR3ErVFUyN21STmwxRHJnay9GTDF5UVZPWURM?=
 =?utf-8?B?ZlVPdEFCOERGUDh1QXFJcy81OCswd1BWZlNYMnduNmpEbGJPK1VUTHljbjQ1?=
 =?utf-8?B?bnRLQnFGVjBOOUtWemdJWWxpbkhtaTdlRUVPOHV1V1g1dTliV3AzSEl3bVRM?=
 =?utf-8?B?WUJvcW8wTEhzcmIrc1BIL2ZpYUtHV00zb2N3ckJhUWQ1MER6WEZOellTTXhx?=
 =?utf-8?B?NHNnZXV5Mi9ZaGZFcGpRcUVLb2FoWEpaVVkwRllLSXF0dSs1UEF1aXhKKzhP?=
 =?utf-8?B?cHBJMGpQZXlrYnMvYWJ4WkRJY2pMSW8vMVgrZm9PaHlhcDRYdjhWanlXM0U3?=
 =?utf-8?B?WnE0VnZNWWF3VGYwMElnL2dmZWtYWXJDOHhwMHFDT1FiMmtqSVlZMXI5VE1z?=
 =?utf-8?B?VXBleDBOeTlZdHJEUDhDdm9SN09QSTR5QlBTT0d2akZFSDB6WHRXNTdwcDly?=
 =?utf-8?B?WlpuVnRCREZiWjUrY01WZjJidS8xbVFpNlQ2Y3lDWW5BOU53TUd4OXViQXR5?=
 =?utf-8?B?WFpjLyttTUFBRnB2d1IwRGdOMFZnelhkV2pjOFIxcXBaYkw3VU5NdUErQ1ZQ?=
 =?utf-8?B?eEtwSWF6SVpYRU4rM0hrT0lwaERsdDNBT1gzaTFTWGNyd2tJMjJDTlpjbFUz?=
 =?utf-8?B?ckRVY2xncFBiSnR0bzI1R0NxSjVpVVBEVXpKOUo3V1BTVi9Yb01UdU9WZDV0?=
 =?utf-8?B?aDNRMHdOMDUvWVJsZXVXZFNleUxWVzBmTkJxNU12RUR5UWRyK0VSa1hLTFBR?=
 =?utf-8?B?YkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 06c893fd-9be4-4918-13f0-08dc20d7c491
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jan 2024 14:37:06.9712
 (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: gvhUxPaZqiQKYXf9hXoSK3iBJ37we4vXjScmuVuLy5Q7pHcjfpX4Vbw/Qyn/OosM5CX26QRnIkMHD1FPJM2HT2/PiEFX+OfKLZwJCpNt1Nk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5144
X-OriginatorOrg: intel.com

From: Petr Tesařík <petr@tesarici.cz>
Date: Fri, 26 Jan 2024 19:48:19 +0100

> On Fri, 26 Jan 2024 17:21:24 +0000
> Robin Murphy <robin.murphy@arm.com> wrote:
> 
>> On 26/01/2024 4:45 pm, Alexander Lobakin wrote:
>>> From: Robin Murphy <robin.murphy@arm.com>
>>> Date: Fri, 26 Jan 2024 15:48:54 +0000
>>>   
>>>> On 26/01/2024 1:54 pm, Alexander Lobakin wrote:  
>>>>> From: Eric Dumazet <edumazet@google.com>
>>>>>
>>>>> Quite often, NIC 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->skip_dma_sync boolean which is set during the device
>>>>> initialization depending on the setup: dev_is_dma_coherent() for direct
>>>>> DMA, !(sync_single_for_device || sync_single_for_cpu) or positive result
>>>>> from the new callback, dma_map_ops::can_skip_sync for non-NULL DMA ops.
>>>>> Then later, if/when swiotlb is used for the first time, the flag
>>>>> is turned off, from swiotlb_tbl_map_single().  
>>>>
>>>> I think you could probably just promote the dma_uses_io_tlb flag from
>>>> SWIOTLB_DYNAMIC to a general SWIOTLB thing to serve this purpose now.  
>>>
>>> Nice catch!
>>>   
>>>>
>>>> Similarly I don't think a new op is necessary now that we have
>>>> dma_map_ops.flags. A simple static flag to indicate that sync may be> skipped under the same conditions as implied for dma-direct - i.e.
>>>> dev_is_dma_coherent(dev) && !dev->dma_use_io_tlb - seems like it ought
>>>> to suffice.  
>>>
>>> In my initial implementation, I used a new dma_map_ops flag, but then I
>>> realized different DMA ops may require or not require syncing under
>>> different conditions, not only dev_is_dma_coherent().
>>> Or am I wrong and they would always be the same?  
>>
>> I think it's safe to assume that, as with P2P support, this will only 
>> matter for dma-direct and iommu-dma for the foreseeable future, and 
>> those do currently share the same conditions as above. Thus we may as 
>> well keep things simple for now, and if anything ever does have cause to 
>> change, it can be the future's problem to keep this mechanism working as 
>> intended.
> 
> Can we have a comment that states this assumption along with the flag?
> Because when it breaks, it will keep someone cursing for days why DMA
> sometimes fails on their device before they find out it's not synced.

BTW, dma_skip_sync is set right before driver->probe(), so that if any
problematic device appears, it could easily be fixed by adding one line
to its probe callback.

> And then wondering why the code makes such silly assumptions...
> 
> My two cents
> Petr T

Thanks,
Olek