Re: [dpdk-dev] [PATCH v4 7/7] ethdev: hide eth dev related structures

[Thomas Monjalon <thomas@monjalon.net>]

05/10/2021 18:19, Ananyev, Konstantin:
> > 04/10/2021 15:56, Konstantin Ananyev:
> > > Move rte_eth_dev, rte_eth_dev_data, rte_eth_rxtx_callback and related
> > > data into private header (ethdev_driver.h).
> > [...]
> > > +/**
> > > + * @internal
> > > + * Structure used to hold information about the callbacks to be called for a
> > > + * queue on RX and TX.
> > > + */
> > > +struct rte_eth_rxtx_callback {
> > > +	struct rte_eth_rxtx_callback *next;
> > > +	union{
> > > +		rte_rx_callback_fn rx;
> > > +		rte_tx_callback_fn tx;
> > > +	} fn;
> > > +	void *param;
> > > +};
> > > +
> > > +/**
> > > + * @internal
> > > + * The generic data structure associated with each ethernet device.
> > > + *
> > > + * Pointers to burst-oriented packet receive and transmit functions are
> > > + * located at the beginning of the structure, along with the pointer to
> > > + * where all the data elements for the particular device are stored in shared
> > > + * memory. This split allows the function pointer and driver data to be per-
> > > + * process, while the actual configuration data for the device is shared.
> > > + */
> > > +struct rte_eth_dev {
> > > +	eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function. */
> > > +	eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function. */
> > > +	eth_tx_prep_t tx_pkt_prepare;
> > > +	/**< Pointer to 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. */
> > 
> > Why not using the new struct rte_eth_fp_ops?
> 
> We don't want to change each and every driver for this change.
> The idea beyond it:
> 1. PMDs keep to setup fast-path function pointers and related data 
>     inside rte_eth_dev struct in the same way they did it before.
> 2. Inside rte_eth_dev_start() and inside rte_eth_dev_probing_finish()
>    (for secondary process) we call eth_dev_fp_ops_setup, which
>    copies these function and data pointers into rte_eth_fp_ops[port_id].
> 3. Inside rte_eth_dev_stop() and inside rte_eth_dev_release_port()
>     we call eth_dev_fp_ops_reset(), which resets rte_eth_fp_ops[port_id]
>     into some dummy values.

OK please add this explanation in the commit log.

> > > +
> > > +	/**
> > > +	 * Next two fields are per-device data but *data is shared between
> > > +	 * primary and secondary processes and *process_private is per-process
> > > +	 * private. The second one is managed by PMDs if necessary.
> > > +	 */
> > > +	struct rte_eth_dev_data *data;  /**< Pointer to device data. */
> > 
> > We should mention that "data" is shared between processes.
> 
> I think the comment above states exactly that.
> In fact, it is just cut and paste from lib/ethdev/rte_ethdev_core.h to 
> lib/ethdev/ethdev_driver.h.

True, but it is confusing, and we cannot have 2 comments for the same field.
The sentence "Next two fields are per-device data" is useless.
Let's comment each field separately.

> > > +	void *process_private; /**< Pointer to per-process device data. */
> > > +	const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
> > > +	struct rte_device *device; /**< Backing device */
> > > +	struct rte_intr_handle *intr_handle; /**< Device interrupt handle */
> > > +	/** User application callbacks for NIC interrupts */
> > > +	struct rte_eth_dev_cb_list link_intr_cbs;
> > > +	/**
> > > +	 * User-supplied functions called from rx_burst to post-process
> > > +	 * received packets before passing them to the user
> > > +	 */
> > > +	struct rte_eth_rxtx_callback *post_rx_burst_cbs[RTE_MAX_QUEUES_PER_PORT];
> > > +	/**
> > > +	 * User-supplied functions called from tx_burst to pre-process
> > > +	 * received packets before passing them to the driver for transmission.
> > > +	 */
> > > +	struct rte_eth_rxtx_callback *pre_tx_burst_cbs[RTE_MAX_QUEUES_PER_PORT];
> > > +	enum rte_eth_dev_state state; /**< Flag indicating the port state */
> > > +	void *security_ctx; /**< Context for security ops */
> > > +
> > > +	uint64_t reserved_64s[4]; /**< Reserved for future fields */
> > > +	void *reserved_ptrs[4];   /**< Reserved for future fields */
> > > +} __rte_cache_aligned;
[...]
> > > +extern struct rte_eth_dev rte_eth_devices[];
> > 
> > Later we should add a function to configure the size of this array dynamically
> > in the early DPDK init stage.
> 
> After we will hide rte_eth_devices[] and friends, we should be able to do
> with them whatever we want.
> But I suppose, that should be a subject of separate patch/discussion,
> Probably not in 21.11 timeframe.  

Yes