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: <77d929b2-c124-d3db-1cd9-8301d1d269d3@intel.com>
Date:   Fri, 19 May 2023 15:56:40 +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>
 <0dfa36f1-a847-739e-4557-fc43e2e8c6a7@intel.com>
 <20230518133627.72747418@kernel.org>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20230518133627.72747418@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?MVpmdkFic0xyOXMzMUZxYVM1YVpNdmI1eENGTHE4WkgzZnhJaVdpRGxRQ0My?=
 =?utf-8?B?bEIyeHo5dkRWbE84ZmJVcmxwVHI4bkpWUklNMjRoMFI2M1J6OHltRzBVUThN?=
 =?utf-8?B?TUpDaVJ2dHlva3pQWU5iZjl2QmZmQ3oyY25WaHJOMDBlTU5UTTlGM2ViamE2?=
 =?utf-8?B?dE9QVGlpTVRyL3QwZWlBMmUyaDdOWk85QUtYdkhEV09NdlVVTHA5ZTV1Zngr?=
 =?utf-8?B?anBCNjY1WWN1dk5KdHRKZTg5ZWtsdU5kR05lSCtUNjVFaXAxZmFPQUtZS3o2?=
 =?utf-8?B?OHdyb1hVcm1RU05BaSs0Z1pDa2dXVVF4UzRZNnp2RWhhYmpCbmxpUEo1SmxD?=
 =?utf-8?B?NjhvWFJsbUQ2TW5tbFVQMFN6NDYrUG0ydGtpeFlYNit2UHhwSzdWeEJJMnAz?=
 =?utf-8?B?bEhFOTRRT3FueXdEZDQ2WGpvZTArNk1ucWs2N2FUUnJhNDZNRHZDOUwrU0p6?=
 =?utf-8?B?WG5TbEVEYzg2WGlHYkpRdS8yU21kb3k0ZkE2djlMeXhqbEZZRjZqWUFLSEF2?=
 =?utf-8?B?cGlEdFBnTTcrYlh6c1h5RTcwbm9NSGE4dS9yK2hkaENrQ3FsZVQwMXcvUUdp?=
 =?utf-8?B?ZC91NTRZeFJVZVhTRUEvcFNiRzdmMFJnRWNaUVhFUTQ4eVRSNTBoeXlpZUtR?=
 =?utf-8?B?bXBabHdGVlIzQnc5M1Qwb0lBWFAyREZMdWJTOTZyY0JLWHdhSUxnUmRaNElV?=
 =?utf-8?B?cmkzQk9DQ3Vrb2pWaWJpQjBidVJhbkZKZzlHTC9PRlVMSVdPTWN2WjdBU01t?=
 =?utf-8?B?Q1EraloraWI3QlJmWlJVSS9EbGVPTlNJMFdFMzdnRjk4a2ZRQ2Z5RktmZ0Nq?=
 =?utf-8?B?di95VlAwQ2hjZURTZ2crcyszbHJ1TUpscUdydU9Xa1YrR1p3Uk40TVV1QjhB?=
 =?utf-8?B?ZkdYQ0tWK0l0aFNKTkhlbzJ0NmZQMlhHQnJpWWljazFxd1lBOTRuNk5hZnhQ?=
 =?utf-8?B?bDU0eXZ0ZWJVWHVRcVJlN0orby9OSnh5R3B1NnhGek03cVhMTlZyNTYrR2I3?=
 =?utf-8?B?UDFuNjRjQjdoU3UwR3JpWno3aURSSjhjVXlheTJDZHNVQlowNGZWNnd1QU1H?=
 =?utf-8?B?SkFqSzd0cEJuTGdsNjNpamxhWjA0WVcyZFFJTzZXNHNsZ1Y0bVhmRXpZVTV5?=
 =?utf-8?B?MXhkTHZlS1F1YXZJajc2OVZIaGRTdnU0RUQwOVpIT2V1NE4xVTdGUWNKaEtL?=
 =?utf-8?B?b2tsbWhOWGVTejZkdVNtdXk2eGY2QVVaMWx1WkVIYlR5aDlZOEVRcHBab081?=
 =?utf-8?B?QnltZ2NVSnJ6SERPM25JOWRvUWlEK1lWdldGYmFCWXlyOEVienQ0YjR3bk95?=
 =?utf-8?B?MG5GOThXMm1ScVZqYSt4VFhBNTlnQU9iR0ZBbjNjcEhJYWNTRjMyMXBWRit1?=
 =?utf-8?B?YXI1Z0hhbms2R3lOV25BeG1zb25KNHE4SzlZblUrTkNRQnFKS2tEM2RadVV3?=
 =?utf-8?B?REs4K3dFN0lNMVV1RmNJTkJDRXg4TzBMTVptd0ZIN0d0aitRdWhMYVhCRVRG?=
 =?utf-8?B?Zk9DMHhuZGhXSElhTXRvL3J2UEJ4OU5RVW1yVUVadVFvejFSTkNDQ09JZ09Z?=
 =?utf-8?B?UGdYd2dVL0RxUXp0WVU4VEJhYit1cEk3czlMUi9sLy8wMUk5TXBHU2FzeC9X?=
 =?utf-8?B?TUc3bFN1Z2FCeDBRMDdvQ0labzQ3U2pHVnZObmgzVytCTm5nVFBHanlFb0ow?=
 =?utf-8?B?c0hmQU8ya3M1TGVvYllEZWJLK0RST1JQaWc1T3NGRzM4cDRZWHFIRU4rell1?=
 =?utf-8?B?UXI5QkU3RHZXM2ZHMTU5SHl0cktpVG9vcXVXUjJBanRNeS83UGdFYnI3MXV1?=
 =?utf-8?B?cGRzWVlTbzdyTE9uODVyRGJ3U2VJUEplcW5ORjVMbHY5V3ZBdnQxUHlUSnh4?=
 =?utf-8?B?alFoUzAvZ0N5bFE0Wi9ncUdOVmlZQkRnWGpGS1JLQjVoY2gwRW51ME52R1Jv?=
 =?utf-8?B?L2hjZTJudlYxallha2dHc2h1NTJHS1VxV0wzSzdMdkNvM3RZR29wUXNMTXVq?=
 =?utf-8?B?WWd3RE1LaTNjaTlldWlqUjRRWUlWSlB5SENDUXF2UUx0RjZGRVk4UGNYdDVy?=
 =?utf-8?B?L0NtWm9TVDl0UCtrblovdHAxc2RKYi9COVpDMDQ0OHJVb3EyNk5qcjRRSFdx?=
 =?utf-8?B?ZFZ0K2RqY2Y3S09JcG1pL3E1M2QvMnJQKzVsWjJlbXNtL2F5S2c0LzJJYlRJ?=
 =?utf-8?B?UlE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ca6a3db3-b143-42cd-31c5-08db58710826
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 May 2023 13:57:49.8326
 (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: pFL8i3C1K3fLVACuvCfvc9PCE3OOlt2XMzzRPbs8pbqqNhJy6DZikyTB9Q/9D9yVStwxFy/+/v/4QCz1pqWQyO7v9vnGlpfb7S5/pmAU8rs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB5041
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.9 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,
        URIBL_BLOCKED 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 13:36:27 -0700

> On Thu, 18 May 2023 17:41:52 +0200 Alexander Lobakin wrote:
>>> 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
> 
> What I've been doing lately is split like this:
> 
> include/net/something.h           (simply includes all other headers)
> include/net/something/types.h     (structs, defines, enums)
> include/net/something/functions.h (inlines and function declarations)
> 
> If that's reasonable -- we should put the helpers under
> 
> include/net/page_pool/functions.h ?

Hmm, all files that need something from page_pool.h usually need both
types and functions. Not sure we'll benefit anything here. OTOH leaving
those sync-for-cpu inlines alone allows to avoid including dma-mapping.h
and currently only IAVF needs them. So my idea is:

- you need smth from PP, but not sync-for-cpu -- more lightweight
  page_pool.h is for you;
- you need sync-for-cpu (or maybe something else with heavy deps in the
  future) -- just include page_pool_drv.h.

I tried moving something else, but couldn't find anything that would
give any win. <linux/mm.h> and <linux/ptr_ring.h> are needed to define
`struct page_pool`, i.e. even being structured like in your example they
would've gone into pp/types.h =\
`struct ptr_ring` itself doesn't require any MM-related definitions, so
would we split it into ptr_ring/{types,functions}.h, we could probably
avoid a couple includes :D

Thanks,
Olek