From: Ferruh Yigit <ferruh.yigit@intel.com>
To: "Zhao1, Wei" <wei.zhao1@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: Mon, 25 Sep 2017 10:31:42 +0100 [thread overview]
Message-ID: <1109bef6-39c2-155e-f60d-a8f4147ba359@intel.com> (raw)
In-Reply-To: <A2573D2ACFCADC41BB3BE09C6DE313CA07C691AC@PGSMSX103.gar.corp.intel.com>
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.
<...>
> 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));
<...>
>> > + 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;
<...>
>> > +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.
<...>
>> > +
>> > + memset(&hw->local_dcbx_config, 0,
>> > + sizeof(struct i40e_dcbx_config));
>>
>> Wrong indentation.
Reminder of this one incase missed.
<...>
>> > +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.
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.
>
>> > + 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.
>> > + 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.
<...>
>> > @@ -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.
<...>
>> > + 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.
<...>
>> 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.
>>
>> > + * that port as the command otion type
>>
>> s/otion/options
>>
Reminder of this one incase missed.
<...>
>> > +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.
<...>
next prev parent reply other threads:[~2017-09-25 9:31 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 [this message]
2017-09-26 7:46 ` Zhao1, Wei
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=1109bef6-39c2-155e-f60d-a8f4147ba359@intel.com \
--to=ferruh.yigit@intel.com \
--cc=dev@dpdk.org \
--cc=wei.zhao1@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).