25/10/2019 16:08, Ferruh Yigit:
> On 10/25/2019 10:36 AM, Thomas Monjalon wrote:
> > 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.
>
> It has been discussed in the mail list and went through multiple discussions,
> patch is out since the August, +1 to cc all maintainers I missed that part,
> but when the patch is reviewed and there is no objection, why block the merge?
I'm not saying blocking the merge.
My bad is that I missed the patch and I am asking for help with a notification
in this case. Same for Andrew I guess.
Note: it is merged in master and I am looking to improve this feature.
> > 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?
> >
> >> +
> >> + /**< 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.
>
> It is used in some drivers as simple scalar path with most features are
> missing.
This is a weak definition...
> >> + 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.
>
> That was a comment by me, most PMDs doesn't support different vector path per
> queue, for those PMDs, to make application life easy PMD can say if it
> supports
> per queue or not with this flag.
> If PMD supports only per port data path, application doesn't need to call this
> per queue if it want.
It would require some doxygen explanations.
> >> +};
> >
> > 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.
>
> The main target of the API is to define the vector path, not all
> optimizations,
> so the number is limited.
>
> When there is a standardized output it can be easier to be consumed by the
> applications.
Why application needs this info to be standardized?
I think it should not. I am really against such standardization.
I think it would hurt more than help because there are a lot more
major infos than just knowing the ISA it is using.
I think such info should be used only for debugging or have a clue
about the expected performance.
You cannot standardize the expectations of a specific implementation.
> >> +/**
> >> + * 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?
>
> Again by a request from me, to not need to break the API if we need to add
> more
> thing in the future.
I would replace it with a string. This is the most flexible API.
> >> +/**
> >> + * 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.
