Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
To: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Yunsheng Lin <linyunsheng@huawei.com>,
	David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	linuxarm@openeuler.org, yisen.zhuang@huawei.com,
	Salil Mehta <salil.mehta@huawei.com>,
	thomas.petazzoni@bootlin.com, Marcin Wojtas <mw@semihalf.com>,
	Russell King - ARM Linux <linux@armlinux.org.uk>,
	hawk@kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	John Fastabend <john.fastabend@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Will Deacon <will@kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	Vlastimil Babka <vbabka@suse.cz>,
	fenghua.yu@intel.com, guro@fb.com, peterx@redhat.com,
	Feng Tang <feng.tang@intel.com>, Jason Gunthorpe <jgg@ziepe.ca>,
	mcroce@microsoft.com, Hugh Dickins <hughd@google.com>,
	Jonathan Lemon <jonathan.lemon@gmail.com>,
	Alexander Lobakin <alobakin@pm.me>,
	Willem de Bruijn <willemb@google.com>,
	wenxu@ucloud.cn, cong.wang@bytedance.com,
	Kevin Hao <haokexin@gmail.com>,
	nogikh@google.com, Marco Elver <elver@google.com>,
	Netdev <netdev@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH net-next RFC 1/2] page_pool: add page recycling support based on elevated refcnt
Date: Thu, 8 Jul 2021 18:47:43 +0300	[thread overview]
Message-ID: <YOceH0VLXVl6QJEI@enceladus> (raw)
In-Reply-To: <CAKgT0Ucnd4Oia8xy2D65O04901+Rh6cepX-d2vK1+0_Of2NwoA@mail.gmail.com>

On Thu, Jul 08, 2021 at 08:41:08AM -0700, Alexander Duyck wrote:
> On Thu, Jul 8, 2021 at 8:36 AM Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Thu, Jul 08, 2021 at 08:29:56AM -0700, Alexander Duyck wrote:
> > > On Thu, Jul 8, 2021 at 8:17 AM Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> 
> <snip>
> 
> > > > What do you think about resetting pp_recycle bit on pskb_expand_head()?
> > >
> > > I assume you mean specifically in the cloned case?
> > >
> >
> > Yes. Even if we do it unconditionally we'll just loose non-cloned buffers from
> > the recycling.
> > I'll send a patch later today.
> 
> If you do it unconditionally you could leak DMA mappings since in the
> non-cloned case we don't bother with releasing the shared info since
> we just did a memcpy of it without the reference count tweaks. We have
> to be really careful here. The idea is that we have to make exactly
> one call to the __page_pool_put_page function for this page.
> 
> > > > If my memory serves me right Eric wanted that from the beginning. Then the
> > > > cloned/expanded SKB won't trigger the recycling.  If that skb hits the free
> > > > path first, we'll end up recycling the fragments eventually.  If the
> > > > original one goes first, we'll just unmap the page(s) and freeing the cloned
> > > > one will free all the remaining buffers.
> > >
> > > I *think* that should be fine. Effectively what we are doing is making
> > > it so that if the original skb is freed first the pages are released,
> > > and if it is released after the clone/expended skb then it can be
> > > recycled.
> >
> > Exactly
> >
> > >
> > > The issue is we have to maintain it so that there will be exactly one
> > > caller of the recycling function for the pages. So any spot where we
> > > are updating skb->head we will have to see if there is a clone and if
> > > so we have to clear the pp_recycle flag on our skb so that it doesn't
> > > try to recycle the page frags as well.
> >
> > Correct. I'll keep looking around in case there's something less fragile we
> > can do
> 
> That is the risk to this kind of thing. We have to make the call once
> and only once and if we either miss it or call it too many times we
> can introduce some serious issues.

And I fully agree. Let me fix the obvious one now and I'll have a closer
look on the recycling function it self. I can probably pick up the
"changed head"/expanded SKB in the generic recycling code and refuse to recycle
these packets. Then we'll just accept the fact that if those kind of
packets are freed last, we won't recycle.

Thanks, that was a very nice catch
/Ilias
> 
> Thanks.
> 
> - Alex

  reply	other threads:[~2021-07-08 15:47 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-30  9:17 [PATCH net-next RFC 0/2] add elevated refcnt support for page pool Yunsheng Lin
2021-06-30  9:17 ` [PATCH net-next RFC 1/2] page_pool: add page recycling support based on elevated refcnt Yunsheng Lin
2021-07-02  9:42   ` Jesper Dangaard Brouer
2021-07-02 10:15     ` Yunsheng Lin
2021-07-06  4:54       ` Ilias Apalodimas
2021-07-06  6:46         ` Yunsheng Lin
2021-07-06  8:18           ` Ilias Apalodimas
2021-07-06 20:45   ` Alexander Duyck
2021-07-07  3:05     ` Yunsheng Lin
2021-07-07 15:01       ` Alexander Duyck
2021-07-07 19:03         ` Ilias Apalodimas
2021-07-07 21:49           ` Alexander Duyck
2021-07-08 14:21             ` Ilias Apalodimas
2021-07-08 14:24               ` Alexander Duyck
2021-07-08 14:50                 ` Ilias Apalodimas
2021-07-08 15:17                   ` Ilias Apalodimas
2021-07-08 15:29                     ` Alexander Duyck
2021-07-08 15:36                       ` Ilias Apalodimas
2021-07-08 15:41                         ` Alexander Duyck
2021-07-08 15:47                           ` Ilias Apalodimas [this message]
2021-07-08  2:27         ` Yunsheng Lin
2021-07-08 15:36           ` Alexander Duyck
2021-07-09  6:26             ` Yunsheng Lin
2021-07-09 14:15               ` Alexander Duyck
2021-07-10  9:16                 ` [Linuxarm] " Yunsheng Lin
2021-06-30  9:17 ` [PATCH net-next RFC 2/2] net: hns3: support skb's frag page recycling based on page pool Yunsheng Lin
2021-07-02  8:36 ` [PATCH net-next RFC 0/2] add elevated refcnt support for " Ilias Apalodimas
2021-07-02 13:39 ` Matteo Croce
2021-07-06 15:51   ` Russell King (Oracle)
2021-07-06 23:19     ` Matteo Croce
2021-07-07 16:50       ` Marcin Wojtas
2021-07-09  4:15         ` Matteo Croce
2021-07-09  6:40           ` Yunsheng Lin
2021-07-09  6:42             ` Ilias Apalodimas

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=YOceH0VLXVl6QJEI@enceladus \
    --to=ilias.apalodimas@linaro.org \
    --cc=akpm@linux-foundation.org \
    --cc=alexander.duyck@gmail.com \
    --cc=alobakin@pm.me \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=cong.wang@bytedance.com \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=elver@google.com \
    --cc=feng.tang@intel.com \
    --cc=fenghua.yu@intel.com \
    --cc=guro@fb.com \
    --cc=haokexin@gmail.com \
    --cc=hawk@kernel.org \
    --cc=hughd@google.com \
    --cc=jgg@ziepe.ca \
    --cc=john.fastabend@gmail.com \
    --cc=jonathan.lemon@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=linuxarm@openeuler.org \
    --cc=linyunsheng@huawei.com \
    --cc=mcroce@microsoft.com \
    --cc=mw@semihalf.com \
    --cc=netdev@vger.kernel.org \
    --cc=nogikh@google.com \
    --cc=peterx@redhat.com \
    --cc=peterz@infradead.org \
    --cc=salil.mehta@huawei.com \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=vbabka@suse.cz \
    --cc=wenxu@ucloud.cn \
    --cc=will@kernel.org \
    --cc=willemb@google.com \
    --cc=willy@infradead.org \
    --cc=yisen.zhuang@huawei.com \
    --subject='Re: [PATCH net-next RFC 1/2] page_pool: add page recycling support based on elevated refcnt' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).