From: "Wang, Haiyue" <haiyue.wang@intel.com> To: "Yigit, Ferruh" <ferruh.yigit@intel.com>, "Richardson, Bruce" <bruce.richardson@intel.com> Cc: Ray Kinsella <mdr@ashroe.eu>, "dev@dpdk.org" <dev@dpdk.org>, "Sun, Chenmin" <chenmin.sun@intel.com> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information Date: Tue, 10 Sep 2019 11:41:38 +0000 Message-ID: <E3B9F2FDCB65864C82CD632F23D8AB8773D6895B@shsmsx102.ccr.corp.intel.com> (raw) In-Reply-To: <4672dfa1-a553-28fc-686e-1e14186a040a@intel.com> > -----Original Message----- > From: Yigit, Ferruh > Sent: Tuesday, September 10, 2019 17:15 > To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com> > Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com> > Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information > > On 9/10/2019 9:37 AM, Wang, Haiyue wrote: > >> -----Original Message----- > >> From: Yigit, Ferruh > >> Sent: Tuesday, September 10, 2019 16:07 > >> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com> > >> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com> > >> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information > >> > >> On 9/10/2019 5:36 AM, Wang, Haiyue wrote: > >>> Thanks Ferruh, Bruce. > >>> > >>>> -----Original Message----- > >>>> From: Yigit, Ferruh > >>>> Sent: Monday, September 9, 2019 21:18 > >>>> To: Richardson, Bruce <bruce.richardson@intel.com> > >>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, > Chenmin > >>>> <chenmin.sun@intel.com> > >>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information > >>>> > >>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote: > >>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote: > >>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote: > >>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote: > >>>>>>>>> -----Original Message----- > >>>>>>>>> From: Yigit, Ferruh > >>>>>>>>> Sent: Friday, September 6, 2019 22:22 > >>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; Wang, Haiyue <haiyue.wang@intel.com> > >>>>>>>>> Cc: dev@dpdk.org > >>>>>>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information > >>>>>>>>> > >>>>>>>>> On 8/13/2019 1:51 PM, Ray Kinsella wrote: > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> On 13/08/2019 04:24, Stephen Hemminger wrote: > >>>>>>>>>>> On Tue, 13 Aug 2019 11:06:10 +0800 > >>>>>>>>>>> Haiyue Wang <haiyue.wang@intel.com> wrote: > >>>>>>>>>>> > >>>>>>>>>>>> Enhance the PMD to support retrieving trace information like > >>>>>>>>>>>> Rx/Tx burst selection etc. > >>>>>>>>>>>> > >>>>>>>>>>>> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com> > >>>>>>>>>>>> --- > >>>>>>>>>>>> lib/librte_ethdev/rte_ethdev.c | 18 ++++++++++++++++++ > >>>>>>>>>>>> lib/librte_ethdev/rte_ethdev.h | 9 +++++++++ > >>>>>>>>>>>> lib/librte_ethdev/rte_ethdev_core.h | 4 ++++ > >>>>>>>>>>>> 3 files changed, 31 insertions(+) > >>>>>>>>>>>> > >>>>>>>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > >>>>>>>>>>>> index 17d183e..6098fad 100644 > >>>>>>>>>>>> --- a/lib/librte_ethdev/rte_ethdev.c > >>>>>>>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.c > >>>>>>>>>>>> @@ -4083,6 +4083,24 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id, > >>>>>>>>>>>> } > >>>>>>>>>>>> > >>>>>>>>>>>> int > >>>>>>>>>>>> +rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id, > >>>>>>>>>>>> + enum rte_eth_trace type, char *buf, int sz) > >>>>>>>>> > >>>>>>>>> Better to use struct as argument instead of individual variables because it is > >>>>>>>>> easier to extend the struct later if needed. > >>>>>>>>> > >>>>>>>>>>>> +{ > >>>>>>>>>>>> + struct rte_eth_dev *dev; > >>>>>>>>>>>> + > >>>>>>>>>>>> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > >>>>>>>>>>>> + > >>>>>>>>>>>> + if (buf == NULL) > >>>>>>>>>>>> + return -EINVAL; > >>>>>>>>>>>> + > >>>>>>>>>>>> + dev = &rte_eth_devices[port_id]; > >>>>>>>>>>>> + > >>>>>>>>>>>> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->trace_info_get, -ENOTSUP); > >>>>>>>>>>>> + > >>>>>>>>>>>> + return dev->dev_ops->trace_info_get(dev, queue_id, type, buf, sz); > >>>>>>>>>>> > >>>>>>>>>>> What if queueid is out of bounds? > >>>>>>>>>>> > >>>>>>>>>>> The bigger problem is that this information is like a log message > >>>>>>>>>>> and unstructured, which makes it device specific and useless for automation. > >>>>>>>>>> > >>>>>>>>>> IMHO - this is much better implemented as a capability bitfield, that > >>>>>>>>>> can be queried. > >>>>>>>>> > >>>>>>>>> +1 to return the datapath capability as bitfield. > >>>>>>>>> > >>>>>>>>> Also +1 to have a new API, > >>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find > >>>>>>>>> something better instead of 'trace' there. > >>>>>>>>> - I think we should limit this API only to get current datapath configuration, > >>>>>>>>> for clarity of the API don't return capability or not datapath related config. > >>>>>>>>> > >>>>>>>>> Also this information not always supported in queue level, what do you think > >>>>>>>>> having ability to get this information in port level, > >>>>>>>>> like this API can return a struct, which may have a field that says if the > >>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think? > >>>>>>>>> > >>>>>>>> > >>>>>>>> #define RX_SCALAR (1ULL < 0) > >>>>>>>> #define RX_VECTOR_AVX2 ... > >>>>>>> > >>>>>>> What about having RX_VECTOR value, later another bit group for the details of > >>>>>>> the vectorization: > >>>>>>> SSE > >>>>>>> AVX2 > >>>>>>> AVX512 > >>>>>>> NEON > >>>>>>> ALTIVEC > >>>>>>> > >>>>>>> Since above options can exist together, what about using values for them instead > >>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term. > >>>>>>> > >>>>>> Rather than having named vector types, we just need to worry about the ones > >>>>>> for the current architecture. Therefore I'd suggest just using vector > >>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting > >>>>>> multiple values, 16 combinations is not enough for all the possibilities. > >>>>>> > >>>>> > >>>>> vector width can be an option too, no objection there. But this is only for > >>>>> current configuration, so it can be a combination, we have now 5 types and > >>>>> allocating space for 16. > >>>>> > >>>> > >>>> correction: it can *not* be a combination > >>> > >>> I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector > >>> mode detail. And for vector width, the SSE, NEON name should indicates it ? > >>> > >>> I renamed the definitions to try to make things clear. > >>> > >>> enum rte_eth_burst_mode_option { > >>> BURST_SCALAR = (1 << 0), > >>> BURST_VECTOR = (1 << 1), > >>> > >>> BURST_VECTOR_MODE_MASK = (0x3F << 2), > >>> BURST_ALTIVEC = (1 << 2), > >>> BURST_NEON = (2 << 2), > >>> BURST_SSE = (3 << 2), > >>> BURST_AVX2 = (4 << 2), > >>> BURST_AVX512 = (5 << 2), > >> > >> Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5 > >> (inclusive) and use their value: > >> > >> BURST_VECTOR_MODE_IDX = 2 > >> BURST_VECTOR_MODE_SIZE = 4 > >> BURST_VECTOR_MODE_MASK = > >> ((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX > >> > >> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX > >> > >> if (vector_mode == 0) // BURST_SSE > >> if (vector_mode == 1) // BURST_AVX2 > >> if (vector_mode == 2) // BURST_AVX512 > >> if (vector_mode == 3) // BURST_NEON > >> .... > >> > >> Can any vector mode be combination of above, if not why use bitfields? > >> > > > > I use it as this to *set* ... > > > > else if (pkt_burst == i40e_recv_scattered_pkts_vec_avx2) > > options = BURST_VECTOR | BURST_AVX2 | BURST_SCATTERED; > > else if (pkt_burst == i40e_recv_pkts_vec_avx2) > > options = BURST_VECTOR | BURST_AVX2; > > else if (pkt_burst == i40e_recv_scattered_pkts_vec) > > options = BURST_VECTOR | BURST_SSE | BURST_SCATTERED; > > else if (pkt_burst == i40e_recv_pkts_vec) > > options = BURST_VECTOR | BURST_SSE; > > > > Then *get* like this, since we reserve the bit group. > > > > static void > > burst_mode_options_display(uint64_t options) > > { > > uint64_t vec_mode = options & BURST_VECTOR_MODE_MASK; > > uint64_t opt; > > > > options &= ~BURST_VECTOR_MODE_MASK; > > > > for (opt = 1; options != 0; opt <<= 1, options >>= 1) { > > if (!(options & 1)) > > continue; > > > > printf(" %s", rte_eth_burst_mode_option_name(opt)); > > > > if (opt == BURST_VECTOR) > > printf("(%s)", > > rte_eth_burst_mode_option_name(vec_mode)); > > } > > } > > > > I can see how you intended use it, only they don't need to be bitfield and using > with value saves bits. > Also I think good to reserve some bits for future modes. > "BURST_VECTOR_MODE_MASK = (0x3F << 2)" has reserved 63 non-zero bits on position 2 ~ 7. Then from bit 8, a new definition: BURST_SCATTERED = (1 << 8). "using with value saves bits" -- Sorry, I didn't get the point. :-( vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX From above, 'vector_mode's bits are from 'options' bits stream, how to save bits ? In my understanding, this is some kind of more-bit-field, not each-bit-field. I defined them together, so can quick check the vector type, like (options & BURST_VECTOR_MODE_MASK) == BURST_NEON. > >> > >>> > >>> BURST_SCATTERED = (1 << 8), > >>> BURST_BULK_ALLOC = (1 << 9), > >>> BURST_NORMAL = (1 << 10), > >> > >> Not sure about this one, what is the difference between scalar? > >> > > > > Extract it from the function name and the debug message. > > > > if (pkt_burst == i40e_recv_scattered_pkts) > > options = BURST_SCALAR | BURST_SCATTERED; > > else if (pkt_burst == i40e_recv_pkts_bulk_alloc) > > options = BURST_SCALAR | BURST_BULK_ALLOC; > > else if (pkt_burst == i40e_recv_pkts) > > options = BURST_SCALAR | BURST_NORMAL; > > What is the difference between 'BURST_SCALAR' & "BURST_SCALAR | BURST_NORMAL" ? IMO, "SCALAR" should be "non-Vector" ? Like "BURST_VECTOR" will append with "SSE/AVX2" etc, "SCALAR" will append with other option bits. "Normal" is just handing the Descriptor one by one as *normal*. As I said, I got this name idea from the original log to try cover the right burst behaviors. :) > > btw, for actual implementation please add 'RTE_ETH_' prefix. > Got it, will add them. > > > >>> BURST_SIMPLE = (1 << 11), > >>> }; > >>> > >>> /** > >>> * Ethernet device RX/TX queue packet burst mode information structure. > >>> * Used to retrieve information about packet burst mode setting. > >>> */ > >>> struct rte_eth_burst_mode { > >>> uint32_t per_queue_support:1; /**< Support to set per queue burst */ > >>> > >>> uint64_t options; > >>> }; > >>> > >>> And three APIs: > >>> > >>> 1. > >>> __rte_experimental > >>> int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id, > >>> struct rte_eth_burst_mode *mode); > >>> > >>> > >>> 2. > >>> __rte_experimental > >>> int rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id, > >>> struct rte_eth_burst_mode *mode); > >>> > >>> 3. > >>> __rte_experimental > >>> const char * > >>> rte_eth_burst_mode_option_name(uint64_t option); > >> > >> What about 'rte_eth_burst_mode_name()' ? > >> > > > > The "mode" scope is bigger than "mode_option", so I defined it as "_mode_option_name()". > > > > struct rte_eth_burst_mode { > > uint32_t per_queue_support:1; /**< Support to set per queue burst */ > > > > uint64_t options; > > }; > > Agreed the scope is bigger in implementation, but "burst mode option name" is > same as "burst mode name" for user, so removing it may make easier for user. > But since the API is generating name from 'options' variable, instead of > directly from the port, OK to keep API name as you suggested. > > > > >>> > >>> > >>> PMD two ops: > >>> > >>> typedef void (*eth_burst_mode_get_t)(struct rte_eth_dev *dev, > >>> uint16_t queue_id, struct rte_eth_burst_mode *mode); > >>> > >>> struct eth_dev_ops { > >>> ... > >>> eth_burst_mode_get_t rx_burst_mode_get; /**< Get RX burst mode */ > >>> eth_burst_mode_get_t tx_burst_mode_get; /**< Get TX burst mode */ > >>> ... > >>> }; > >>> > >
next prev parent reply index Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-13 3:06 [dpdk-dev] [RFC v2 0/3] show the Rx/Tx burst description field Haiyue Wang 2019-08-13 3:06 ` [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information Haiyue Wang 2019-08-13 3:24 ` Stephen Hemminger 2019-08-13 4:37 ` Wang, Haiyue 2019-08-13 9:57 ` David Marchand 2019-08-13 11:21 ` Wang, Haiyue 2019-08-13 12:51 ` Ray Kinsella 2019-09-06 14:21 ` Ferruh Yigit 2019-09-07 2:42 ` Wang, Haiyue 2019-09-09 11:23 ` Ferruh Yigit 2019-09-09 12:40 ` Bruce Richardson 2019-09-09 12:50 ` Ferruh Yigit 2019-09-09 13:17 ` Ferruh Yigit 2019-09-10 4:36 ` Wang, Haiyue 2019-09-10 8:06 ` Ferruh Yigit 2019-09-10 8:37 ` Wang, Haiyue 2019-09-10 9:14 ` Ferruh Yigit 2019-09-10 11:41 ` Wang, Haiyue [this message] 2019-09-10 15:00 ` Ferruh Yigit 2019-09-10 15:17 ` Wang, Haiyue 2019-09-10 15:33 ` Ferruh Yigit 2019-09-10 15:35 ` Wang, Haiyue 2019-09-10 14:19 ` Wang, Haiyue 2019-09-10 15:03 ` Ferruh Yigit 2019-09-10 15:18 ` Wang, Haiyue 2019-09-10 15:36 ` Ferruh Yigit 2019-09-10 15:38 ` Wang, Haiyue 2019-09-10 15:06 ` Ferruh Yigit 2019-09-10 15:21 ` Wang, Haiyue 2019-09-10 15:35 ` Ferruh Yigit 2019-09-10 15:37 ` Wang, Haiyue 2019-10-26 16:45 ` Thomas Monjalon 2019-10-27 4:10 ` Wang, Haiyue 2019-08-15 9:07 ` Ray Kinsella 2019-08-13 3:06 ` [dpdk-dev] [RFC v2 2/3] testpmd: show the Rx/Tx burst description Haiyue Wang 2019-08-13 3:06 ` [dpdk-dev] [RFC v2 3/3] net/ice: support the Rx/Tx burst description trace Haiyue Wang
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=E3B9F2FDCB65864C82CD632F23D8AB8773D6895B@shsmsx102.ccr.corp.intel.com \ --to=haiyue.wang@intel.com \ --cc=bruce.richardson@intel.com \ --cc=chenmin.sun@intel.com \ --cc=dev@dpdk.org \ --cc=ferruh.yigit@intel.com \ --cc=mdr@ashroe.eu \ /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
DPDK-dev Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \ dev@dpdk.org public-inbox-index dpdk-dev Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git