[PATCH net-next] net: page_pool: make page_pool_create inline

[PATCH net-next] net: page_pool: make page_pool_create inline

[Lorenzo Bianconi]

Make page_pool_create utility routine inline a remove exported symbol.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 include/net/page_pool/types.h |  6 +++++-
 net/core/page_pool.c          | 10 ----------
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
index 3828396ae60c..0057e584df9a 100644
--- a/include/net/page_pool/types.h
+++ b/include/net/page_pool/types.h
@@ -203,9 +203,13 @@ struct page_pool {
 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);
+static inline struct page_pool *
+page_pool_create(const struct page_pool_params *params)
+{
+	return page_pool_create_percpu(params, -1);
+}
 
 struct xdp_mem_info;
 
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 89c835fcf094..6e0753e6a95b 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -289,16 +289,6 @@ page_pool_create_percpu(const struct page_pool_params *params, int cpuid)
 }
 EXPORT_SYMBOL(page_pool_create_percpu);
 
-/**
- * page_pool_create() - create a page pool
- * @params: parameters, see struct page_pool_params
- */
-struct page_pool *page_pool_create(const struct page_pool_params *params)
-{
-	return page_pool_create_percpu(params, -1);
-}
-EXPORT_SYMBOL(page_pool_create);
-
 static void page_pool_return_page(struct page_pool *pool, struct page *page);
 
 noinline
-- 
2.43.0

Re: [PATCH net-next] net: page_pool: make page_pool_create inline

[Jakub Kicinski]

On Wed, 14 Feb 2024 19:01:28 +0100 Lorenzo Bianconi wrote:
> Make page_pool_create utility routine inline a remove exported symbol.

But why? If you add the kdoc back the LoC saved will be 1.

>  include/net/page_pool/types.h |  6 +++++-
>  net/core/page_pool.c          | 10 ----------
>  2 files changed, 5 insertions(+), 11 deletions(-)

No strong opinion, but if you want to do it please put the helper 
in helpers.h  Let's keep the static inlines clearly separated.

Re: [PATCH net-next] net: page_pool: make page_pool_create inline

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 716 bytes --]

> On Wed, 14 Feb 2024 19:01:28 +0100 Lorenzo Bianconi wrote:
> > Make page_pool_create utility routine inline a remove exported symbol.
> 
> But why? If you add the kdoc back the LoC saved will be 1.

I would remove the symbol exported since it is just a wrapper for
page_pool_create_percpu()

> 
> >  include/net/page_pool/types.h |  6 +++++-
> >  net/core/page_pool.c          | 10 ----------
> >  2 files changed, 5 insertions(+), 11 deletions(-)
> 
> No strong opinion, but if you want to do it please put the helper 
> in helpers.h  Let's keep the static inlines clearly separated.

ack, fine to me. I put it there since we already have some inlines in
page_pool/types.h.

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next] net: page_pool: make page_pool_create inline

[Ilias Apalodimas]

Hi Lorenzo,

On Wed, 14 Feb 2024 at 21:14, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> > On Wed, 14 Feb 2024 19:01:28 +0100 Lorenzo Bianconi wrote:
> > > Make page_pool_create utility routine inline a remove exported symbol.
> >
> > But why? If you add the kdoc back the LoC saved will be 1.
>
> I would remove the symbol exported since it is just a wrapper for
> page_pool_create_percpu()

I don't mind either. But the explanation above must be part of the
commit message.

Thanks
/Ilias
>
> >
> > >  include/net/page_pool/types.h |  6 +++++-
> > >  net/core/page_pool.c          | 10 ----------
> > >  2 files changed, 5 insertions(+), 11 deletions(-)
> >
> > No strong opinion, but if you want to do it please put the helper
> > in helpers.h  Let's keep the static inlines clearly separated.
>
> ack, fine to me. I put it there since we already have some inlines in
> page_pool/types.h.
>
> Regards,
> Lorenzo

Re: [PATCH net-next] net: page_pool: make page_pool_create inline

[Jesper Dangaard Brouer]

On 01/03/2024 11.27, Ilias Apalodimas wrote:
> 
> On Wed, 14 Feb 2024 at 21:14, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>>
>>> On Wed, 14 Feb 2024 19:01:28 +0100 Lorenzo Bianconi wrote:
>>>> Make page_pool_create utility routine inline a remove exported symbol.
>>>
>>> But why? If you add the kdoc back the LoC saved will be 1.
>>
>> I would remove the symbol exported since it is just a wrapper for
>> page_pool_create_percpu()
> 
> I don't mind either. But the explanation above must be part of the
> commit message.

I hope my benchmark module will still work...

https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/lib/bench_page_pool_simple.c#L181


--Jesper

Re: [PATCH net-next] net: page_pool: make page_pool_create inline

[Ilias Apalodimas]

Hi Jesper,

On Fri, 1 Mar 2024 at 12:33, Jesper Dangaard Brouer <hawk@kernel.org> wrote:
>
>
>
> On 01/03/2024 11.27, Ilias Apalodimas wrote:
> >
> > On Wed, 14 Feb 2024 at 21:14, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >>
> >>> On Wed, 14 Feb 2024 19:01:28 +0100 Lorenzo Bianconi wrote:
> >>>> Make page_pool_create utility routine inline a remove exported symbol.
> >>>
> >>> But why? If you add the kdoc back the LoC saved will be 1.
> >>
> >> I would remove the symbol exported since it is just a wrapper for
> >> page_pool_create_percpu()
> >
> > I don't mind either. But the explanation above must be part of the
> > commit message.
>
> I hope my benchmark module will still work...
>
> https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/lib/bench_page_pool_simple.c#L181

Since we rely on this so much, perhaps we should make an effort to
merge it, even in a version that only works for x86.
I know it's fairly easy to compile and run locally, but having it as
part of the CI is better -- more people will be able to look at the
results quickly and determine if we have a performance degradation

Cheers
/Ilias
>
>
> --Jesper