The check is duplicated in 2 places, factor it out into a common helper.
Signed-off-by: Mina Almasry <[email protected]>
Reviewed-by: Yunsheng Lin <[email protected]>
---
net/core/page_pool.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index d706fe5548df..dd364d738c00 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -657,6 +657,11 @@ static bool page_pool_recycle_in_cache(struct page *page,
return true;
}
+static bool __page_pool_page_can_be_recycled(const struct page *page)
+{
+ return page_ref_count(page) == 1 && !page_is_pfmemalloc(page);
+}
+
/* If the page refcnt == 1, this will try to recycle the page.
* if PP_FLAG_DMA_SYNC_DEV is set, we'll try to sync the DMA area for
* the configured size min(dma_sync_size, pool->max_len).
@@ -678,7 +683,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
* page is NOT reusable when allocated when system is under
* some pressure. (page_is_pfmemalloc)
*/
- if (likely(page_ref_count(page) == 1 && !page_is_pfmemalloc(page))) {
+ if (likely(__page_pool_page_can_be_recycled(page))) {
/* Read barrier done in page_ref_count / READ_ONCE */
if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
@@ -793,7 +798,7 @@ static struct page *page_pool_drain_frag(struct page_pool *pool,
if (likely(page_pool_unref_page(page, drain_count)))
return NULL;
- if (page_ref_count(page) == 1 && !page_is_pfmemalloc(page)) {
+ if (__page_pool_page_can_be_recycled(page)) {
if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
page_pool_dma_sync_for_device(pool, page, -1);
--
2.44.0.278.ge034bb2e1d-goog
On Fri, Mar 8, 2024 at 3:50 PM David Wei <[email protected]> wrote:
>
> On 2024-03-08 12:44, Mina Almasry wrote:
> > The check is duplicated in 2 places, factor it out into a common helper.
> >
> > Signed-off-by: Mina Almasry <[email protected]>
> > Reviewed-by: Yunsheng Lin <[email protected]>
> > ---
> > net/core/page_pool.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index d706fe5548df..dd364d738c00 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -657,6 +657,11 @@ static bool page_pool_recycle_in_cache(struct page *page,
> > return true;
> > }
> >
> > +static bool __page_pool_page_can_be_recycled(const struct page *page)
>
> Could this be made inline?
>
Looking at the rest of the static functions in this file, they don't
specify inline, just static. I guess the compiler is smart enough to
inline static functions in .c files when it makes sense (and does not
when it doesn't)?
But this doesn't seem to be a kernel wide thing. net/core/dev.c does
have static inline functions in it, only page_pool.c doesn't do it. I
guess if there are no objections I can make it static inline to ask
the compiler to inline it. Likely after the merge window reopens if it
closes today.
On Fri, 8 Mar 2024 16:04:14 -0800 Mina Almasry wrote:
> > Could this be made inline?
> >
>
> Looking at the rest of the static functions in this file, they don't
> specify inline, just static. I guess the compiler is smart enough to
> inline static functions in .c files when it makes sense (and does not
> when it doesn't)?
>
> But this doesn't seem to be a kernel wide thing. net/core/dev.c does
> have static inline functions in it, only page_pool.c doesn't do it. I
> guess if there are no objections I can make it static inline to ask
> the compiler to inline it. Likely after the merge window reopens if it
> closes today.
It's all good. We have a policy in netdev of "no inline unless you can
prove it makes a difference". It will not make a difference here and it
will mute the "unused function" warning.
On Fri, 8 Mar 2024 at 22:45, Mina Almasry <[email protected]> wrote:
>
> The check is duplicated in 2 places, factor it out into a common helper.
>
> Signed-off-by: Mina Almasry <[email protected]>
> Reviewed-by: Yunsheng Lin <[email protected]>
> ---
> net/core/page_pool.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index d706fe5548df..dd364d738c00 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -657,6 +657,11 @@ static bool page_pool_recycle_in_cache(struct page *page,
> return true;
> }
>
> +static bool __page_pool_page_can_be_recycled(const struct page *page)
> +{
> + return page_ref_count(page) == 1 && !page_is_pfmemalloc(page);
> +}
> +
> /* If the page refcnt == 1, this will try to recycle the page.
> * if PP_FLAG_DMA_SYNC_DEV is set, we'll try to sync the DMA area for
> * the configured size min(dma_sync_size, pool->max_len).
> @@ -678,7 +683,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
> * page is NOT reusable when allocated when system is under
> * some pressure. (page_is_pfmemalloc)
> */
> - if (likely(page_ref_count(page) == 1 && !page_is_pfmemalloc(page))) {
> + if (likely(__page_pool_page_can_be_recycled(page))) {
> /* Read barrier done in page_ref_count / READ_ONCE */
>
> if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
> @@ -793,7 +798,7 @@ static struct page *page_pool_drain_frag(struct page_pool *pool,
> if (likely(page_pool_unref_page(page, drain_count)))
> return NULL;
>
> - if (page_ref_count(page) == 1 && !page_is_pfmemalloc(page)) {
> + if (__page_pool_page_can_be_recycled(page)) {
> if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
> page_pool_dma_sync_for_device(pool, page, -1);
>
> --
> 2.44.0.278.ge034bb2e1d-goog
>
Reviewed-by: Ilias Apalodimas <[email protected]>
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
On Fri, 8 Mar 2024 12:44:58 -0800 you wrote:
> The check is duplicated in 2 places, factor it out into a common helper.
>
> Signed-off-by: Mina Almasry <[email protected]>
> Reviewed-by: Yunsheng Lin <[email protected]>
> ---
> net/core/page_pool.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
Here is the summary with links:
- [net-next,v1] net: page_pool: factor out page_pool recycle check
https://git.kernel.org/netdev/net-next/c/46f40172b681
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On 2024-03-08 16:04, Mina Almasry wrote:
> On Fri, Mar 8, 2024 at 3:50 PM David Wei <[email protected]> wrote:
>>
>> On 2024-03-08 12:44, Mina Almasry wrote:
>>> The check is duplicated in 2 places, factor it out into a common helper.
>>>
>>> Signed-off-by: Mina Almasry <[email protected]>
>>> Reviewed-by: Yunsheng Lin <[email protected]>
>>> ---
>>> net/core/page_pool.c | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>>> index d706fe5548df..dd364d738c00 100644
>>> --- a/net/core/page_pool.c
>>> +++ b/net/core/page_pool.c
>>> @@ -657,6 +657,11 @@ static bool page_pool_recycle_in_cache(struct page *page,
>>> return true;
>>> }
>>>
>>> +static bool __page_pool_page_can_be_recycled(const struct page *page)
>>
>> Could this be made inline?
>>
>
> Looking at the rest of the static functions in this file, they don't
> specify inline, just static. I guess the compiler is smart enough to
> inline static functions in .c files when it makes sense (and does not
> when it doesn't)?
>
> But this doesn't seem to be a kernel wide thing. net/core/dev.c does
> have static inline functions in it, only page_pool.c doesn't do it. I
> guess if there are no objections I can make it static inline to ask
> the compiler to inline it. Likely after the merge window reopens if it
> closes today.
Thanks for checking. Otherwise the change looks good to me, pulling out
the same check in two locations into a(n inline) function then calling
that function instead.
On 2024-03-08 12:44, Mina Almasry wrote:
> The check is duplicated in 2 places, factor it out into a common helper.
>
> Signed-off-by: Mina Almasry <[email protected]>
> Reviewed-by: Yunsheng Lin <[email protected]>
> ---
> net/core/page_pool.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index d706fe5548df..dd364d738c00 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -657,6 +657,11 @@ static bool page_pool_recycle_in_cache(struct page *page,
> return true;
> }
>
> +static bool __page_pool_page_can_be_recycled(const struct page *page)
Could this be made inline?
> +{
> + return page_ref_count(page) == 1 && !page_is_pfmemalloc(page);
> +}
> +
> /* If the page refcnt == 1, this will try to recycle the page.
> * if PP_FLAG_DMA_SYNC_DEV is set, we'll try to sync the DMA area for
> * the configured size min(dma_sync_size, pool->max_len).
> @@ -678,7 +683,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
> * page is NOT reusable when allocated when system is under
> * some pressure. (page_is_pfmemalloc)
> */
> - if (likely(page_ref_count(page) == 1 && !page_is_pfmemalloc(page))) {
> + if (likely(__page_pool_page_can_be_recycled(page))) {
> /* Read barrier done in page_ref_count / READ_ONCE */
>
> if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
> @@ -793,7 +798,7 @@ static struct page *page_pool_drain_frag(struct page_pool *pool,
> if (likely(page_pool_unref_page(page, drain_count)))
> return NULL;
>
> - if (page_ref_count(page) == 1 && !page_is_pfmemalloc(page)) {
> + if (__page_pool_page_can_be_recycled(page)) {
> if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
> page_pool_dma_sync_for_device(pool, page, -1);
>