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]

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