From: Mateusz Polchlopek <mateusz.polchlo...@intel.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.loba...@intel.com>
> Signed-off-by: Mateusz Polchlopek <mateusz.polchlo...@intel.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
