netdev - Re: [Intel-wired-lan] [PATCH iwl-next v8 10/14] iavf: flatten union iavf_32byte_rx

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: <12194f7c-e9b6-472d-979e-5a19e6dbc05f@intel.com>
Date: Tue, 30 Jul 2024 16:37:04 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v8 10/14] iavf: flatten union
 iavf_32byte_rx_desc

From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
Date: Tue, 30 Jul 2024 05:15:05 -0400

> [PATCH iwl-next v8 10/14] iavf: flatten union iavf_32byte_rx_desc

I feel like the description is not precise. You change the descriptor
representation from single small fields to quad-words, so it should be
something like

iavf: define Rx descriptors as qwords

> The union iavf_32byte_rx_desc consists of two unnamed structs defined
> inside. One of them represents legacy 32 byte descriptor and second the
> 16 byte descriptor (extended to 32 byte). Each of them consists of
> bunch of unions, structs and __le fields that represent specific fields
> in descriptor.
> 
> This commit changes the representation of iavf_32byte_rx_desc union
> to store four __le64 fields (qw0, qw1, qw2, qw3) that represent
> quad-words. Those quad-words will be then accessed by calling
> leXY_get_bits macros in upcoming commits.
> 
> Suggested-by: Alexander Lobakin <aleksander.lobakin@...el.com>
> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>

OMG great job!

[...]

> @@ -1163,7 +1164,7 @@ static int iavf_clean_rx_irq(struct iavf_ring *rx_ring, int budget)
>  		 * which is always zero because packet split isn't used, if the
>  		 * hardware wrote DD then the length will be non-zero
>  		 */
> -		qword = le64_to_cpu(rx_desc->wb.qword1.status_error_len);
> +		qword = le64_to_cpu(rx_desc->qw1);
>  
>  		/* This memory barrier is needed to keep us from reading
>  		 * any other fields out of the rx_desc until we have
> @@ -1219,7 +1220,7 @@ static int iavf_clean_rx_irq(struct iavf_ring *rx_ring, int budget)
>  		/* probably a little skewed due to removing CRC */
>  		total_rx_bytes += skb->len;
>  
> -		qword = le64_to_cpu(rx_desc->wb.qword1.status_error_len);
> +		qword = le64_to_cpu(rx_desc->qw1);
>  		rx_ptype = FIELD_GET(IAVF_RXD_QW1_PTYPE_MASK, qword);
>  
>  		/* populate checksum, VLAN, and protocol */
> @@ -1227,11 +1228,17 @@ static int iavf_clean_rx_irq(struct iavf_ring *rx_ring, int budget)
>  
>  		if (qword & BIT(IAVF_RX_DESC_STATUS_L2TAG1P_SHIFT) &&
>  		    rx_ring->flags & IAVF_TXRX_FLAGS_VLAN_TAG_LOC_L2TAG1)
> -			vlan_tag = le16_to_cpu(rx_desc->wb.qword0.lo_dword.l2tag1);
> -		if (rx_desc->wb.qword2.ext_status &
> -		    cpu_to_le16(BIT(IAVF_RX_DESC_EXT_STATUS_L2TAG2P_SHIFT)) &&
> +			vlan_tag = le64_get_bits(rx_desc->qw0,
> +						 IAVF_RX_DESC_LEGACY_QW0_L2TAG1_M);
> +
> +		ext_status = le64_get_bits(rx_desc->qw2,
> +					   IAVF_RX_DESC_LEGACY_QW2_EXT_STATUS_M);
> +
> +		if (ext_status &
> +		    BIT(IAVF_RX_DESC_EXT_STATUS_L2TAG2P_SHIFT) &&

A pair of parenthesis for `ext_status & SHIFT`.

BTW, shouldn't this BIT(L2TAG2P_SHIFT) be defined as L2TAG2P_M next to
this shift?

>  		    rx_ring->flags & IAVF_RXR_FLAGS_VLAN_TAG_LOC_L2TAG2_2)
> -			vlan_tag = le16_to_cpu(rx_desc->wb.qword2.l2tag2_2);
> +			vlan_tag = le64_get_bits(rx_desc->qw2,
> +						 IAVF_RX_DESC_LEGACY_QW2_L2TAG2_2_M);

Additional optimization would be to read each qword only once. Here you
have at least 2-3 reads per each qword. On x86_64, this is fine anyway,
so I think this is out of this series' scope.
The main issue is addressed anyway -- the reads are now 64-bit wide.
Just make sure everything works as before -- checksums, hashes, VLAN
tags etc.

>  
>  		iavf_trace(clean_rx_irq_rx, rx_ring, rx_desc, skb);
>  		iavf_receive_skb(rx_ring, skb, vlan_tag);

[...]

> +struct iavf_rx_desc {

kdoc describing each qword?

For example, I've no idea what this 'ext_status' does mean. If you
described ::qw2 here, I'd probably understand it :>

> +	aligned_le64 qw0;
> +	aligned_le64 qw1;
> +	aligned_le64 qw2;
> +	aligned_le64 qw3;
> +} __aligned(4 * sizeof(__le64));;

Double ';;' at the end =\

>  
>  enum iavf_rx_desc_status_bits {
>  	/* Note: These are predefined bit offsets */
> @@ -574,4 +504,10 @@ struct iavf_eth_stats {
>  	u64 tx_discards;		/* tdpc */
>  	u64 tx_errors;			/* tepc */
>  };
> +
> +#define IAVF_RX_DESC_LEGACY_QW0_RSS_M		GENMASK_ULL(63, 32)
> +#define IAVF_RX_DESC_LEGACY_QW0_L2TAG1_M	GENMASK_ULL(33, 16)
> +#define IAVF_RX_DESC_LEGACY_QW2_L2TAG2_2_M	GENMASK_ULL(63, 48)
> +#define IAVF_RX_DESC_LEGACY_QW2_EXT_STATUS_M	GENMASK_ULL(11, 0)

I'd define them right after declaring the qwords they are in, like

struct iavf_rx_desc {
	aligned_le64 qw0;
#define IAVF_RX_DESC_LEGACY_RSS_M	GENMASK_ULL(63, 32)
	...

	aligned_le64 qw1;

	aligned_le64 qw2;
#define IAVF_RX_DESC_LEGACY_L2TAG2_2_M	GENMASK_ULL(11, 0)

And these '_QW*' in the middle of the definitions are probably not needed.

> +
>  #endif /* _IAVF_TYPE_H_ */

Thanks,
Olek