[v4,3/7] ethdev: copy ethdev 'fast' API into separate structure
Checks
Commit Message
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 | 27 +++++++++++++++++++
lib/ethdev/rte_ethdev_core.h | 45 +++++++++++++++++++++++++++++++
4 files changed, 131 insertions(+)
Comments
04/10/2021 15:55, Konstantin Ananyev:
> 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.
I don't understand why 'fast' is quoted.
It looks strange.
> +/* 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);
I assume "fp" stands for fast path.
Please write "fast path" completely in the comments.
> + /* expose selection of PMD rx/tx function */
> + eth_dev_fp_ops_setup(rte_eth_fp_ops + port_id, dev);
[...]
> + /* point rx/tx functions to dummy ones */
> + eth_dev_fp_ops_reset(rte_eth_fp_ops + port_id);
Nit: Rx/Tx
or could be "fast path", to be consistent.
> + /*
> + * for secondary process, at that point we expect device
> + * to be already 'usable', so shared data and all function pointers
> + * for 'fast' devops have to be setup properly inside rte_eth_dev.
> + */
> + if (rte_eal_process_type() == RTE_PROC_SECONDARY)
> + eth_dev_fp_ops_setup(rte_eth_fp_ops + dev->data->port_id, dev);
> +
> rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
>
> dev->state = RTE_ETH_DEV_ATTACHED;
> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
> index 948c0b71c1..fe47a660c7 100644
> --- a/lib/ethdev/rte_ethdev_core.h
> +++ b/lib/ethdev/rte_ethdev_core.h
> @@ -53,6 +53,51 @@ typedef int (*eth_rx_descriptor_status_t)(void *rxq, uint16_t offset);
> typedef int (*eth_tx_descriptor_status_t)(void *txq, uint16_t offset);
> /**< @internal Check the status of a Tx descriptor */
>
> +/**
> + * @internal
> + * Structure used to hold opaque pointernals to internal ethdev RX/TXi
typos in above line
> + * queues data.
> + * The main purpose to expose these pointers at all - allow compiler
> + * to fetch this data for 'fast' ethdev inline functions in advance.
> + */
> +struct rte_ethdev_qdata {
> + void **data;
> + /**< points to array of internal queue data pointers */
> + void **clbk;
> + /**< points to array of queue callback data pointers */
> +};
> +
> +/**
> + * @internal
> + * 'fast' ethdev funcions and related data are hold in a flat array.
> + * one entry per ethdev.
> + */
> +struct rte_eth_fp_ops {
> +
> + /** first 64B line */
> + eth_rx_burst_t rx_pkt_burst;
> + /**< PMD receive function. */
> + eth_tx_burst_t tx_pkt_burst;
> + /**< PMD transmit function. */
> + eth_tx_prep_t tx_pkt_prepare;
> + /**< PMD transmit prepare function. */
> + eth_rx_queue_count_t rx_queue_count;
> + /**< Get the number of used RX descriptors. */
> + eth_rx_descriptor_status_t rx_descriptor_status;
> + /**< Check the status of a Rx descriptor. */
> + eth_tx_descriptor_status_t tx_descriptor_status;
> + /**< Check the status of a Tx descriptor. */
> + uintptr_t reserved[2];
uintptr_t size is not fix.
I think you mean uint64_t.
> +
> + /** second 64B line */
> + struct rte_ethdev_qdata rxq;
> + struct rte_ethdev_qdata txq;
> + uintptr_t reserved2[4];
> +
> +} __rte_cache_aligned;
> +
> +extern struct rte_eth_fp_ops rte_eth_fp_ops[RTE_MAX_ETHPORTS];
> 04/10/2021 15:55, Konstantin Ananyev:
> > 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.
>
> I don't understand why 'fast' is quoted.
> It looks strange.
>
>
> > +/* 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);
>
> I assume "fp" stands for fast path.
Yes.
> Please write "fast path" completely in the comments.
Ok.
> > + /* expose selection of PMD rx/tx function */
> > + eth_dev_fp_ops_setup(rte_eth_fp_ops + port_id, dev);
> [...]
> > + /* point rx/tx functions to dummy ones */
> > + eth_dev_fp_ops_reset(rte_eth_fp_ops + port_id);
>
> Nit: Rx/Tx
> or could be "fast path", to be consistent.
>
> > + /*
> > + * for secondary process, at that point we expect device
> > + * to be already 'usable', so shared data and all function pointers
> > + * for 'fast' devops have to be setup properly inside rte_eth_dev.
> > + */
> > + if (rte_eal_process_type() == RTE_PROC_SECONDARY)
> > + eth_dev_fp_ops_setup(rte_eth_fp_ops + dev->data->port_id, dev);
> > +
> > rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
> >
> > dev->state = RTE_ETH_DEV_ATTACHED;
> > diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
> > index 948c0b71c1..fe47a660c7 100644
> > --- a/lib/ethdev/rte_ethdev_core.h
> > +++ b/lib/ethdev/rte_ethdev_core.h
> > @@ -53,6 +53,51 @@ typedef int (*eth_rx_descriptor_status_t)(void *rxq, uint16_t offset);
> > typedef int (*eth_tx_descriptor_status_t)(void *txq, uint16_t offset);
> > /**< @internal Check the status of a Tx descriptor */
> >
> > +/**
> > + * @internal
> > + * Structure used to hold opaque pointernals to internal ethdev RX/TXi
>
> typos in above line
>
> > + * queues data.
> > + * The main purpose to expose these pointers at all - allow compiler
> > + * to fetch this data for 'fast' ethdev inline functions in advance.
> > + */
> > +struct rte_ethdev_qdata {
> > + void **data;
> > + /**< points to array of internal queue data pointers */
> > + void **clbk;
> > + /**< points to array of queue callback data pointers */
> > +};
> > +
> > +/**
> > + * @internal
> > + * 'fast' ethdev funcions and related data are hold in a flat array.
> > + * one entry per ethdev.
> > + */
> > +struct rte_eth_fp_ops {
> > +
> > + /** first 64B line */
> > + eth_rx_burst_t rx_pkt_burst;
> > + /**< PMD receive function. */
> > + eth_tx_burst_t tx_pkt_burst;
> > + /**< PMD transmit function. */
> > + eth_tx_prep_t tx_pkt_prepare;
> > + /**< PMD transmit prepare function. */
> > + eth_rx_queue_count_t rx_queue_count;
> > + /**< Get the number of used RX descriptors. */
> > + eth_rx_descriptor_status_t rx_descriptor_status;
> > + /**< Check the status of a Rx descriptor. */
> > + eth_tx_descriptor_status_t tx_descriptor_status;
> > + /**< Check the status of a Tx descriptor. */
> > + uintptr_t reserved[2];
>
> uintptr_t size is not fix.
> I think you mean uint64_t.
Nope, I meant 'uintptr_t' here.
That way it fits really nicely to both 64-bit and 32-bit systems.
For 64-bit systems we have all function pointers on first 64B line,
and all data pointers on second 64B line.
For 32-bit systems we have all fields within first 64B line.
> > +
> > + /** second 64B line */
> > + struct rte_ethdev_qdata rxq;
> > + struct rte_ethdev_qdata txq;
> > + uintptr_t reserved2[4];
> > +
> > +} __rte_cache_aligned;
> > +
> > +extern struct rte_eth_fp_ops rte_eth_fp_ops[RTE_MAX_ETHPORTS];
>
>
05/10/2021 18:41, Ananyev, Konstantin:
> > > +struct rte_eth_fp_ops {
> > > +
> > > + /** first 64B line */
> > > + eth_rx_burst_t rx_pkt_burst;
> > > + /**< PMD receive function. */
> > > + eth_tx_burst_t tx_pkt_burst;
> > > + /**< PMD transmit function. */
> > > + eth_tx_prep_t tx_pkt_prepare;
> > > + /**< PMD transmit prepare function. */
> > > + eth_rx_queue_count_t rx_queue_count;
> > > + /**< Get the number of used RX descriptors. */
> > > + eth_rx_descriptor_status_t rx_descriptor_status;
> > > + /**< Check the status of a Rx descriptor. */
> > > + eth_tx_descriptor_status_t tx_descriptor_status;
> > > + /**< Check the status of a Tx descriptor. */
> > > + uintptr_t reserved[2];
> >
> > uintptr_t size is not fix.
> > I think you mean uint64_t.
>
> Nope, I meant 'uintptr_t' here.
> That way it fits really nicely to both 64-bit and 32-bit systems.
> For 64-bit systems we have all function pointers on first 64B line,
> and all data pointers on second 64B line.
> For 32-bit systems we have all fields within first 64B line.
OK but then the next comment is partially wrong:
> > > +
> > > + /** second 64B line */
> > > + struct rte_ethdev_qdata rxq;
> > > + struct rte_ethdev_qdata txq;
> > > + uintptr_t reserved2[4];
> > > +
> > > +} __rte_cache_aligned;
> > > > +struct rte_eth_fp_ops {
> > > > +
> > > > + /** first 64B line */
> > > > + eth_rx_burst_t rx_pkt_burst;
> > > > + /**< PMD receive function. */
> > > > + eth_tx_burst_t tx_pkt_burst;
> > > > + /**< PMD transmit function. */
> > > > + eth_tx_prep_t tx_pkt_prepare;
> > > > + /**< PMD transmit prepare function. */
> > > > + eth_rx_queue_count_t rx_queue_count;
> > > > + /**< Get the number of used RX descriptors. */
> > > > + eth_rx_descriptor_status_t rx_descriptor_status;
> > > > + /**< Check the status of a Rx descriptor. */
> > > > + eth_tx_descriptor_status_t tx_descriptor_status;
> > > > + /**< Check the status of a Tx descriptor. */
> > > > + uintptr_t reserved[2];
> > >
> > > uintptr_t size is not fix.
> > > I think you mean uint64_t.
> >
> > Nope, I meant 'uintptr_t' here.
> > That way it fits really nicely to both 64-bit and 32-bit systems.
> > For 64-bit systems we have all function pointers on first 64B line,
> > and all data pointers on second 64B line.
> > For 32-bit systems we have all fields within first 64B line.
>
> OK but then the next comment is partially wrong:
True.
In fact, after I replied to you, just thought that might be
better to have first 64B line for RX functions and data,
second 64B line for TX functions and data.
Will probably give it a try.
Anyway, will update the comment.
>
> > > > +
> > > > + /** second 64B line */
> > > > + struct rte_ethdev_qdata rxq;
> > > > + struct rte_ethdev_qdata txq;
> > > > + uintptr_t reserved2[4];
> > > > +
> > > > +} __rte_cache_aligned;
>
>
@@ -174,3 +174,55 @@ rte_eth_devargs_parse_representor_ports(char *str, void *data)
RTE_LOG(ERR, EAL, "wrong representor format: %s\n", str);
return str == NULL ? -1 : 0;
}
+
+static uint16_t
+dummy_eth_rx_burst(__rte_unused void *rxq,
+ __rte_unused struct rte_mbuf **rx_pkts,
+ __rte_unused uint16_t nb_pkts)
+{
+ RTE_ETHDEV_LOG(ERR, "rx_pkt_burst for unconfigured port\n");
+ rte_errno = ENOTSUP;
+ return 0;
+}
+
+static uint16_t
+dummy_eth_tx_burst(__rte_unused void *txq,
+ __rte_unused struct rte_mbuf **tx_pkts,
+ __rte_unused uint16_t nb_pkts)
+{
+ RTE_ETHDEV_LOG(ERR, "tx_pkt_burst for unconfigured port\n");
+ rte_errno = ENOTSUP;
+ return 0;
+}
+
+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;
+}
@@ -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_ */
@@ -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;
@@ -578,6 +581,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
rte_eth_dev_callback_process(eth_dev,
RTE_ETH_EVENT_DESTROY, NULL);
+ eth_dev_fp_ops_reset(rte_eth_fp_ops + eth_dev->data->port_id);
+
rte_spinlock_lock(ð_dev_shared_data->ownership_lock);
eth_dev->state = RTE_ETH_DEV_UNUSED;
@@ -1788,6 +1793,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);
+
rte_ethdev_trace_start(port_id);
return 0;
}
@@ -1810,6 +1818,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);
@@ -4568,6 +4579,14 @@ rte_eth_mirror_rule_reset(uint16_t port_id, uint8_t rule_id)
return eth_err(port_id, (*dev->dev_ops->mirror_rule_reset)(dev, rule_id));
}
+RTE_INIT(eth_dev_init_fp_ops)
+{
+ uint32_t i;
+
+ for (i = 0; i != RTE_DIM(rte_eth_fp_ops); i++)
+ eth_dev_fp_ops_reset(rte_eth_fp_ops + i);
+}
+
RTE_INIT(eth_dev_init_cb_lists)
{
uint16_t i;
@@ -4736,6 +4755,14 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
if (dev == NULL)
return;
+ /*
+ * for secondary process, at that point we expect device
+ * to be already 'usable', so shared data and all function pointers
+ * for 'fast' devops have to be setup properly inside rte_eth_dev.
+ */
+ if (rte_eal_process_type() == RTE_PROC_SECONDARY)
+ eth_dev_fp_ops_setup(rte_eth_fp_ops + dev->data->port_id, dev);
+
rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
dev->state = RTE_ETH_DEV_ATTACHED;
@@ -53,6 +53,51 @@ typedef int (*eth_rx_descriptor_status_t)(void *rxq, uint16_t offset);
typedef int (*eth_tx_descriptor_status_t)(void *txq, uint16_t offset);
/**< @internal Check the status of a Tx descriptor */
+/**
+ * @internal
+ * Structure used to hold opaque pointernals to internal ethdev RX/TXi
+ * queues data.
+ * The main purpose to expose these pointers at all - allow compiler
+ * to fetch this data for 'fast' ethdev inline functions in advance.
+ */
+struct rte_ethdev_qdata {
+ void **data;
+ /**< points to array of internal queue data pointers */
+ void **clbk;
+ /**< points to array of queue callback data pointers */
+};
+
+/**
+ * @internal
+ * 'fast' ethdev funcions and related data are hold in a flat array.
+ * one entry per ethdev.
+ */
+struct rte_eth_fp_ops {
+
+ /** first 64B line */
+ eth_rx_burst_t rx_pkt_burst;
+ /**< PMD receive function. */
+ eth_tx_burst_t tx_pkt_burst;
+ /**< PMD transmit function. */
+ eth_tx_prep_t tx_pkt_prepare;
+ /**< PMD transmit prepare function. */
+ eth_rx_queue_count_t rx_queue_count;
+ /**< Get the number of used RX descriptors. */
+ eth_rx_descriptor_status_t rx_descriptor_status;
+ /**< Check the status of a Rx descriptor. */
+ eth_tx_descriptor_status_t tx_descriptor_status;
+ /**< Check the status of a Tx descriptor. */
+ uintptr_t reserved[2];
+
+ /** second 64B line */
+ struct rte_ethdev_qdata rxq;
+ struct rte_ethdev_qdata txq;
+ uintptr_t reserved2[4];
+
+} __rte_cache_aligned;
+
+extern struct rte_eth_fp_ops rte_eth_fp_ops[RTE_MAX_ETHPORTS];
+
/**
* @internal