Message-ID: <CACGkMEsmN-Z2Kjhqt75-hb-fhW+00kiD0yi=CuX9Zb-nwZn4mA@mail.gmail.com>
Date: Wed, 3 Dec 2025 14:40:19 +0800
From: Jason Wang <jasowang@...hat.com>
To: Jon Kohler <jon@...anix.com>
Cc: Jesper Dangaard Brouer <hawk@...nel.org>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
	Willem de Bruijn <willemdebruijn.kernel@...il.com>, Andrew Lunn <andrew+netdev@...n.ch>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, John Fastabend <john.fastabend@...il.com>, 
	Stanislav Fomichev <sdf@...ichev.me>, open list <linux-kernel@...r.kernel.org>, 
	"open list:XDP (eXpress Data Path):Keyword:(?:b|_)xdp(?:b|_)" <bpf@...r.kernel.org>, Sebastian Andrzej Siewior <bigeasy@...utronix.de>, 
	Alexander Lobakin <aleksander.lobakin@...el.com>
Subject: Re: [PATCH net-next v2 5/9] tun: use bulk NAPI cache allocation in tun_xdp_one

On Wed, Dec 3, 2025 at 12:35 PM Jon Kohler <jon@...anix.com> wrote:
>
>
>
> > On Dec 2, 2025, at 11:10 PM, Jason Wang <jasowang@...hat.com> wrote:
> >
> > On Wed, Dec 3, 2025 at 1:46 AM Jon Kohler <jon@...anix.com> wrote:
> >>
> >>
> >>
> >>> On Dec 2, 2025, at 12:32 PM, Jesper Dangaard Brouer <hawk@...nel.org> wrote:
> >>>
> >>>
> >>>
> >>> On 02/12/2025 17.49, Jon Kohler wrote:
> >>>>> On Nov 27, 2025, at 10:02 PM, Jason Wang <jasowang@...hat.com> wrote:
> >>>>>
> >>>>> On Wed, Nov 26, 2025 at 3:19 AM Jon Kohler <jon@...anix.com> wrote:
> >>>>>>
> >>>>>> Optimize TUN_MSG_PTR batch processing by allocating sk_buff structures
> >>>>>> in bulk from the per-CPU NAPI cache using napi_skb_cache_get_bulk.
> >>>>>> This reduces allocation overhead and improves efficiency, especially
> >>>>>> when IFF_NAPI is enabled and GRO is feeding entries back to the cache.
> >>>>>
> >>>>> Does this mean we should only enable this when NAPI is used?
> >>>> No, it does not mean that at all, but I see what that would be confusing.
> >>>> I can clean up the commit msg on the next go around
> >>>>>>
> >>>>>> If bulk allocation cannot fully satisfy the batch, gracefully drop only
> >>>>>> the uncovered portion, allowing the rest of the batch to proceed, which
> >>>>>> is what already happens in the previous case where build_skb() would
> >>>>>> fail and return -ENOMEM.
> >>>>>>
> >>>>>> Signed-off-by: Jon Kohler <jon@...anix.com>
> >>>>>
> >>>>> Do we have any benchmark result for this?
> >>>> Yes, it is in the cover letter:
> >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_project_netdevbpf_cover_20251125200041.1565663-2D1-2Djon-40nutanix.com_&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=NGPRGGo37mQiSXgHKm5rCQ&m=D7piJwOOQSj7C1puBlbh5dmAc-qsLw6E660yC5jJXWZk9ppvjOqT9Xc61ewYSmod&s=yUPhRdqt2lVnW5FxiOpvKE34iXKyGEWk502Dko1i3PI&e=
> >
> > Ok but it only covers UDP, I think we want to see how it performs for
> > TCP as well as latency. Btw is the test for IFF_NAPI or not?
>
> This test was without IFF_NAPI, but I could get the NAPI numbers too
> More on that below
>
> >
> >>>>>> ---
> >>>>>> drivers/net/tun.c | 30 ++++++++++++++++++++++++------
> >>>>>> 1 file changed, 24 insertions(+), 6 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> >>>>>> index 97f130bc5fed..64f944cce517 100644
> >>>>>> --- a/drivers/net/tun.c
> >>>>>> +++ b/drivers/net/tun.c
> >>> [...]
> >>>>>> @@ -2454,6 +2455,7 @@ static int tun_xdp_one(struct tun_struct *tun,
> >>>>>>               ret = tun_xdp_act(tun, xdp_prog, xdp, act);
> >>>>>>               if (ret < 0) {
> >>>>>>                       /* tun_xdp_act already handles drop statistics */
> >>>>>> +                       kfree_skb_reason(skb, SKB_DROP_REASON_XDP);
> >>>>>
> >>>>> This should belong to previous patches?
> >>>> Well, not really, as we did not even have an SKB to free at this point
> >>>> in the previous code
> >>>>>
> >>>>>>                       put_page(virt_to_head_page(xdp->data));
> >>>
> >>> This calling put_page() directly also looks dubious.
> >>>
> >>>>>>                       return ret;
> >>>>>>               }
> >>>>>> @@ -2463,6 +2465,7 @@ static int tun_xdp_one(struct tun_struct *tun,
> >>>>>>                       *flush = true;
> >>>>>>                       fallthrough;
> >>>>>>               case XDP_TX:
> >>>>>> +                       napi_consume_skb(skb, 1);
> >>>>>>                       return 0;
> >>>>>>               case XDP_PASS:
> >>>>>>                       break;
> >>>>>> @@ -2475,13 +2478,15 @@ static int tun_xdp_one(struct tun_struct *tun,
> >>>>>>                               tpage->page = page;
> >>>>>>                               tpage->count = 1;
> >>>>>>                       }
> >>>>>> +                       napi_consume_skb(skb, 1);
> >>>>>
> >>>>> I wonder if this would have any side effects since tun_xdp_one() is
> >>>>> not called by a NAPI.
> >>>> As far as I can tell, this napi_consume_skb is really just an artifact of
> >>>> how it was named and how it was traditionally used.
> >>>> Now this is really just a napi_consume_skb within a bh disable/enable
> >>>> section, which should meet the requirements of how that interface
> >>>> should be used (again, AFAICT)
> >>>
> >>> Yicks - this sounds super ugly.  Just wrapping napi_consume_skb() in bh
> >>> disable/enable section and then assuming you get the same protection as
> >>> NAPI is really dubious.
> >>>
> >>> Cc Sebastian as he is trying to cleanup these kind of use-case, to make
> >>> kernel preemption work.
> >>>
> >>>
> >>>>>
> >>>>>>                       return 0;
> >>>>>>               }
> >>>>>>       }
> >>>>>>
> >>>>>> build:
> >>>>>> -       skb = build_skb(xdp->data_hard_start, buflen);
> >>>>>> +       skb = build_skb_around(skb, xdp->data_hard_start, buflen);
> >>>>>>       if (!skb) {
> >>>>>> +               kfree_skb_reason(skb, SKB_DROP_REASON_NOMEM);
> >>>> Though to your point, I dont think this actually does anything given
> >>>> that if the skb was somehow nuked as part of build_skb_around, there
> >>>> would not be an skb to free. Doesn’t hurt though, from a self documenting
> >>>> code perspective tho?
> >>>>>>               dev_core_stats_rx_dropped_inc(tun->dev);
> >>>>>>               return -ENOMEM;
> >>>>>>       }
> >>>>>> @@ -2566,9 +2571,11 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
> >>>>>>       if (m->msg_controllen == sizeof(struct tun_msg_ctl) &&
> >>>>>>           ctl && ctl->type == TUN_MSG_PTR) {
> >>>>>>               struct bpf_net_context __bpf_net_ctx, *bpf_net_ctx;
> >>>>>> +               int flush = 0, queued = 0, num_skbs = 0;
> >>>>>>               struct tun_page tpage;
> >>>>>>               int n = ctl->num;
> >>>>>> -               int flush = 0, queued = 0;
> >>>>>> +               /* Max size of VHOST_NET_BATCH */
> >>>>>> +               void *skbs[64];
> >>>>>
> >>>>> I think we need some tweaks
> >>>>>
> >>>>> 1) TUN is decoupled from vhost, so it should have its own value (a
> >>>>> macro is better)
> >>>> Sure, I can make another constant that does a similar thing
> >>>>> 2) Provide a way to fail or handle the case when more than 64
> >>>> What if we simply assert that the maximum here is 64, which I think
> >>>> is what it actually is in practice?
> >
> > I still prefer a fallback.
>
> Ack, will chew on that for the next one, let’s settle on the larger
> elephant in the room which is the NAPI stuff below, as none of this
> goes anywhere without resolving that first.
>
> >
> >>>>>
> >>>>>>
> >>>>>>               memset(&tpage, 0, sizeof(tpage));
> >>>>>>
> >>>>>> @@ -2576,13 +2583,24 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
> >>>>>>               rcu_read_lock();
> >>>>>>               bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
> >>>>>>
> >>>>>> -               for (i = 0; i < n; i++) {
> >>>>>> +               num_skbs = napi_skb_cache_get_bulk(skbs, n);
> >>>>>
> >>>>> Its document said:
> >>>>>
> >>>>> """
> >>>>> * Must be called *only* from the BH context.
> >>>>> “"”
> >>>> We’re in a bh_disable section here, is that not good enough?
> >>>
> >>> Again this feels very ugly and prone to painting ourselves into a
> >>> corner, assuming BH-disabled sections have same protection as NAPI.
> >>>
> >>> (The napi_skb_cache_get/put function are operating on per CPU arrays
> >>> without any locking.)
> >>
> >> Happy to take suggestions on an alternative approach.
> >>
> >> Thoughts:
> >> 1. Instead of having IFF_NAPI be an opt-in thing, clean up tun so it
> >>   is *always* NAPI’d 100% of the time?
> >
> > IFF_NAPI will have some overheads and it is introduced basically for
> > testing if I was not wrong.
>
> IIRC it was originally introduced for testing, but under some circumstances
> can be wildly faster, see commit fb3f903769e805221eb19209b3d9128d398038a1
> ("tun: support NAPI for packets received from batched XDP buffs")
>
> You may be thinking of IFF_NAPI_FRAGS, which seems very much “test only”
> at this point.
>
> Anyhow, assuming you are thinking of IFF_NAPI itself:
> - Are the overheads you’ve got in mind completely structural/unavoidable?

NAPI will introduce some latency but I'm not sure if it can be
amortized by the bulking logic you want to introduce. So I think we
need benchmark numbers to decide.

> - Or is that something that would be worth while looking at?

I think so.

>
> As a side note, one thing I did play with that is absolutely silly faster
> is using IFF_NAPI with NAPI threads. Under certain scenarios (high tput
> that is normally copy bound), gains were nutty (like ~75%+), so the point
> is there may be some very interesting juice to squeeze going down that
> path.

I see.

>
> Coming back to the main path here, the whole reason I’m going down this
> patchset is to try to pickup optimizations that are available in other
> general purpose drivers, which are all NAPI-ized. We’re at the point now
> where tun is getting left behind for things like this because of non-full
> time NAPI.
>
> Said another way, I think it would be an advantage to NAPI-ize tun and
> make it more like regular ole network drivers, so that the generic core
> work being done will benefit tun by default.

The change looks non-trivial, maybe it would be easier to start to
optimize NAPI path first.

>
> >> Outside of people who have
> >>   wired this up in their apps manually, on the virtualization side
> >>   there is currently no support from QEMU/Libvirt to enable IFF_NAPI.
> >>   Might be a nice simplification/cleanup to just “do it” full time?
> >>   Then we can play all these sorts of games under the protection of
> >>   NAPI?
> >
> > A full benchmark needs to be run for this to see.
>
> Do you have a suggested test/suite of tests you’d prefer me to run
> so that I can make sure I’m gathering the data that you’d like to
> see?

Just FYI, something like this:

https://lkml.org/lkml/2012/12/7/272

Thanks

>
> >> 2. (Some other non-dubious way of protecting this, without refactoring
> >>   for either conditional NAPI (yuck?) or refactoring for full time
> >>   NAPI? This would be nice, happy to take tips!
> >> 3. ... ?
> >>
> >
>

Powered by blists - more mailing lists