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

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 10/11/21 7:52 PM, Ananyev, Konstantin wrote:
> 
> 
>> 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?

Taking into account that some PMds would like to set dummy
pointers in some specifics conditions, I think "not ready"
is the best option here.

> 
> ...
> 
>>
>>> +	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?
> 

See [1]

[1] https://www.doxygen.nl/manual/grouping.html#memgroup