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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

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