Message-ID: <4f65c860-9f0f-11ea-6e91-c35a62f084b3@intel.com>
Date:   Tue, 14 Mar 2023 13:27:50 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH bpf-next v3 4/4] xdp: remove unused
 {__,}xdp_release_frame()
Content-Language: en-US
To:     Yunsheng Lin <linyunsheng@huawei.com>
CC:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Larysa Zaremba <larysa.zaremba@intel.com>,
        =?UTF-8?Q?Toke_H=c3=b8iland-J=c3=b8rgensen?= <toke@redhat.com>,
        Song Liu <song@kernel.org>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        John Fastabend <john.fastabend@gmail.com>,
        Menglong Dong <imagedong@tencent.com>,
        "Mykola Lysenko" <mykolal@fb.com>,
        "David S. Miller" <davem@davemloft.net>,
        "Jakub Kicinski" <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>, <bpf@vger.kernel.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230313215553.1045175-1-aleksander.lobakin@intel.com>
 <20230313215553.1045175-5-aleksander.lobakin@intel.com>
 <42712a3f-544f-6e45-1468-9f9fae7922e8@huawei.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <42712a3f-544f-6e45-1468-9f9fae7922e8@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?Y25aTFBRdEkvRGdTSmc0ZFU3cm5uUTFzOXNHNEFzbFlXanVOZTBUd092ME9L?=
 =?utf-8?B?c2xIS2d1WTB4UHVWUmM0Rk9IT3JtTVUzOFliZ0VtZlVBRkN4NDJySW91OGVk?=
 =?utf-8?B?cDE2UWFNelpyVnNyaUNRd1MxVk1wL245UG52SGx4WEFBdmRkVE5VTkE5Q0R4?=
 =?utf-8?B?TWVpbmhjcmxhT1VKS1hveFY1dWtKV0pkLzNTL292MEJJc1NkOVpibkZKOFBK?=
 =?utf-8?B?SldFTXloNktIaFo1dWdiRHFHbmdQTnZNMEFRT2dpOUc4WFhsbk5tUzdsVUV1?=
 =?utf-8?B?OEFMZmRGOTNUcDQvMUNwTEJNMzhKRzdXZ3crVHNVTFhtZGM0SmV6bklSMWdF?=
 =?utf-8?B?KzBIUmNqUEtwZFcwZExEZUNNNk1yY0dHamRkeko2QURFakJVeFdMcFNCZkxh?=
 =?utf-8?B?ejdPWkttY2IrOUpVSncrR1JoUFRkTDIvVXpBLzR3dWZQQnF5TEYyeUlVMEhF?=
 =?utf-8?B?aEZtM1ZNWHhJMVFiOTNTcWdQNE1wU1hPQVRmbm5FMXNsc3UyejhBbWh0Ujc0?=
 =?utf-8?B?N1RiR2d1dzdyMXJKSGFQMmc1K01LU0d2V0srb0V5Q2FKbCthOEtSVWQ1Snh6?=
 =?utf-8?B?V2U4c0VyeEZ1Z2dqMUtYWUVwTE9remlORDlqN3lEWlpGeXBHaHR4ZkIzbGZ5?=
 =?utf-8?B?VEhQZXhkRldoZWtjWDBWWXBRdVVPZzBpSjF0ZHdVVld5eVpPeEhnaFM4QkQ4?=
 =?utf-8?B?T1hsTzZ3MDZCVEZJald3SFZSWTgzTWs1ZTNmaGFlV1ZyZ2ptekhzU0pyWEtN?=
 =?utf-8?B?TDYwSlA1Zk1HVG5uSzk0NjFVYVA5ZnVmQkJTNEUwcXF6cW1tS0FIT20xM0ov?=
 =?utf-8?B?cXNHSW54aVczZ0dWT1VBUjZuQ3RZbWx6M2ZqN0RqeENQYjQyZVFyTXlURk9a?=
 =?utf-8?B?NGwrckFpSnVCMGtCeUNOR0FFakhmNk80ZVd2QnBETmZDV2RzRlNaWGR2SFJU?=
 =?utf-8?B?Z0VEekFZMkdoVWZhc3FoZ3hXMkIrWlVDS3N4VXV5alJSR2dKRkVkUjh1bDJS?=
 =?utf-8?B?Yk00S0ZzQkVOanEwYi83aE4xVi9SaWVuVmMzcU5XYjZzREdlaHd0YjQrM3pB?=
 =?utf-8?B?aXhZMlA1Wk4vRk5ON25XK0hBWWZ5V0dxMFU1LzN3NDBqWkNGL3VMVzlXbER0?=
 =?utf-8?B?L0U4ZVVHOHhMdEhuV0xNdk5Majd6clZoblZYcG1HUmhVZDVxbTNiNHhvdjFa?=
 =?utf-8?B?NUZ1bTYrdzIrVENIRy82YXBQSjdnV3F2SlhxY3A1NGIyWjg2dThXUTZFeCtm?=
 =?utf-8?B?SzY5S3habTdvdlVlZk1QcnhOZmVtcFQyZzZGN1F4T2xoL3NYMVVkRTlUMWhs?=
 =?utf-8?B?RjkxMkJuUzZBQkVTQWxVUHFMeXk4bGVyQ1JOeXBxSEFXLy9sRVgzS0RBRTFz?=
 =?utf-8?B?Tkh3SFl6cC83K2ExYnU3cEc3RzF3T1o5U205eUk1UEtSWTVIYVMvOXVyWTZB?=
 =?utf-8?B?MTB0TmtZczlWU0h0dWF1ck1PU2NxWmdTYXlvTTBaQ2o4YklkUzZwSytyRWoz?=
 =?utf-8?B?eU1CN21uUWRab2Fhc2JuaUJZSmhNNTVnTXpHclROSXpwQWZhcTVxc2YvcGU5?=
 =?utf-8?B?OHVjekR2NXVENkRpQXBod3l3TlJ5RDlPWmtUMlp6K01TQmRwNW1nV2hjREl6?=
 =?utf-8?B?aGhQUFZVc0hsamc3ckJ0ZXZMbDhlazB5REtxSjY5V1NBMWprczRhWHhzNEFB?=
 =?utf-8?B?VExJazlqMHcxV1JXWWFRYW1hSEw4eWVkMEVFOUcyZ3UvVjI2cmtlOVBaN044?=
 =?utf-8?B?SXBTTkJVQ00ycDVia2xTZ3pIUHpnTWZURVNJZ1gzd1ZYbTY4czBUUm50WjEr?=
 =?utf-8?B?N09SZHZBZEZjRC82c0NnZVpTTTJKYStCQnpLeDZYS0MrbEZXQjVLdTFpdEVt?=
 =?utf-8?B?VzJSNUtwT1Znd3V1SmRSWis3UGtZNHp1Z3VXM1RXam1USWJBSXU3aUh0RHU4?=
 =?utf-8?B?UzZ2VlkrVmZpOXpKWUNiaFNCUVFveE1VVS9qZjJheGc2d0JJYTlWYTBmcDZT?=
 =?utf-8?B?cWUvNEZzQVVEdTlFNlR2UHJCTnV4SGtZWjQyQngzZFlMbUllTWNzNXJxSUNx?=
 =?utf-8?B?U3VqaFoxS3h1VldSVXVyYytpUFVqUm9HK2FNZjVQaXd2YWVJWFJ2VmFxZ3dN?=
 =?utf-8?B?cGU4MmxZNGF5SGgybFZEU2VGTnRhSXFlUEVRU0g2dEk2TWwvVUJSeGh5a0ZT?=
 =?utf-8?Q?qWEuwDTcDiBTG4QutH40DI8=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ed561250-0051-4f85-afe9-08db2487aef5
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Mar 2023 12:28:57.9141
 (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: CSeqIJ39+x9CUw5gS/WPwDCHDTmzZ3tatcmGtX0dJIyoBCjoWcAkXKqSa+V+zJ57ge1XQGE7+P6PJ2gEB7p23Ffj69inAXlpFVMBwUfQTc4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR11MB8046
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Yunsheng Lin <linyunsheng@huawei.com>
Date: Tue, 14 Mar 2023 19:37:23 +0800

> On 2023/3/14 5:55, Alexander Lobakin wrote:
>> __xdp_build_skb_from_frame() was the last user of
>> {__,}xdp_release_frame(), which detaches pages from the page_pool.

[...]

>> -/* Only called for MEM_TYPE_PAGE_POOL see xdp.h */
>> -void __xdp_release_frame(void *data, struct xdp_mem_info *mem)
>> -{
>> -	struct xdp_mem_allocator *xa;
>> -	struct page *page;
>> -
>> -	rcu_read_lock();
>> -	xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
>> -	page = virt_to_head_page(data);
>> -	if (xa)
>> -		page_pool_release_page(xa->page_pool, page);
> 
> page_pool_release_page() is only call here when xa is not NULL
> and mem->type == MEM_TYPE_PAGE_POOL.
> 
> But skb_mark_for_recycle() is call when mem->type == MEM_TYPE_PAGE_POOL
> without checking xa, it does not seems symmetric to patch 3, if this is
> intended?

Intended. page_pool_return_skb_page() checks for %PP_SIGNATURE and if
a page doesn't belong to any PP, it will be returned to the MM layer.
Moreover, cases `mem->type == MEM_TYPE_PAGE_POOL && xa == NULL` are more
of an exception rather than regular -- this means the page was released
from its PP before reaching the function and IIRC it's even impossible
with our current drivers. Adding a hashtable lookup to
{__,}xdp_build_skb_from_frame() would only add hotpath overhead with no
positive impact.

> 
>> -	rcu_read_unlock();
>> -}
>> -EXPORT_SYMBOL_GPL(__xdp_release_frame);
>> -
>>  void xdp_attachment_setup(struct xdp_attachment_info *info,
>>  			  struct netdev_bpf *bpf)
>>  {
>>

Thanks,
Olek