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: <61965a16-55bf-2f81-8938-856f834d35bf@intel.com>
Date:   Tue, 1 Aug 2023 15:25:38 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH net-next 2/9] net: skbuff: don't include
 <net/page_pool/types.h> to <linux/skbuff.h>
Content-Language: en-US
To:     Yunsheng Lin <linyunsheng@huawei.com>
CC:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Larysa Zaremba <larysa.zaremba@intel.com>,
        Alexander Duyck <alexanderduyck@fb.com>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        "Ilias Apalodimas" <ilias.apalodimas@linaro.org>,
        Simon Horman <simon.horman@corigine.com>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230727144336.1646454-1-aleksander.lobakin@intel.com>
 <20230727144336.1646454-3-aleksander.lobakin@intel.com>
 <f283dfa9-d599-7311-1c2f-4317c2f7957d@huawei.com>
 <5ba84af2-51d1-de5d-14cc-752c08e5371f@intel.com>
 <5830338c-d025-2fb8-46a6-58508493b2cf@huawei.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <5830338c-d025-2fb8-46a6-58508493b2cf@huawei.com>
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?MHNaWUo0QmwwaEd4dnJJelA0TnRQcm80SUZKZ21XVmlVSEFJSkdySnQzTXQ1?=
 =?utf-8?B?RVFRZmF6S0NmendibDRqYk5wblAwRThJL3Y1SDJhVDUwNldjZnF3WGJFYUJ5?=
 =?utf-8?B?OFJDY1p3UGJsZWd0aWl6OWIrSTRiS2UrbGRrejVqNWJnL0tVTEQ0NEJ0dlVQ?=
 =?utf-8?B?VXl0bEgyUEk0cjdsUzNGTGgxdk9xNlVjeU53KzFPVFBkaTlKQkgwaDJud241?=
 =?utf-8?B?ODFjcDl5VG1Gaml0akpRdDQxS3NYZUUrdzhuY0JqUDRodjBkckhUT0VLczZO?=
 =?utf-8?B?c3RLK0hRenBYZ0ZoUEV4Z0xmdDVpSU5oMklZcTFJYUNpOGJPT1EvbzhWWkRj?=
 =?utf-8?B?L0VoUmY4ekwyOTloZ3hjTjBGTG8wUTlDdnYyaFZBSVRaQXhzU3NMYzdTRUM4?=
 =?utf-8?B?TTAvMWYrQ3NiK3JuZktKSTJZUVZpay90UWpjdm9UYVNBeFo2b2JxWnpyRTUr?=
 =?utf-8?B?VVhFcElZbWJsK2VQenB3K2FEVlVkemdHMm9MUGIrRG9saTRoM0RzbWxEZjNw?=
 =?utf-8?B?K3pHUUdEM2Roc0xjK2NrajVmODB0ZWJOSHp3MG5vYmlEUy9wQ3M4bW9ydWNE?=
 =?utf-8?B?ZzVjSEpZdS9SMHgvbEQ2UnZuM1NKMzFaS1dDN0phQnJNSEdKVmNtNzhRcUZx?=
 =?utf-8?B?dERYaFFZaGYzMC9pM1FHaXd1NkM0ZWpSOTBTQkV0Uks3WVhNQ3h3cEw1Qjhj?=
 =?utf-8?B?TmxUKzhSTDBJUklzQnRzeVFNVHVORDJyOTVEQmtuVzlPYXNIUUJZVDRiV0F1?=
 =?utf-8?B?NEpib0RVZXRUT2hQSlB4bFNXdXJRUVFjQjAxQktqSjExZ2xQcHNwVlFYUXYy?=
 =?utf-8?B?ZGdHMk92d2d2UnBZblZvRnYzMnFiWUNzNHF2d1RqZDgzTzY3QVdZTlRQb2FV?=
 =?utf-8?B?K3cvV0tQdU9qWTdKelg5SGRxQURhNlF0M0ozVkdHcjNFSS9WY3NnU0VuRFli?=
 =?utf-8?B?aDB5UC96bjNXRENaMTdRZG9RcEFydjhrMEJLODlzSGpBejhPUGp2ZExOVVdR?=
 =?utf-8?B?ZklPMWJYOVB1eEFnSDAvNk1UZXRpVm8wTzdCQ2Q4bTEvTmNNbDgzVEtySEx1?=
 =?utf-8?B?QmNXL2g3YUR5NmVtMWpPc2JTQ2xjU0EwVDlsRWlYUmRWTk8xYzIrb2J6Y3dC?=
 =?utf-8?B?R3JmSE56QXBPWHc2K25jL3hYN3dvTGR1Tk1JT2ppR0Z1d2w2VzZOUUppalJq?=
 =?utf-8?B?ZDJEdXUrQ3RJVmZOMHliMjFSU1FDNndIR1NLaWtMcXhacDRrUi8zdFRYaStz?=
 =?utf-8?B?MHlBQ0t2SXhTZXJxdkdTb0ZLU1dMQXA5S3VOVHVFdXU0MzBkcmNucEdHOHFi?=
 =?utf-8?B?RStoTk4wTnh0VmE0Ni9wTEIzVDR4YW1NTC9Sc0JySVYrS0ZXU3NVMDVzamVJ?=
 =?utf-8?B?WlJzL09vNWR2Q2xPOFp5bm5taGliNUZKZSs0VWhBOFl6ays3TUFEWjRrS1Vl?=
 =?utf-8?B?QkdaNGNXK1Y2UlhhdUsrdFU4T2Q0UUxaQkpEWjFLMHF6eTJ1RjhuUS82RHkz?=
 =?utf-8?B?cUhtNWhQWG5ZYlRHSVg3QWQ5TFcyanpLNjRzMDRxNlVXNEFQcElDTUM0RUZX?=
 =?utf-8?B?VWRDU09qRmplMW5wQzB4RWJGaUt5ejBIWEo1cEl6d0FRQzVGenVSMFV0bXdr?=
 =?utf-8?B?VGpodm5ZZGt5cm10cUdSemFaV2xoUHVUOUxneVJhNDBRS2M2OWJ2TTViVjFT?=
 =?utf-8?B?L2swZ1NXUmJ1cElGSi9NNkJkdVFGRmRVOERTbDFVYVIrQ1NPQWdBZjlsQm96?=
 =?utf-8?B?cTIwbFNjQzMrVC9zcVlxeGRlUHl6UjZycWMrQi9lazkrZkEvR0UvYVM4Tngx?=
 =?utf-8?B?eWZQUmxFZk02cmFvTmplKzFxUzh5UjdJdFREQlNLNkNSTCtMWXFPcFVFdG93?=
 =?utf-8?B?cmc3eGV0UFcyeEdoY3B4RFpBWHJydDdxemx4SGF3enJFVW1DWk9lNk9WQyt6?=
 =?utf-8?B?bVB4WnN1ZDJ3Zk1Bc3ZNZDJWd3BuWjdBRW1xcExUWDFzbnAyKzNjOUNDWERu?=
 =?utf-8?B?VnQ2MXZ0UnhMYWl1aEZMWEpHM1IxQmdiU0F0VkJVS1FWSkxWdW9PZUtodjM3?=
 =?utf-8?B?emN1QjlHM0d2MUpiU20vQjJyT2hraXhaQWZaSTFYZFVMK3JrQXdocEFsWnNN?=
 =?utf-8?B?Mk5nbjRuRFpKR0tkZkhNVXZ5TGVrNlZHZVlrRnNnU2dMS09KUUtlWDNKWFpo?=
 =?utf-8?B?RVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d1963db2-3748-4ec1-4388-08db92930ba5
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Aug 2023 13:27:25.9870
 (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: UrgKbbHjsL4F/qnNriGbku4Fb1UVwpt2/xjMI158xVTEicKWKpZEttfKLqJBNIFWI9IesigAazS4ZZ5VuA5qC69wjBj6I+ulkPnrldGqk2Q=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4871
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.2 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_BLOCKED,RCVD_IN_MSPIKE_H2,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: Yunsheng Lin <linyunsheng@huawei.com>
Date: Sat, 29 Jul 2023 19:40:19 +0800

> On 2023/7/28 21:58, Alexander Lobakin wrote:
>> From: Yunsheng Lin <linyunsheng@huawei.com>
>> Date: Fri, 28 Jul 2023 20:02:51 +0800
>>
>>> On 2023/7/27 22:43, Alexander Lobakin wrote:
>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>>
>>> ...
>>>
>>>> +bool page_pool_return_skb_page(struct page *page, bool napi_safe)
>>>
>>> Still having the 'page_pool_' prefix seems odd here when it is in the
>>> skbuff.c where most have skb_ or napi_ prefix, is it better to rename
>>> it to something like napi_return_page_pool_page()?
>>
>> Given that how the function that goes next is named, maybe
>> skb_pp_return_page() (or skb_pp_put_page())?
> 
> skb_pp_put_page() seems better.
> 
> And I like napi_pp_put_page() with 'napi_' prefix better as
> it does not take a skb as parameter and the naming is aligned
> with the 'napi_safe' parameter.

Ah, I see. Sounds reasonable. I'll pick napi_pp_put_page() for the next
round, I like this one.

> 
>>
>>>
>>>> +{
>>>> +	struct napi_struct *napi;
>>>> +	struct page_pool *pp;
>>>> +	bool allow_direct;
>>>> +
>>>> +	page = compound_head(page);
>>>> +
>>>> +	/* page->pp_magic is OR'ed with PP_SIGNATURE after the allocation
>>>> +	 * in order to preserve any existing bits, such as bit 0 for the
>>>> +	 * head page of compound page and bit 1 for pfmemalloc page, so
>>>> +	 * mask those bits for freeing side when doing below checking,
>>>> +	 * and page_is_pfmemalloc() is checked in __page_pool_put_page()
>>>> +	 * to avoid recycling the pfmemalloc page.
>>>> +	 */
>>>> +	if (unlikely((page->pp_magic & ~0x3UL) != PP_SIGNATURE))
>>>> +		return false;
>>>> +
>>>> +	pp = page->pp;
>>>> +
>>>> +	/* Allow direct recycle if we have reasons to believe that we are
>>>> +	 * in the same context as the consumer would run, so there's
>>>> +	 * no possible race.
>>>> +	 */
>>>> +	napi = READ_ONCE(pp->p.napi);
>>>> +	allow_direct = napi_safe && napi &&
>>>> +		READ_ONCE(napi->list_owner) == smp_processor_id();
>>>> +
>>>> +	/* Driver set this to memory recycling info. Reset it on recycle.
>>>> +	 * This will *not* work for NIC using a split-page memory model.
>>>> +	 * The page will be returned to the pool here regardless of the
>>>> +	 * 'flipped' fragment being in use or not.
>>>> +	 */
>>>> +	page_pool_put_full_page(pp, page, allow_direct);
>>>> +
>>>> +	return true;
>>>> +}
>>>> +EXPORT_SYMBOL(page_pool_return_skb_page);
>>>> +
>>>>  static bool skb_pp_recycle(struct sk_buff *skb, void *data, bool napi_safe)
>>
>> (this one)
>>
>>>>  {
>>>>  	if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
> 
> We may need the 'IS_ENABLED(CONFIG_PAGE_POOL' checking in the newly
> moved function too.

The first person who noticed this, for sure we should have it!

> 
>>>>
>>
>> Thanks,
>> Olek
>>
>> .
>>

Thanks,
Olek