[PATCH 7/8] enic : cleanup of enic wq request completion path

Satish Kharat via B4 Relay posted 8 patches 11 months, 2 weeks ago
There is a newer version of this series
[PATCH 7/8] enic : cleanup of enic wq request completion path
Posted by Satish Kharat via B4 Relay 11 months, 2 weeks ago
From: Satish Kharat <satishkh@cisco.com>

Cleans up the enic wq request completion path needed for 16k wq size
support.

Co-developed-by: Nelson Escobar <neescoba@cisco.com>
Signed-off-by: Nelson Escobar <neescoba@cisco.com>
Co-developed-by: John Daley <johndale@cisco.com>
Signed-off-by: John Daley <johndale@cisco.com>
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
 drivers/net/ethernet/cisco/enic/enic_main.c |  7 +--
 drivers/net/ethernet/cisco/enic/enic_wq.c   | 98 +++++++++++++++--------------
 drivers/net/ethernet/cisco/enic/enic_wq.h   |  9 +--
 3 files changed, 55 insertions(+), 59 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 52174843f02f1fecc75666367ad5034cbbcf8f07..54aa3953bf7b6ed4fdadd7b9871ee7bbcf6614ea 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -1332,8 +1332,7 @@ static int enic_poll(struct napi_struct *napi, int budget)
 	unsigned int  work_done, rq_work_done = 0, wq_work_done;
 	int err;
 
-	wq_work_done = vnic_cq_service(&enic->cq[cq_wq], wq_work_to_do,
-				       enic_wq_service, NULL);
+	wq_work_done = enic_wq_cq_service(enic, cq_wq, wq_work_to_do);
 
 	if (budget > 0)
 		rq_work_done = enic_rq_cq_service(enic, cq_rq, rq_work_to_do);
@@ -1435,8 +1434,8 @@ static int enic_poll_msix_wq(struct napi_struct *napi, int budget)
 	wq_irq = wq->index;
 	cq = enic_cq_wq(enic, wq_irq);
 	intr = enic_msix_wq_intr(enic, wq_irq);
-	wq_work_done = vnic_cq_service(&enic->cq[cq], wq_work_to_do,
-				       enic_wq_service, NULL);
+
+	wq_work_done = enic_wq_cq_service(enic, cq, wq_work_to_do);
 
 	vnic_intr_return_credits(&enic->intr[intr], wq_work_done,
 				 0 /* don't unmask intr */,
diff --git a/drivers/net/ethernet/cisco/enic/enic_wq.c b/drivers/net/ethernet/cisco/enic/enic_wq.c
index 88fdc462839a8360000eb8526be64118ea35c0e2..37e8f6eeae3fabd3391b8fcacc5f3420ad091b17 100644
--- a/drivers/net/ethernet/cisco/enic/enic_wq.c
+++ b/drivers/net/ethernet/cisco/enic/enic_wq.c
@@ -6,8 +6,12 @@
 #include "enic.h"
 #include "enic_wq.h"
 
-static void cq_desc_dec(const struct cq_desc *desc_arg, u8 *type, u8 *color,
-			u16 *q_number, u16 *completed_index)
+#define ENET_CQ_DESC_COMP_NDX_BITS 14
+#define ENET_CQ_DESC_COMP_NDX_MASK GENMASK(ENET_CQ_DESC_COMP_NDX_BITS - 1, 0)
+
+static inline void enic_wq_cq_desc_dec(const struct cq_desc *desc_arg, bool ext_wq,
+				       u8 *type, u8 *color, u16 *q_number,
+				       u16 *completed_index)
 {
 	const struct cq_desc *desc = desc_arg;
 	const u8 type_color = desc->type_color;
@@ -25,47 +29,13 @@ static void cq_desc_dec(const struct cq_desc *desc_arg, u8 *type, u8 *color,
 
 	*type = type_color & CQ_DESC_TYPE_MASK;
 	*q_number = le16_to_cpu(desc->q_number) & CQ_DESC_Q_NUM_MASK;
-	*completed_index = le16_to_cpu(desc->completed_index) &
-		CQ_DESC_COMP_NDX_MASK;
-}
-
-unsigned int vnic_cq_service(struct vnic_cq *cq, unsigned int work_to_do,
-			     int (*q_service)(struct vnic_dev *vdev, struct cq_desc *cq_desc,
-					      u8 type, u16 q_number, u16 completed_index,
-					      void *opaque), void *opaque)
-{
-	struct cq_desc *cq_desc;
-	unsigned int work_done = 0;
-	u16 q_number, completed_index;
-	u8 type, color;
 
-	cq_desc = (struct cq_desc *)((u8 *)cq->ring.descs +
-		   cq->ring.desc_size * cq->to_clean);
-	cq_desc_dec(cq_desc, &type, &color,
-		    &q_number, &completed_index);
-
-	while (color != cq->last_color) {
-		if ((*q_service)(cq->vdev, cq_desc, type, q_number,
-				 completed_index, opaque))
-			break;
-
-		cq->to_clean++;
-		if (cq->to_clean == cq->ring.desc_count) {
-			cq->to_clean = 0;
-			cq->last_color = cq->last_color ? 0 : 1;
-		}
-
-		cq_desc = (struct cq_desc *)((u8 *)cq->ring.descs +
-			cq->ring.desc_size * cq->to_clean);
-		cq_desc_dec(cq_desc, &type, &color,
-			    &q_number, &completed_index);
-
-		work_done++;
-		if (work_done >= work_to_do)
-			break;
-	}
-
-	return work_done;
+	if (ext_wq)
+		*completed_index = le16_to_cpu(desc->completed_index) &
+			ENET_CQ_DESC_COMP_NDX_MASK;
+	else
+		*completed_index = le16_to_cpu(desc->completed_index) &
+			CQ_DESC_COMP_NDX_MASK;
 }
 
 void enic_free_wq_buf(struct vnic_wq *wq, struct vnic_wq_buf *buf)
@@ -93,15 +63,15 @@ static void enic_wq_free_buf(struct vnic_wq *wq, struct cq_desc *cq_desc,
 	enic_free_wq_buf(wq, buf);
 }
 
-int enic_wq_service(struct vnic_dev *vdev, struct cq_desc *cq_desc, u8 type,
-		    u16 q_number, u16 completed_index, void *opaque)
+static void enic_wq_service(struct vnic_dev *vdev, struct cq_desc *cq_desc,
+			    u8 type, u16 q_number, u16 completed_index)
 {
 	struct enic *enic = vnic_dev_priv(vdev);
 
 	spin_lock(&enic->wq[q_number].lock);
 
 	vnic_wq_service(&enic->wq[q_number].vwq, cq_desc,
-			completed_index, enic_wq_free_buf, opaque);
+			completed_index, enic_wq_free_buf, NULL);
 
 	if (netif_tx_queue_stopped(netdev_get_tx_queue(enic->netdev, q_number)) &&
 	    vnic_wq_desc_avail(&enic->wq[q_number].vwq) >=
@@ -111,7 +81,41 @@ int enic_wq_service(struct vnic_dev *vdev, struct cq_desc *cq_desc, u8 type,
 	}
 
 	spin_unlock(&enic->wq[q_number].lock);
-
-	return 0;
 }
 
+unsigned int enic_wq_cq_service(struct enic *enic, unsigned int cq_index, unsigned int work_to_do)
+{
+	u16 q_number, completed_index;
+	u8 type, color;
+	unsigned int work_done = 0;
+	struct vnic_cq *cq = &enic->cq[cq_index];
+	struct cq_desc *cq_desc = (struct cq_desc *)((u8 *)cq->ring.descs +
+		cq->ring.desc_size * cq->to_clean);
+
+	bool ext_wq = cq->ring.size > ENIC_MAX_WQ_DESCS;
+
+	enic_wq_cq_desc_dec(cq_desc, ext_wq, &type, &color,
+			    &q_number, &completed_index);
+
+	while (color != cq->last_color) {
+		enic_wq_service(cq->vdev, cq_desc, type, q_number, completed_index);
+
+		cq->to_clean++;
+
+		if (cq->to_clean == cq->ring.desc_count) {
+			cq->to_clean = 0;
+			cq->last_color = cq->last_color ? 0 : 1;
+		}
+
+		if (++work_done >= work_to_do)
+			break;
+
+		cq_desc = (struct cq_desc *)((u8 *)cq->ring.descs +
+			cq->ring.desc_size * cq->to_clean);
+
+		enic_wq_cq_desc_dec(cq_desc, ext_wq, &type, &color,
+				    &q_number, &completed_index);
+	}
+
+	return work_done;
+}
diff --git a/drivers/net/ethernet/cisco/enic/enic_wq.h b/drivers/net/ethernet/cisco/enic/enic_wq.h
index 051fe8a0cbbaba603414fb49b8a1cc919a2e1347..c54df0a7f061dc544816a2c64bb42f2d617c00bb 100644
--- a/drivers/net/ethernet/cisco/enic/enic_wq.h
+++ b/drivers/net/ethernet/cisco/enic/enic_wq.h
@@ -2,12 +2,5 @@
  * Copyright 2025 Cisco Systems, Inc.  All rights reserved.
  */
 
-unsigned int vnic_cq_service(struct vnic_cq *cq, unsigned int work_to_do,
-			     int (*q_service)(struct vnic_dev *vdev, struct cq_desc *cq_desc,
-					      u8 type, u16 q_number, u16 completed_index,
-					      void *opaque), void *opaque);
-
 void enic_free_wq_buf(struct vnic_wq *wq, struct vnic_wq_buf *buf);
-
-int enic_wq_service(struct vnic_dev *vdev, struct cq_desc *cq_desc, u8 type,
-		    u16 q_number, u16 completed_index, void *opaque);
+unsigned int enic_wq_cq_service(struct enic *enic, unsigned int cq_index, unsigned int work_to_do);

-- 
2.48.1
Re: [PATCH 7/8] enic : cleanup of enic wq request completion path
Posted by Paolo Abeni 11 months, 1 week ago
On 2/28/25 1:30 AM, Satish Kharat wrote:
> diff --git a/drivers/net/ethernet/cisco/enic/enic_wq.c b/drivers/net/ethernet/cisco/enic/enic_wq.c
> index 88fdc462839a8360000eb8526be64118ea35c0e2..37e8f6eeae3fabd3391b8fcacc5f3420ad091b17 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_wq.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_wq.c
> @@ -6,8 +6,12 @@
>  #include "enic.h"
>  #include "enic_wq.h"
>  
> -static void cq_desc_dec(const struct cq_desc *desc_arg, u8 *type, u8 *color,
> -			u16 *q_number, u16 *completed_index)
> +#define ENET_CQ_DESC_COMP_NDX_BITS 14
> +#define ENET_CQ_DESC_COMP_NDX_MASK GENMASK(ENET_CQ_DESC_COMP_NDX_BITS - 1, 0)
> +
> +static inline void enic_wq_cq_desc_dec(const struct cq_desc *desc_arg, bool ext_wq,
> +				       u8 *type, u8 *color, u16 *q_number,
> +				       u16 *completed_index)

Please avoid 'inline' function in c files.

> @@ -111,7 +81,41 @@ int enic_wq_service(struct vnic_dev *vdev, struct cq_desc *cq_desc, u8 type,
>  	}
>  
>  	spin_unlock(&enic->wq[q_number].lock);
> -
> -	return 0;
>  }
>  
> +unsigned int enic_wq_cq_service(struct enic *enic, unsigned int cq_index, unsigned int work_to_do)
> +{
> +	u16 q_number, completed_index;
> +	u8 type, color;
> +	unsigned int work_done = 0;
> +	struct vnic_cq *cq = &enic->cq[cq_index];
> +	struct cq_desc *cq_desc = (struct cq_desc *)((u8 *)cq->ring.descs +
> +		cq->ring.desc_size * cq->to_clean);

I think the code would be more readable creating an helper to access the
`cq->to_clean` descriptor.

> +
> +	bool ext_wq = cq->ring.size > ENIC_MAX_WQ_DESCS;

Please respect the revers christmas tree order for variable definition,
and avoid empty lines in the variable definition area.

The above also applies to other patches.

Please include the target tree name (net-next in this case) in the subj
prefix in the next iteration.

Thanks,

Paolo