netdev - Re: [PATCH net-next v2 2/3] net: atlantic: Implement xdp data plane

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:   Mon, 21 Mar 2022 15:18:36 +0100
From:   Igor Russkikh <irusskikh@...vell.com>
To:     Taehee Yoo <ap420073@...il.com>, <davem@...emloft.net>,
        <kuba@...nel.org>, <pabeni@...hat.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 2/3] net: atlantic: Implement xdp data plane

Hi Taehee,

Thanks for taking care of that!
Just for your information - I've started xdp draft sometime ago,
but never had a time to complete it.
If interested, you can check it here:
https://github.com/Aquantia/AQtion/commit/165cc46cb3fa68eca3110d846db1744a0feee916 

Couple of comments on your implementation follows.

On 3/19/2022 3:04 PM, Taehee Yoo wrote:
> It supports XDP_PASS, XDP_DROP and multi buffer.
> 
> From now on aq_nic_map_skb() supports xdp_frame to send packet.
> So, TX path of both skb and xdp_frame can use aq_nic_map_skb().
> aq_nic_xmit() is used to send packet with skb and internally it
> calls aq_nic_map_skb(). aq_nic_xmit_xdpf() is used to send packet with
> xdp_frame and internally it calls aq_nic_map_skb().

>  unsigned int aq_nic_map_skb(struct aq_nic_s *self, struct sk_buff *skb,
> -			    struct aq_ring_s *ring)
> +			    struct xdp_frame *xdpf, struct aq_ring_s
> *ring)
>  {

Its not a huge problem, but here you are using one function (aq_nic_map_skb) with two
completely separate paths: either skb != NULL or xdpf != NULL.
This makes the function abit cumbersome and error prone.

> +	if (xdpf) {
> +		sinfo = xdp_get_shared_info_from_frame(xdpf);
> +		total_len = xdpf->len;
> +		dx_buff->len = total_len;
> +		data_ptr = xdpf->data;
> +		if (xdp_frame_has_frags(xdpf)) {
> +			nr_frags = sinfo->nr_frags;
> +			total_len += sinfo->xdp_frags_size;
> +		}
> +		goto start_xdp;

May be instead of doing this jump - just introduce a separate function
like `aq_map_xdp` specially for xdp case.

> +int aq_ring_rx_clean(struct aq_ring_s *self,
> +		     struct napi_struct *napi,
> +		     int *work_done,
> +		     int budget)
> +{
> +	if (static_branch_unlikely(&aq_xdp_locking_key))
> +		return __aq_ring_xdp_clean(self, napi, work_done, budget);
> +	else
> +		return __aq_ring_rx_clean(self, napi, work_done, budget);
> +}

Is that really required to split into `xdp_clean` and `rx_clean` ?
They are very similar, may be try to unify?

Regards,
  Igor