[dpdk-dev] [PATCH v1 3/3] ethdev: enhance the API for getting burst mode information

[Wang, Haiyue]

> -----Original Message-----
> From: Thomas Monjalon <thomas at monjalon.net>
> Sent: Monday, November 4, 2019 06:21
> To: Wang, Haiyue <haiyue.wang at intel.com>
> Cc: dev at dpdk.org; arybchenko at solarflare.com; Yigit, Ferruh <ferruh.yigit at intel.com>;
> jerinjacobk at gmail.com; Ye, Xiaolong <xiaolong.ye at intel.com>; Kinsella, Ray <ray.kinsella at intel.com>;
> Sun, Chenmin <chenmin.sun at intel.com>; Slava Ovsiienko <viacheslavo at mellanox.com>
> Subject: Re: [PATCH v1 3/3] ethdev: enhance the API for getting burst mode information
> 
> 03/11/2019 04:52, Wang, Haiyue:
> > Hi Thomas,
> >
> > Reply the missed:
> >
> > From: Thomas Monjalon <thomas at monjalon.net>
> > >
> > > 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.
> 
> So what means zero? neither scalar nor vector?
> 

No log output, since PMD doesn't want to say something.

> > 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 ...
> 
> I don't understand what is hard in parsing a bit.
> But I'm probably not skilled enough.
> 
> Also I would be interested to understand what means scalar exactly here?
> It is processing packets one by one?
> And vector, does it mean four packets at once?
> How can I differentiate a function which is processing packets two packets at once?
> 

About vector, What I know is : git log -p drivers/net/mlx5/mlx5_rxtx_vec_altivec.h

For scalar, what I know is the function written by pure C syntax.


> > > > +/**< 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.
> >  */
> 
> OK this is a better description, thanks.
> Indeed this parameter looks to be a good idea.
> If we would drop this bit-field, the per-queue bit could be
> a parameter of the function.
> 

I've no objection to drop bit-field. If the experimental code can have something good,
that's fine enough.