netdev - Re: [RFC PATCH net-next v2 2/2] net: veth: Improving page pool pages recycling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sat, 12 Aug 2023 10:02:39 +0800
From: Liang Chen <liangchen.linux@...il.com>
To: Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, linyunsheng@...wei.com, brouer@...hat.com, hawk@...nel.org, 
	ilias.apalodimas@...aro.org, daniel@...earbox.net, ast@...nel.org, 
	netdev@...r.kernel.org
Subject: Re: [RFC PATCH net-next v2 2/2] net: veth: Improving page pool pages recycling

On Sat, Aug 12, 2023 at 2:16 AM Jesper Dangaard Brouer
<jbrouer@...hat.com> wrote:
>
>
> On 01/08/2023 08.19, Liang Chen wrote:
> [...]
> > diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> > index 509e901da41d..ea1b344e5db4 100644
> > --- a/drivers/net/veth.c
> > +++ b/drivers/net/veth.c
> [...]
> > @@ -848,6 +850,7 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
> >               goto out;
> >       }
> >
> > +     skb_orig = skb;
> >       __skb_push(skb, skb->data - skb_mac_header(skb));
> >       if (veth_convert_skb_to_xdp_buff(rq, xdp, &skb))
> >               goto drop;
> > @@ -862,9 +865,18 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
> >       case XDP_PASS:
> >               break;
> >       case XDP_TX:
> > -             veth_xdp_get(xdp);
> > -             consume_skb(skb);
> > -             xdp->rxq->mem = rq->xdp_mem;
> > +             if (skb != skb_orig) {
> > +                     xdp->rxq->mem = rq->xdp_mem_pp;
> > +                     kfree_skb_partial(skb, true);
> > +             } else if (!skb->pp_recycle) {
> > +                     xdp->rxq->mem = rq->xdp_mem;
> > +                     kfree_skb_partial(skb, true);
> > +             } else {
> > +                     veth_xdp_get(xdp);
> > +                     consume_skb(skb);
> > +                     xdp->rxq->mem = rq->xdp_mem;
> > +             }
> > +
>
> Above code section, and below section looks the same.
> It begs for a common function.
>
> >               if (unlikely(veth_xdp_tx(rq, xdp, bq) < 0)) {
> >                       trace_xdp_exception(rq->dev, xdp_prog, act);
> >                       stats->rx_drops++;
> > @@ -874,9 +886,18 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
> >               rcu_read_unlock();
> >               goto xdp_xmit;
> >       case XDP_REDIRECT:
> > -             veth_xdp_get(xdp);
> > -             consume_skb(skb);
> > -             xdp->rxq->mem = rq->xdp_mem;
> > +             if (skb != skb_orig) {
> > +                     xdp->rxq->mem = rq->xdp_mem_pp;
> > +                     kfree_skb_partial(skb, true);
> > +             } else if (!skb->pp_recycle) {
> > +                     xdp->rxq->mem = rq->xdp_mem;
> > +                     kfree_skb_partial(skb, true);
> > +             } else {
> > +                     veth_xdp_get(xdp);
> > +                     consume_skb(skb);
> > +                     xdp->rxq->mem = rq->xdp_mem;
> > +             }
> > +
>
> The common function can be named to reflect what the purpose of this
> code section is.  According to my understanding, the code steals the
> (packet) data section from the SKB and free the SKB.  And
> prepare/associate the correct memory type in xdp_buff->rxq.
>
> Function name proposals:
>   __skb_steal_data
>   __free_skb_and_steal_data
>   __free_skb_and_steal_data_for_xdp
>   __free_skb_and_xdp_steal_data
>   __skb2xdp_steal_data
>
> When doing this in a function, it will also allow us to add some
> comments explaining the different cases and assumptions.  These
> assumptions can get broken as a result of (future) changes in
> surrounding the code, thus we need to explain our assumptions/intent (to
> help our future selves).
>
> For code readability, I think we should convert (skb != skb_orig) into a
> boolean that says what this case captures, e.g. local_pp_alloc.
>
> Func prototype:
>   __skb2xdp_steal_data(skb, xdp, rq, bool local_pp_alloc)
>
>
> Always feel free to challenge my view,
> --Jesper
>

Thank you for the detailed suggestion! We will move the code into a
function as you pointed out and comment on each of the different cases
to explain our assumption and intent.

Thanks,
Liang