Hi Thomas,
Reply the missed:
> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Saturday, November 2, 2019 06:46
> To: Wang, Haiyue <haiyue.w...@intel.com>
> Cc: dev@dpdk.org; arybche...@solarflare.com; Yigit, Ferruh
> <ferruh.yi...@intel.com>;
> jerinjac...@gmail.com; Ye, Xiaolong <xiaolong...@intel.com>; Kinsella, Ray
> <ray.kinse...@intel.com>;
> Sun, Chenmin <chenmin....@intel.com>; Slava Ovsiienko
> <viachesl...@mellanox.com>
> Subject: Re: [PATCH v1 3/3] ethdev: enhance the API for getting burst mode
> information
>
> Thank you for trying to address comments done late.
>
> 31/10/2019 18:11, Haiyue Wang:
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > -enum rte_eth_burst_mode_option {
> > - RTE_ETH_BURST_SCALAR = (1 << 0),
> > - RTE_ETH_BURST_VECTOR = (1 << 1),
> > -
> > - /**< 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),
> > -
> > - RTE_ETH_BURST_PER_QUEUE = (1 << 19), /**< Support per queue burst */
> > -};
> > +#define RTE_ETH_BURST_SCALAR (1ULL << 0)
> > +#define RTE_ETH_BURST_VECTOR (1ULL << 1)
>
> Only one bit is needed: if it is not vector, it is scalar.
> I think you can remove the scalar bit.
>
1. 'options' can be all zero, so we can't assume it is 'scalar', so let's the
PMD
set it explicitly.
2. As replied before, it seems a little redundant, but a bit-opaque definition
will make string format easily, like a direct 'for' loop:
----------------------------------
static const struct {
uint64_t option;
const char *name;
} rte_burst_option_names[] = {
{ RTE_ETH_BURST_SCALAR, "Scalar" },
{ RTE_ETH_BURST_VECTOR, "Vector" },
{ RTE_ETH_BURST_ALTIVEC, "AltiVec" },
{ RTE_ETH_BURST_NEON, "Neon" },
----------------------------------
And in fact, only one vector type will be returned, but if use the bit save
definition
like register, there will be something like 'RTE_ETH_BURST_VERTOR_TYPE_MASK',
this will
make PMD and string format harder ...
> > +/**< bits[15:2] are reserved for each vector type */
>
> Why 15:2 instead of 2:15?
>
Changed as:
/**< bits 2 ~ 15 are reserved for each vector type */
> > +#define RTE_ETH_BURST_ALTIVEC (1ULL << 2)
> > +#define RTE_ETH_BURST_NEON (1ULL << 3)
> > +#define RTE_ETH_BURST_SSE (1ULL << 4)
> > +#define RTE_ETH_BURST_AVX2 (1ULL << 5)
> > +#define RTE_ETH_BURST_AVX512 (1ULL << 6)
>
> Of course, I still believe that giving a special treatment
> to vector instructions is wrong.
> You did not justify why it needs to be defined in bits
> instead of string. I am not asking again because anyway you
> don't really reply. I think you are executing an order you received
> and I don't want to blame you more.
> I suspect a real hidden issue in Intel CPUs that you try to mitigate.
> No need to reply to this comment.
> Anyway I will propose to replace this API in the next release.
>
> > +/**< Support per queue burst */
> > +#define RTE_ETH_BURST_PER_QUEUE (1ULL << 63)
>
> This comment is meaningless.
> If this bit has a usage, please explain how to use this bit
> in the comment.
>
Changes as:
/**< If the Rx/Tx queues have different burst mode description, the bit will be
* set by PMD, then the application can enumerate to retrive burst description
* for all PMD queues.
*/
