Hi Thomas & Andrew,
> -----Original Message-----
> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> Sent: Friday, October 25, 2019 17:37
> To: Wang, Haiyue <haiyue.w...@intel.com>; Yigit, Ferruh
> <ferruh.yi...@intel.com>
> Cc: dev@dpdk.org; Ye, Xiaolong <xiaolong...@intel.com>; Kinsella, Ray
> <ray.kinse...@intel.com>;
> Iremonger, Bernard <bernard.iremon...@intel.com>; Sun, Chenmin
> <chenmin....@intel.com>;
> arybche...@solarflare.com; viachesl...@mellanox.com;
> step...@networkplumber.org;
> david.march...@redhat.com; jer...@marvell.com
> Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add the API for getting burst
> mode information
>
> 15/10/2019 09:51, Haiyue Wang:
> > Some PMDs have more than one RX/TX burst paths, add the ethdev API
> > that allows an application to retrieve the mode information about
> > Rx/Tx packet burst such as Scalar or Vector, and Vector technology
> > like AVX2.
>
> I missed this patch. I and Andrew, maintainers of ethdev, were not CC'ed.
> Ferruh, I would expect to be Cc'ed and/or get a notification before merging.
> Hopefully it is not too late to fix this API before releasing 19.11.
>
> I think the idea of getting infos from PMD internal mode is not bad.
> But I strongly disagree with standardizing the names. More below.
>
> [...]
> > +enum rte_eth_burst_mode_option {
> > + RTE_ETH_BURST_SCALAR = (1 << 0),
> > + RTE_ETH_BURST_VECTOR = (1 << 1),
>
> 2 bits for a boolean value?
>
They have the Boolean value, but prefer to keep bit opaque, then can
loop to access all the bits ...
> > +
> > + /**< bits[15:2] are reserved for each vector type */
> > + RTE_ETH_BURST_ALTIVEC = (1 << 2),
> > + RTE_ETH_BURST_NEON = (1 << 3),
> > + RTE_ETH_BURST_SSE = (1 << 4),
> > + RTE_ETH_BURST_AVX2 = (1 << 5),
> > + RTE_ETH_BURST_AVX512 = (1 << 6),
> > +
> > + RTE_ETH_BURST_SCATTERED = (1 << 16), /**< Support scattered packets */
> > + RTE_ETH_BURST_BULK_ALLOC = (1 << 17), /**< Support mbuf bulk alloc */
> > + RTE_ETH_BURST_SIMPLE = (1 << 18),
>
> What means simple? Looks meaningless.
>
The below is the 'simple' code, yes, not a good name, will try to extract
more generic name.
/* Use a simple Tx queue if possible (only fast free is allowed) */
ad->tx_simple_allowed =
(txq->offloads ==
(txq->offloads & DEV_TX_OFFLOAD_MBUF_FAST_FREE) &&
txq->tx_rs_thresh >= ICE_TX_MAX_BURST);
> > + RTE_ETH_BURST_PER_QUEUE = (1 << 19), /**< Support per queue burst */
>
> What is per-queue burst? No need to add a comment if not adding any info.
> The burst API is *already* per-queue.
>
Not for this *burst API* per-queue, but for 'dev->rx/tx_pkt_burst', if the
PMD can do burst selection per-queue internally, like NOT ALL VECTOR / SCALAR.
> > +};
>
> How can we imagine standardizing the PMD optimizations?
> PMD developers are free to have as many burst implementation as they want.
> If we want to report info about what is used, it can be only a free string.
>
> > +/**
> > + * Ethernet device RX/TX queue packet burst mode information structure.
> > + * Used to retrieve information about packet burst mode setting.
> > + */
> > +struct rte_eth_burst_mode {
> > + uint64_t options;
> > +};
>
> Why a struct for an integer?
>
For expanding the information for mode, don't have to change the API parameters.
> > +/**
> > + * Retrieve information about the Rx packet burst mode.
> > + *
> > + * @param port_id
> > + * The port identifier of the Ethernet device.
> > + * @param queue_id
> > + * The Rx queue on the Ethernet device for which information
> > + * will be retrieved.
> > + * @param mode
> > + * A pointer to a structure of type *rte_eth_burst_mode* to be filled
> > + * with the information of the packet burst mode.
>
> No reference to the enum rte_eth_burst_mode_option or RTE_ETH_BURST_ prefix?
>
> > + *
> > + * @return
> > + * - 0: Success
> > + * - -ENOTSUP: routine is not supported by the device PMD.
> > + * - -EINVAL: The port_id or the queue_id is out of range.
> > + */
> > +__rte_experimental
> > +int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
> > + struct rte_eth_burst_mode *mode);
> [...]
> > +/**
> > + * Retrieve name about burst mode option.
> > + *
> > + * @param mode
> > + * The burst mode option of type *rte_eth_burst_mode_option*.
> > + *
> > + * @return
> > + * - "": Not found
> > + * - "xxx": name of the mode option.
> > + */
> > +__rte_experimental
> > +const char *
> > +rte_eth_burst_mode_option_name(uint64_t option);
>
> rte_eth_burst_mode_name would be a better function name.
> But anyway, this function should not exist.
> The string should be freely returned by the PMD
> in the burst_mode_get functions.
>
Design like : common part (options: vector, AVX2 etc) + device specific ?
struct rte_eth_burst_mode {
uint64_t options;
char dev_specific[128]; /* PMD has specific burst mode information */
};
Then rte_eth_burst_mode_option_name is good name to keep its small scope
on *options* stringification.
