linux-kernel - Re: [PATCH RESEND bpf-next 02/15] ice: make RX HW timestamp reading code more reusable

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: <ac1895cc-a7b0-e40c-7dfc-8ab301f39893@intel.com>
Date:   Fri, 19 May 2023 18:52:13 +0200
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Larysa Zaremba <larysa.zaremba@...el.com>
CC:     <bpf@...r.kernel.org>, Stanislav Fomichev <sdf@...gle.com>,
        "Alexei Starovoitov" <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "Andrii Nakryiko" <andrii@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "Martin KaFai Lau" <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Tony Nguyen <anthony.l.nguyen@...el.com>,
        Anatoly Burakov <anatoly.burakov@...el.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Alexander Lobakin <alexandr.lobakin@...el.com>,
        "Magnus Karlsson" <magnus.karlsson@...il.com>,
        Maryam Tahhan <mtahhan@...hat.com>,
        <xdp-hints@...-project.net>, <netdev@...r.kernel.org>,
        <intel-wired-lan@...ts.osuosl.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND bpf-next 02/15] ice: make RX HW timestamp reading
 code more reusable

From: Larysa Zaremba <larysa.zaremba@...el.com>
Date: Fri, 12 May 2023 17:25:54 +0200

> Previously, we only needed RX HW timestamp in skb path,
> hence all related code was written with skb in mind.
> But with the addition of XDP hints via kfuncs to the ice driver,
> the same logic will be needed in .xmo_() callbacks.

[...]

> @@ -2176,9 +2174,8 @@ ice_ptp_rx_hwtstamp(struct ice_rx_ring *rx_ring,
>  	ts_high = le32_to_cpu(rx_desc->wb.flex_ts.ts_high);
>  	ts_ns = ice_ptp_extend_32b_ts(cached_time, ts_high);
>  
> -	hwtstamps = skb_hwtstamps(skb);
> -	memset(hwtstamps, 0, sizeof(*hwtstamps));
> -	hwtstamps->hwtstamp = ns_to_ktime(ts_ns);
> +	*dst = ts_ns;
> +	return true;

Can't we use the same I wrote in the prev. comment, i.e. return 0 or
timestamp? I don't think ts == 0 is valid.

>  }
>  
>  /**

[...]

> + * The driver receives a notification in the receive descriptor with timestamp.
> + * The timestamp is in ns, so we must convert the result first.
> + */
> +static void
> +ice_ptp_rx_hwts_to_skb(struct ice_rx_ring *rx_ring,
> +		       union ice_32b_rx_flex_desc *rx_desc,
> +		       struct sk_buff *skb)
> +{
> +	struct skb_shared_hwtstamps *hwtstamps;
> +	u64 ts_ns;
> +
> +	if (!ice_ptp_copy_rx_hwts_from_desc(rx_ring, rx_desc, &ts_ns))
> +		return;
> +
> +	hwtstamps = skb_hwtstamps(skb);
> +	memset(hwtstamps, 0, sizeof(*hwtstamps));
> +	hwtstamps->hwtstamp = ns_to_ktime(ts_ns);

Ok, my optimizations aren't in this series :D
If you look at the hwtimestamps in skb, you'll see all that can be
minimized to just:

	*skb_hwtstamps(skb) = (struct skb_shared_hwtstamps){
		.hwtstamp	= ns_to_ktime(ts_ns),
	};

Compiler will probably do its job, but I wouldn't always rely on it.
Sometimes it's even able to not expand memset(8 bytes) to *(u64 *) = 0.

> +}
> +
>  /**
>   * ice_process_skb_fields - Populate skb header fields from Rx descriptor
>   * @rx_ring: Rx descriptor ring packet is being transacted on
> @@ -210,7 +235,7 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring,
>  	ice_rx_csum(rx_ring, skb, rx_desc, ptype);
>  
>  	if (rx_ring->ptp_rx)
> -		ice_ptp_rx_hwtstamp(rx_ring, rx_desc, skb);
> +		ice_ptp_rx_hwts_to_skb(rx_ring, rx_desc, skb);
>  }
>  
>  /**

Thanks,
Olek