On Thu, Sep 14, 2023 at 06:12:23PM +0200, Alexander Lobakin wrote:

From: Larysa Zaremba <redacted>
Date: Thu, 24 Aug 2023 21:26:40 +0200

quoted

Previously, we only needed RX hash 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.

Separate generic process of reading RX hash from a descriptor
into a separate function.

Signed-off-by: Larysa Zaremba <redacted>

I like the patch, except three minors above,

quoted

---
 drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 37 +++++++++++++------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
index c8322fb6f2b3..8f7f6d78f7bf 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c

@@ -63,28 +63,43 @@ static enum pkt_hash_types ice_ptype_to_htype(u16 ptype)
 }
 
 /**
- * ice_rx_hash - set the hash value in the skb
+ * ice_get_rx_hash - get RX hash value from descriptor
+ * @rx_desc: specific descriptor
+ *
+ * Returns hash, if present, 0 otherwise.
+ */
+static u32
+ice_get_rx_hash(const union ice_32b_rx_flex_desc *rx_desc)

The whole declaration could easily fit into one line :>

I agree
 

quoted

+{
+	const struct ice_32b_rx_flex_desc_nic *nic_mdid;
+
+	if (rx_desc->wb.rxdid != ICE_RXDID_FLEX_NIC)

Not really related: have you tried to measure branch hit/miss here?
Can't it be a candidate for unlikely()?

I have not measured this, but at least in my test setup, I have never seen any 
other rxdid, so unlikely() is a good idea. If it harms some particular 
applications, we can always remove this later on request :D

quoted

+		return 0;
+
+	nic_mdid = (struct ice_32b_rx_flex_desc_nic *)rx_desc;
+	return le32_to_cpu(nic_mdid->rss_hash);

I think the common convention in the kernel is to separate the last
return from the main body with a newline.
To not leave the cast above alone, you can embed it into the declaration.

I am fine with leaving the cast alone.

	const struct ice_32b_rx_flex_desc_nic *mdid = (typeof(mdid))rx_desc;

This is a compile-time cast w/o any maths anyway, so doing it before
checking for the descriptor type doesn't hurt in any way.

	if (!= FLEX)
		return 0;

	return le32_ ...

(or via a ternary)

quoted

+}

[...]

Thanks,
Olek