Re: [dpdk-dev] [PATCH v3 3/7] ethdev: copy ethdev 'fast' API into separate structure

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>]

>Copy public function pointers (rx_pkt_burst(), etc.) and related
>pointers to internal data from rte_eth_dev structure into a
>separate flat array. That array will remain in a public header.
>The intention here is to make rte_eth_dev and related structures
>internal.
>That should allow future possible changes to core eth_dev structures
>to be transparent to the user and help to avoid ABI/API breakages.
>The plan is to keep minimal part of data from rte_eth_dev public,
>so we still can use inline functions for 'fast' calls
>(like rte_eth_rx_burst(), etc.) to avoid/minimize slowdown.
>
>Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>---
> lib/ethdev/ethdev_private.c  | 52
>++++++++++++++++++++++++++++++++++++
> lib/ethdev/ethdev_private.h  |  7 +++++
> lib/ethdev/rte_ethdev.c      | 17 ++++++++++++
> lib/ethdev/rte_ethdev_core.h | 45
>+++++++++++++++++++++++++++++++
> 4 files changed, 121 insertions(+)
>

<snip>

>+void
>+eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
>+{
>+	static void *dummy_data[RTE_MAX_QUEUES_PER_PORT];
>+	static const struct rte_eth_fp_ops dummy_ops = {
>+		.rx_pkt_burst = dummy_eth_rx_burst,
>+		.tx_pkt_burst = dummy_eth_tx_burst,
>+		.rxq = {.data = dummy_data, .clbk = dummy_data,},
>+		.txq = {.data = dummy_data, .clbk = dummy_data,},
>+	};
>+
>+	*fpo = dummy_ops;
>+}
>+
>+void
>+eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
>+		const struct rte_eth_dev *dev)
>+{
>+	fpo->rx_pkt_burst = dev->rx_pkt_burst;
>+	fpo->tx_pkt_burst = dev->tx_pkt_burst;
>+	fpo->tx_pkt_prepare = dev->tx_pkt_prepare;
>+	fpo->rx_queue_count = dev->rx_queue_count;
>+	fpo->rx_descriptor_status = dev->rx_descriptor_status;
>+	fpo->tx_descriptor_status = dev->tx_descriptor_status;
>+
>+	fpo->rxq.data = dev->data->rx_queues;
>+	fpo->rxq.clbk = (void **)(uintptr_t)dev->post_rx_burst_cbs;
>+
>+	fpo->txq.data = dev->data->tx_queues;
>+	fpo->txq.clbk = (void **)(uintptr_t)dev->pre_tx_burst_cbs;
>+}
>diff --git a/lib/ethdev/ethdev_private.h b/lib/ethdev/ethdev_private.h
>index 3724429577..40333e7651 100644
>--- a/lib/ethdev/ethdev_private.h
>+++ b/lib/ethdev/ethdev_private.h
>@@ -26,4 +26,11 @@ eth_find_device(const struct rte_eth_dev
>*_start, rte_eth_cmp_t cmp,
> /* Parse devargs value for representor parameter. */
> int rte_eth_devargs_parse_representor_ports(char *str, void *data);
>
>+/* reset eth 'fast' API to dummy values */
>+void eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo);
>+
>+/* setup eth 'fast' API to ethdev values */
>+void eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
>+		const struct rte_eth_dev *dev);
>+
> #endif /* _ETH_PRIVATE_H_ */
>diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>index 424bc260fa..9fbb1bc3db 100644
>--- a/lib/ethdev/rte_ethdev.c
>+++ b/lib/ethdev/rte_ethdev.c
>@@ -44,6 +44,9 @@
> static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
> struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS];
>
>+/* public 'fast' API */
>+struct rte_eth_fp_ops rte_eth_fp_ops[RTE_MAX_ETHPORTS];
>+
> /* spinlock for eth device callbacks */
> static rte_spinlock_t eth_dev_cb_lock = RTE_SPINLOCK_INITIALIZER;
>
>@@ -1788,6 +1791,9 @@ rte_eth_dev_start(uint16_t port_id)
> 		(*dev->dev_ops->link_update)(dev, 0);
> 	}
>
>+	/* expose selection of PMD rx/tx function */
>+	eth_dev_fp_ops_setup(rte_eth_fp_ops + port_id, dev);
>+

Secondary process will not set these properly I believe as it might not
call start() if it does primary process ops will not be set.

One simple solution is to call ops_setup() around rte_eth_dev_attach_secondary()
but if application doesn't invoke start() on Primary the ops will not be set for it.


> 	rte_ethdev_trace_start(port_id);
> 	return 0;
> }
>@@ -1810,6 +1816,9 @@ rte_eth_dev_stop(uint16_t port_id)
> 		return 0;
> 	}
>
>+	/* point rx/tx functions to dummy ones */
>+	eth_dev_fp_ops_reset(rte_eth_fp_ops + port_id);
>+
> 	dev->data->dev_started = 0;
> 	ret = (*dev->dev_ops->dev_stop)(dev);
> 	rte_ethdev_trace_stop(port_id, ret);
>2.26.3