From: "Zhao1, Wei" <wei.zhao1@intel.com>
To: "Yigit, Ferruh" <ferruh.yigit@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3 1/2] net/i40e: queue region set and flush
Date: Tue, 26 Sep 2017 07:46:55 +0000 [thread overview]
Message-ID: <A2573D2ACFCADC41BB3BE09C6DE313CA07C69638@PGSMSX103.gar.corp.intel.com> (raw)
In-Reply-To: <1109bef6-39c2-155e-f60d-a8f4147ba359@intel.com>
Hi, Ferruh
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, September 25, 2017 5:32 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 1/2] net/i40e: queue region set and flush
>
> On 9/25/2017 8:40 AM, Zhao1, Wei wrote:
> > Hi, Ferruh
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Wednesday, September 20, 2017 6:36 PM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v3 1/2] net/i40e: queue region set and
> > flush
> >>
> >> On 9/15/2017 4:13 AM, Wei Zhao wrote:
> >> > This feature enable queue regions configuration for RSS in PF/VF,
> >> > so that different traffic classes or different packet
> >> > classification types can be separated to different queues in
> >> > different queue regions.This patch can set queue region range, it
> >> > include queue number in a region and the index of first queue.
> >>
> >> Is following correct:
> >> So instead of distributing packets to the multiple queues, this will
> > distribute
> >> packets into queue reqions which may consists of multiple queues.
> >>
> >> If so, is there a way to control how packets distributed within same
> >> queue region to multiple queues?
> >>
> >
> > distributed within same queue region is based on a rss algorithm, it
> > is implemented by NIC self, software can not control it.
>
> I was thinking RSS used to select queue region, but no first queue region
> selected, later RSS used to select queue within the region.
> Thanks for clarification.
OK
>
> <...>
>
> > sizeof(vsi->info.tc_mapping));
> >> > + (void)rte_memcpy(&vsi->info.queue_mapping,
> >> > + &ctxt.info.queue_mapping,
> >> > + sizeof(vsi->info.queue_mapping));
> >>
> >> Please keep line allignment same with above line.
> >
> > oK, change in v4, but can not the same.
> > (void)rte_memcpy(&vsi->info.queue_mapping,
> &ctxt.info.queue_mapping,
> >
> > sizeof(vsi->info.queue_mapping));
> > will over 80 maxnumber.
>
> Someting like following, new lines aligned:
>
> (void)rte_memcpy(&vsi->info.queue_mapping,
> &ctxt.info.queue_mapping,
> sizeof(vsi->info.queue_mapping));
Ok, change in v4
>
> <...>
>
> >> > + if ((i == info->queue_region_number) &&
> >> > + (i <=
> >> > +I40E_REGION_MAX_INDEX)) {
> >>
> >>
> >> Please use one more level indentaion here, and pharantesis looks
> >> extra can you please double check?
> >
> > You mean,you want the following moede ?
> > if (i == info->queue_region_number) {
> > if(i <= I40E_REGION_MAX_INDEX) {
> > ...........................
> > }
> > }
> >
>
> No, continuation of the "if" should be easy to recognized from body of the
> "if", something like:
>
> if ((i == info->queue_region_number) &&
> (i <= I40E_REGION_MAX_INDEX)) {
> info->region[i].region_id = conf_ptr->region_id;
> info->region[i].queue_num = conf_ptr->queue_num;
>
> <...>
>
Ok, change in v4
>
> >> > +static int
> >> > +i40e_set_region_flowtype_pf(struct i40e_hw *hw,
> >> > + struct i40e_pf *pf, struct
> > rte_i40e_rss_region_conf
> >> *conf_ptr)
> >>
> >> What do you think starting all functions, even they are static, with
> >> "i40e_queue_region_" prefix?
> > ret = i40e_set_queue_region(pf,
> > conf_ptr);
> > break;
> > case RTE_PMD_I40E_REGION_FLOWTYPE_PF_SET:
> > ret = i40e_set_region_flowtype_pf(hw,
> > pf, conf_ptr);
> > break;
> > case RTE_PMD_I40E_REGION_FLOWTYPE_VF_SET:
> > ret = -EINVAL;
> > break;
> > case RTE_PMD_I40E_UP_REGION_SET:
> > ret = i40e_set_up_region(pf,
> > conf_ptr);
> >
> > I have use the format ofi40e_set_ as prefix?
> > Because many set is not related directly to queue region.
>
> When looking to this patch what functions are doing is clear, but it also should
> be clear when looking the source code some time later what that function is
> about.
>
> That is why I believe a namespace is useful, like "i40e_queue_region_xx", so
> you can immediately say this function is about "RSS queue region" feature.
>
> This function is "rte_pmd_i40e_queue_region_conf", it is applying provided
> queue region configuration based of region op type, so I assumed all these
> functions are directly related to the fature.
>
> If you believe function name will be wrong with that prefix, of course just
> don't use it, but please try to stick with a name space, that will make easy to
> understand these features in all source code.
>
> <...>
Ok, fix in v4
>
>
> >> > +
> >> > + memset(&hw->local_dcbx_config, 0,
> >> > + sizeof(struct i40e_dcbx_config));
> >>
> >> Wrong indentation.
>
> Reminder of this one incase missed.
Ok, change in v4
>
> <...>
>
> >> > +int rte_pmd_i40e_queue_region_conf(uint8_t port,
> >> > + struct
> > rte_i40e_rss_region_conf *conf_ptr) {
> >> > + struct rte_eth_dev *dev = &rte_eth_devices[port];
> >>
> >> you need to verify port_id, since this is public API now. Please
> >> check
> > other
> >> APIs in this file.
>
> Reminder of this one incase missed.
Ok, change in v4
>
> I mean is_i40e_supported() call that other APIs have.
>
> <...>
>
> >> > +
> >> > + if (!is_i40e_supported(dev))
> >> > + return -ENOTSUP;
> >> > +
> >> > + switch (op_type) {
> >> > + case RTE_PMD_I40E_QUEUE_REGION_SET:
> >> > + ret = i40e_set_queue_region(pf, conf_ptr);
> >> > + break;
> >>
> >> Does it make sense to add another type to get the current queue
> >> region config?
>
> Reminder of this one incase missed.
I will add a get command.
>
> >
> >> > + case RTE_PMD_I40E_REGION_FLOWTYPE_PF_SET:
> >> > + ret = i40e_set_region_flowtype_pf(hw, pf,
> > conf_ptr);
> >> > + break;
> >> > + case RTE_PMD_I40E_REGION_FLOWTYPE_VF_SET:
> >> > + ret = -EINVAL;
> >>
> >> Will this be implemented later, or not a valid case at all?
>
> Reminder of this one incase missed.
Ok, change in v4
>
> >> > + break;
> >> > + case RTE_PMD_I40E_UP_REGION_SET:> +
> > ret =
> >> i40e_set_up_region(pf, conf_ptr);
> >> > + break;
> >> > + case RTE_PMD_I40E_REGION_ALL_FLUSH_ON:
> >> > + ret = i40e_flush_region_all_conf(hw, pf, 1);
> >> > + break;
> >> > + case RTE_PMD_I40E_REGION_ALL_FLUSH_OFF:
> >> > + ret = i40e_flush_region_all_conf(hw, pf, 0);
> >>
> >> Can you please describe what flush_on and flush_off are, you can
> >> comment to code if also.
>
> Reminder of this one incase missed.
I will comment to code in v4.
ALL config from CLI at first will only keep in DPDK software stored in driver, only after " FLUSH_ON ", it commit all configure to HW.
Because I have to set hardware config at a time, so I have to record all CLI command at first.
" FLUSH_OFF " is just clean all configuration about queue region just now, and restore all to DPDK i40e driver default config when start up.
The following is my test process in CLI:
./x86_64-native-linuxapp-gcc/app/testpmd -c 1ffff -n 4 -- -i --nb-cores=8 --rxq=8 --txq=8 --port-topology=chained set fwd rxonly port config all rss all queue-region set port 0 region_id 0 queue_start_index 2 queue_num 2 queue-region set port 0 region_id 1 queue_start_index 4 queue_num 2
queue-region set pf port 0 region_id 0 flowtype 31
queue-region set pf port 0 region_id 1 flowtype 33
queue-region set pf port 0 region_id 1 flowtype 34
queue-region set port 0 UP 1 region_id 0
queue-region set port 0 UP 3 region_id 1
queue-region flush on port 0
start set verbose 1 sendp([Ether(dst="3C:FD:FE:9F:70:B8", src="52:00:00:00:00:00")/Dot1Q(prio=3)/IP(src="10.0.0.1",dst="192.168.0.2")/TCP(dport=80, sport=80 )/("X"*48)], iface="ens5f2", count=10) sendp([Ether(dst="3C:FD:FE:9F:70:B8", src="52:00:00:00:00:00")/IP(src="10.0.0.1",dst="192.168.0.2")/SCTP(dport=80, sport=80 )/("X"*48)], iface="ens5f2", count=10) sendp([Ether(dst="3C:FD:FE:9F:70:B8", src="52:00:00:00:00:00")/IP(src="10.0.0.1",dst="192.168.0.2")/UDP(dport=80, sport=80 )/("X"*48)], iface="ens5f2", count=10)
>
> <...>
>
> >> > @@ -146,6 +160,18 @@ struct rte_pmd_i40e_ptype_mapping { };
> >> >
> >> > /**
> >> > + * Queue region information get from CLI.
> >>
> >> It doesn't need to be from CLI, can drop that part
>
> Reminder of this one incase missed.
>
Ok, change in v4
> <...>
>
> >> > + uint8_t user_priority;
> >> > + enum rte_pmd_i40e_queue_region_op op;
> >>
> >> Extra space before "op"
> >
> > Maybe, I SHOULD add uint8_t raw[3] before “op”
>
> Sorry, I didn't get this one.
Ok
>
> <...>
>
> >> rte_pmd_i40e_ptype_mapping_replace(uint8_t
> >> > port, int rte_pmd_i40e_add_vf_mac_addr(uint8_t port, uint16_t
> >> >vf_id,
> >> > struct
> > ether_addr *mac_addr);
> >> >
> >> > +/**
> >> > + * Get RSS queue region info from CLI and do configuration for
> >>
> >> Again now this is an API, not just for testpmd, please drop CLI
>
> Reminder of this one incase missed.
Ok, change in v4
>
> >>
> >> > + * that port as the command otion type
> >>
> >> s/otion/options
> >>
>
> Reminder of this one incase missed.
Ok, change in v4
>
> <...>
>
> >> > +int rte_pmd_i40e_queue_region_conf(uint8_t port,
> >>
> >> Can you please use port_id as variable name to be consistent.
>
> Reminder of this one incase missed.
Ok, change in v4
>
> <...>
next prev parent reply other threads:[~2017-09-26 7:47 UTC|newest]
Thread overview: 87+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-24 3:26 [dpdk-dev] [PATCH 0/2] net/i40e: API to configure queue regions for RSS Wei Zhao
2017-08-24 3:26 ` [dpdk-dev] [PATCH 1/2] net/i40e: queue region set and flush Wei Zhao
2017-08-31 16:18 ` Ferruh Yigit
2017-09-01 2:38 ` Zhao1, Wei
2017-09-06 9:11 ` Ferruh Yigit
2017-09-15 11:00 ` Ferruh Yigit
2017-09-20 3:20 ` Zhao1, Wei
2017-09-20 10:32 ` Ferruh Yigit
2017-09-18 3:38 ` Zhao1, Wei
2017-09-05 23:52 ` Chilikin, Andrey
2017-09-06 7:21 ` Zhao1, Wei
2017-08-24 3:26 ` [dpdk-dev] [PATCH 2/2] app/testpmd: add API for configuration of queue region Wei Zhao
2017-09-13 6:04 ` [dpdk-dev] [PATCH v2 0/2] net/i40e: API to configure queue regions for RSS Wei Zhao
2017-09-13 6:04 ` [dpdk-dev] [PATCH v2 1/2] net/i40e: queue region set and flush Wei Zhao
2017-09-13 6:04 ` [dpdk-dev] [PATCH v2 2/2] app/testpmd: add API for configuration of queue region Wei Zhao
2017-09-15 3:13 ` [dpdk-dev] [PATCH v3 0/2] net/i40e: API to configure queue regions for RSS Wei Zhao
2017-09-15 3:13 ` [dpdk-dev] [PATCH v3 1/2] net/i40e: queue region set and flush Wei Zhao
2017-09-20 10:36 ` Ferruh Yigit
2017-09-21 6:48 ` Zhao1, Wei
2017-09-21 7:10 ` Ferruh Yigit
2017-09-21 7:26 ` Zhao1, Wei
2017-09-21 15:45 ` Ferruh Yigit
2017-09-25 7:40 ` Zhao1, Wei
2017-09-25 9:31 ` Ferruh Yigit
2017-09-26 7:46 ` Zhao1, Wei [this message]
2017-09-26 8:54 ` Zhao1, Wei
2017-09-27 19:13 ` Ferruh Yigit
2017-09-28 2:40 ` Zhao1, Wei
2017-09-21 19:53 ` Chilikin, Andrey
2017-09-22 8:49 ` Zhao1, Wei
2017-09-22 20:13 ` Chilikin, Andrey
2017-09-25 2:55 ` Zhao1, Wei
2017-09-24 16:01 ` Wu, Jingjing
2017-09-25 3:26 ` Zhao1, Wei
2017-09-25 5:55 ` Zhao1, Wei
2017-09-15 3:13 ` [dpdk-dev] [PATCH v3 2/2] app/testpmd: add API for configuration of queue region Wei Zhao
2017-09-20 10:45 ` Ferruh Yigit
2017-09-25 9:25 ` Zhao1, Wei
2017-09-25 9:43 ` Ferruh Yigit
2017-09-26 5:30 ` Zhao1, Wei
2017-09-28 9:04 ` [dpdk-dev] [PATCH v4 0/3] net/i40e: API to configure queue regions for RSS Wei Zhao
2017-09-28 9:04 ` [dpdk-dev] [PATCH v4 1/3] net/i40e: queue region set and flush Wei Zhao
2017-09-28 9:04 ` [dpdk-dev] [PATCH v4 2/3] app/testpmd: add API for configuration of queue region Wei Zhao
2017-09-28 9:04 ` [dpdk-dev] [PATCH v4 3/3] doc/testpmd_app_ug: add doc info for " Wei Zhao
2017-09-28 9:10 ` [dpdk-dev] [PATCH v4 0/3] net/i40e: API to configure queue regions for RSS Wei Zhao
2017-09-28 9:10 ` [dpdk-dev] [PATCH v4 1/3] net/i40e: queue region set and flush Wei Zhao
2017-09-28 9:10 ` [dpdk-dev] [PATCH v4 2/3] app/testpmd: add API for configuration of queue region Wei Zhao
2017-09-28 9:10 ` [dpdk-dev] [PATCH v4 3/3] doc/testpmd_app_ug: add doc info for " Wei Zhao
2017-09-29 2:56 ` [dpdk-dev] [PATCH v5 0/3] net/i40e: API to configure queue regions for RSS Wei Zhao
2017-09-29 2:56 ` [dpdk-dev] [PATCH v5 1/3] net/i40e: queue region set and flush Wei Zhao
2017-09-29 4:54 ` Wu, Jingjing
2017-09-29 8:27 ` Zhao1, Wei
2017-09-29 2:56 ` [dpdk-dev] [PATCH v5 2/3] app/testpmd: add API for configuration of queue region Wei Zhao
2017-09-29 5:04 ` Wu, Jingjing
2017-09-29 5:21 ` Zhao1, Wei
2017-09-29 2:56 ` [dpdk-dev] [PATCH v5 3/3] doc/testpmd_app_ug: add doc info for " Wei Zhao
2017-09-29 8:11 ` [dpdk-dev] [PATCH v6] net/i40e: API to configure queue regions for RSS Wei Zhao
2017-09-29 8:11 ` [dpdk-dev] [PATCH v6] app/testpmd: add API for configuration of queue region Wei Zhao
2017-09-29 8:11 ` [dpdk-dev] [PATCH v6] net/i40e: queue region set and flush Wei Zhao
2017-09-29 9:00 ` Wu, Jingjing
2017-09-29 9:16 ` [dpdk-dev] [PATCH v7 0/2] net/i40e: API to configure queue regions for RSS Wei Zhao
2017-09-29 9:16 ` [dpdk-dev] [PATCH v7 1/2] net/i40e: queue region set and flush Wei Zhao
2017-09-29 12:22 ` Wu, Jingjing
2017-10-10 1:45 ` Zhao1, Wei
2017-10-03 17:54 ` Ferruh Yigit
2017-10-10 6:11 ` Zhao1, Wei
2017-09-29 9:16 ` [dpdk-dev] [PATCH v7 2/2] app/testpmd: add API for configuration of queue region Wei Zhao
2017-09-29 14:29 ` Wu, Jingjing
2017-10-03 18:04 ` Ferruh Yigit
2017-10-10 1:46 ` Zhao1, Wei
2017-10-10 2:55 ` Zhao1, Wei
2017-10-10 3:01 ` Zhao1, Wei
2017-10-11 2:15 ` Ferruh Yigit
2017-09-29 10:15 ` [dpdk-dev] [PATCH v7 0/2] net/i40e: API to configure queue regions for RSS Peng, Yuan
2017-10-11 8:49 ` [dpdk-dev] [PATCH v8 " Wei Zhao
2017-10-11 8:49 ` [dpdk-dev] [PATCH v8 1/2] net/i40e: queue region set and flush Wei Zhao
2017-10-11 8:49 ` [dpdk-dev] [PATCH v8 2/2] app/testpmd: add API for configuration of queue region Wei Zhao
2017-10-11 8:55 ` [dpdk-dev] [PATCH v8 0/2] net/i40e: API to configure queue regions for RSS Wei Zhao
2017-10-11 8:55 ` [dpdk-dev] [PATCH v8 1/2] net/i40e: queue region set and flush Wei Zhao
2017-10-13 10:06 ` Chilikin, Andrey
2017-10-18 3:00 ` Zhao1, Wei
2017-10-18 13:00 ` Chilikin, Andrey
2017-10-19 2:18 ` Zhao1, Wei
2017-10-11 8:55 ` [dpdk-dev] [PATCH v8 2/2] app/testpmd: add API for configuration of queue region Wei Zhao
2017-10-11 11:29 ` [dpdk-dev] [PATCH v8 0/2] net/i40e: API to configure queue regions for RSS Peng, Yuan
2017-10-11 21:06 ` Ferruh Yigit
2017-10-13 1:52 ` Zhao1, Wei
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=A2573D2ACFCADC41BB3BE09C6DE313CA07C69638@PGSMSX103.gar.corp.intel.com \
--to=wei.zhao1@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@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).