Re: [dpdk-dev] [PATCH v5 4/7] ethdev: copy fast-path API into separate structure

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

> On 10/7/21 2:27 PM, Konstantin Ananyev wrote:
> > 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-path calls
> > (like rte_eth_rx_burst(), etc.) to avoid/minimize slowdown.
> > The whole idea beyond this new schema:
> > 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.
> > 4. fast-path ethdev API (rte_eth_rx_burst(), etc.) will use that new
> >    flat array to call PMD specific functions.
> > That approach should allow us to make rte_eth_devices[] private
> > without introducing regression and help to avoid changes in drivers code.
> >
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> Overall LGTM, few nits below.
> 
> > ---
> >  lib/ethdev/ethdev_private.c  | 52 ++++++++++++++++++++++++++++++++++
> >  lib/ethdev/ethdev_private.h  |  7 +++++
> >  lib/ethdev/rte_ethdev.c      | 27 ++++++++++++++++++
> >  lib/ethdev/rte_ethdev_core.h | 55 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 141 insertions(+)
> >
> > diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
> > index 012cf73ca2..3eeda6e9f9 100644
> > --- a/lib/ethdev/ethdev_private.c
> > +++ b/lib/ethdev/ethdev_private.c
> > @@ -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");
> 
> May be "unconfigured" -> "stopped" ? Or "non-started" ?

Yes, it can be configured but not started.
So 'not started' seems like a better wording here.
Another option probably: 'not ready'.
What people think?

...

> 
> > +	rte_errno = ENOTSUP;
> > +	return 0;
> > +}
> > +
> > +struct rte_eth_fp_ops {
> > +
> > +	/**
> > +	 * Rx fast-path functions and related data.
> > +	 * 64-bit systems: occupies first 64B line
> > +	 */
> 
> As I understand the above comment is for a group of below
> fields. If so, Doxygen annocation for member groups should
> be used.

Ok, and how to do it?