From: Dragos Tatulea <dtatulea@nvidia.com>
To: "almasrymina@google.com" <almasrymina@google.com>
Cc: "davem@davemloft.net" <davem@davemloft.net>,
"kuba@kernel.org" <kuba@kernel.org>,
"ilias.apalodimas@linaro.org" <ilias.apalodimas@linaro.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"jacob.e.keller@intel.com" <jacob.e.keller@intel.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"edumazet@google.com" <edumazet@google.com>,
Jianbo Liu <jianbol@nvidia.com>,
"pabeni@redhat.com" <pabeni@redhat.com>
Subject: Re: [RFC PATCH] net: Fix one page_pool page leak from skb_frag_unref
Date: Thu, 25 Apr 2024 19:47:55 +0000 [thread overview]
Message-ID: <63222bf6a298ae38e77b0c0f49d13581dd9d3a74.camel@nvidia.com> (raw)
In-Reply-To: <CAHS8izPkRJyLctmyj+Ppc5j3Qq5O1u3aPe5h9mnFNHDU2OxA=A@mail.gmail.com>
On Thu, 2024-04-25 at 12:20 -0700, Mina Almasry wrote:
> On Thu, Apr 25, 2024 at 1:17 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> >
> > On Wed, 2024-04-24 at 15:08 -0700, Mina Almasry wrote:
> > > If that doesn't work, I think I prefer
> > > reverting a580ea994fd3 ("net: mirror skb frag ref/unref helpers")
> > > rather than merging this fix to make sure we removed the underlying
> > > cause of the issue.
> > This is the safest bet.
> >
> > So, to recap, I see 2 possibilities:
> >
> > 1) Revert a580ea994fd3 ("net: mirror skb frag ref/unref helpers"): safe, but it
> > will probably have to come back in one way or another.
> > 2) Drop the recycle checks from skb_frag_ref/unref: this enforces the rule of
> > always referencing/dereferencing pages based on their type (page_pool or
> > normal).
> >
>
> If this works, I would be very happy. I personally think ref/unref
> should be done based on the page type. For me the net stack using the
> regular {get|put}_page on a pp page isn't great. It requires special
> handling to make sure the ref + unref are in sync. Also if the last pp
> ref is dropped while there are pending regular refs,
> __page_pool_page_can_be_recycled() check will fail and the page will
> not be recycled.
>
> On the other hand, since 0a149ab78ee2 ("page_pool: transition to
> reference count management after page draining") I'm not sure there is
> any reason to continue to use get/put_page on pp-pages, we can use the
> new pp-ref instead.
>
> I don't see any regressions with this diff (needs cleanup), but your
> test setup seems much much better than mine (I think this is the
> second reffing issue you manage to repro):
>
> diff --git a/include/linux/skbuff_ref.h b/include/linux/skbuff_ref.h
> index 4dcdbe9fbc5f..4c72227dce1b 100644
> --- a/include/linux/skbuff_ref.h
> +++ b/include/linux/skbuff_ref.h
> @@ -31,7 +31,7 @@ static inline bool napi_pp_get_page(struct page *page)
> static inline void skb_page_ref(struct page *page, bool recycle)
> {
> #ifdef CONFIG_PAGE_POOL
> - if (recycle && napi_pp_get_page(page))
> + if (napi_pp_get_page(page))
> return;
> #endif
> get_page(page);
> @@ -69,7 +69,7 @@ static inline void
> skb_page_unref(struct page *page, bool recycle)
> {
> #ifdef CONFIG_PAGE_POOL
> - if (recycle && napi_pp_put_page(page))
> + if (napi_pp_put_page(page))
> return;
> #endif
> put_page(page);
>
>
This is option 2. I thought this would fix everything. But I just tested and
it's not the case: we are now reaching a negative pp_ref_count. So probably
somewhere a regular page reference is still being taken...
Thanks,
Dragos
next prev parent reply other threads:[~2024-04-25 19:48 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-24 16:56 [RFC PATCH] net: Fix one page_pool page leak from skb_frag_unref Dragos Tatulea
2024-04-24 17:25 ` Dragos Tatulea
2024-04-24 22:08 ` Mina Almasry
2024-04-25 8:17 ` Dragos Tatulea
2024-04-25 19:20 ` Mina Almasry
2024-04-25 19:47 ` Dragos Tatulea [this message]
2024-04-25 20:42 ` Mina Almasry
2024-04-26 14:59 ` Dragos Tatulea
2024-04-26 19:03 ` Mina Almasry
2024-04-26 23:08 ` Jakub Kicinski
2024-04-27 4:24 ` Mina Almasry
2024-04-29 15:00 ` Jakub Kicinski
2024-05-02 20:08 ` Mina Almasry
2024-05-03 0:46 ` Jakub Kicinski
2024-04-26 23:05 ` Jakub Kicinski
2024-04-29 7:39 ` Dragos Tatulea
2024-05-01 6:20 ` Dragos Tatulea
2024-05-01 7:48 ` Mina Almasry
2024-05-01 7:58 ` Dragos Tatulea
2024-05-01 14:24 ` Jakub Kicinski
2024-05-01 14:28 ` Jakub Kicinski
2024-05-01 16:23 ` Mina Almasry
2024-04-26 23:07 ` Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=63222bf6a298ae38e77b0c0f49d13581dd9d3a74.camel@nvidia.com \
--to=dtatulea@nvidia.com \
--cc=almasrymina@google.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=ilias.apalodimas@linaro.org \
--cc=jacob.e.keller@intel.com \
--cc=jianbol@nvidia.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.