Message-ID: <CAHS8izOiR3zvqCyRdBJchqGe-tYnoCpyYvwxroVFA6-_-i+EyQ@mail.gmail.com>
Date: Thu, 18 Jan 2024 05:56:51 -0800
From: Mina Almasry <almasrymina@...gle.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Jason Gunthorpe <jgg@...dia.com>, Yunsheng Lin <linyunsheng@...wei.com>, linux-kernel@...r.kernel.org, 
	netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Christian König <christian.koenig@....com>, 
	Shakeel Butt <shakeelb@...gle.com>
Subject: Re: [RFC PATCH net-next v5 2/2] net: add netmem to skb_frag_t

On Wed, Jan 17, 2024 at 10:54 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Mina Almasry wrote:
> > On Wed, Jan 17, 2024 at 10:00 AM Mina Almasry <almasrymina@...gle.com> wrote:
> > >
> > > On Tue, Jan 16, 2024 at 4:16 AM Jason Gunthorpe <jgg@...dia.com> wrote:
> > > >
> > > > On Tue, Jan 16, 2024 at 07:04:13PM +0800, Yunsheng Lin wrote:
> > > > > On 2024/1/16 8:01, Jason Gunthorpe wrote:
> > > > > > On Mon, Jan 15, 2024 at 03:23:33PM -0800, Mina Almasry wrote:
> > > > > >>>> You did not answer my question that I asked here, and ignoring this
> > > > > >>>> question is preventing us from making any forward progress on this
> > > > > >>>> discussion. What do you expect or want skb_frag_page() to do when
> > > > > >>>> there is no page in the frag?
> > > > > >>>
> > > > > >>> I would expect it to do nothing.
> > > > > >>
> > > > > >> I don't understand. skb_frag_page() with an empty implementation just
> > > > > >> results in a compiler error as the function needs to return a page
> > > > > >> pointer. Do you actually expect skb_frag_page() to unconditionally
> > > > > >> cast frag->netmem to a page pointer? That was explained as
> > > > > >> unacceptable over and over again by Jason and Christian as it risks
> > > > > >> casting devmem to page; completely unacceptable and will get nacked.
> > > > > >> Do you have a suggestion of what skb_frag_page() should do that will
> > > > > >> not get nacked by mm?
> > > > > >
> > > > > > WARN_ON and return NULL seems reasonable?
> > > > >
> > >
> > > That's more or less what I'm thinking.
> > >
> > > > > While I am agreed that it may be a nightmare to debug the case of passing
> > > > > a false page into the mm system, but I am not sure what's the point of
> > > > > returning NULL to caller if the caller is not expecting or handling
> > > > > the
> > > >
> > > > You have to return something and NULL will largely reliably crash the
> > > > thread. The WARN_ON explains in detail why your thread just crashed.
> > > >
> > >
> > > Agreed.
> > >
> > > > > NULL returning[for example, most of mm API called by the networking does not
> > > > > seems to handling NULL as input page], isn't the NULL returning will make
> > > > > the kernel panic anyway? Doesn't it make more sense to just add a BUG_ON()
> > > > > depending on some configuration like CONFIG_DEBUG_NET or CONFIG_DEVMEM?
> > > > > As returning NULL seems to be causing a confusion for the caller of
> > > > > skb_frag_page() as whether to or how to handle the NULL returning case.
> > > >
> > > > Possibly, though Linus doesn't like BUG_ON on principle..
> > > >
> > > > I think the bigger challenge is convincing people that this devmem
> > > > stuff doesn't just open a bunch of holes in the kernel where userspace
> > > > can crash it.
> > > >
> > >
> > > It does not, and as of right now there are no pending concerns from
> > > any netdev maintainers regarding mishandled devmem checks at least.
> > > This is because the devmem series comes with a full audit of
> > > skb_frag_page() callers [1] and all areas in the net stack attempting
> > > to access the skb [2].
> > >
> > > [1] https://patchwork.kernel.org/project/netdevbpf/patch/20231218024024.3516870-10-almasrymina@google.com/
> > > [2] https://patchwork.kernel.org/project/netdevbpf/patch/20231218024024.3516870-11-almasrymina@google.com/
> > >
> > > > The fact you all are debating what to do with skb_frag_page() suggests
> > > > to me there isn't confidence...
> > > >
> > >
> > > The debate raging on is related to the performance of skb_frag_page(),
> > > not correctness (and even then, I don't think it's related to
> > > perf...). Yunsheng would like us to optimize skb_frag_page() using an
> > > unconditional cast from netmem to page. This in Yunsheng's mind is a
> > > performance optimization as we don't need to add an if statement
> > > checking if the netmem is a page. I'm resistant to implement that
> > > change so far because:
> > >
> > > (a) unconditionally casting from netmem to page negates the compiler
> > > type safety that you and Christian are laying out as a requirement for
> > > the devmem stuff.
> > > (b) With likely/unlikely or static branches the check to make sure
> > > netmem is page is a no-op for existing use cases anyway, so AFAIU,
> > > there is no perf gain from optimizing it out anyway.
> > >
> >
> > Another thought, if anyone else is concerned about the devmem checks
> > performance,  potentially we could introduce CONFIG_NET_DEVMEM which
> > when disabled prevents the user from using devmem at all (disables the
> > netlink API).
> >
> > When that is disabled, skb_frag_page(), netmem_to_page() & friends can
> > assume netmem is always a page and do a straight cast between netmem &
> > page. When it's enabled, it will check that netmem == page before
> > doing a cast, and return NULL if it is not a page.
> >
> > I think this is technically viable and I think preserves the compiler
> > type safety requirements set by mm folks. From my POV though, bloating
> > the kernel with a a new CONFIG just to optimize out no-op checks seems
> > unnecessary, but if there is agreement that the checks are a concern,
> > adding CONFIG_NET_DEVMEM should address it while being acceptable to
> > mm maintainers.
>
> I agree. A concern with CONFIGs is that what matters in practice is
> which default the distros compile with. In this case, adding hurdles
> to using the feature likely for no real reason.
>
> Static branches are used throughout the kernel in performance
> sensitive paths, exactly because they allow optional paths effectively
> for free. I'm quite surprised that this issue is being raised so
> strongly here, as they are hardly new or controversial.
>
> But perhaps best is to show with data. Is there a representative page
> pool benchmark that exercises the most sensitive case (XDP_DROP?) that
> we can run with and without a static branch to demonstrate that any
> diff is within the noise?
>

Yes, Jesper has a page_pool benchmark that he pointed me to in RFC v2:

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

When running the results on RFCv2, the results were:

net-next @ b44693495af8
https://pastebin.com/raw/JuU7UQXe

+ devmem TCP changes:
https://pastebin.com/raw/mY1L6U4r

On RFC v2 the benchmark showed only a single instruction regression in
the page_pool fast path & the change deemed acceptable to Jesper from
a performance POV [1].

I have not run the benchmark continually on follow up iterations of
the RFC, but I think I'll start doing so in the future.

[1] https://lore.kernel.org/netdev/7aedc5d5-0daf-63be-21bc-3b724cc1cab9@redhat.com/

> > > But none of this is related to correctness. Code calling
> > > skb_frag_page() will fail or crash if it's not handled correctly
> > > regardless of the implementation details of skb_frag_page(). In the
> > > devmem series we add support to handle it correctly via [1] & [2].
> > >
> > > --
> > > Thanks,
> > > Mina
> >
> >
> >
> > --
> > Thanks,
> > Mina
>
>


-- 
Thanks,
Mina

Powered by blists - more mailing lists