Hi Ferruh,
>
> Overall this enables us hiding the ethdev internals, which is good. But it
> duplicates most of the datapath function (rx burst for this patch) per each
> PMD ops.
Yes, same as right now rte_eth_rx/tx_burst() code can be duplicated in dozen
places
inside user-level code. And as any other 'static inline' function that we
define and use inside DPDK.
Personally I don't see why it is a problem.
>
> I wonder if we can have the callbacks ('_rte_eth_rx_epilog()') as separate
> function, this still enables us to hide the structs. Of course additional
> function call will bring some overhead, but if we enabled callbacks and
> calling
> them per packet, do we really care about additional function call?
Callbacks are not per packet, but per burst of packets - same as actual RX/TX.
A drawback with such approach - we either have to keep
post_rx_burst_cbs[RTE_MAX_QUEUES_PER_PORT] visible to the user
(which I'd prefer not to), or call epilolg() unconditionally - which means
performance drop.
>
> > Signed-off-by: Konstantin Ananyev <konstantin.anan...@intel.com>
>
> <...>
>
> > @@ -3229,7 +3289,7 @@ int
> > ice_rx_burst_mode_get(struct rte_eth_dev *dev, __rte_unused uint16_t
> > queue_id,
> > struct rte_eth_burst_mode *mode)
> > {
> > - eth_rx_burst_t pkt_burst = dev->rx_pkt_burst;
> > + rte_eth_rx_burst_t pkt_burst = rte_eth_get_rx_burst(dev->data->port_id);
>
> Does it makes easier to orginanise the patchset to have a separate patch to
> switch first to 'rte_eth_get_rx_burst()' / 'rte_eth_set_rx_burst()' with old
> implementation ('dev->rx_pkt_burst' get/set), and later just change the
> 'rte_eth_get_rx_burst()' / 'rte_eth_set_rx_burst()' implementation when
> structure is updated.
This is doable, don't know would be there any benefit from that or not.
>
> <...>
>
> > --- a/drivers/net/ice/ice_rxtx_vec_sse.c
> > +++ b/drivers/net/ice/ice_rxtx_vec_sse.c
> > @@ -587,13 +587,15 @@ _ice_recv_raw_pkts_vec(struct ice_rx_queue *rxq,
> > struct rte_mbuf **rx_pkts,
> > * - nb_pkts > ICE_VPMD_RX_BURST, only scan ICE_VPMD_RX_BURST
> > * numbers of DD bits
> > */
> > -uint16_t
> > +static inline uint16_t
>
> These functions eventually will be called via a function pointer, so is there
> a
> benefit to request them to 'inline', why not just 'static' ?
Agree.
> <...>
>
> > +_RTE_ETH_RX_DEF(ice_recv_scattered_pkts_vec)
> > +
>
> This will duplicate most of the Rx burst function for each PMD Rx ops.
>
> <...>
>
> > +
> > +#define _RTE_ETH_FUNC(fn) _rte_eth_##fn
> > +
>
> Do we need this macro? The functions are still 'static', so they won't be
> visible to application and there won't be a namespace problem.
Not all RX/TX burst functions are defined as 'static'.
> Dropping and just use the original fucntion name may reduce the changes in the
> drivers.
It allows to keep existing RX/TX functions intact - no need to change
prototype, add prolog/epilog, etc. manually.
Instead these macros help to create a wrapper functions around existing ones,
that will
become new public entry points.
All that should help to make changes faster and in a safer manner.
Though these macros are just helper ones to simplify the transition.
if someone will prefer to make changes in all their RX/TX function by hand -
that is still possible.
> <...>
>
> > +__rte_experimental
> > +rte_eth_rx_burst_t rte_eth_get_rx_burst(uint16_t port_id);
> > +
> > +__rte_experimental
> > +int rte_eth_set_rx_burst(uint16_t port_id, rte_eth_rx_burst_t rxf);
>
> can s/__rte_experimental/__rte_internal/
OK.
> <...>
>
> > +
> > +__rte_experimental
> > +rte_eth_rx_burst_t
> > +rte_eth_get_rx_burst(uint16_t port_id)
> > +{
> > + if (port_id >= RTE_DIM(rte_eth_burst_api)) {
> > + rte_errno = EINVAL;
> > + return NULL;
> > + }
> > + return rte_eth_burst_api[port_id].rx_pkt_burst;
> > +}
> > +
> > +__rte_experimental
> > +int
> > +rte_eth_set_rx_burst(uint16_t port_id, rte_eth_rx_burst_t rxf)
> > +{
> > + if (port_id >= RTE_DIM(rte_eth_burst_api))
> > + return -EINVAL;
> > +
> > + rte_eth_burst_api[port_id].rx_pkt_burst = rxf;
> > + return 0;
> > +}
>
> Since these are internal functions for drivers, it can be easier for drivers
> to
> use directly with 'struct rte_eth_dev *eth_dev', instead of 'port_id'.
>
> So instead of APIs getting 'port_id' as parameter, they can get 'struct
> rte_eth_dev *eth_dev'? Drivers for sure will have 'eth_dev' references for
> their
> device.
I am fine either way - it is a control path internal function.
> Overall, I think make sense for all public APIs to have handler ('port_id') as
> parameter, and all driver APIs to have 'eth_device' as paramter.
>
> <...>
>
> > @@ -4981,44 +4981,11 @@ static inline uint16_t
> > rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> > struct rte_mbuf **rx_pkts, const uint16_t nb_pkts)
> > {
> > - struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > - uint16_t nb_rx;
> > -
> > -#ifdef RTE_ETHDEV_DEBUG_RX
> > - RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > - RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);
> > -
> > - if (queue_id >= dev->data->nb_rx_queues) {
> > - RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", queue_id);
> > + if (port_id >= RTE_MAX_ETHPORTS)
>
> As an API, it makes sense to validate the input. But not sure to add these
> checks as part of this set, as previous versions don't have it. Perhaps we can
> add them with separate patch and discussion.
You mean 'if (port_id >= RTE_MAX_ETHPORTS)'?
No, this check is not present in current version.
Obviously, it can be removed, though I think it would be good to have it:
will help to keep thigs less error-prone.
I don’t think it would really impact the performance, in some cases
compiler will even be able to optimise out such check.
> <...>
>
> > +++ b/lib/ethdev/version.map
> > @@ -249,6 +249,11 @@ EXPERIMENTAL {
> > rte_mtr_meter_policy_delete;
> > rte_mtr_meter_policy_update;
> > rte_mtr_meter_policy_validate;
> > +
> > + # added in 21.11
> > + rte_eth_burst_api;
> > + rte_eth_get_rx_burst;
> > + rte_eth_set_rx_burst;
>
> I think these APIs intented to use only by drivers, so instead of
> experimental,
> they can be added as 'INTERNAL'.
OK.
