Re: [PATCH bpf-next v3 4/4] xdp: remove unused {__,}xdp_release_frame()

[Alexander Lobakin]

From: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
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