> 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];
>
>
