| Message ID | 20240308204500.1112858-1-almasrymina@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 46f40172b68154106cae660c90c7801b61080892 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1] net: page_pool: factor out page_pool recycle check | expand |
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 <almasrymina@google.com> > Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com> > --- > 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); >
On Fri, Mar 8, 2024 at 3:50 PM David Wei <dw@davidwei.uk> 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 <almasrymina@google.com> > > Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com> > > --- > > 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 2024-03-08 16:04, Mina Almasry wrote: > On Fri, Mar 8, 2024 at 3:50 PM David Wei <dw@davidwei.uk> 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 <almasrymina@google.com> >>> Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com> >>> --- >>> 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 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 <almasrymina@google.com> wrote: > > The check is duplicated in 2 places, factor it out into a common helper. > > Signed-off-by: Mina Almasry <almasrymina@google.com> > Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com> > --- > 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 <ilias.apalodimas@linaro.org>
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: 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 <almasrymina@google.com> > Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com> > --- > 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!
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);