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
| ||
|
Message-ID: <b5b9df4f-35d8-f20d-6507-3c6c3fafb386@redhat.com> Date: Fri, 11 Aug 2023 20:16:41 +0200 From: Jesper Dangaard Brouer <jbrouer@...hat.com> To: Liang Chen <liangchen.linux@...il.com>, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, linyunsheng@...wei.com Cc: 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 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
Powered by blists - more mailing lists