Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b19f43b8-f973-2627-3ef1-ce53007ece0f@intel.com>
Date:   Thu, 18 May 2023 15:53:43 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH net-next 07/11] net: page_pool: add DMA-sync-for-CPU
 inline helpers
Content-Language: en-US
To:     Ilias Apalodimas <ilias.apalodimas@linaro.org>
CC:     Jakub Kicinski <kuba@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        "Maciej Fijalkowski" <maciej.fijalkowski@intel.com>,
        Magnus Karlsson <magnus.karlsson@intel.com>,
        Michal Kubiak <michal.kubiak@intel.com>,
        "Larysa Zaremba" <larysa.zaremba@intel.com>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        Christoph Hellwig <hch@lst.de>, <netdev@vger.kernel.org>,
        <intel-wired-lan@lists.osuosl.org>, <linux-kernel@vger.kernel.org>
References: <20230516161841.37138-1-aleksander.lobakin@intel.com>
 <20230516161841.37138-8-aleksander.lobakin@intel.com>
 <20230517211211.1d1bbd0b@kernel.org> <ZGXNzX77/5cXqAhe@hera>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <ZGXNzX77/5cXqAhe@hera>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?djZFMjF6NGxwb0pkRnlEbDY2ckFSaDNucEUrTFdMWHZDUDJqNTc2TUErazRZ?=
 =?utf-8?B?dUEyRXRHbEFsTXB3WDY3NHZXTWFYQ1ZpWjBBRXUrTmhRbjdiVlFWS2VmQ1pZ?=
 =?utf-8?B?OE05VlgwOWFyWUpkSDFuWVRBTWhoRDdrR1plT3d5Mlk4MHN2b2FnMU42SHNI?=
 =?utf-8?B?NWNsc3ZCY2pjcUpSREkzMmQ3NEg4Q1dVQ3pHYUxwUXBJK0VqbWVKMVg4YWxP?=
 =?utf-8?B?eUFSMi9wQ2M4ZE9aYmVMOHVCNHBzMlZ5QjJFamVreWFudnVjTGNaWGo0Y3Zy?=
 =?utf-8?B?WU1HM1ZLNHJ3SWRFR3VkcDJWN251RmJOR28yb0tzK1E5U1FWTDFXa3VEOEM5?=
 =?utf-8?B?bWF1bVJGQnpWRTd4dElpby9iRUp2S0tQZWJDbkV2WldDZlVqV1BTdmk2RzRu?=
 =?utf-8?B?QlJnejFLajBlWTZDWXo5YmFpN2NmSHVrSGxvN0RxQjVOb0Y3NjZkMXV5QTQw?=
 =?utf-8?B?TUJMeEFsR0lGRGVtS20xcjFEclFMaCsrR2ZVYmZyTWxoMHhXWUdvUEJNWWVJ?=
 =?utf-8?B?YUJWbmF2SU1KSDZxWnIyUDhZZlVIR1BtdndaNlBua2RMaTBldEhxUGpkbDdw?=
 =?utf-8?B?WjVSeWRjZTZqc2o3dU9qY2RwM2FEWW9NSUR0LzRnVERJQ2o0UHlQZ1pQczhm?=
 =?utf-8?B?SG1icGRaT3l1aWhnVGR6R3J0YTlSUnpQQm9oN3pCZGdUd28vY2hEUDlqZDFW?=
 =?utf-8?B?YjNtR0pvVlB1YVpFdkVjeWptOUFPS2NQbmJIemd2T20rYkZLdVowelNBN1Nt?=
 =?utf-8?B?YngzTzAxR044Q212RkN4VTh5YnNDV00wbUpNUzRxOWRSb1VGaUc5cWdWUFFn?=
 =?utf-8?B?R3NsUGZNSmdvcEtsT0swcE5TL1BERzRLQWdHZ2VqY29IVEZOWklUNkIwdllM?=
 =?utf-8?B?SlhkU2V5elluc1NNU1pxMkhqZzYwV09SQnc0QkY1YnBpZndjQlFkdDh2RXFj?=
 =?utf-8?B?NDNaT3M0dVRETHN0UzJvY0VqWVNwSnM4d1NwV1l5NUM2a05mK2o2WC9pbG9l?=
 =?utf-8?B?bDNKcVhBcXBjZHI5QXBiMGgwYTJ2SkZnZVU5NERMWXlaY3J6YTVLbEpmMjdr?=
 =?utf-8?B?M1RHbWdTRWt5UEJUMFFGMHZLY1poRkRYNFJBeVFzYnFkbFk5d2J0c1lIbjZy?=
 =?utf-8?B?USsweG9aYUZJb2c1bUNpUjZqTGhpU1ZhMHhGRGF1MmJ3VEJ5YTBNM1ZYL0Rx?=
 =?utf-8?B?NTBjVS91MDUyYktkNFZWdmZxa0F4cGlVZEx3LzVmc29ESkFXYW53WUZkajRh?=
 =?utf-8?B?WDdvWGhQSE0zWFBtVnhmUmJnV0s3dzhZUjhyV1ArcmRkVzNBNGNNTUlPUURq?=
 =?utf-8?B?YnROcHNOTmdhNmVEcW1NeVg1RkNJenhWdlNiUXBvengzN2hLa1EyOU9qb3VJ?=
 =?utf-8?B?YjVJWCtzRzhpSzBEajJoZDZ3dllRczFXS0NCRG5BSHdhcnFyMk9NMmc1RGVR?=
 =?utf-8?B?cTBQTFZFUTNid1JOOEx6WCtQNWsxTTNMSFVzbjVQeXl3Ky9ycXh2c1JIZ1ps?=
 =?utf-8?B?ajdRdUlKMnZqZTFZa2JYSEN1S1pSV3ZOWHV1ZmFHcFl5UnZ4OUFJUk4vQTdD?=
 =?utf-8?B?NUZNc3lDeHdXVUkxcnl6WldhVDlMdHdwRUcwaE9kYThOM2FVKzE1a3V3dGZm?=
 =?utf-8?B?V1FsYkFCNVoyaGpBSWtONHBNNmliWTVzTE1mUE5kdks3T3c4QnFkVXp6dHB6?=
 =?utf-8?B?OHdpS2hMUzEyT01PNWZuK0l3SGUrRkRmeEgwQjZwM1BIdVdYRytkT1RLMGpk?=
 =?utf-8?B?bGVVRHluWTRoSjZMcXlKNWNvSXgxWnd0d1Z6eFVhTThUdjBPK1FRT0MyVldZ?=
 =?utf-8?B?NGxmUUtKRGJVYkpKNnN0NjZzMW9sRFJ4elZaSk1vMmRCMTFrTStxYS9uRENh?=
 =?utf-8?B?Zmc3Um5ueVRuSjJMODlVNTlMVnEvMjMxNVRmaGNLcll3NGZIZmRzeXV2RGFN?=
 =?utf-8?B?S0xSSk95YzdsYlI2Rm41dnJtRXErY2M1ZVM2V2hTOGJwNWpYUXRUNmUxTWY1?=
 =?utf-8?B?S1VQazI1V2o4T3M0VkdYZnhzaktseGwvTXZNSzVRVmhmaW50a3NhYXl5aldF?=
 =?utf-8?B?ckVMSVRSZDhoemx6dXFOcUtRN0w4aVRXZ2Q0ZVNRQTgraVpyRlowdEJ3em1m?=
 =?utf-8?B?UlRQVkNPZWtCaE1iSWJkMDE5L2h6WGM0TGxvbEdRUG1oZ3Y2YmdNSWR5azhz?=
 =?utf-8?B?eGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 35bddc70-9349-483e-dc44-08db57a76e55
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 May 2023 13:54:42.9372
 (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: PzbNMX2pvTKzWyIDnHRdUR1+eQUrT0b/ccSZpqk2Ee5vNAgvxZUoIg90kHh2kmfcFWZ1MKMril40EipIamLbITO4CP9WU2Y3VO+biG6v3Kw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB6110
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Date: Thu, 18 May 2023 10:03:41 +0300

> Hi all,
> 
>> On Wed, May 17, 2023 at 09:12:11PM -0700, Jakub Kicinski wrote:
>> On Tue, 16 May 2023 18:18:37 +0200 Alexander Lobakin wrote:
>>> Each driver is responsible for syncing buffers written by HW for CPU
>>> before accessing them. Almost each PP-enabled driver uses the same
>>> pattern, which could be shorthanded into a static inline to make driver
>>> code a little bit more compact.

[...]

>>> +	dma_sync_single_range_for_cpu(pool->p.dev,
>>> +				      page_pool_get_dma_addr(page),
>>> +				      pool->p.offset, dma_sync_size,
>>> +				      page_pool_get_dma_dir(pool));
>>
>> Likely a dumb question but why does this exist?
>> Is there a case where the "maybe" version is not safe?
>>
> 
> I got similar concerns here.  Syncing for the cpu is currently a
> responsibility for the driver.  The reason for having an automated DMA sync
> is that we know when we allocate buffers for the NIC to consume so we can
> safely sync them accordingly.  I am fine having a page pool version for the
> cpu sync, but do we really have to check the pp flags for that?  IOW if you
> are at the point that you need to sync a buffer for the cpu *someone*
> already mapped it for you.  Regardsless of who mapped it the sync is
> identical

The flag in the "maybe" version is the continuation of the shortcut from
6/11. If the flag is not set, but you asked PP to do syncs, that means
it enabled the shortcut to not go through function call ladders for
nothing. The ladder is basically the same for sync-for-CPU as the one
described in 6/11 for sync-for-dev.
I could place that in the driver, but I feel like it's better to have
that one level up to reduce boilerplating.

> 
>>> +}
>>> +
>>> +/**
>>> + * page_pool_dma_maybe_sync_for_cpu - sync Rx page for CPU if needed
>>> + * @pool: page_pool which this page belongs to
>>> + * @page: page to sync
>>> + * @dma_sync_size: size of the data written to the page
>>> + *
>>> + * Performs DMA sync for CPU, but only when required (swiotlb, IOMMU etc.).
>>> + */
>>> +static inline void
>>> +page_pool_dma_maybe_sync_for_cpu(const struct page_pool *pool,
>>> +				 const struct page *page, u32 dma_sync_size)
>>> +{
>>> +	if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
>>> +		page_pool_dma_sync_for_cpu(pool, page, dma_sync_size);
>>> +}
> 
> Thanks
> /Ilias

Thanks,
Olek