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: <0dfa36f1-a847-739e-4557-fc43e2e8c6a7@intel.com>
Date:   Thu, 18 May 2023 17:41:52 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [Intel-wired-lan] [PATCH net-next 07/11] net: page_pool: add
 DMA-sync-for-CPU inline helpers
Content-Language: en-US
To:     Jakub Kicinski <kuba@kernel.org>
CC:     Jesper Dangaard Brouer <hawk@kernel.org>,
        Larysa Zaremba <larysa.zaremba@intel.com>,
        <netdev@vger.kernel.org>,
        Ilias Apalodimas <ilias.apalodimas@linaro.org>,
        <linux-kernel@vger.kernel.org>, "Christoph Hellwig" <hch@lst.de>,
        Eric Dumazet <edumazet@google.com>,
        Michal Kubiak <michal.kubiak@intel.com>,
        <intel-wired-lan@lists.osuosl.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        Magnus Karlsson <magnus.karlsson@intel.com>
References: <20230516161841.37138-1-aleksander.lobakin@intel.com>
 <20230516161841.37138-8-aleksander.lobakin@intel.com>
 <20230517211211.1d1bbd0b@kernel.org>
 <9feef136-7ff3-91a4-4198-237b07a91c0c@intel.com>
 <20230518075643.3a242837@kernel.org>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20230518075643.3a242837@kernel.org>
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?TFdoNDBCVlZMREM2VDJVWmNUb0NqMHIrcGdESVNuQ3luT3BFU3RIQ1VuNEdr?=
 =?utf-8?B?eE16Yy9Fa1Nvb05tNXJ5YVJBVVVkOWh1TmE5ekc1NTJMUE9sL3BSdFdiV3Z5?=
 =?utf-8?B?YW95NlZlU21ZWUp5MnI2QWsxbzZjWE9sTmsyWG1tS25ORUJ4bndMUkdTY04r?=
 =?utf-8?B?Zk16Q0pvd3hCYUZEYS9GKy9paWttVWFjbzlyM1dvTEtWYVBFdSsyVVkwRE9W?=
 =?utf-8?B?THdJREl2ejlxZG5XVkJidG5kcmlNTDlGS01rSWE1OXlRZ1dOZ1dJMFM2aG54?=
 =?utf-8?B?Nk9RTmh3MkxjaUFSeTZPTXRISXRrMEFpNTZhRTNGWFEwL2I2aEdvSHpYZmJU?=
 =?utf-8?B?WEpuYnVtRkRLSXU4UDJXSUF6bUdNVUU2QkpQSVhEZkl4Zzk5VjRHYURGbTZJ?=
 =?utf-8?B?NSs5TWcxYnhZRjRUUS95ZGx3ZVBuNlNOS3loOTNWWm00a3lIalUrWVlRK3NN?=
 =?utf-8?B?MHh0THZvWDl3Y3Z4Tlh0emN0NGcyWEFJNlRrSzR4NWtEK21RUnllZHZNZ1JE?=
 =?utf-8?B?SHdNOEowVzc5SzA0bW40dHJrTVljazF4Y0ZlazZ0U3RNWGJuUHE5WHRPc1hn?=
 =?utf-8?B?Q1l0QlBhdDUvQlpFNjJzWEw2dTVKdEN0MDdhRkNNUjJpdXNWemRjWVdMQjRG?=
 =?utf-8?B?d2djbmcvWUhteGhUZjZDUVJlUjZjRWxHcHYzNVJNaktlVHhaL05Pdm5WRTBV?=
 =?utf-8?B?aUV5VWp0SWxCZk5NaU1zQTcvSjBpZHNGQ3B2bUJFSWZoOFZ5QnBtQ0hnM0hV?=
 =?utf-8?B?T0pGSmpaU29NeUlLSFZpKzFIQlp1aVZYYWpNdVh2WlExbWx0ZUM5NysycWpa?=
 =?utf-8?B?THQwcHhtSG9SZTdBbm4reXg4K1FMa09PRE9YR0dIeWwySGkwNmxVVkxBYTlw?=
 =?utf-8?B?T3BpSWZlVDUyS2dTSGFFNmo0dHRReG50YWtzb2oyLzZ4cVFrdGdLdWJTY1Fa?=
 =?utf-8?B?ZmVzU25yRkNEOVlZVVpPQmdteTRaQW9EcHZudUZwU1BWSlV6MHRtKzhVdjQ1?=
 =?utf-8?B?RlFHYnZzWm5WdVhlT1hXQVVoMDdPcXNXVzcyaEVXdVBFd2laQXI0UXdzeGZB?=
 =?utf-8?B?Z1RaS3E3MzhMaFFqR0VFVytMQkU5dWlBcDZubWMzb2RIOXhHWWNHRnIwNFJr?=
 =?utf-8?B?a0NuK2FjNEMrc2FkbEZESkovclV6cmNUcy9qd3lmWWlEQWxQcFE3NGNtUUNw?=
 =?utf-8?B?RHRiNDBGYzJvMUUyNmw1eWg2TUpyalBNVHJlcGRFRHhiMUJVMjlVYnE1RzdH?=
 =?utf-8?B?eWFSQVRwN3NlWnh4Ukt4ajhiY2M5TnNqdmVDc1Z3c0VtRkdqTkVJZTlGb0R1?=
 =?utf-8?B?MGYwbm1xUFN2dVZ4eWZzdXZGaXQ1WHJlNDhvK1lwYk9oSzZkMEVQY3lINnc2?=
 =?utf-8?B?Y3FLdnJJbnc4aDdldlhUNlQxR2hyUFpiOW15d3hHT0pEUVM5Y2htcUVkWGlE?=
 =?utf-8?B?Y1pxSlV1bzA5WmtUaTJFNTBpRlV1a1VqRXN1U2VuTytFcjNZNnJtVXNCNWdo?=
 =?utf-8?B?bU9HTC9qTlJRaFNYaTFrUXEwQ0h5a1dhZFZjbkxBYjZKQ3ZzR1BHUTNBWUkr?=
 =?utf-8?B?ZXNyY1RGM0RObEM2V3JOUUF6dHBydHhlKzhacVpKL29IUysvV25MSjV3TW4v?=
 =?utf-8?B?OFh5VmQxbTNnQkJYcXlvYk9Fcjk2TWxOZW1iM2hNanNtVmEweDBZWnRBamJJ?=
 =?utf-8?B?cEsvanBYdXRRY0dmeDNLMXRoVVM2cTczRDFJQkRqdmJJT3hoejFGTmUydG91?=
 =?utf-8?B?Y0N1NU9lNzYzODRldHVTL1o5S0VQNzJMOFh5R2hQTGFOS1QyT3lkcjR1aDZk?=
 =?utf-8?B?eEl5SlZtZTZtN25hdnhMV09yWEE0SExxL1Ztd2lpTDlFd0U0T3l0R01aMWFD?=
 =?utf-8?B?S04vTFZKQzY0QTJJaUtzQU1ERXRacmlFVnp0WGh3ZWZNbHdaZFZKUGw0ZXhy?=
 =?utf-8?B?a2drUUgyWjlXRG5SMkd2RXY0dkZvVFViK2pRRzA4MnZaR0RzTFY2ZzJjSFMz?=
 =?utf-8?B?WCtyemhhT3lQRWo4NU1BSE1oMzE1MkpPZXZzSnY4VzJoTW5ya1FPZ29PZU4y?=
 =?utf-8?B?S1ZsUnpqYW9lVTBMN21XRXVuTWlKQ2FYNWZuRmRaVEIrdlUxYVFqUXBiNkFZ?=
 =?utf-8?B?VTA2UzZvdlB6b2hlNUZYTkczeGQ5ZzVrZm9yaG9mS0RPWTByOGtaaEpuOUtT?=
 =?utf-8?B?TGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4d91b120-7ddc-41b4-db0c-08db57b68a21
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 May 2023 15:42:52.0416
 (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: woM+mz5mPOJgz0ew00WvsqF3LIpIF8eenEpDKs35mxTMpVSKbiNvBXqGcHCbq99oHDWzhi0MJUSFDWDw6Kl7RLmgM72J0LvLHzhzMSAvKOQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CYYPR11MB8359
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,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: Jakub Kicinski <kuba@kernel.org>
Date: Thu, 18 May 2023 07:56:43 -0700

> On Thu, 18 May 2023 15:45:33 +0200 Alexander Lobakin wrote:
>>>> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
>>>> index 8435013de06e..f740c50b661f 100644
>>>> --- a/include/net/page_pool.h
>>>> +++ b/include/net/page_pool.h
>>>> @@ -32,7 +32,7 @@
>>>>  
>>>>  #include <linux/mm.h> /* Needed by ptr_ring */
>>>>  #include <linux/ptr_ring.h>
>>>> -#include <linux/dma-direction.h>
>>>> +#include <linux/dma-mapping.h>  
>>>
>>> highly nit picky - but isn't dma-mapping.h pretty heavy?
>>> And we include page_pool.h in skbuff.h. Not that it matters
>>> today, but maybe one day we'll succeed putting skbuff.h
>>> on a diet -- so perhaps it's better to put "inline helpers
>>> with non-trivial dependencies" into a new header?  
>>
>> Maybe we could rather stop including page_pool.h into skbuff.h? It's
>> used there only for  1 external, which could be declared directly in
>> skbuff.h. When Matteo was developing PP recycling, he was storing
>> mem_info in skb as well, but then it was optimized and we don't do that
>> anymore.
>> It annoys sometimes to see the whole kernel rebuilt each time I edit
>> pag_pool.h :D In fact, only PP-enabled drivers and core code need it.
> 
> Or maybe we can do both? I think that separating types, defines and
> simple wrappers from helpers should be considered good code hygiene.

I'll definitely take a look, I also like the idea of minimalistic and
lightweight headers.
page_pool.h and page_pool_drv.h? :D

> 
>>>>  #define PP_FLAG_DMA_MAP		BIT(0) /* Should page_pool do the DMA
>>>>  					* map/unmap  
>>>   
>>>> +/**
>>>> + * page_pool_dma_sync_for_cpu - sync Rx page for CPU after it's written by HW
>>>> + * @pool: page_pool which this page belongs to
>>>> + * @page: page to sync
>>>> + * @dma_sync_size: size of the data written to the page
>>>> + *
>>>> + * Can be used as a shorthand to sync Rx pages before accessing them in the
>>>> + * driver. Caller must ensure the pool was created with %PP_FLAG_DMA_MAP.
>>>> + */
>>>> +static inline void page_pool_dma_sync_for_cpu(const struct page_pool *pool,
>>>> +					      const struct page *page,
>>>> +					      u32 dma_sync_size)
>>>> +{
>>>> +	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?  
>>
>> If the driver doesn't set DMA_SYNC_DEV flag, then the "maybe" version
>> will never do anything. But we may want to use these helpers in such
>> drivers too?
> 
> Oh, I see, the polarity of the flag is awkward. Hm.
> Maybe just rename things, drop the "maybe_" and prefix the non-checking
> version with __ ? We expect drivers to call the version which check the
> flag mostly (AFAIU), so it should have the most obvious name.
> Plus perhaps a sentence in the kdoc explaining why __ exists would be
> good, if it wasn't obvious to me it may not be obvious to others..

Ah, +, good point.

Thanks,
Olek