Re: [PATCH v6 net-next 1/5] net: add generic per-cpu page_pool allocator

[Jesper Dangaard Brouer <hawk@kernel.org>]

On 29/01/2024 16.44, Toke Høiland-Jørgensen wrote:
> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes:
> 
>>> Lorenzo Bianconi <lorenzo@kernel.org> writes:
>>>
>>>> Introduce generic percpu page_pools allocator.
>>>> Moreover add page_pool_create_percpu() and cpuid filed in page_pool struct
>>>> in order to recycle the page in the page_pool "hot" cache if
>>>> napi_pp_put_page() is running on the same cpu.
>>>> This is a preliminary patch to add xdp multi-buff support for xdp running
>>>> in generic mode.
>>>>
>>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>>>> ---
>>>>   include/net/page_pool/types.h |  3 +++
>>>>   net/core/dev.c                | 40 +++++++++++++++++++++++++++++++++++
>>>>   net/core/page_pool.c          | 23 ++++++++++++++++----
>>>>   net/core/skbuff.c             |  5 +++--
>>>>   4 files changed, 65 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
>>>> index 76481c465375..3828396ae60c 100644
>>>> --- a/include/net/page_pool/types.h
>>>> +++ b/include/net/page_pool/types.h
>>>> @@ -128,6 +128,7 @@ struct page_pool_stats {
>>>>   struct page_pool {
>>>>   	struct page_pool_params_fast p;
>>>>   
>>>> +	int cpuid;
>>>>   	bool has_init_callback;
>>>>   
>>>>   	long frag_users;
>>>> @@ -203,6 +204,8 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
>>>>   struct page *page_pool_alloc_frag(struct page_pool *pool, unsigned int *offset,
>>>>   				  unsigned int size, gfp_t gfp);
>>>>   struct page_pool *page_pool_create(const struct page_pool_params *params);
>>>> +struct page_pool *page_pool_create_percpu(const struct page_pool_params *params,
>>>> +					  int cpuid);
>>>>   
>>>>   struct xdp_mem_info;
>>>>   
>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>> index cb2dab0feee0..bf9ec740b09a 100644
>>>> --- a/net/core/dev.c
>>>> +++ b/net/core/dev.c
>>>> @@ -153,6 +153,8 @@
>>>>   #include <linux/prandom.h>
>>>>   #include <linux/once_lite.h>
>>>>   #include <net/netdev_rx_queue.h>
>>>> +#include <net/page_pool/types.h>
>>>> +#include <net/page_pool/helpers.h>
>>>>   
>>>>   #include "dev.h"
>>>>   #include "net-sysfs.h"
>>>> @@ -442,6 +444,8 @@ static RAW_NOTIFIER_HEAD(netdev_chain);
>>>>   DEFINE_PER_CPU_ALIGNED(struct softnet_data, softnet_data);
>>>>   EXPORT_PER_CPU_SYMBOL(softnet_data);
>>>>   
>>>> +DEFINE_PER_CPU_ALIGNED(struct page_pool *, page_pool);
>>>
>>> I think we should come up with a better name than just "page_pool" for
>>> this global var. In the code below it looks like it's a local variable
>>> that's being referenced. Maybe "global_page_pool" or "system_page_pool"
>>> or something along those lines?
>>
>> ack, I will fix it. system_page_pool seems better, agree?
> 
> Yeah, agreed :)

Naming it "system_page_pool" is good by me.

Should we add some comments about concurrency expectations when using this?
Or is this implied by "PER_CPU" define?

PP alloc side have a lockless array/stack, and the per_cpu stuff do
already imply only one CPU is accessing this, and implicitly (also) we
cannot handle reentrance cause by preemption.  So, the caller have the
responsibility to call this from appropriate context.

--Jesper