ethdev: remove callback checks from fast path

Message ID 20250429181132.2544771-1-skori@marvell.com (mailing list archive)
State Superseded
Delegated to: Stephen Hemminger
Headers
Series ethdev: remove callback checks from fast path |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-sample-apps-testing pending Testing pending
ci/iol-broadcom-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-abi-testing warning Testing issues
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Functional success Functional Testing PASS
ci/aws-unit-testing success Unit Testing PASS

Commit Message

Sunil Kumar Kori April 29, 2025, 6:11 p.m. UTC
From: Sunil Kumar Kori <skori@marvell.com>

rte_eth_fp_ops contains ops for fast path APIs. Each API
validates availability of callback and then invoke it.

Removing these NULL checks instead using dummy callbacks.

Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
---
 lib/ethdev/ethdev_driver.c | 47 ++++++++++++++++++++++
 lib/ethdev/ethdev_driver.h | 82 ++++++++++++++++++++++++++++++++++++++
 lib/ethdev/ethdev_pci.h    | 19 +++++++++
 lib/ethdev/rte_ethdev.h    | 20 +---------
 4 files changed, 150 insertions(+), 18 deletions(-)
  

Comments

Stephen Hemminger April 29, 2025, 11:55 p.m. UTC | #1
Addressed
On Tue, 29 Apr 2025 23:41:30 +0530
<skori@marvell.com> wrote:

> @@ -7131,14 +7118,11 @@ rte_eth_tx_queue_count(uint16_t port_id, uint16_t queue_id)
>  		goto out;
>  	}
>  #endif
> -	if (fops->tx_queue_count == NULL) {
> -		rc = -ENOTSUP;
> -		goto out;
> -	}
> -
>  	rc = fops->tx_queue_count(qd);
>  
> +#ifdef RTE_ETHDEV_DEBUG_TX
>  out:
> +#endif

I think you could just fix the ETHDEV_DEBUG_TX to just return.
Other ethdev functions skip calling tracing for
the case of errors.
  
Stephen Hemminger April 29, 2025, 11:59 p.m. UTC | #2
Addressed
On Tue, 29 Apr 2025 23:41:30 +0530
<skori@marvell.com> wrote:

>  
> +static inline void
> +rte_eth_set_dummy_fops(struct rte_eth_dev *eth_dev)

Not fastpath, do not add inline here

> +{
> +	eth_dev->rx_pkt_burst = rte_eth_pkt_burst_dummy;
> +	eth_dev->tx_pkt_burst = rte_eth_pkt_burst_dummy;
> +	eth_dev->tx_pkt_prepare = rte_eth_tx_pkt_prepare_dummy;
> +	eth_dev->rx_queue_count = rte_eth_rx_queue_count_dummy;
> +	eth_dev->tx_queue_count = rte_eth_tx_queue_count_dummy;
> +	eth_dev->rx_descriptor_status = rte_eth_descriptor_status_dummy;
> +	eth_dev->tx_descriptor_status = rte_eth_descriptor_status_dummy;
> +	eth_dev->recycle_tx_mbufs_reuse = rte_eth_recycle_tx_mbufs_reuse_dummy;
> +	eth_dev->recycle_rx_descriptors_refill = rte_eth_recycle_rx_descriptors_refill_dummy;
> +}
> +
>  /**
>   * Copy pci device info to the Ethernet device data.
>   * Shared memory (eth_dev->data) only updated by primary process, so it is safe
> @@ -147,6 +161,11 @@ rte_eth_dev_pci_generic_probe(struct rte_pci_device *pci_dev,
>  	if (!eth_dev)
>  		return -ENOMEM;
>  
> +	/* Update fast path ops with dummy callbacks. Driver will update
> +	 * them with required callbacks in the init function.
> +	 */
> +	rte_eth_set_dummy_fops(eth_dev);
> +

What about non PCI ethdev's?
And in PCI devices there is lots of duplicated code already initilizes these.
Needs to be consolidated and documented. Actually the whole probe process needs more documentation.

Also need to make sure secondary process setup is not broken.
  
Morten Brørup April 30, 2025, 7:26 a.m. UTC | #3
Addressed
> From: Sunil Kumar Kori <skori@marvell.com>
> Sent: Tuesday, 29 April 2025 20.12
> 
> rte_eth_fp_ops contains ops for fast path APIs. Each API
> validates availability of callback and then invoke it.
> 
> Removing these NULL checks instead using dummy callbacks.

The description should mention the motivation for this patch: mbuf recycling performance optimization.

A few nits below.

> +RTE_EXPORT_INTERNAL_SYMBOL(rte_eth_rx_queue_count_dummy)
> +uint32_t
> +rte_eth_rx_queue_count_dummy(void *queue __rte_unused)
> +{
> +	return -ENOTSUP;
> +}

Instead of type casting back and forth, change the type of the RX queue count callback [1]:
-typedef uint32_t (*eth_rx_queue_count_t)(void *rxq);
+typedef int (*eth_rx_queue_count_t)(void *rxq);

So it resembles the TX queue count callback, eth_tx_queue_count_t, which already returns int.

[1]: https://elixir.bootlin.com/dpdk/v25.03/source/lib/ethdev/rte_ethdev_core.h#L48

Although my suggestion is formally an API change, I suppose changing from unsigned to signed should be acceptable.

> +RTE_EXPORT_INTERNAL_SYMBOL(rte_eth_recycle_rx_descriptors_refill_dummy
> )
> +void
> +rte_eth_recycle_rx_descriptors_refill_dummy(void *queue __rte_unused,
> +		uint16_t nb __rte_unused)
> +{
> +

This empty line looks strange. Perhaps add a comment /* No action. */ to indicate that no code is missing here.

> +}
  

Patch

diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index ec0c1e1176..75073f98cf 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -847,6 +847,53 @@  rte_eth_pkt_burst_dummy(void *queue __rte_unused,
 	return 0;
 }
 
+RTE_EXPORT_INTERNAL_SYMBOL(rte_eth_tx_pkt_prepare_dummy)
+uint16_t
+rte_eth_tx_pkt_prepare_dummy(void *queue __rte_unused,
+		struct rte_mbuf **pkts __rte_unused,
+		uint16_t nb_pkts)
+{
+	return nb_pkts;
+}
+
+RTE_EXPORT_INTERNAL_SYMBOL(rte_eth_rx_queue_count_dummy)
+uint32_t
+rte_eth_rx_queue_count_dummy(void *queue __rte_unused)
+{
+	return -ENOTSUP;
+}
+
+RTE_EXPORT_INTERNAL_SYMBOL(rte_eth_tx_queue_count_dummy)
+int
+rte_eth_tx_queue_count_dummy(void *queue __rte_unused)
+{
+	return -ENOTSUP;
+}
+
+RTE_EXPORT_INTERNAL_SYMBOL(rte_eth_descriptor_status_dummy)
+int
+rte_eth_descriptor_status_dummy(void *queue __rte_unused,
+		uint16_t offset __rte_unused)
+{
+	return -ENOTSUP;
+}
+
+RTE_EXPORT_INTERNAL_SYMBOL(rte_eth_recycle_tx_mbufs_reuse_dummy)
+uint16_t
+rte_eth_recycle_tx_mbufs_reuse_dummy(void *queue __rte_unused,
+		struct rte_eth_recycle_rxq_info *recycle_rxq_info __rte_unused)
+{
+	return 0;
+}
+
+RTE_EXPORT_INTERNAL_SYMBOL(rte_eth_recycle_rx_descriptors_refill_dummy)
+void
+rte_eth_recycle_rx_descriptors_refill_dummy(void *queue __rte_unused,
+		uint16_t nb __rte_unused)
+{
+
+}
+
 RTE_EXPORT_INTERNAL_SYMBOL(rte_eth_representor_id_get)
 int
 rte_eth_representor_id_get(uint16_t port_id,
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 2b4d2ae9c3..ec00f16ed3 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1874,6 +1874,88 @@  rte_eth_pkt_burst_dummy(void *queue __rte_unused,
 		struct rte_mbuf **pkts __rte_unused,
 		uint16_t nb_pkts __rte_unused);
 
+/**
+ * @internal
+ * Dummy DPDK callback for Tx packet prepare.
+ *
+ * @param queue
+ *  Pointer to Tx queue
+ * @param pkts
+ *  Packet array
+ * @param nb_pkts
+ *  Number of packets in packet array
+ */
+__rte_internal
+uint16_t
+rte_eth_tx_pkt_prepare_dummy(void *queue __rte_unused,
+		struct rte_mbuf **pkts __rte_unused,
+		uint16_t nb_pkts __rte_unused);
+
+/**
+ * @internal
+ * Dummy DPDK callback for Rx queue count.
+ *
+ * @param queue
+ *  Pointer to Rx queue
+ */
+__rte_internal
+uint32_t
+rte_eth_rx_queue_count_dummy(void *queue __rte_unused);
+
+/**
+ * @internal
+ * Dummy DPDK callback for Tx queue count.
+ *
+ * @param queue
+ *  Pointer to Tx queue
+ */
+__rte_internal
+int
+rte_eth_tx_queue_count_dummy(void *queue __rte_unused);
+
+/**
+ * @internal
+ * Dummy DPDK callback for descriptor status.
+ *
+ * @param queue
+ *  Pointer to Rx/Tx queue
+ * @param offset
+ *  The offset of the descriptor starting from tail (0 is the next
+ *  packet to be received by the driver).
+ */
+__rte_internal
+int
+rte_eth_descriptor_status_dummy(void *queue __rte_unused,
+		uint16_t offset __rte_unused);
+
+/**
+ * @internal
+ * Dummy DPDK callback for recycle Tx mbufs reuse.
+ *
+ * @param queue
+ *  Pointer to Tx queue
+ * @param recycle_rxq_info
+ *  Pointer to recycle Rx queue info
+ */
+__rte_internal
+uint16_t
+rte_eth_recycle_tx_mbufs_reuse_dummy(void *queue __rte_unused,
+		struct rte_eth_recycle_rxq_info *recycle_rxq_info __rte_unused);
+
+/**
+ * @internal
+ * Dummy DPDK callback Rx descriptor refill.
+ *
+ * @param queue
+ *  Pointer Rx queue
+ * @param offset
+ *  number of descriptors to refill
+ */
+__rte_internal
+void
+rte_eth_recycle_rx_descriptors_refill_dummy(void *queue __rte_unused,
+		uint16_t nb __rte_unused);
+
 /**
  * Allocate an unique switch domain identifier.
  *
diff --git a/lib/ethdev/ethdev_pci.h b/lib/ethdev/ethdev_pci.h
index 2229ffa252..1bd49ab822 100644
--- a/lib/ethdev/ethdev_pci.h
+++ b/lib/ethdev/ethdev_pci.h
@@ -16,6 +16,20 @@ 
 extern "C" {
 #endif
 
+static inline void
+rte_eth_set_dummy_fops(struct rte_eth_dev *eth_dev)
+{
+	eth_dev->rx_pkt_burst = rte_eth_pkt_burst_dummy;
+	eth_dev->tx_pkt_burst = rte_eth_pkt_burst_dummy;
+	eth_dev->tx_pkt_prepare = rte_eth_tx_pkt_prepare_dummy;
+	eth_dev->rx_queue_count = rte_eth_rx_queue_count_dummy;
+	eth_dev->tx_queue_count = rte_eth_tx_queue_count_dummy;
+	eth_dev->rx_descriptor_status = rte_eth_descriptor_status_dummy;
+	eth_dev->tx_descriptor_status = rte_eth_descriptor_status_dummy;
+	eth_dev->recycle_tx_mbufs_reuse = rte_eth_recycle_tx_mbufs_reuse_dummy;
+	eth_dev->recycle_rx_descriptors_refill = rte_eth_recycle_rx_descriptors_refill_dummy;
+}
+
 /**
  * Copy pci device info to the Ethernet device data.
  * Shared memory (eth_dev->data) only updated by primary process, so it is safe
@@ -147,6 +161,11 @@  rte_eth_dev_pci_generic_probe(struct rte_pci_device *pci_dev,
 	if (!eth_dev)
 		return -ENOMEM;
 
+	/* Update fast path ops with dummy callbacks. Driver will update
+	 * them with required callbacks in the init function.
+	 */
+	rte_eth_set_dummy_fops(eth_dev);
+
 	ret = dev_init(eth_dev);
 	if (ret)
 		rte_eth_dev_release_port(eth_dev);
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index ea7f8c4a1a..aa67b69134 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -6399,8 +6399,6 @@  rte_eth_rx_queue_count(uint16_t port_id, uint16_t queue_id)
 		return -EINVAL;
 #endif
 
-	if (p->rx_queue_count == NULL)
-		return -ENOTSUP;
 	return (int)p->rx_queue_count(qd);
 }
 
@@ -6471,8 +6469,6 @@  rte_eth_rx_descriptor_status(uint16_t port_id, uint16_t queue_id,
 	if (qd == NULL)
 		return -ENODEV;
 #endif
-	if (p->rx_descriptor_status == NULL)
-		return -ENOTSUP;
 	return p->rx_descriptor_status(qd, offset);
 }
 
@@ -6542,8 +6538,6 @@  static inline int rte_eth_tx_descriptor_status(uint16_t port_id,
 	if (qd == NULL)
 		return -ENODEV;
 #endif
-	if (p->tx_descriptor_status == NULL)
-		return -ENOTSUP;
 	return p->tx_descriptor_status(qd, offset);
 }
 
@@ -6786,9 +6780,6 @@  rte_eth_tx_prepare(uint16_t port_id, uint16_t queue_id,
 	}
 #endif
 
-	if (!p->tx_pkt_prepare)
-		return nb_pkts;
-
 	return p->tx_pkt_prepare(qd, tx_pkts, nb_pkts);
 }
 
@@ -6985,8 +6976,6 @@  rte_eth_recycle_mbufs(uint16_t rx_port_id, uint16_t rx_queue_id,
 		return 0;
 	}
 #endif
-	if (p1->recycle_tx_mbufs_reuse == NULL)
-		return 0;
 
 #ifdef RTE_ETHDEV_DEBUG_RX
 	if (rx_port_id >= RTE_MAX_ETHPORTS ||
@@ -7010,8 +6999,6 @@  rte_eth_recycle_mbufs(uint16_t rx_port_id, uint16_t rx_queue_id,
 		return 0;
 	}
 #endif
-	if (p2->recycle_rx_descriptors_refill == NULL)
-		return 0;
 
 	/* Copy used *rte_mbuf* buffer pointers from Tx mbuf ring
 	 * into Rx mbuf ring.
@@ -7131,14 +7118,11 @@  rte_eth_tx_queue_count(uint16_t port_id, uint16_t queue_id)
 		goto out;
 	}
 #endif
-	if (fops->tx_queue_count == NULL) {
-		rc = -ENOTSUP;
-		goto out;
-	}
-
 	rc = fops->tx_queue_count(qd);
 
+#ifdef RTE_ETHDEV_DEBUG_TX
 out:
+#endif
 	rte_eth_trace_tx_queue_count(port_id, queue_id, rc);
 	return rc;
 }