DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: "Wang, Haiyue" <haiyue.wang@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"arybchenko@solarflare.com" <arybchenko@solarflare.com>,
	"Yigit, Ferruh" <ferruh.yigit@intel.com>,
	"jerinjacobk@gmail.com" <jerinjacobk@gmail.com>,
	"Ye, Xiaolong" <xiaolong.ye@intel.com>,
	"Kinsella, Ray" <ray.kinsella@intel.com>,
	"Sun, Chenmin" <chenmin.sun@intel.com>,
	Slava Ovsiienko <viacheslavo@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH v1 3/3] ethdev: enhance the API for getting burst mode information
Date: Sun, 03 Nov 2019 23:20:49 +0100	[thread overview]
Message-ID: <65534969.fODW5VeCLa@xps> (raw)
In-Reply-To: <E3B9F2FDCB65864C82CD632F23D8AB8773D8DE7E@shsmsx102.ccr.corp.intel.com>

03/11/2019 04:52, Wang, Haiyue:
> Hi Thomas,
> 
> Reply the missed:
> 
> From: Thomas Monjalon <thomas@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?

> 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?

> > > +/**< 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.



  reply	other threads:[~2019-11-03 22:20 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-31 17:11 [dpdk-dev] [PATCH v1 1/3] net/ice: remove the specific burst mode bit set Haiyue Wang
2019-10-31 17:11 ` [dpdk-dev] [PATCH v1 2/3] net/i40e: " Haiyue Wang
2019-10-31 17:11 ` [dpdk-dev] [PATCH v1 3/3] ethdev: enhance the API for getting burst mode information Haiyue Wang
2019-11-01 22:46   ` Thomas Monjalon
2019-11-02  5:29     ` Wang, Haiyue
2019-11-02  6:55       ` Liu, Yu Y
2019-11-02  8:38         ` Slava Ovsiienko
2019-11-02 19:21           ` Damjan Marion (damarion)
2019-11-03  7:50             ` Slava Ovsiienko
2019-11-05 15:12               ` Ferruh Yigit
2019-11-05 15:54                 ` Stephen Hemminger
2019-11-03 20:46             ` Ray Kinsella
2019-11-03  2:34           ` Wang, Haiyue
2019-11-03  3:03             ` Wang, Haiyue
2019-11-03  8:59             ` Slava Ovsiienko
2019-11-03 11:38               ` Wang, Haiyue
2019-11-03 19:31                 ` Slava Ovsiienko
2019-11-04  1:01                   ` Wang, Haiyue
2019-11-02 18:31         ` Thomas Monjalon
2019-11-03  3:52     ` Wang, Haiyue
2019-11-03 22:20       ` Thomas Monjalon [this message]
2019-11-04  0:51         ` Wang, Haiyue

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=65534969.fODW5VeCLa@xps \
    --to=thomas@monjalon.net \
    --cc=arybchenko@solarflare.com \
    --cc=chenmin.sun@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=haiyue.wang@intel.com \
    --cc=jerinjacobk@gmail.com \
    --cc=ray.kinsella@intel.com \
    --cc=viacheslavo@mellanox.com \
    --cc=xiaolong.ye@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).