diff mbox series

[iwl-next,v8,09/14] libeth: move idpf_rx_csum_decoded and idpf_rx_extracted

Message ID 20240730091509.18846-10-mateusz.polchlopek@intel.com
State Changes Requested
Headers show
Series Add support for Rx timestamping for both ice and iavf drivers. | expand

Commit Message

Mateusz Polchlopek July 30, 2024, 9:15 a.m. UTC 
Structs idpf_rx_csum_decoded and idpf_rx_extracted are used both in
idpf and iavf Intel drivers. This commit changes the prefix from
idpf_* to libeth_* and moves mentioned structs to libeth's rx.h header
file.

Usage in idpf driver has been adjusted.

Suggested-by: Alexander Lobakin <aleksander.lobakin@intel.com>
Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
---
 .../ethernet/intel/idpf/idpf_singleq_txrx.c   | 20 +++++------
 drivers/net/ethernet/intel/idpf/idpf_txrx.c   |  8 ++---
 drivers/net/ethernet/intel/idpf/idpf_txrx.h   | 19 -----------
 include/net/libeth/rx.h                       | 34 +++++++++++++++++++
 4 files changed, 48 insertions(+), 33 deletions(-)

Comments

Alexander Lobakin July 30, 2024, 2:24 p.m. UTC | #1 
From: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
Date: Tue, 30 Jul 2024 05:15:04 -0400

> Structs idpf_rx_csum_decoded and idpf_rx_extracted are used both in
> idpf and iavf Intel drivers. This commit changes the prefix from
> idpf_* to libeth_* and moves mentioned structs to libeth's rx.h header
> file.

[...]

> diff --git a/include/net/libeth/rx.h b/include/net/libeth/rx.h
> index 43574bd6612f..197ff0f5e34c 100644
> --- a/include/net/libeth/rx.h
> +++ b/include/net/libeth/rx.h
> @@ -258,4 +258,38 @@ static inline void libeth_rx_pt_set_hash(struct sk_buff *skb, u32 hash,
>  	skb_set_hash(skb, hash, pt.payload_layer);
>  }
>  
> +/**
> + * struct libeth_rx_csum_decoded - csum errors indicators

1. I'd name it just 'libeth_rx_csum'.
2. The brief you put at the end of the doc can be moved here.

 * struct libeth_rx_csum - Rx checksum offload status

> + * @l3l4p: L3 and L4 integrity check

What does it mean? kdoc should explain everything to help people understand.
From this desc, I don't get whether it's a "valid checksum" status or
"invalid checksum" status. Whether it means that both L3 and L4 are
correct or can be only one of them?

> + * @ipe: IP checksum error indication

'indication' can be omitted. Just 'IP checksum error'.

> + * @eipe: External (most outer) IP header (only relevant for tunneled packets)

1. 'outermost'.
2. Please start each field's description from a lowercase letter.

 * @eipe: external

...

3. 'only relevant for tunneled packets' can be just 'only for tunnels'

> + * @eudpe: External (most outer) UDP checksum error (only relevant for tunneled
> + *         packets)
> + * @ipv6exadd: IPv6 with Destination Options Header or Routing Header

Maybe 'IPv6 header with extension headers'?

> + * @l4e: L4 integrity error indication

Same as for l3l4p.

> + *
> + * Checksum offload bits decoded from the receive descriptor.
> + */
> +struct libeth_rx_csum_decoded {
> +	u32 l3l4p:1;
> +	u32 ipe:1;
> +	u32 eipe:1;
> +	u32 eudpe:1;
> +	u32 ipv6exadd:1;
> +	u32 l4e:1;
> +	u32 pprs:1;
> +	u32 nat:1;
> +	u32 raw_csum_inv:1;
> +	u32 raw_csum:16;
> +};

1. Please place this struct (as well as the second one) after
   &libeth_rx_pt and align the indentation to it (my style is
   that I keep one indentation level per file).
3. You need a padding, so that ::raw_csum will be at offset of 2 bytes,
   i.e. aligned naturally. See how I align ::hash type in &libeth_rx_pt.
   I.e.

	u32					raw_csum_inv:1;

	u32					pad:7;
	u32					raw_csum:16;

4. I'd invert ::raw_csum_inv to ::raw_csum_valid.
5. kernel-doc for each field is needed. Here a half of fields doesn't
   have it.

scripts/kernel-doc -none -Wall include/net/libeth/rx.h

> +
> +struct libeth_rx_extracted {

The structure name says nothing.
Maybe

struct libeth_rqe_info {

"Receive queue element info".

I chose RQE, not "packet", as this info is per-descriptor, not
per-packet.

> +	unsigned int	size;

'len'?

> +	u16		vlan_tag;

Is it C-Tag or S-Tag or both depending on something or...?

> +	u16		rx_ptype;

Just 'ptype'?

> +	u8		end_of_packet:1;

Just 'eop'?

> +	u8		rxe:1;
> +};

1. kdoc. I've no idea what 'rxe' does mean.
2. Why `u8` and `u16`, but `unsigned int`, not `u32`?
   (I'd make everything u32:bits BTW)

3. You waste 4 bytes for only 2 bits.
   Since maximum ptype is currently 1024, while u16 can store up to
   65535, you could do

struct libeth_rqe_info {
	u32				len;
	u32				vlan_tag:16;

	u32				ptype:14;
	u32				eop:1;
	u32				rxe:1;
};

Most important: do you realize that not all of the fields are needed in
the same place, but some of them earlier, some later, and sometime not
even all of them are neede? For example, if it's not an EOP frag, you
would only need ::len and ::eop and that's it. The rest is only needed
for EOP fragments.
That means, reading all of them at once to one struct can lead to perf
regressions. One way to avoid this is to read-fields-when-needed, e.g.
always fill ::len and ::eop and the rest later if/when needed.

Also, can we just do a ptype lookup right away when we read ptype from
the descriptor and then pass &libeth_rx_pt everywhere, so that we won't
even need the ::ptype field here?

> +
> +
>  #endif /* __LIBETH_RX_H */

Thanks,
Olek
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
index fe64febf7436..6d2a173f61e2 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
@@ -609,7 +609,7 @@  static bool idpf_rx_singleq_is_non_eop(const union virtchnl2_rx_desc *rx_desc)
  */
 static void idpf_rx_singleq_csum(struct idpf_rx_queue *rxq,
 				 struct sk_buff *skb,
-				 struct idpf_rx_csum_decoded csum_bits,
+				 struct libeth_rx_csum_decoded csum_bits,
 				 struct libeth_rx_pt decoded)
 {
 	bool ipv4, ipv6;
@@ -675,10 +675,10 @@  static void idpf_rx_singleq_csum(struct idpf_rx_queue *rxq,
  *
  * Return: parsed checksum status.
  **/
-static struct idpf_rx_csum_decoded
+static struct libeth_rx_csum_decoded
 idpf_rx_singleq_base_csum(const union virtchnl2_rx_desc *rx_desc)
 {
-	struct idpf_rx_csum_decoded csum_bits = { };
+	struct libeth_rx_csum_decoded csum_bits = { };
 	u32 rx_error, rx_status;
 	u64 qword;
 
@@ -710,10 +710,10 @@  idpf_rx_singleq_base_csum(const union virtchnl2_rx_desc *rx_desc)
  *
  * Return: parsed checksum status.
  **/
-static struct idpf_rx_csum_decoded
+static struct libeth_rx_csum_decoded
 idpf_rx_singleq_flex_csum(const union virtchnl2_rx_desc *rx_desc)
 {
-	struct idpf_rx_csum_decoded csum_bits = { };
+	struct libeth_rx_csum_decoded csum_bits = { };
 	u16 rx_status0, rx_status1;
 
 	rx_status0 = le16_to_cpu(rx_desc->flex_nic_wb.status_error0);
@@ -812,7 +812,7 @@  idpf_rx_singleq_process_skb_fields(struct idpf_rx_queue *rx_q,
 				   u16 ptype)
 {
 	struct libeth_rx_pt decoded = rx_q->rx_ptype_lkup[ptype];
-	struct idpf_rx_csum_decoded csum_bits;
+	struct libeth_rx_csum_decoded csum_bits;
 
 	/* modifies the skb - consumes the enet header */
 	skb->protocol = eth_type_trans(skb, rx_q->netdev);
@@ -914,7 +914,7 @@  bool idpf_rx_singleq_buf_hw_alloc_all(struct idpf_rx_queue *rx_q,
  */
 static void
 idpf_rx_singleq_extract_base_fields(const union virtchnl2_rx_desc *rx_desc,
-				    struct idpf_rx_extracted *fields)
+				    struct libeth_rx_extracted *fields)
 {
 	u64 qword;
 
@@ -937,7 +937,7 @@  idpf_rx_singleq_extract_base_fields(const union virtchnl2_rx_desc *rx_desc,
  */
 static void
 idpf_rx_singleq_extract_flex_fields(const union virtchnl2_rx_desc *rx_desc,
-				    struct idpf_rx_extracted *fields)
+				    struct libeth_rx_extracted *fields)
 {
 	fields->size = FIELD_GET(VIRTCHNL2_RX_FLEX_DESC_PKT_LEN_M,
 				 le16_to_cpu(rx_desc->flex_nic_wb.pkt_len));
@@ -955,7 +955,7 @@  idpf_rx_singleq_extract_flex_fields(const union virtchnl2_rx_desc *rx_desc,
 static void
 idpf_rx_singleq_extract_fields(const struct idpf_rx_queue *rx_q,
 			       const union virtchnl2_rx_desc *rx_desc,
-			       struct idpf_rx_extracted *fields)
+			       struct libeth_rx_extracted *fields)
 {
 	if (rx_q->rxdids == VIRTCHNL2_RXDID_1_32B_BASE_M)
 		idpf_rx_singleq_extract_base_fields(rx_desc, fields);
@@ -980,7 +980,7 @@  static int idpf_rx_singleq_clean(struct idpf_rx_queue *rx_q, int budget)
 
 	/* Process Rx packets bounded by budget */
 	while (likely(total_rx_pkts < (unsigned int)budget)) {
-		struct idpf_rx_extracted fields = { };
+		struct libeth_rx_extracted fields = { };
 		union virtchnl2_rx_desc *rx_desc;
 		struct idpf_rx_buf *rx_buf;
 
diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
index 585c3dadd9bf..328b25d7bb63 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
@@ -2928,7 +2928,7 @@  idpf_rx_hash(const struct idpf_rx_queue *rxq, struct sk_buff *skb,
  * skb->protocol must be set before this function is called
  */
 static void idpf_rx_csum(struct idpf_rx_queue *rxq, struct sk_buff *skb,
-			 struct idpf_rx_csum_decoded csum_bits,
+			 struct libeth_rx_csum_decoded csum_bits,
 			 struct libeth_rx_pt decoded)
 {
 	bool ipv4, ipv6;
@@ -2979,10 +2979,10 @@  static void idpf_rx_csum(struct idpf_rx_queue *rxq, struct sk_buff *skb,
  *
  * Return: parsed checksum status.
  **/
-static struct idpf_rx_csum_decoded
+static struct libeth_rx_csum_decoded
 idpf_rx_splitq_extract_csum_bits(const struct virtchnl2_rx_flex_desc_adv_nic_3 *rx_desc)
 {
-	struct idpf_rx_csum_decoded csum = { };
+	struct libeth_rx_csum_decoded csum = { };
 	u8 qword0, qword1;
 
 	qword0 = rx_desc->status_err0_qw0;
@@ -3093,7 +3093,7 @@  static int
 idpf_rx_process_skb_fields(struct idpf_rx_queue *rxq, struct sk_buff *skb,
 			   const struct virtchnl2_rx_flex_desc_adv_nic_3 *rx_desc)
 {
-	struct idpf_rx_csum_decoded csum_bits;
+	struct libeth_rx_csum_decoded csum_bits;
 	struct libeth_rx_pt decoded;
 	u16 rx_ptype;
 
diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.h b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
index 6215dbee5546..0b11e7c6547a 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.h
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
@@ -254,25 +254,6 @@  enum idpf_tx_ctx_desc_eipt_offload {
 	IDPF_TX_CTX_EXT_IP_IPV4         = 0x3
 };
 
-/* Checksum offload bits decoded from the receive descriptor. */
-struct idpf_rx_csum_decoded {
-	u32 l3l4p : 1;
-	u32 ipe : 1;
-	u32 eipe : 1;
-	u32 eudpe : 1;
-	u32 ipv6exadd : 1;
-	u32 l4e : 1;
-	u32 pprs : 1;
-	u32 nat : 1;
-	u32 raw_csum_inv : 1;
-	u32 raw_csum : 16;
-};
-
-struct idpf_rx_extracted {
-	unsigned int size;
-	u16 rx_ptype;
-};
-
 #define IDPF_TX_COMPLQ_CLEAN_BUDGET	256
 #define IDPF_TX_MIN_PKT_LEN		17
 #define IDPF_TX_DESCS_FOR_SKB_DATA_PTR	1
diff --git a/include/net/libeth/rx.h b/include/net/libeth/rx.h
index 43574bd6612f..197ff0f5e34c 100644
--- a/include/net/libeth/rx.h
+++ b/include/net/libeth/rx.h
@@ -258,4 +258,38 @@  static inline void libeth_rx_pt_set_hash(struct sk_buff *skb, u32 hash,
 	skb_set_hash(skb, hash, pt.payload_layer);
 }
 
+/**
+ * struct libeth_rx_csum_decoded - csum errors indicators
+ * @l3l4p: L3 and L4 integrity check
+ * @ipe: IP checksum error indication
+ * @eipe: External (most outer) IP header (only relevant for tunneled packets)
+ * @eudpe: External (most outer) UDP checksum error (only relevant for tunneled
+ *         packets)
+ * @ipv6exadd: IPv6 with Destination Options Header or Routing Header
+ * @l4e: L4 integrity error indication
+ *
+ * Checksum offload bits decoded from the receive descriptor.
+ */
+struct libeth_rx_csum_decoded {
+	u32 l3l4p:1;
+	u32 ipe:1;
+	u32 eipe:1;
+	u32 eudpe:1;
+	u32 ipv6exadd:1;
+	u32 l4e:1;
+	u32 pprs:1;
+	u32 nat:1;
+	u32 raw_csum_inv:1;
+	u32 raw_csum:16;
+};
+
+struct libeth_rx_extracted {
+	unsigned int	size;
+	u16		vlan_tag;
+	u16		rx_ptype;
+	u8		end_of_packet:1;
+	u8		rxe:1;
+};
+
+
 #endif /* __LIBETH_RX_H */